Leader election lock name should include driver name #129

zhucan · 2019-06-19T07:05:09Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
CSI has many controllers with leader election functionality. It's best to disambiguate between all the different locks used for LE

Which issue(s) this PR fixes:

Fixes #128

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add a prefix "external-snapshotter-leader" and the driver name to the snapshotter leader election lock name.

zhucan · 2019-06-19T07:06:15Z

/assign @xing-yang

xing-yang · 2019-06-19T18:20:27Z

Please remove > before /kind bug

zhucan · 2019-06-20T02:45:48Z

Please remove > before /kind bug

copy that

xing-yang · 2019-07-11T01:23:41Z

cmd/csi-snapshotter/main.go

@@ -214,7 +215,8 @@ func main() {
 	if !*leaderElection {
 		run(context.TODO())
 	} else {
-		le := leaderelection.NewLeaderElection(kubeClient, leaderElectionLockName, run)
+		lockName := fmt.Sprintf("%s-%s", prefix, strings.Replace(*snapshotterName, "/", "-", -1))


I think we don't need this prefix at all. The lockName used in the provisioner does not have a prefix:
https://github.com/kubernetes-csi/external-provisioner/blob/master/cmd/csi-provisioner/csi-provisioner.go#L202

We do need the prefix otherwise the name will conflict with other sidecar's locks. provisioner unfortunately has more backwards compatibility concerns so it's not so easy to fix: kubernetes-csi/external-provisioner#295

Sound good.

xing-yang · 2019-07-11T03:20:12Z

cmd/csi-snapshotter/main.go

-	version                = "unknown"
-	leaderElectionLockName = "external-snapshotter-leader-election"
+	version = "unknown"
+	prefix  = "external-snapshotter-leader"


@msau42 Is there any limitation on the length of the lockName? This prefix is a little longer. Other than that, I'm fine with it.

@zhucan Can you shorten this prefix to just "snapshotter"?
@msau42 Are there any naming convention? If not, I'll just go with "snapshotter". Thanks.

The attacher and resizer all use "external-" prefix. Probably we should just be the same to be consistent.

In terms of length, the name is used as the ObjectMeta.Name, which is 253 characters

xing-yang · 2019-07-11T23:59:40Z

/lgtm
/approve

k8s-ci-robot · 2019-07-11T23:59:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xing-yang, zhucan

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:

~~OWNERS~~ [xing-yang]

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

7bc70e5 Merge pull request kubernetes-csi#129 from pohly/squash-documentation e0b02e7 README.md: document usage of --squash 316cb95 Merge pull request kubernetes-csi#132 from yiyang5055/bugfix/boilerplate 26e2ab1 fix: default boilerplate path 1add8c1 Merge pull request kubernetes-csi#133 from pohly/kubernetes-1.20-tag 3e811d6 prow.sh: fix "on-master" prow jobs git-subtree-dir: release-tools git-subtree-split: 7bc70e5

bc0504a Merge pull request kubernetes-csi#140 from jsafrane/remove-unused-k8s-libs 5b1de1a go-get-kubernetes.sh: remove unused k8s libs 49b4269 Merge pull request kubernetes-csi#120 from pohly/add-kubernetes-release a1e1127 Merge pull request kubernetes-csi#139 from pohly/kind-for-kubernetes-latest 1c0fb09 prow.sh: use KinD main for latest Kubernetes 1d77cfc Merge pull request kubernetes-csi#138 from pohly/kind-update-0.10 bff2fb7 prow.sh: KinD 0.10.0 95eac33 Merge pull request kubernetes-csi#137 from pohly/fix-go-version-check 437e431 verify-go-version.sh: fix check after removal of travis.yml 1748b16 Merge pull request kubernetes-csi#136 from pohly/go-1.16 ec844ea remove travis.yml, Go 1.16 df76aba Merge pull request kubernetes-csi#134 from andyzhangx/add-build-arg e314a56 add build-arg ARCH for building multi-arch images, e.g. ARG ARCH FROM k8s.gcr.io/build-image/debian-base-${ARCH}:v2.1.3 7bc70e5 Merge pull request kubernetes-csi#129 from pohly/squash-documentation e0b02e7 README.md: document usage of --squash 316cb95 Merge pull request kubernetes-csi#132 from yiyang5055/bugfix/boilerplate 26e2ab1 fix: default boilerplate path 1add8c1 Merge pull request kubernetes-csi#133 from pohly/kubernetes-1.20-tag 3e811d6 prow.sh: fix "on-master" prow jobs 1d60e77 Merge pull request kubernetes-csi#131 from pohly/kubernetes-1.20-tag 9f10459 prow.sh: support building Kubernetes for a specific version f7e7ee4 docs: steps for adding testing against new Kubernetes release fe1f284 Merge pull request kubernetes-csi#121 from kvaps/namespace-check 8fdf0f7 Merge pull request kubernetes-csi#128 from fengzixu/master 1c94220 fix: fix a bug of csi-sanity a4c41e6 Merge pull request kubernetes-csi#127 from pohly/fix-boilerplate ece0f50 check namespace for snapshot-controller dbd8967 verify-boilerplate.sh: fix path to script 9289fd1 Merge pull request kubernetes-csi#125 from sachinkumarsingh092/optional-spelling-boilerplate-checks ad29307 Make the spelling and boilerplate checks optional 5f06d02 Merge pull request kubernetes-csi#124 from sachinkumarsingh092/fix-spellcheck-boilerplate-tests 48186eb Fix spelling and boilerplate errors 71690af Merge pull request kubernetes-csi#122 from sachinkumarsingh092/include-spellcheck-boilerplate-tests 981be3f Adding spelling and boilerplate checks. 2bb7525 Merge pull request kubernetes-csi#117 from fengzixu/master 4ab8b15 use the tag to replace commit of csi-test 5d74e45 change the csi-test import path to v4 7dcd0a9 upgrade csi-test to v4.0.2 git-subtree-dir: release-tools git-subtree-split: bc0504a

README.md: document usage of --squash

…ncy-openshift-4.16-csi-snapshot-validation-webhook OCPBUGS-24996: Updating ose-csi-snapshot-validation-webhook-container image to be consistent with ART

Leader election lock name should include driver name

94238a8

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 Jun 19, 2019

k8s-ci-robot requested review from lpabon and saad-ali June 19, 2019 07:05

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 19, 2019

k8s-ci-robot assigned xing-yang Jun 19, 2019

failed unit test

e250652

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 20, 2019

zhucan mentioned this pull request Jun 25, 2019

REQUEST: New membership for zhucan kubernetes/org#953

Closed

6 tasks

xing-yang reviewed Jul 11, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 11, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2019

k8s-ci-robot merged commit d69cf48 into kubernetes-csi:master Jul 12, 2019

pohly mentioned this pull request Feb 3, 2021

master: update release-tools #468

Merged

ggriffiths mentioned this pull request Apr 16, 2021

Add latest release-tools to release-4.0 branch #498

Merged

xing-yang pushed a commit to xing-yang/external-snapshotter that referenced this pull request Jul 26, 2021

Merge pull request kubernetes-csi#129 from pohly/squash-documentation

7bc70e5

README.md: document usage of --squash

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leader election lock name should include driver name #129

Leader election lock name should include driver name #129

zhucan commented Jun 19, 2019 •

edited by xing-yang

Loading

zhucan commented Jun 19, 2019

xing-yang commented Jun 19, 2019

zhucan commented Jun 20, 2019

xing-yang Jul 11, 2019

msau42 Jul 11, 2019

xing-yang Jul 11, 2019

xing-yang Jul 11, 2019

xing-yang Jul 11, 2019 •

edited

Loading

msau42 Jul 11, 2019

xing-yang Jul 11, 2019

xing-yang commented Jul 11, 2019

k8s-ci-robot commented Jul 11, 2019

Leader election lock name should include driver name #129

Leader election lock name should include driver name #129

Conversation

zhucan commented Jun 19, 2019 • edited by xing-yang Loading

zhucan commented Jun 19, 2019

xing-yang commented Jun 19, 2019

zhucan commented Jun 20, 2019

xing-yang Jul 11, 2019

Choose a reason for hiding this comment

msau42 Jul 11, 2019

Choose a reason for hiding this comment

xing-yang Jul 11, 2019

Choose a reason for hiding this comment

xing-yang Jul 11, 2019

Choose a reason for hiding this comment

xing-yang Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

msau42 Jul 11, 2019

Choose a reason for hiding this comment

xing-yang Jul 11, 2019

Choose a reason for hiding this comment

xing-yang commented Jul 11, 2019

k8s-ci-robot commented Jul 11, 2019

zhucan commented Jun 19, 2019 •

edited by xing-yang

Loading

xing-yang Jul 11, 2019 •

edited

Loading