The order of master patches and stable backports
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 Hi all, I'm concerned that in some cases [1] we merge patches in stable branches before they reach master. My understanding is that in general such approach may lead to multiple kinds of issues: 1. if a master patch is not merged in the end due to negative review feedback, we will need to cope with a stable-only patch. 2. if a master patch is modified later before its merge, we need to cope with the need to apply last minute modifications to stable branches. In the worst cases, the difference won't even be noticed, and we'll result in unneeded differences between master and stable code. 3. also, if a patch is merged into e.g. havana and not icehouse, and then a user upgrades to icehouse, he will see a regression due to the missing patch that was once fixed in a prev-stable release. We could say that there are some cases where the strict ordering of backports is not applicable, like in [1]. Quoting Thierry, " @Ihar: stable branch changes come in two categories. There are project bugfixes (which are straight backports of stuff that has to land in master to ensure we have no regression) and infrastructure patches (to keep the test infrastructure working on stable branches). The latter do NOT have to land in master: in most cases, they are not even applicable there. This is basically a branch-specific infrastructure patch to keep the stable branch running. The fact that it has a master "equivalent" is more of a coincidence. Not having that patch in master wouldn't be a regression. To answer your question, if the patch has to be reworked, that would result in subsequent patchsets. " So some cases are not affected by problem (3), but still, (1) and (2) apply. I understand that in some cases we should not wait for long, like security fixes or infrastructure being broken right now. Though the case in question is not the latter (the infrastructure will become broken only if infra team enables latest tox without making sure all the branches are properly patched). So I think the following quote by Sean is not correct: "stable/* is going to be horribly broken if this doesn't land. We could let all the stable branches die, but I don't think that's sane." ...and hence, the rush with which we're pushing those patches into stable branches is not justified. I would like to hear from all of you on the matter, and I'm going to update our stable workflow rules based on your feedback. Comments? [1]: https://review.openstack.org/#/c/109750/ /Ihar -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.22 (Darwin) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCgAGBQJT5H1TAAoJEC5aWaUY1u57pOAH/2xFM3aC6J2juFGth4Z6bPsV le0gjz7bWl1KyTgyBBkD0LsZbkOg1UdKL5Vrg5Ua4iT0j2NP+Q6QCy/AuR94cGko v5n1AuQm8YQxB1wQAmuG8xG2YAHtiX7gx/5Kw/kIZgkVdd8UHQXp28dBwu9td1+k TLo7MfJ7jde/BAlB6OLCrPC7yctog4xANrZOYw3u2tzVqh4YK7o67QceAVKcNIW6 EgVTSF+f6SyzJYMHKB58kusLFMmmNQDCdL0/gqoJ3qgPMA5ASh6ZjXzFfVggR5pT UnqEJ+0yN47RzRWaH6KlAhhVDW7EwKJwgvlggr8dwnA7ei/TYDWqunsEQ4Wcbmk= =JXuG -----END PGP SIGNATURE-----
Ihar Hrachyshka wrote:
I'm concerned that in some cases [1] we merge patches in stable branches before they reach master. My understanding is that in general such approach may lead to multiple kinds of issues:
1. if a master patch is not merged in the end due to negative review feedback, we will need to cope with a stable-only patch.
Actually, the main goal is to make sure we don't insert a regression: bugs fixed in the stable branch should also be fixed on the development release, and the easier way to ensure that is to block stable bugfixes until master incorporates the same bugfix.
2. if a master patch is modified later before its merge, we need to cope with the need to apply last minute modifications to stable branches. In the worst cases, the difference won't even be noticed, and we'll result in unneeded differences between master and stable code.
3. also, if a patch is merged into e.g. havana and not icehouse, and then a user upgrades to icehouse, he will see a regression due to the missing patch that was once fixed in a prev-stable release.
We could say that there are some cases where the strict ordering of backports is not applicable, like in [1]. Quoting Thierry,
" @Ihar: stable branch changes come in two categories. There are project bugfixes (which are straight backports of stuff that has to land in master to ensure we have no regression) and infrastructure patches (to keep the test infrastructure working on stable branches). The latter do NOT have to land in master: in most cases, they are not even applicable there. This is basically a branch-specific infrastructure patch to keep the stable branch running. The fact that it has a master "equivalent" is more of a coincidence. Not having that patch in master wouldn't be a regression. To answer your question, if the patch has to be reworked, that would result in subsequent patchsets. "
So some cases are not affected by problem (3), but still, (1) and (2) apply.
Let me elaborate on infra patches. Those are not bugfixes, nor is it really affecting the source code. Those are changes in peripheral files (which are often not even included in tarballs) so that the CI continues to work on stable branches despite recent changes made to the infrastructure. In some cases those patches need to be applied to all branches, in some cases just to a specific branch. So they are, by nature, branch-specific original patches rather than backports: the concern in (1) doesn't really apply IMHO. The concern in (2) is still valid though: by proposing and merging those branch-specific patches in parallel, if we realize that they are insufficient, a follow-up patch may have to be applied to all branches. Merging one first and then apply the same logic to the others would avoid that. Note that the order doesn't really matter though: the stable/havana patch could be the first one (temporarily breaking CI on stable/havana actually hurts the project a lot less than temporarily breaking CI on master). So I'd leave that ordering choice to the Infrastructure person that pushes those patches, and not block them on master landing. They know what they are doing. Now, I understand that this is not documented at all, it's more of an oral tradition within the stable branch team, and we need to properly document that exception to the rules. -- Thierry Carrez (ttx)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On 08/08/14 10:42, Thierry Carrez wrote:
Ihar Hrachyshka wrote:
I'm concerned that in some cases [1] we merge patches in stable branches before they reach master. My understanding is that in general such approach may lead to multiple kinds of issues:
1. if a master patch is not merged in the end due to negative review feedback, we will need to cope with a stable-only patch.
Actually, the main goal is to make sure we don't insert a regression: bugs fixed in the stable branch should also be fixed on the development release, and the easier way to ensure that is to block stable bugfixes until master incorporates the same bugfix.
2. if a master patch is modified later before its merge, we need to cope with the need to apply last minute modifications to stable branches. In the worst cases, the difference won't even be noticed, and we'll result in unneeded differences between master and stable code.
3. also, if a patch is merged into e.g. havana and not icehouse, and then a user upgrades to icehouse, he will see a regression due to the missing patch that was once fixed in a prev-stable release.
We could say that there are some cases where the strict ordering of backports is not applicable, like in [1]. Quoting Thierry,
" @Ihar: stable branch changes come in two categories. There are project bugfixes (which are straight backports of stuff that has to land in master to ensure we have no regression) and infrastructure patches (to keep the test infrastructure working on stable branches). The latter do NOT have to land in master: in most cases, they are not even applicable there. This is basically a branch-specific infrastructure patch to keep the stable branch running. The fact that it has a master "equivalent" is more of a coincidence. Not having that patch in master wouldn't be a regression. To answer your question, if the patch has to be reworked, that would result in subsequent patchsets. "
So some cases are not affected by problem (3), but still, (1) and (2) apply.
Let me elaborate on infra patches. Those are not bugfixes, nor is it really affecting the source code. Those are changes in peripheral files (which are often not even included in tarballs) so that the CI continues to work on stable branches despite recent changes made to the infrastructure.
In some cases those patches need to be applied to all branches, in some cases just to a specific branch. So they are, by nature, branch-specific original patches rather than backports: the concern in (1) doesn't really apply IMHO.
The concern in (2) is still valid though: by proposing and merging those branch-specific patches in parallel, if we realize that they are insufficient, a follow-up patch may have to be applied to all branches. Merging one first and then apply the same logic to the others would avoid that. Note that the order doesn't really matter though: the stable/havana patch could be the first one (temporarily breaking CI on stable/havana actually hurts the project a lot less than temporarily breaking CI on master). So I'd leave that ordering choice to the
Though CI is not broken in the case I've asked to comment. The only issue with not pushing those patches immediately is that infra won't be able to upgrade to the latest tox as quickly as they may probably like. Doesn't sound like a blockage in gate. So I think we have time to consider the patches with due review process and not introduce exceptions to the rules.
Infrastructure person that pushes those patches, and not block them on master landing. They know what they are doing.
Now, I understand that this is not documented at all, it's more of an oral tradition within the stable branch team, and we need to properly document that exception to the rules.
Not much traction on the thread. :) Anyway. I've updated this specific rule in [1] with the following: "(Note: some patches may get exception from this specific rule. These are patches that do not touch production code, like test-only patches, or tox.ini changes that fix major gate breakage, etc.; or security patches that should not take much time to merge once the patches are published. In those cases, stable patches may be pushed into gate without waiting for all consequent branches to be fixed. Warning: in case review process reveals issues in master patch which require rework after stable patches are merged, it's expected that additional changes are merged into stable branches to avoid unneeded difference between branches. So use the exception with due care.)" Hopefully, it's clear enough. If not, please update. [1]: https://wiki.openstack.org/wiki/StableBranch#Appropriate_Fixes /Ihar -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.22 (Darwin) iQEcBAEBCgAGBQJT8yvlAAoJEC5aWaUY1u57CGgIAJ+tt9Jw442KGInQpUyj/mOn Pj1aPoO4w7l/87xstWIbUTAOr81+R+RkX9Ggxclk8d7nmhdMQImzP+Bj1UktBDLb MlncyDln/xN3QMd17+EME+XP3aaLptSWlabywdl3/I4SbXg7+JbytTvLlwxECECv lPPccMuc6dv731QHAAaSE4aL00dQJuT/eRYgFmXoSzX6fCCQbwt9LC/wAYeguhhE iYtMAiqwp4kO8ai6DQCJaRemm9XCutcNjpK/ZkJOzA0DP+SOhgil21cCd83Cloru Ejs2lzHTQ7R3zytcMhauxHzvRM9H+cY1HZuTRFDs8CShOrdkBsKWS68VhepHZZg= =YKAp -----END PGP SIGNATURE-----
Ihar Hrachyshka wrote:
I've updated this specific rule in [1] with the following:
"(Note: some patches may get exception from this specific rule. These are patches that do not touch production code, like test-only patches, or tox.ini changes that fix major gate breakage, etc.; or security patches that should not take much time to merge once the patches are published. In those cases, stable patches may be pushed into gate without waiting for all consequent branches to be fixed. Warning: in case review process reveals issues in master patch which require rework after stable patches are merged, it's expected that additional changes are merged into stable branches to avoid unneeded difference between branches. So use the exception with due care.)"
Hopefully, it's clear enough. If not, please update.
It *is* clear. Thanks for adding that up! -- Thierry Carrez (ttx)
participants (2)
-
Ihar Hrachyshka
-
Thierry Carrez