Open Stack

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.

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