SEP 4 - First draft for a change in development policy

[aplanas]

Proposal to change the development process to focus all the changes first in develop (master) branch.

Fix saltstack/salt#51997

[cachedout]

I am a 👎 on this. This is actually what we used to do. It was extremely hard to track this workflow from a management perspective and it frequently resulted in messes that had to be untangled. It made releases really hard to manage as well.

I'm sympathetic to the desire, however, to lower the size of the merge forward footprint and the troubles that delays can cause. It used to be the case that we did merge forwards just about every day but I don't know how often it's happening now.

Perhaps we could explore a policy to squash merge forwards and see what could be done to improve the velocity of the merge forwards?

[Ch3LL]

I agree with @cachedout 's comments. I understand the desire to want to do this but in my opinion it is much easier to manage as we currently have this setup.

One reason we might be feeling this pain point of using the merge forward strategy is @cachedout 's concern here: " It used to be the case that we did merge forwards just about every day but I don't know how often it's happening now."

I can expand on this, currently we are attempting to do merge forwards two times a week, but we can definitely try to increase this as it has become very apparent it will solve some of these issues with the current workflow. ping @garethgreenaway since you and I do merge forwards each week what do you think about increasing the velocity we do merge forwards currently. If we did them more frequent we would also deal with less merge conflicts.

[aplanas]

I am a -1 on this. This is actually what we used to do.

Right, I am not aware of the history of Salt development. I can assume that the previous model was developing against master, track the bugs with issues, doing the review in master, and tracking the backporting via the issue. Am I right in this?

It was extremely hard to track this workflow from a management perspective and it frequently resulted in messes that had to be untangled. It made releases really hard to manage as well.

How so? Other big projects are managed as described. How this was not suitable for SaltStack?

I'm sympathetic to the desire, however, to lower the size of the merge forward footprint and the troubles that delays can cause. It used to be the case that we did merge forwards just about every day but I don't know how often it's happening now.

I can see that this is part of the problem. Yes. And fixing this can help.

But my main concern is something like this:

saltstack/salt#50812

This change is against 2018.3, and not against master. For me is hard to see why is OK that master is the last one to receive and review changes of this magnitude. I did not make a full review, that I saw more commits like that one in released branches and not in master.

Perhaps we could explore a policy to squash merge forwards and see what could be done to improve the velocity of the merge forwards?

I am not sure that will help. Mainly because squashing will not decrease the size of files and the magnitude of changes, and will make more complicate the rebase, mainly because you lost the context of the change (how did that and why). Also will make simply impossible track the original change in the history.

[aplanas]

I agree with @cachedout 's comments. I understand the desire to want to do this but in my opinion it is much easier to manage as we currently have this setup.

That is a good argument. If the experience is that the current workflow is more easy to manage, that is a good point.

One reason we might be feeling this pain point of using the merge forward strategy is @cachedout 's concern here: " It used to be the case that we did merge forwards just about every day but I don't know how often it's happening now."

I can expand on this, currently we are attempting to do merge forwards two times a week, but we can definitely try to increase this as it has become very apparent it will solve some of these issues with the current workflow. ping @garethgreenaway since you and I do merge forwards each week what do you think about increasing the velocity we do merge forwards currently. If we did them more frequent we would also deal with less merge conflicts.

This will decrease the size of the PR, but no necessarily the time until master get in sync with the current code. But if there is a consensus that it will be decreased, I am OK with this change.

[cachedout]

How so? Other big projects are managed as described. How this was not suitable for SaltStack?

This is a very reasonable and important question. I don't have any concrete data but I have two main theories.

1. A lot of large projects are doing more active feature work than Salt is. My guess is that they're seeing a higher percentage of commits to master that don't require a backport than the Salt project would. (Though I would certainly be persuaded by hard numbers if somebody wants to go gather data.)

2. Other large projects have the responsibility for this process spread out among teams or more than a single person. SaltStack has elected to keep this control in-house and thus the bottleneck becomes whatever the company can allocate to this task. Historically, that has been one person doing the work which is also spread among many other duties. There ends up being a lot of pressure to simplify this task as much as possible.

