On Sat, Mar 30, 2019 at 1:40 PM Tom Barron <tpb@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@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.