[dev][stable][release][manila] Should we revert these stable branch backports?
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. 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. 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. 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.
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. 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.
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?
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.
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.
On 31/03/19 21:15 -0700, Goutham Pacha Ravi wrote:
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.
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.
On Mon, Apr 01, 2019 at 06:35:44AM -0400, Tom Barron wrote:
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.
That brings up an interesting point and one which is largely tangental to the stable backport issue but AFAICT the 'deprecated' names are still called out as the correct way to configure the driver. At least in the case of the Unity diver: https://docs.openstack.org/manila/latest/admin/emc_unity_driver.html#configu... That could be fixed and backported to stein. I sort of think that sets the timer to 'U' but that certainly isn't supported by our policy and is a project team decision. Yours Tony.
On 12/04/19 12:50 +1000, Tony Breeds wrote:
On Mon, Apr 01, 2019 at 06:35:44AM -0400, Tom Barron wrote:
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.
That brings up an interesting point and one which is largely tangental to the stable backport issue but AFAICT the 'deprecated' names are still called out as the correct way to configure the driver. At least in the case of the Unity diver:
https://docs.openstack.org/manila/latest/admin/emc_unity_driver.html#configu...
That could be fixed and backported to stein. I sort of think that sets the timer to 'U' but that certainly isn't supported by our policy and is a project team decision.
Yours Tony.
Thanks for this catch, Tony. Helen, you've been making a few EMC driver documentation fixes recently, so would you be so kind as to submit a review for this one as well? Also, is there any problem setting the timer for removal to 'U' instead of Train as Tony suggests? Thanks, -- Tom
On Wed, Mar 27, 2019 at 08:56:54AM -0400, Tom Barron 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.
Thanks for writing this all up Tom. I'm sorry it's taken me so long to get to it.
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.
So someone that is using the VMAX driver from 5.0.3 has set emc_interface_ports in the config. Assuming we don't revert and release 5.0.4[1], that same config file will continue to work but there will now be a deprecation warning in the logs. The VMAX user probably had to look at the code to workout that the config sample was out of sync with the code? Yours Tony. [1] or probably 5.1.0 as a signal to operators
On 12/04/19 12:47 +1000, Tony Breeds wrote:
On Wed, Mar 27, 2019 at 08:56:54AM -0400, Tom Barron 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.
Thanks for writing this all up Tom. I'm sorry it's taken me so long to get to it.
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.
So someone that is using the VMAX driver from 5.0.3 has set emc_interface_ports in the config. Assuming we don't revert and release 5.0.4[1], that same config file will continue to work but there will now be a deprecation warning in the logs.
The VMAX user probably had to look at the code to workout that the config sample was out of sync with the code?
Right, or some support person told them some magic.
Yours Tony.
[1] or probably 5.1.0 as a signal to operators
Agree that 5.1.0 makes sense, thanks.
On Fri, Apr 12, 2019 at 09:52:35AM -0400, Tom Barron wrote:
On 12/04/19 12:47 +1000, Tony Breeds wrote:
So someone that is using the VMAX driver from 5.0.3 has set emc_interface_ports in the config. Assuming we don't revert and release 5.0.4[1], that same config file will continue to work but there will now be a deprecation warning in the logs.
The VMAX user probably had to look at the code to workout that the config sample was out of sync with the code?
Right, or some support person told them some magic.
Yours Tony.
[1] or probably 5.1.0 as a signal to operators
Agree that 5.1.0 makes sense, thanks.
Thanks Tom. I see that you've made that change already so I'll approve https://review.openstack.org/#/c/650250/ Yours Tony.
participants (3)
-
Goutham Pacha Ravi
-
Tom Barron
-
Tony Breeds