Open Stack

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.
>