Skip to content

[cinder-csi-plugin] add leader election#1638

Merged
lingxiankong merged 8 commits into
kubernetes:masterfrom
m-yosefpor:master
Sep 15, 2021
Merged

[cinder-csi-plugin] add leader election#1638
lingxiankong merged 8 commits into
kubernetes:masterfrom
m-yosefpor:master

Conversation

@m-yosefpor

@m-yosefpor m-yosefpor commented Sep 2, 2021 •
edited by lingxiankong
Loading

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Adding support for running HA CSI controller.

Which issue this PR fixes(if applicable):
fixes #702

Special notes for reviewers:

Increase replicas to 3, all operations should work as expected (attach, detach, provision, deprovision, resize, snapshot).

I tested it on k8s 1.20, all operations are completing without errors. Also I can see successful leader election change events:

29m         Normal    LeaderElection      lease/csi-cinderplugin                               csi-cinder-controllerplugin-0 became leader
3m30s       Normal    LeaderElection      lease/csi-cinderplugin                               csi-cinder-controllerplugin-0 became leader
29m         Normal    LeaderElection      lease/external-attacher-leader-csi-cinderplugin      csi-cinder-controllerplugin-0 became leader
3m30s       Normal    LeaderElection      lease/external-attacher-leader-csi-cinderplugin      csi-cinder-controllerplugin-0 became leader
29m         Normal    LeaderElection      lease/external-resizer-csi-cinderplugin              csi-cinder-controllerplugin-0 became leader
3m30s       Normal    LeaderElection      lease/external-resizer-csi-cinderplugin              csi-cinder-controllerplugin-0 became leader
29m         Normal    LeaderElection      lease/external-snapshotter-leader-csi-cinderplugin   csi-cinder-controllerplugin-0 became leader
3m30s       Normal    LeaderElection      lease/external-snapshotter-leader-csi-cinderplugin   csi-cinder-controllerplugin-0 became leader

