-
Notifications
You must be signed in to change notification settings - Fork 647
[cinder-csi-plugin] Support of one storage class for Multi region/clouds #2843
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
base: master
Are you sure you want to change the base?
[cinder-csi-plugin] Support of one storage class for Multi region/clouds #2843
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @Boston01! |
Hi @Boston01. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
13f0d6c
to
c6a8f80
Compare
Much needed thanks a lot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
|
||
Example of configuration with 3 regions (The default is backward compatible with mono cluster configuration but not mandatory). | ||
Example of configuration with 3 zones (The default is backward compatible with mono cluster configuration but not mandatory). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, I do not like that this PR is now mixing terminology everywhere. If you are using 3 different API endpoints, it usually means that you are using 3 different REGIONS not zones (like PR title says).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zetaab In openstack a region is a zone if you use the general cloud provider definition of a region, a region is composed of multiple zones.
What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Region seems more appropriate here. Regions are effectively separate clouds, sharing only a common Keystone deployment. Does your deployment do this? If not, perhaps Cloud would be a more appropriate term. I don't like using the term Zone, since this overloads the term since it's more commonly used for Availability Zones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hello @stephenfin , i get what you mean, i will refactor this Pull request and replace zone with region, we use different terminology but we're talking the same thing, thank you for your reply. Hi @zetaab i saw that you approve this Pull Request, great thanks, i update as you mention before about the mixing the zones and regions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks. If possible, could you rebase on master also since this is relatively old now and it'd be good to drag in the latest changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @stephenfin do not worry i will do it during this weekend when i have some free time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Indeed i'm affraid that you mixed concepts of AZ/zones and regions/clouds, I was created previous contribution with multi regions/clouds notion, but not AZ.
In my understood of current codebase cinder-csi-plugin should be able ton consume volumes on an openstack cluster with multi-AZ.
But you probably need to have a distributed backend accross your AZ like ceph with cross AZ replication to be able to share a same volume across different zones.
I have no access to a multi AZ openstack cluster for now but if you simply wanna use a single StorageClass which is able to consume volume across different openstack regions/cluster, I can share you this project which fully and simply respond to my needs => https://github.com/sergelogvinov/hybrid-csi-plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MatthieuFin, you're totally right, i mixed concepts of AZ/zones and regions/clouds.
I'm working for a company that have 3 regions/cloud OpenStack and each region have this own pure storage.
When i started working over there, they logic was a zone is equivalent to a region and i did the pull request based on their experience, but i learn more about OpenStack during last period and i discover that my pull request work for them but i mixed up zones and regions.
I going to do a refactoring for this Pull Request in order to make work and robust for Openstack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @stephenfin and @zetaab i did the refacto of the pull request by taking account the region as you suggested. I already redeployed on our staging, preprod and prod environments and it is working fine.
You can have a look on the pull request and let me know if i can improve others stuff.
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
pkg/csi/cinder/driver.go
Outdated
topologyKey = "topology." + driverName + "/zone" | ||
driverName = "cinder.csi.openstack.org" | ||
topologyKey = "topology." + driverName + "/zone" | ||
withTopologyKey = "topology.kubernetes.io/zone" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply use cinder-csi-plugin args --additional-topology=topology.kubernetes.io/zone
instead of hardcoding one ? As explained here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @MatthieuFin your comment is very important i will update when i have free time, thank you again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hello @MatthieuFin, the "--additional-topology" is not available on the controller service, that's why i do not want to use it, if i use it on the controller service i will do a big refactor that may need a lot of changes, that's why i did the minimum of code in order to make this pull request for for your three regions(par1, par2, par3).
d99c979
to
a49d61d
Compare
21e5dcb
to
7a3277e
Compare
7a3277e
to
e051421
Compare
@Boston01: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What this PR does / why we need it: Supporting multiple regions and clouds requires creating additional secrets for the OpenStack cloud provider. This ensures the correct identification of the region from which a requested volume originates, allowing it to be properly attached to an instance within the same region or zone.
To bring OpenStack to the same level as AWS, GCP, and AKS in terms of high availability and seamless storage management with a single storage class, this pull request introduces enhancements that enable OpenStack to handle storage provisioning in a similar manner.
Which issue this PR fixes(if applicable):
fixes #
Special notes for reviewers:
Define a single storage class
Define a statefulset with 3 replicas with an anti-affinity in order to have on pod per zone
List of storage class
List of pods on the spread wide kubernetes with zone par1, par2 and par3
List of persistent volumes
List of persistent volumes claims
Release note: