[dev][stable][release][manila] Should we revert these stable branch backports?

Tom Barron tpb at dyncloud.net
Mon Apr 1 10:35:44 UTC 2019


On 31/03/19 21:15 -0700, Goutham Pacha Ravi wrote:
>On Sat, Mar 30, 2019 at 1:40 PM Tom Barron <tpb at dyncloud.net> wrote:
>>
>> On 27/03/19 11:26 -0700, Goutham Pacha Ravi wrote:
>> >On Wed, Mar 27, 2019 at 5:58 AM Tom Barron <tpb at dyncloud.net> wrote:
>> >>
>> >> In manila we recently merged backports of a change [1] that aims
>> >> to fix up faulty configuration option deprecations in the
>> >> Dell-EMC VMAX driver.  The question at hand is whether (a) we
>> >> should revert these backports on the grounds that the
>> >> deprecations were faulty and therefore are only effective from
>> >> Stein forwards, or whether (b) the deprecations actually worked
>> >> and took effect back when Ocata was the development branch but
>> >> had faults that should be corrected all the way back to Pike
>> >> before it goes EM.
>> >>
>> >> I think (b) is the correct answer but let's check.
>> >>
>> >> On Jan 10 2017 a review [2] merged that deprecated generic
>> >> Dell-EMC driver options in favor of model-specific options [2].
>> >> There were two models at the time, Unity and VNX.  This change
>> >> was in itself unproblematic.
>> >>
>> >> On Jan 24 2017 a review [3] merged that introduced a third
>> >> Dell-EMC model, VMAX.  This new code introduced VMAX specific
>> >> options, consistent with the deprecation of the generic Dell-EMC
>> >> options.  However it had two problems which review [1] corrects.
>> >> When it defined the new VMAX-specific options it failed to
>> >> indicate the corresponding old generic options via
>> >> 'deprecated_name' [4].  Worse, the code that consumed the options
>> >> actually looked for the old generic 'emc_interface_ports' option
>> >> instead of the new 'vmax_ethernet_ports' option.  The only way to
>> >> set a value for this option was to use its deprecated
>> >> form.
>> >>
>> >> The change that we backported fixes both of these problems.
>> >>
>> >> I think it is a valid stable backport.
>> >
>> >
>> >I don't agree with some parts of the change. However, I didn't review
>> >this patch in time, so my opinion is late, and possibly annoying now.
>> >A couple of things with this change are weird, and we could decide to
>> >fix these:
>> >
>> >1) The EMC VMAX driver never had "emc_nas_server_container" or
>> >"emc_nas_pool_names" as config options, but the bug fix [1], adds
>> >these as deprecated names for valid options (vmax_server_container,
>> >vmax_share_data_pools), in a retrospective manner.
>>
>> True, they are added as synonyms for the corresponing model-specific
>> options, and are marked as deprecated.  This aligns the VMAX config
>> with the other EMC models.
>>
>> >2) The EMC VMAX driver always used "emc_interface_ports", however,
>> >there was no such configuration option wrt that backend. How would
>> >users know to set it? Things passed because there was a "safe_get"
>> >operation, and perhaps the vendor had called this out in their docs?
>> >It's good they fixed this part in Stein.
>>
>> There is no question that the VMAX driver code was broken.
>>
>> >
>> >
>> >> If people set this "ports" option using the deprecated option it
>> >> will still work.  The deprecated option has not been removed yet
>> >> and will never be removed from any of the stable branches.
>> >>
>> >> If they tried to set it via the proper option then they would have
>> >> had a functional problem, a bug, which this change now fixes.
>> >
>> >Agree with this point.
>> >
>> >> If on the other hand we say we cannot backport this then until
>> >> stable/stein there will be no way for VMAX users of earlier stable
>> >> branches to set this option except by magic -- by using an old
>> >> deprecated form of the option that without the backported fix is not
>> >> even visible in the customer-facing configuration for VMAX.
>> >> All this said, I could well be missing something so I welcome
>> >> analysis by stable/core folks.  The backports in question have not
>> >> yet been released so it would not be a problem to revert them.
>> >
>> >I think it would be worth back porting the deprecation and association
>> >between "emc_interface_ports" and "vmax_ethernet_ports" in the
>> >interest of customers/users. However, I think [1] introduces another
>> >bug by exposing net new options for this driver
>> >("emc_nas_server_container" and "emc_nas_pool_names") as deprecated
>> >forms of existing options ("vmax_server_container",
>> >"vmax_share_data_pools"), and that is concerning.
>> >
>>
>> What is the new bug that this change introduces?  Won't the net new
>> options work and isn't the configuration backwards compatible in the
>> sense that there are no options that used to work that will not work
>> now?
>
>The bug is introducing deprecated forms of the options, and back
>porting that to older releases. Perhaps there's some business/customer
>understanding reasoning I don't know of. It can be termed harmless,
>because the original and legitimate options will continue to work.

Hmm, that doesn't seem to me to be an error, flaw, failure or fault 
that causes the program or system to produce an incorrect or 
unexpected result, or to behave in unintended ways.  The change in 
question gets this VMAX model to *work*, and to work in a way that is 
consistent with config options and user expectations for the other EMC 
models.

Without introducing a bug a backport could still be a violation of 
stable/branch review guidelines [1] but that doesn't seem to be an 
issue either. This backport doesn't introduce a new feature, change 
HTTP APIs, change the AMQP API, change notification definitions, make 
DB schema changes, or make incompatible config file changes.

At the risk of stating the obvious, but for the record, this backport 
doesn't change *when* these deprecations occurred for VMAX.  They 
occurred in Stein development cycle, not during the Ocata development 
cycle, and the earliest the deprecated options could be removed is 
Train.

[1] https://docs.openstack.org/project-team-guide/stable-branches.html

>
>>
>> >> Thanks!
>> >>
>> >> -- Tom Barron
>> >>
>> >> [1] https://review.openstack.org/#/c/608725/
>> >>
>> >> [2] https://review.openstack.org/#/c/415079/
>> >>
>> >> [3] https://review.openstack.org/#/c/404859/
>> >>
>> >> [4] If this was the only issue then arguably it wasn't an error
>> >> since the VMAX driver never operated with the old options.
>> >>



More information about the openstack-discuss mailing list