Adding new config parameter that fixes a bug
Hi guys, Small question about [0]. The patch fixes a bug wrt VMs live migration but it also adds a new configuration flag to allow admins to disable multi-attach. IMHO, adding a new configuration flag shouldn't be allowed in a backport but the patch in matter fixes a bug. What would the right behavior be in that case? 1) Modify the patch so that it doesn't adds a new configuration parameter. 2) Let it land as-is. 3) Don't let it land at all. I'm more for #1 and #3 (maybe more 1 since it fixes a bug). Notice that adding that parameter doesn't change current behavior. Cheers, FF [0] https://review.openstack.org/#/c/29488/1 -- { name: "Flavio Percoco", gpg: "87112EC1", internal: "8261386", phone: "+390687502386", irc: ["fpercoco", "flaper87"]}
On 05/21/2013 05:16 AM, Flavio Percoco wrote:
Hi guys,
Small question about [0].
The patch fixes a bug wrt VMs live migration but it also adds a new configuration flag to allow admins to disable multi-attach. IMHO, adding a new configuration flag shouldn't be allowed in a backport but the patch in matter fixes a bug. What would the right behavior be in that case?
1) Modify the patch so that it doesn't adds a new configuration parameter.
2) Let it land as-is.
3) Don't let it land at all.
I'm more for #1 and #3 (maybe more 1 since it fixes a bug). Notice that adding that parameter doesn't change current behavior.
Cheers, FF
Hey Flavio-- I'd suggest #1 or #3. I would personally prefer not to manually backport patches unless the severity of the bugs they are fixed are critical. There were some cinder patches proposed for 2013.1.1 that similarly included a bunch of unrelated stuff with a pretty minimal fix. Ideally, the merge review into master would dictate that the commit be broken up into smaller pieces as a best practice, and that will make it easier to land to stable branches if needed. I'm curious to know how others feel about when its appropriate to create new commits that backport subsets of master commits vs. straight cherry-picking as a default. As for the review you cited, it looks like the bug has neither a grizzly-backport-potential tag or severity set on the bug. I'd check with a core dev to see if thats something that should be considered for backport. Current policy on the wiki [1] states changes that introduce "Incompatible config file changes" are forbidden from the stable branches. IMHO, adding a new config flag that is both documented and comes with a sane default that does not change existing behavior is okay (what do others think?) That said, its unclear whether that flag thats been added in 294881 is required to fix the bug or unrelated. Adam [1] https://wiki.openstack.org/wiki/StableBranch#Appropriate_Fixes
On 21/05/13 12:05 -0700, Adam Gandelman wrote:
On 05/21/2013 05:16 AM, Flavio Percoco wrote:
Hi guys,
Small question about [0].
The patch fixes a bug wrt VMs live migration but it also adds a new configuration flag to allow admins to disable multi-attach. IMHO, adding a new configuration flag shouldn't be allowed in a backport but the patch in matter fixes a bug. What would the right behavior be in that case?
1) Modify the patch so that it doesn't adds a new configuration parameter.
2) Let it land as-is.
3) Don't let it land at all.
I'm more for #1 and #3 (maybe more 1 since it fixes a bug). Notice that adding that parameter doesn't change current behavior.
I'd suggest #1 or #3. I would personally prefer not to manually backport patches unless the severity of the bugs they are fixed are critical. There were some cinder patches proposed for 2013.1.1 that similarly included a bunch of unrelated stuff with a pretty minimal fix. Ideally, the merge review into master would dictate that the commit be broken up into smaller pieces as a best practice, and that will make it easier to land to stable branches if needed. I'm curious to know how others feel about when its appropriate to create new commits that backport subsets of master commits vs. straight cherry-picking as a default.
I'm not a fan of modifying backports manually either, I guess we should figure out the trade-off between modifying it or letting unreleated stuff land.
As for the review you cited, it looks like the bug has neither a grizzly-backport-potential tag or severity set on the bug. I'd check with a core dev to see if thats something that should be considered for backport. Current policy on the wiki [1] states changes that introduce "Incompatible config file changes" are forbidden from the stable branches. IMHO, adding a new config flag that is both documented and comes with a sane default that does not change existing behavior is okay (what do others think?) That said, its unclear whether that flag thats been added in 294881 is required to fix the bug or unrelated.
The flag is totally unreleated and actually adds a new "feature", which is the possibility for admins to forbid multi-attach. However, the flag has a sane default and doesn't change the default behavior but it isn't required to fix the bug. Related to the first paragraph, this seems like one of the cases where letting unrelated stuff land might make sense. Cheers, FF P.S: Also, this stuff should be avoided during master reviews, fixing a bug and adding a flag in the same patch is not good. I'll raise this in the mailing list. -- { name: "Flavio Percoco", gpg: "87112EC1", internal: "8261386", phone: "+390687502386", irc: ["fpercoco", "flaper87"]}
On 05/21/2013 11:02 PM, Flavio Percoco wrote:
On 21/05/13 12:05 -0700, Adam Gandelman wrote:
On 05/21/2013 05:16 AM, Flavio Percoco wrote:
Hi guys,
Small question about [0].
The patch fixes a bug wrt VMs live migration but it also adds a new configuration flag to allow admins to disable multi-attach. IMHO, adding a new configuration flag shouldn't be allowed in a backport but the patch in matter fixes a bug. What would the right behavior be in that case?
I have not looked at the specific case. If the backport is backward compatible then why should it make a difference. That is, the default value should reflect the current state. In some cases a configuration variable is required for certain bug fixes. What am I missing?
1) Modify the patch so that it doesn't adds a new configuration parameter.
2) Let it land as-is.
3) Don't let it land at all.
I'm more for #1 and #3 (maybe more 1 since it fixes a bug). Notice that adding that parameter doesn't change current behavior.
I'd suggest #1 or #3. I would personally prefer not to manually backport patches unless the severity of the bugs they are fixed are critical. There were some cinder patches proposed for 2013.1.1 that similarly included a bunch of unrelated stuff with a pretty minimal fix. Ideally, the merge review into master would dictate that the commit be broken up into smaller pieces as a best practice, and that will make it easier to land to stable branches if needed. I'm curious to know how others feel about when its appropriate to create new commits that backport subsets of master commits vs. straight cherry-picking as a default.
I'm not a fan of modifying backports manually either, I guess we should figure out the trade-off between modifying it or letting unreleated stuff land.
As for the review you cited, it looks like the bug has neither a grizzly-backport-potential tag or severity set on the bug. I'd check with a core dev to see if thats something that should be considered for backport. Current policy on the wiki [1] states changes that introduce "Incompatible config file changes" are forbidden from the stable branches. IMHO, adding a new config flag that is both documented and comes with a sane default that does not change existing behavior is okay (what do others think?) That said, its unclear whether that flag thats been added in 294881 is required to fix the bug or unrelated.
The flag is totally unreleated and actually adds a new "feature", which is the possibility for admins to forbid multi-attach. However, the flag has a sane default and doesn't change the default behavior but it isn't required to fix the bug.
Related to the first paragraph, this seems like one of the cases where letting unrelated stuff land might make sense.
Cheers, FF
P.S: Also, this stuff should be avoided during master reviews, fixing a bug and adding a flag in the same patch is not good. I'll raise this in the mailing list.
2013/5/22 Gary Kotton <gkotton@redhat.com>:
I have not looked at the specific case. If the backport is backward compatible then why should it make a difference. That is, the default value should reflect the current state. In some cases a configuration variable is required for certain bug fixes.
Agreed, adding config parameter on a stable branch would be allowed if default stays the same and required for the bugfix, but in this case it is not required.
What am I missing?
This:
The flag is totally unreleated and actually adds a new "feature", which is the possibility for admins to forbid multi-attach. However, the flag has a sane default and doesn't change the default behavior but it isn't required to fix the bug.
Cheers, Alan
On 05/22/2013 10:35 AM, Alan Pevec wrote:
2013/5/22 Gary Kotton<gkotton@redhat.com>:
I have not looked at the specific case. If the backport is backward compatible then why should it make a difference. That is, the default value should reflect the current state. In some cases a configuration variable is required for certain bug fixes. Agreed, adding config parameter on a stable branch would be allowed if default stays the same and required for the bugfix, but in this case it is not required.
What am I missing? This:
The flag is totally unreleated and actually adds a new "feature", which is the possibility for admins to forbid multi-attach. However, the flag has a sane default and doesn't change the default behavior but it isn't required to fix the bug.
:). Guess I am the village idiot today.
Cheers, Alan
Guess I am the village idiot today.
Maybe I am, now I had a closer look and have some questions. I have put -2 on https://review.openstack.org/29488 until it's clarified: * why is this an issue only in a particular driver? Do all other storage arrays allow multihost by default? * new config parameters are fine on stable if they preserve the current behaviour. In this case current behaviour is considered buggy and changed by default, but I'm not sure how is ensured that multihost attach is safe? Is there any locking preventing the same volume being used simultaneously from two nodes? * IMHO on stable we should be conservative and keep the current safer behaviour i.e. storwize_svc_multihostmap_enabled=False by default with an appropriate warning in the parameter description, so that users can change it but are also fully aware of the implications. It also requires an entry in the release notes. Cheers, Alan
participants (4)
-
Adam Gandelman
-
Alan Pevec
-
Flavio Percoco
-
Gary Kotton