Open Stack

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"]}