Cordon all nodes, deletetd csi-cinder-controllerplugin-0 (so it won't be scheduled again), leader changed to csi-cinder-controllerplugin-2 successfully and all CSI operations still working.

Release note:

[cinder-csi-plugin] Add leader election flag (for HA CSI controller) in the manifests and switch from StatefulSet to Deployment. The existing cinder-csi-plugin helm chart should be uninstalled before installing the new version. Action required

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 2, 2021
@k8s-ci-robot

k8s-ci-robot commented Sep 2, 2021

Copy link
Copy Markdown
Contributor

Welcome @m-yosefpor!

It looks like this is your first PR to kubernetes/cloud-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/cloud-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot

k8s-ci-robot commented Sep 2, 2021

Copy link
Copy Markdown
Contributor

Hi @m-yosefpor. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 2, 2021
@m-yosefpor

m-yosefpor commented Sep 2, 2021

Copy link
Copy Markdown
Contributor Author

/assign @ramineni

@theopenlab-ci

theopenlab-ci Bot commented Sep 2, 2021

Copy link
Copy Markdown

Build succeeded.

@theopenlab-ci

theopenlab-ci Bot commented Sep 2, 2021

Copy link
Copy Markdown

Build succeeded.

@m-yosefpor

m-yosefpor commented Sep 2, 2021

Copy link
Copy Markdown
Contributor Author

@ramineni do we need to also have e2e tests for an HA setup?

@bgagnon

bgagnon commented Sep 3, 2021

Copy link
Copy Markdown

I believe the use of StatefulSet with spec.replicas: 1 was to ensure a single replica at all times. With leader election, it can become a Deployment instead.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 4, 2021
@theopenlab-ci

theopenlab-ci Bot commented Sep 4, 2021

Copy link
Copy Markdown

Build failed.

@m-yosefpor

m-yosefpor commented Sep 4, 2021

Copy link
Copy Markdown
Contributor Author

I believe the use of StatefulSet with spec.replicas: 1 was to ensure a single replica at all times. With leader election, it can become a Deployment instead.

Agree. now with leader election, even with replicas: 1 users should be able to have a rolling update strategy with no down time rollouts. Also I switched to HA 3 replicas by default, which is also a sane default value used in other csi helm charts by default (such as ceph-csi)

@theopenlab-ci

theopenlab-ci Bot commented Sep 4, 2021

Copy link
Copy Markdown

Build succeeded.

@jichenjc

jichenjc commented Sep 5, 2021

Copy link
Copy Markdown
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 5, 2021
jichenjc
jichenjc reviewed Sep 6, 2021
View reviewed changes
Comment thread charts/cinder-csi-plugin/templates/controllerplugin-deployment.yaml
Comment thread charts/cinder-csi-plugin/templates/controllerplugin-deployment.yaml
@theopenlab-ci

theopenlab-ci Bot commented Sep 6, 2021

Copy link
Copy Markdown

Build failed.

@theopenlab-ci

theopenlab-ci Bot commented Sep 6, 2021

Copy link
Copy Markdown

Build succeeded.

@theopenlab-ci

theopenlab-ci Bot commented Sep 6, 2021

Copy link
Copy Markdown

Build failed.

@theopenlab-ci

theopenlab-ci Bot commented Sep 7, 2021

Copy link
Copy Markdown

Build failed.

@m-yosefpor

m-yosefpor commented Sep 7, 2021

Copy link
Copy Markdown
Contributor Author

@ramineni all your suggestions applied.

@theopenlab-ci

theopenlab-ci Bot commented Sep 7, 2021

Copy link
Copy Markdown

Build succeeded.

@theopenlab-ci

theopenlab-ci Bot commented Sep 7, 2021

Copy link
Copy Markdown

Build succeeded.

@theopenlab-ci

theopenlab-ci Bot commented Sep 7, 2021

Copy link
Copy Markdown

Build failed.

@theopenlab-ci

theopenlab-ci Bot commented Sep 7, 2021

Copy link
Copy Markdown

Build failed.

@lingxiankong lingxiankong changed the title feat: [cinder-csi-plugin] add leader election [cinder-csi-plugin] add leader election Sep 9, 2021
lingxiankong
lingxiankong suggested changes Sep 9, 2021
View reviewed changes
Comment thread charts/cinder-csi-plugin/Chart.yaml Outdated
@theopenlab-ci

theopenlab-ci Bot commented Sep 10, 2021

Copy link
Copy Markdown

Build failed.

@theopenlab-ci

theopenlab-ci Bot commented Sep 10, 2021

Copy link
Copy Markdown

Build failed.

@theopenlab-ci

theopenlab-ci Bot commented Sep 10, 2021

Copy link
Copy Markdown

Build failed.

@theopenlab-ci

theopenlab-ci Bot commented Sep 10, 2021

Copy link
Copy Markdown

Build succeeded.

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 11, 2021
@lingxiankong

lingxiankong commented Sep 11, 2021

Copy link
Copy Markdown
Contributor

/lgtm

@m-yosefpor I edited the release note, please double check.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2021
@m-yosefpor

m-yosefpor commented Sep 11, 2021

Copy link
Copy Markdown
Contributor Author

/lgtm

@m-yosefpor I edited the release note, please double check.

SGTM. Thanks.

@ramineni

ramineni commented Sep 14, 2021

Copy link
Copy Markdown
Contributor

@m-yosefpor Thanks. Looks good.

/lgtm

@ramineni

ramineni commented Sep 14, 2021

Copy link
Copy Markdown
Contributor

/approve

@k8s-ci-robot

k8s-ci-robot commented Sep 14, 2021

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ramineni

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2021
@lingxiankong lingxiankong merged commit fd836e9 into kubernetes:master Sep 15, 2021
@m-yosefpor

m-yosefpor commented Sep 18, 2021

Copy link
Copy Markdown
Contributor Author

@lingxiankong any ETA on when the next release for openstack-cinder-csi helm chart will be?

@lingxiankong

lingxiankong commented Sep 19, 2021

Copy link
Copy Markdown
Contributor

@lingxiankong any ETA on when the next release for openstack-cinder-csi helm chart will be?

Hi @m-yosefpor, the current chart release policy is we only do the official release together with the project, so for this specific one, we have to wait until v1.23 release.

I know that may sound weird, I will talk with the team and see what we could do.

@pmint93 pmint93 mentioned this pull request Jun 19, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jichenjc jichenjc jichenjc left review comments

@adisky adisky Awaiting requested review from adisky

@chrigl chrigl Awaiting requested review from chrigl

@lingxiankong lingxiankong Awaiting requested review from lingxiankong

+1 more reviewer

@ramineni ramineni ramineni left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lingxiankong lingxiankong

@ramineni ramineni

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cinder-csi-plugin does not have options for leader election

6 participants

@m-yosefpor @k8s-ci-robot @bgagnon @jichenjc @ramineni @lingxiankong