[garethgreenaway]

To add some addition context on the merge forward, as @Ch3LL said we attempt to do them multiple times a week to ensure we're keeping up with changes that need to go between branches. Unfortunately we're not always able to do so, failing tests in the merge forwards themselves as well as failing tests in general have slowed down the frequency when those merge forwards can happen. There are efforts in place to stabilize the testing suite and once that happens more we get back up to the two/week effort.

[max-arnold]

Cherry-picking (backporting) is much harder to do (and track) than merge forward. Also this process creates different commit IDs and you will lose some useful things you can do with Git.

Could merge forward process be automated in some cases (if tests pass)?

[cachedout]

Could merge forward process be automated in some cases (if tests pass)?

I bet it wouldn't be too hard to do this using the project that @rallytime started for managing PRs in the Salt repo: https://github.com/rallytime/tamarack

[rallytime]

Automating merge-forwards in code would not be hard to do. The tricky part about it is in practice.

The vast number of merge-forwards have merge conflicts which need to be resolved by hand. When I was running the merge-forward process, it was rare that a merge-forward would occur without a conflict and at least 90% of all merge-forwards had conflicts.

The more branches diverge from each other, the higher the probability of conflicts during the merge-forward. This also depends largely on the velocity of changes being merged into each divergent branch.

An argument could certainly be made for making pull requests less conflict-prone, which would facilitate an automated process. If there were a systematic process adopted by maintainers and contributors that avoids conflicts as much as possible in pull requests, then an automated solution would be much more viable. I don't know what that would look like exactly, but I wanted to at least put the idea out there.

Note that some situations that cause merge conflicts will be difficult to avoid. Historically, the main conflict-causing changes have been around needing to split up the utils init file into separate pieces; PY2/PY3 support, particularly around unicode fixes; and any other large refactors that needed to be done in core functionality. In my time managing the merge-forward process there was always at least one major refactor per release branch that caused some trickiness.

Hopefully this explanation and context is helpful. I always wanted to automate the merge-forward process, but wasn't able to come up with a viable solution to the hurdles I've described above.

I would definitely be interested to hear any ideas people might have! :D

[cachedout]

@rallytime How much do you think those problems would be reduced if the merge-forward process happened per-PR? I'm thinking about a workflow where, instead of batching them all together, from the moment a PR is merged, a bot picks it up and tries to get it merged forward. I understand that we'd still have the same number of conflicts in total, give or take, but if we were left with only the conflicts to resolve, would this be an improvement?

[garethgreenaway]

The major question I would have with the idea of having any process in place to auto merge-forward individual PRs would be where they're merged forward to? If someone adds a PR into the 2018.3, does the automated process generate additional PRs into 2019.3 and develop? Or just 2019.3 and we're still responsible for pushing those changes into the develop branch from 2019.3? In the case of the former situation, it would potentially triple the number of PRs we have to review. I'm not against the idea of automating processes, but this sounds like the additional overhead would out weight any gains. Additionally, when we're close to a release there are additional branches, the RC branches, that we're merging changes into.

[rallytime]

How much do you think those problems would be reduced if the merge-forward process happened per-PR?

@cachedout One immediate thing that would happen if something like this were implemented would be that merge conflict resolution would be pushed from the core team resolving them in batches to individual contributors needing to resolve the conflicts per PR. I'm not saying that this is a good or a bad thing, but something that would need to be considered in the discussion.

I think @garethgreenaway raises some legitimate points. If a PR was merged forward per change, that is a lot easier to handle on branches that don't move nearly so much or if you only have a single branch to merge forward from. But how would that work with multiple branches? What would that do the number of merge requests that can be reviewed in a day? How would that increase the load on reviewers? How would merge conflicts be handled on a per-PR basis from a bot? None of these questions are show-stoppers, but certainly things to think about.

