Open Stack

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


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

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