use "cluster" config instead of same "host" config value

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/charm-cinder/+/811472

Reviewed: https://review.opendev.org/c/openstack/charm-cinder/+/811472
Committed: https://opendev.org/openstack/charm-cinder/commit/2603a774b112dc6e25f9e7b2071124ea3d3d0012
Submitter: "Zuul (22348)"
Branch: master

commit 2603a774b112dc6e25f9e7b2071124ea3d3d0012
Author: Rodrigo Barbieri <email address hidden>
Date: Tue Sep 28 15:37:15 2021 -0300

    Group cinder-volume services using cluster config

    Usage of "host" config for grouping cinder-volume
    services for the purpose of stateless config and/or
    active-active HA is not recommended.

    The usage of "cluster" config is recommended for
    achieving that behavior. This change replaces the
    usage of "host" with "cluster".

    Change-Id: I249ce179cfdaf2394ee4e8481a8c9644d667ea99
    Closes-bug: #1945239

This is actually too drastic change from a charm ecosystem point of view and will break a backward compatibility although the patch itself is in a good intention and nothing is wrong from an upstream config point of view.

I'd really like to see the follow-up change gets merged before the change gets into stable as a part of 21.10 release.
https://review.opendev.org/c/openstack/charm-cinder/+/814845

Hi Nobuto, for pure-storage they should report stateless=False for releases they don't support AA HA. See discussion at https://review.opendev.org/c/openstack/charm-cinder/+/814845

> Hi Nobuto, for pure-storage they should report stateless=False for releases they don't support AA HA. See discussion at https://review.opendev.org/c/openstack/charm-cinder/+/814845

I get your point and indeed "stateless=True" was misused for some time. But the problems here are:

1. storage backend setup by a subordinate charm (previously stateless=True) will stop working just after upgrading the cinder charm with "Volume driver reported an error: Active-Active configuration is not currently supported by driver". Thus I stated as a backward compatibility even if the original value was misused.

This needs to be addressed in each charm in a coordinated way (we can't release cinder first without waiting for *all* of the supported charms report their support status correctly, and the coordinate upgrade must be covered in the release notes in my opinion.

2. Have you considered the multi-backend scenarios and mixture of stateless and statefull backend? Let's say focal-ussuri with Ceph(rbd) and Purestorage.

With the new behavior and if charm-purestorage reports stateless=False, what happens to volume service status of Ceph? Previously host= was in cinder.conf but with the new behavior, neither of host= or cluster= will be in cinder.conf while ceph expects one of those?

I may be too scared of the change probably but wanted to raise my concern here.

Hi Nobuto,

for (2), backends with different clustering config should be grouped in different cinder charm apps. For example:

charm: cinder, app: cinder-api
charm: cinder, app: cinder-volume-aa-drivers
charm: cinder-ceph, subordinate of cinder-volume-aa-drivers
charm: cinder, app: cinder-volume-ap-drivers
charm: cinder-purestorage, subordinate of cinder-volume-ap-drivers
charm: cinder-netapp, subordinate of cinder-volume-ap-drivers

"ap" above stands for "active-passive". "aa" for "active-active"

I have seen an environment configured as above without issues.

for (1), you are correct, and this was discussed when I was implementing the change [0], however, we didn't consider backwards compatibillity because the only cinder driver that came to our minds and would not be affected was the cinder-netapp one. We missed the cinder-purestorage one that was already existing. Let's enumerate the scenarios:

a) pure-storage deployment >= victoria: this will be changed to use cluster instead of host but will remain in clustered mode without errors, as the driver >= victoria supports AA HA

b) pure-storage deployment < victoria: this will be changed to stateless=false and therefore the environment will be affected. Ideally, since the environment is running in an unsupported config, the operators should try to migrate to a non-clustered mode and active-passive set up. The workaround for backwards compatibility that I can see here is a charm config option (on the cinder-purestorage sub-charm) to override the stateless config to address this case.

@Freyes is working on a patch as part [1] and I believe that workaround could be included.

[0] https://review.opendev.org/c/openstack/charm-cinder/+/811472
[1] https://bugs.launchpad.net/charm-cinder-purestorage/+bug/1947702

Did we actually test purestorage for example with stateless=False including
a fail-over scenarios?

cinder-volume units associated with purestorage doesn't have any state
actually although it's not call out active-active supported explicitly
until Victoria. If we deploy it with focal-ussuri, and create a volume with
cinder-volume-unit-a (which is chosen by cinder scheduler), then unit-a is
down out of 3 units, can we operate the volume still?

I thought the specific host name of unit-a was recorded in cinder db if
there is no common host= option.

2021年10月21日(木) 23:35 Rodrigo Barbieri <email address hidden>:

> Hi Nobuto,
>
> for (2), backends with different clustering config should be grouped in
> different cinder charm apps. For example:
>
> charm: cinder, app: cinder-api
> charm: cinder, app: cinder-volume-aa-drivers
> charm: cinder-ceph, subordinate of cinder-volume-aa-drivers
> charm: cinder, app: cinder-volume-ap-drivers
> charm: cinder-purestorage, subordinate of cinder-volume-ap-drivers
> charm: cinder-netapp, subordinate of cinder-volume-ap-drivers
>
> "ap" above stands for "active-passive". "aa" for "active-active"
>
> I have seen an environment configured as above without issues.