I fully admit that I may be thinking too close to the problem. I am sure there is some middle ground that can be found, but I haven't come up with any good solutions yet.

[waynew]

Does anyone have an objection to this SEP entering the final comment phase?

[aplanas]

@waynew I agree, but I am not able to contribute too much, as this is an internal process. Do you know if the scripts that take care of this process is available in github? Maybe I can help there.

[waynew]

@aplanas technically it's public - all of the merge forwards happen via PR. If community members wanted to step up and help with the merge forwards I don't think that anyone would object 🙂 I'm not sure exactly what that would look like, though 🤷‍♂️ But if there's enough interest I'm sure we could make that happen.

[damon-atkins]

You could have a once a day Jenkins job which tries to merge forward one old version per day. i.e. it creates the pull request for review. This should work as long as conflicts are resolved within 7 days.
On Tuesday its N-3, Wednesday N-2, Thursday N-1 where N is develop.
If Tuesdays conflict is not resolved Wednesday still goes ahead.

If you want community members to help then you would need some doco to guide them.

[aplanas]

@damon-atkins that predictability is a big +1 from my side, and the documentation is another one

[waynew]

@damon-atkins That's a rather interesting approach. To make sure I understand this right:

2018.3===A=======L=======M========================>
          `----, `-,     `-----,               
                \   \           \                 
2019.2=======B===C===D===E=======N================>
             `-, `--,`-, `-,     `-----,        
                \   \   \   \           \         
develop======F===G===H===I===J===K=======O========>
                                                  
Weekday  S   M   T   W   Th  F   S   M   T   W

Does this look like the workflow that you're describing?

1. Sunday, commit A happens. A merge forward PR is magically created.
2. Monday, commit B happens in 2019.2 and F on develop. Merge forward from 2019 -> develop is created.
3. Tuesday, commit A gets merged from 2018, and L happens on 2018. B gets merged into develop.

And so on and so forth?

So as long as the merge forward PRs are merged in the right order, and before the next week, everything should be A-OK.

If I understand correctly, I am fan of this idea. I don't know if a nightly merge forward is too fast, but regardless of the period N, I do think that the idea has merit. I'm not terribly familiar with how intense the merge forward process is, I believe @Ch3LL and @garethgreenaway are usually the ones dealing with that and may be able to offer some insights.

In general though I think I'd be in favor of having something automatically and regularly create the merge forward PRs, assuming the source branch is green.

[gtmanfred]

That works great, until you run into one of the following

• There is a merge conflict that takes more than a day to resolve, which used to happen fairly frequently because core engineers had more responsibilities than just merge forwards.

• The previous nights pr is not merged and then someone merges the first one when there is a second one setup, and then you have prs that can't be merged, etc, and you have to go back and clean it up. Unless you are suggesting automatically merging the prs, then you have a bunch more complication to add in for watching the tests (which i /think/ jenkins can do, but drone cannot, though i don't know where the possibly migration to drone is sitting...) but I would also say that automatically merging prs is a bad idea... and then it continues to get complicated.

Also, what happens when develop diverges from a previous branch because of a large refactor, then you are going to start running into conflicts on every merge forward for that section of code.

[waynew]

@gtmanfred Yeah - that's assuming the period is 1 day. Also, in theory one could merge several PRs back to back. In fact, as long as the merge forwards are simply merged forward in order theoretically there wouldn't be a problem with having say, 15 merges stacked up.

I mean, aside from having a bunch of noise for no real good reason

Also, what happens when develop diverges from a previous branch because of a large refactor, then you are going to start running into conflicts on every merge forward for that section of code.

It depends on the refactor and the future changes.

Typically, once you have made one change to a section and then you merge to another branch, and subsequent changes to that section get merged without problem, because git realizes, "oh yes, you changed this section, and you already told me that the new changes go in here"

Of course making multiple unrelated changes could be problematic.

So if we do decide to auto create our merge forwards, those are good things to keep in mind when feeling out what period N we should do.

[gtmanfred]

