Skip to content

Use leader election library in csi-lib-utils #31

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 2, 2019
Merged

Use leader election library in csi-lib-utils #31

merged 3 commits into from
Apr 2, 2019

Conversation

andrewsykim
Copy link
Contributor

Also updates kubernetes client to v1.14.0 and adds an option to use lease based leader election (highly recommended) over Endpoints.

/assign @msau42 @xing-yang @jsafrane

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 29, 2019
@k8s-ci-robot k8s-ci-robot requested review from mlmhl and saad-ali March 29, 2019 20:31
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 29, 2019
andrewsykim
andrewsykim commented Mar 29, 2019
View reviewed changes
cmd/csi-resizer/main.go Outdated
if !*enableLeaderElection {
run(context.TODO())
} else {
if *leaderElectionIdentity == "" {
Copy link
Contributor Author

@andrewsykim andrewsykim Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to deprecate this flag since csi-lib-utils will check the pod hostname if the identity is not explicitly set.

msau42
msau42 reviewed Mar 29, 2019
View reviewed changes
cmd/csi-resizer/main.go Outdated
@@ -42,6 +44,7 @@ var (
showVersion = flag.Bool("version", false, "Show version")

enableLeaderElection = flag.Bool("leader-election", false, "Enable leader election.")
leaderElectionType = flag.String("leader-election-type", "endpoints", "the type of leader election, options are 'endpoints' (default) or 'lease' (strongly recommended)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

resizer is still alpha (and not officially released yet). Let's just start from lease :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gladly :)

Copy link
Contributor Author

@andrewsykim andrewsykim Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Going to also remove identity, retry, lease duration & renew flags. Easier to add later than to remove

@msau42
Copy link
Collaborator

msau42 commented Mar 29, 2019

/assign @gnufied

mlmhl
mlmhl reviewed Mar 31, 2019
View reviewed changes
cmd/csi-resizer/main.go Outdated
"before it is replaced by another candidate. This is only applicable if leader "+
"election is enabled.")
enableLeaderElection = flag.Bool("leader-election", false, "Enable leader election.")
leaderElectionNamespace = flag.String("leader-election-namespace", "Namespace where this resizer runs.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test this code? The flag.String method needs three arguments but you only provides two, this will make the CI test failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, thanks

@gnufied
Copy link
Contributor

gnufied commented Apr 1, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2019
@andrewsykim
Copy link
Contributor Author

/hold

Waiting for kubernetes-csi/csi-lib-utils#24

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2019
@msau42
Copy link
Collaborator

msau42 commented Apr 2, 2019

Update commit msg and RBAC

@andrewsykim
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2019
@msau42
Copy link
Collaborator

msau42 commented Apr 2, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2019
@msau42
Copy link
Collaborator

msau42 commented Apr 2, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, msau42

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

The pull request process is described here

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 Apr 2, 2019
@k8s-ci-robot k8s-ci-robot merged commit 0a3af36 into kubernetes-csi:master Apr 2, 2019
msau42
msau42 reviewed Apr 2, 2019
View reviewed changes
deploy/kubernetes/rbac.yaml
@@ -71,6 +71,9 @@ rules:
- apiGroups: [""]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops can we remove the endpoints rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I'll follow-up

@andrewsykim andrewsykim mentioned this pull request Apr 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlmhl mlmhl mlmhl left review comments

@msau42 msau42 msau42 left review comments

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees

@gnufied gnufied

@xing-yang xing-yang

@jsafrane jsafrane

@msau42 msau42

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@andrewsykim @msau42 @gnufied @k8s-ci-robot @mlmhl @xing-yang @jsafrane