Is the requirement called out explicitly somewhere? It looks like another
drawback from the previous behavior.

It might be a corner case but FC backend requires physical hosts and if we
want to have two backend respectively stateless=True and False, do we need
4 physical hosts at least just to run cinder volume services (2 for one
backend for redundancy + 2 for another backend)? That may break a design of
existing deployments probably.

Hi Nobuto,

We didn't test scenarios where the flag flips. We don't want this to happen. Deployments that are running purestorage with host=<same_value> shouldn't be running it this way, as per upstream recommendations. We have not investigated steps to migrate an env that was previously deployed with host=<same_value> to host=<diff_value>.

I'd assume purestorage upstream Cinder CI would have tested Active-passive failover, given that they didn't support AA HA until Victoria. For the charm, I believe we don't have functional tests covering any type failure scenario, AA or AP.

The deployment recommendations of separate cinder charm apps for AA and AP hasn't been documented yet in our charm guides. But yes, your example is correct, you will need 4 nodes for that case because we cannot mix stateless=False and True in the same charm app.

Please let us know more details of the instances where it will impact existing envs so we can work that out.

On Thu, 2021-10-21 at 15:24 +0000, Rodrigo Barbieri wrote:
> Hi Nobuto,
>
> We didn't test scenarios where the flag flips. We don't want this to
> happen. Deployments that are running purestorage with
> host=<same_value>
> shouldn't be running it this way, as per upstream recommendations.

For OpenStack <=Ussuri,>=Pike we should be using the host=<value> for
purestorage (the behavior until 21.04), because that driver didn't
enable AA-HA before Victoria, that's why it's getting into error state.

that's why I like your approach in https://review.opendev.org/c/openstack/charm-cinder/+/814845 to have a state where host= is enabled not cluster= by having a separate flag as active_active_ha in addition to the existing "stateless" flag.

@Freyes: I am not sure I understand your comment, because in my understanding it contradicts it self:

a) host=<value> (or not setting it at all) is for active-passive
b) (the behavior until 21.04) is host=<same_value>.

host=<same_value> is when we want to cluster the cinder-volume services in the way that is not supported/recommended by upstream

If you are saying that we *should* be using host=<same_value> for purestorage <= Ussuri,>=Pike, this is incorrect in case of new deployments. For existing deployments sure something should be done to be able to maintain the existing config (host=<same_value>) so the flag doesn't flip and doesn't cause issues (mentioned below)

@Nobuto: that patch is good workaround to achieve a way to maintain the existing config for purestorage. I will further develop this in the other LP.

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/charm-cinder/+/815087

Reviewed: https://review.opendev.org/c/openstack/charm-cinder/+/815087
Committed: https://opendev.org/openstack/charm-cinder/commit/c5896a884a54d0171415d79a82a284e2ff6b2ca9
Submitter: "Zuul (22348)"
Branch: master

commit c5896a884a54d0171415d79a82a284e2ff6b2ca9
Author: Billy Olsen <email address hidden>
Date: Thu Oct 21 18:32:07 2021 -0700

    Revert "Group cinder-volume services using cluster config"

    This reverts commit 2603a774b112dc6e25f9e7b2071124ea3d3d0012, which
    converted cinder to use cluster configuration for stateless services.
    While well-intentioned, this causes problems due to the lack of a
    proper migration for those backends which are not able to support
    ACTIVE-ACTIVE configuration.

    Configurations that do not support ACTIVE_ACTIVE driver will have the
    volume service fail to start, which will cause a service outage.

    Change-Id: I7bf4baaf80e5bb58b5c1cf55a2065f3bc50dbced
    Related-Bug: #1945239

Related fix proposed to branch: stable/21.10
Review: https://review.opendev.org/c/openstack/charm-cinder/+/814939

Reviewed: https://review.opendev.org/c/openstack/charm-cinder/+/814939
Committed: https://opendev.org/openstack/charm-cinder/commit/47d76283c5c7c121f39c80aa6527b39bd7e6a5dd
Submitter: "Zuul (22348)"
Branch: stable/21.10

commit 47d76283c5c7c121f39c80aa6527b39bd7e6a5dd
Author: Billy Olsen <email address hidden>
Date: Thu Oct 21 18:32:07 2021 -0700

    Revert "Group cinder-volume services using cluster config"

    This reverts commit 2603a774b112dc6e25f9e7b2071124ea3d3d0012, which
    converted cinder to use cluster configuration for stateless services.
    While well-intentioned, this causes problems due to the lack of a
    proper migration for those backends which are not able to support
    ACTIVE-ACTIVE configuration.

    Configurations that do not support ACTIVE_ACTIVE driver will have the
    volume service fail to start, which will cause a service outage.

    Change-Id: I7bf4baaf80e5bb58b5c1cf55a2065f3bc50dbced
    Related-Bug: #1945239
    (cherry picked from commit c5896a884a54d0171415d79a82a284e2ff6b2ca9)

Setting back to 'in progress' since we reverted the patch due to regressions found.