Re: [openstack-dev] [stable][octavia] Backport patch adding new configuration options
On Mon, Oct 8, 2018 at 12:34 PM Matt Riedemann wrote: > > On 10/8/2018 11:05 AM, Carlos Goncalves wrote: > > The Octavia team merged a patch in master [1] that fixed an issue where > > load balancers could be deleted whenever queue_event_streamer driver is > > enabled and RabbitMQ goes down [2]. > > > > As this is a critical bug, we would like to backport as much back as > > possible. The question is whether these backports comply with the stable > > policy because it adds two new configuration options and deprecates one. > > The patch was prepared so that the deprecated option has precedence if > > set over the other two. > > > > Reading the review guidelines [3], I only see "Incompatible config file > > changes" as relevant, but the patch doesn't seem to go against that. We > > had a patch that added a new config option backported to Queens that > > raised some concern, so we'd like to be on the safe side this time ;-) > > > > We'd appreciate guidance to whether such backports are acceptable or not. > > > > Well, a few things: > > * I would have introduced the new config options as part of the bug fix > but *not* deprecated the existing option in the same change but rather > as a follow up. Then the new options, which do nothing by default (?), > could be backported and the deprecation would remain on master. > > * The release note mentions the new options as a feature, but that's not > really correct is it? They are for fixing a bug, not new feature > functionality as much. > > In general, as long as the new options don't introduce new behavior by > default for existing configuration (as you said, the existing option > takes precedence if set), and don't require configuration then it should > be OK to backport those new options. But the sticky parts here are (1) > deprecating an option on stable (we shouldn't do that) and (2) the > release note mentioning a feature. I would classify this as a critical bug fix. I think it's important to fix the bug on stable branches, even for deployments that will get the fix but not change their configuration options. How that's done with respect to configuration options & backports is another matter, but I do think that whatever approach is chosen should end up with the bug fixed on stable branches without requiring operators to use new options or otherwise make changes to their existing configuration files. > > What I'd probably do is (1) change the 'feature' release note to a > 'fixes' release note on master and then (2) backport the change but (a) > drop the deprecation and (b) fix the release note in the backport to not > call out a feature (since it's not a feature I don't think?) - and just > make it clear with a note in the backport commit message why the > backport is different from the original change. > > -- > > Thanks, > > Matt > > __ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [stable][octavia] Backport patch adding new configuration options
On 10/8/2018 11:05 AM, Carlos Goncalves wrote: The Octavia team merged a patch in master [1] that fixed an issue where load balancers could be deleted whenever queue_event_streamer driver is enabled and RabbitMQ goes down [2]. As this is a critical bug, we would like to backport as much back as possible. The question is whether these backports comply with the stable policy because it adds two new configuration options and deprecates one. The patch was prepared so that the deprecated option has precedence if set over the other two. Reading the review guidelines [3], I only see "Incompatible config file changes" as relevant, but the patch doesn't seem to go against that. We had a patch that added a new config option backported to Queens that raised some concern, so we'd like to be on the safe side this time ;-) We'd appreciate guidance to whether such backports are acceptable or not. Well, a few things: * I would have introduced the new config options as part of the bug fix but *not* deprecated the existing option in the same change but rather as a follow up. Then the new options, which do nothing by default (?), could be backported and the deprecation would remain on master. * The release note mentions the new options as a feature, but that's not really correct is it? They are for fixing a bug, not new feature functionality as much. In general, as long as the new options don't introduce new behavior by default for existing configuration (as you said, the existing option takes precedence if set), and don't require configuration then it should be OK to backport those new options. But the sticky parts here are (1) deprecating an option on stable (we shouldn't do that) and (2) the release note mentioning a feature. What I'd probably do is (1) change the 'feature' release note to a 'fixes' release note on master and then (2) backport the change but (a) drop the deprecation and (b) fix the release note in the backport to not call out a feature (since it's not a feature I don't think?) - and just make it clear with a note in the backport commit message why the backport is different from the original change. -- Thanks, Matt __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
[openstack-dev] [stable][octavia] Backport patch adding new configuration options
Stable team, The Octavia team merged a patch in master [1] that fixed an issue where load balancers could be deleted whenever queue_event_streamer driver is enabled and RabbitMQ goes down [2]. As this is a critical bug, we would like to backport as much back as possible. The question is whether these backports comply with the stable policy because it adds two new configuration options and deprecates one. The patch was prepared so that the deprecated option has precedence if set over the other two. Reading the review guidelines [3], I only see "Incompatible config file changes" as relevant, but the patch doesn't seem to go against that. We had a patch that added a new config option backported to Queens that raised some concern, so we'd like to be on the safe side this time ;-) We'd appreciate guidance to whether such backports are acceptable or not. Thanks, Carlos [1] https://review.openstack.org/#/c/581585/ [2] https://storyboard.openstack.org/#!/story/2002937 [3] https://docs.openstack.org/project-team-guide/stable-branches.html#review-guidelines [4] https://review.openstack.org/#/c/593954/ __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