So, the current process as I have seen it is the following:

1. make a new branch off of the branch that you want to merge into.
2. merge the old branch into the newer branch
3. push your branch to your remote and then open a PR.
4. the branch is merged with a merge commit into master (not fast forward merged)

With these steps, depending on the order that they are merged, and how long they take for the merge conflict to be resolved, it could stack up a bunch of unmergable prs, which will just end up with a bunch of closed prs.

So with the current process, you could get backed up really fast. Which then ends up with more prs that need to be reviewed and merged, and based on the number of open PRs, that doesn't look like an issue we would want to add to.

As for refactors, when we moved everything out of salt/utils/__init__.py into salt/utils/*.py there were a number of things that had to be taken care of during the merge forward process for a while.

[DmitryKuzmenko]

Returning back to the original request I want to leave my 5 cents here. Why I disagree with that original idea is that if we'd backporting instead of merging forward we'll need to operate commits one-by-one because of many develop's changes aren't desired in the released branches. So in our release process it would be a pain. While merge forward provides us the easy enough way to have all the fresh fixes in the recent branches. And we can decide where the fix should go to during the triaging process.

[damon-atkins]

On Tuesday its N-3, Wednesday N-2, Thursday N-1 where N is develop.

The idea above was to be very very simple. i.e. no dependency at all.

• Tue 2017.7 Pull Request into 2018.3, allow 5 days to resolve merge conflicts
• Wed 2018.3 Pull Request into 2019.2, will happen even if the above has unresolved merge conflicts, allow 5 days to resolve merge conflicts
• Thur 2019.2 into develop, will happen even if the above has unresolved merge conflicts, allow 5 days to resolve merge conflicts.

Still need the resources to do the above.

[waynew]

We've talked about this a little more in one of our team meetings, and I want to share what I/we learned.

First, everyone is in agreement that our current merge forward process has some warts. There are several reasons behind that, and we have some plans to fix what we can.

The #1 challenge in the merge forward process is that our test suite is flaky. That is, entirely green test suites are not the norm. We're working hard to fix that. The reason this causes problems is because when we merge code forward and there are broken tests (which is basically guaranteed), it's difficult to tell if the tests were already broken or if the merge forward broke it.

Our plan to fix that is to get all of the test suites green. We're requiring all tests passing on PRs. In the past if the failing test was just a flaky test and maybe not related to that PR we'd go ahead and merge it anyway. The other change is that now we're requiring tests on PRs - if you make a bug fix, we'll require a regression test. If you add a feature, we need some unit tests. We are also only going to allow merging a green branch into another green branch.

Focusing on stability should greatly reduce the amount of time required to actually perform a merge forward.

As far as automating the process goes, there's really nothing that would work well. Even if we followed one of these proposed approaches, the main problem behind merge forwards isn't that we're not creating them.

Another problem (obviously?) is that we do have kind of a dearth of documentation on the topic. We have a couple of paragraphs but they don't discuss any of the reasons why we use merge forwards instead of any other work flow. @garethgreenaway will be working on enhancing that documentation, so if there's anything you want to make sure he addresses, feel free to mention him.

---

Otherwise, from what I can tell the general consensus is that we aren't going to change the merge forward process in general, though we do have plans to improve it. This discussion I feel has been helpful at least for me, but hopefully some others. As I was checking our our SEP documentation I did realize that we don't have a solid rejection criteria, other than it doesn't get the required 5 👍's.

If someone feels strongly that we should require an explicit core team vote on this before we merge it as a rejected SEP, I'll make that happen. Otherwise, if I don't hear opposition by Monday I'll go ahead and merge this SEP as rejected. (But again, thank you for bringing this up, it feels like we clarified a lot of things, and I know I learned 🙂 )

[aplanas]

@waynew Thanks for driving this initiative. I am very glad that we discussed this, and I hope that the outcome will help have minimal and faster merges to develop. Feel free to close it on Monday from my side.

Thanks to all for the information provided and for the willing to revisit this problem.