Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option ignoreVolumeAZ to allow for differences between volume and compute AZ names #322

Merged
merged 5 commits into from
Aug 17, 2021
Merged

Add option ignoreVolumeAZ to allow for differences between volume and compute AZ names #322

merged 5 commits into from
Aug 17, 2021

Conversation

lotharbach
Copy link
Contributor

Add option ignoreVolumeAZ to allow for differences between volume and compute AZ names

How to categorize this PR?
/area control-plane
/area storage
/kind enhancement
/platform openstack

What this PR does / why we need it:
Introduce CloudProfile option ignoreVolumeAZ which directly translates to option ignore-volume-az in [BlockStorage] section of csi config.

This option and where it is needed is described upstream as:

ignore-volume-az Optional. When Topology feature enabled, by default, PV volume node affinity is populated with volume accessible topology, which is volume AZ. But, some of the openstack users do not have compute zones named exactly the same as volume zones. This might cause pods to go in pending state as no nodes available in volume AZ. Enabling ignore-volume-az=true, ignores volumeAZ and schedules on any of the available node AZ. Default false.

https://github.com/kubernetes/cloud-provider-openstack/blob/master/docs/cinder-csi-plugin/using-cinder-csi-plugin.md#block-storage

This enabled us to use gardener with an openstack that uses compute zones like abc1-a, abc1-b, abc1-c but the volume zone is always abc1.

Release note:

Add option ignoreVolumeAZ to allow for differences between volume and compute AZ names.

@lotharbach lotharbach requested review from a team as code owners July 30, 2021 10:34
@gardener-robot-ci-2
Copy link
Contributor

Thank you @gesslein for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@gardener-robot gardener-robot added kind/api-change API change with impact on API users needs/second-opinion Needs second review by someone else area/control-plane Control plane related area/storage Storage related kind/enhancement Enhancement, improvement, extension platform/openstack OpenStack platform/infrastructure labels Jul 30, 2021
@gardener-robot
Copy link

@gesslein Thank you for your contribution.

@gardener-robot gardener-robot added needs/review Needs review size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Jul 30, 2021
@dkistner
Copy link
Member

dkistner commented Jul 30, 2021

Hi @gesslein,

thanks for the contribution.
I had a first high level scan and looks so far good to me.
Will have another more detailed one beginning next week.

Just one ask. Is there any version constraint regarding this feature in the cinder-csi-driver?
We have still have v1.19.0 in use and it seems to be introduced with v1.20.0. https://github.com/kubernetes/cloud-provider-openstack/releases/tag/v1.20.0

/needs second-opinion
/invite @kon-angelo

@lotharbach
Copy link
Contributor Author

Thanks for the quick feedback.

You are right, I did not think about older versions not having that option. I did now test with a k8s 1.19.x and respectively cinder-csi-plugin v1.19.0, and it did indeed not work. But the option did no harm either.

For k8s 1.18 it is a bit puzzling, because the option does exist in the legacy cloud provider https://v1-18.docs.kubernetes.io/docs/concepts/cluster-administration/cloud-providers/#block-storage but works differently there. I just did a quick hack to add the option to all cloudproviders.conf's and was able to schedule pods with volumes then.

To make it not so confusing, and I'm not sure 1.18 is of much relevance for new gardener adopters, I think only adding this for 1.20 and newer is the way to go. And I hope make it as clear as possible in the documentation that only 1.20.x and newer supports this.

@gardener-robot
Copy link

@kon-angelo You have pull request review open invite, please check

Copy link
Member

@dkistner dkistner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ho @gesslein
sorry for the delay.
Thanks for testing the with v1.19/v1.18 and for adding the version switch in the csi config.
I agree adopting it with v1.20 sound good to me.

I'm now fine with it.
/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review needs/second-opinion Needs second review by someone else labels Aug 16, 2021
@dkistner
Copy link
Member

/squash

@gardener-robot gardener-robot added the merge/squash Should be merged via 'Squash and merge' label Aug 16, 2021
Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@rfranzke rfranzke added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 16, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 16, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added the needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Aug 16, 2021
@dkistner dkistner merged commit 921072c into gardener:master Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane Control plane related area/storage Storage related kind/api-change API change with impact on API users kind/enhancement Enhancement, improvement, extension merge/squash Should be merged via 'Squash and merge' needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/openstack OpenStack platform/infrastructure reviewed/lgtm Has approval for merging size/s Size of pull request is small (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants