Use leader election library from csi-lib-utils #135

andrewsykim · 2019-03-29T18:54:55Z

Also adds support for lease based leader election which will be the preferred resource for leader election. We still need to support ConfigMap based leader election for backwards compatibility.

Leader election now optionally uses Lease object instead of ConfigMap. This change requires change of RBAC rules.

andrewsykim · 2019-03-29T18:55:05Z

/assign @msau42 @xing-yang

andrewsykim · 2019-03-29T19:22:00Z

cmd/csi-attacher/main.go

@@ -60,6 +61,7 @@ var (
 	timeout           = flag.Duration("timeout", 15*time.Second, "Timeout for waiting for attaching or detaching the volume.")

 	enableLeaderElection    = flag.Bool("leader-election", false, "Enable leader election.")
+	leaderElectionType      = flag.String("leader-election-type", "configmap", "the type of leader election, options are 'configmap' (default) or 'lease' (recommended)")
 	leaderElectionNamespace = flag.String("leader-election-namespace", "", "Namespace where this attacher runs.")
 	leaderElectionIdentity  = flag.String("leader-election-identity", "", "Unique identity of this attacher. Typically name of the pod where the attacher runs.")


I don't think identity is needed anymore since the lib will use the hostname if it's not set. Should we deprecate the flag here now and default to whatever is in csi-lib-utils?

Yes, please deprecate it (print a warning when it's used)

@andrewsykim There's https://github.com/kubernetes-csi/csi-lib-utils/blob/master/deprecatedflags/deprecatedflags.go which can be used for deprecated flags.

ah, thank you!

msau42 · 2019-03-29T19:49:15Z

/assign @jsafrane

jsafrane

The code looks ok, please deprecate leader-election-identity and update README.md.

jsafrane · 2019-04-01T10:24:19Z

cmd/csi-attacher/main.go

@@ -60,6 +61,7 @@ var (
 	timeout           = flag.Duration("timeout", 15*time.Second, "Timeout for waiting for attaching or detaching the volume.")

 	enableLeaderElection    = flag.Bool("leader-election", false, "Enable leader election.")
+	leaderElectionType      = flag.String("leader-election-type", "configmap", "the type of leader election, options are 'configmap' (default) or 'lease' (recommended)")
 	leaderElectionNamespace = flag.String("leader-election-namespace", "", "Namespace where this attacher runs.")
 	leaderElectionIdentity  = flag.String("leader-election-identity", "", "Unique identity of this attacher. Typically name of the pod where the attacher runs.")


Yes, please deprecate it (print a warning when it's used)

andrewsykim · 2019-04-01T21:22:20Z

/hold

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

pohly · 2019-04-02T08:24:05Z

How does that affect RBAC rules?

msau42 · 2019-04-02T18:13:26Z

Good point. RBAC rules need to be updated to add Lease. (we still need to keep the configmap rules for backwards compat), but drivers can decide to further refine them as needed.

pohly · 2019-04-02T18:50:13Z

(we still need to keep the configmap rules for backwards compat)

In which case does that help? If the current code no longer needs them, then they can be removed.

No-one should be using these updated RBAC rules with an older sidecar release, that's not part of our compatibility guarantees.

msau42 · 2019-04-02T19:18:34Z

The current code still has to support the old leaderelection method for backwards compatibility. So we still need rules for both ways.

msau42 · 2019-04-02T19:54:46Z

README.md

@@ -42,9 +42,7 @@ Note that the external-attacher does not scale with more replicas. Only one exte

 * `--leader-election`: Enables leader election. This is useful when there are multiple replicas of the same external-attacher running for one CSI driver. Only one of them may be active (=leader). A new leader will be re-elected when current leader dies or becomes unresponsive for ~15 seconds.

-* `--leader-election-namespace <namespace>`: Namespace where the external-attacher runs and where leader election object will be created. It is recommended that this parameter is populated from Kubernetes DownwardAPI.
-
-* `--leader-election-identity <id>`: Unique identity of this replica of external-attacher container. It must be unique among all running replicas of the external-attacher. Pod name populated from Kubernetes DownwardAPI is recommended.


Move this to the Deprecated arguments section

Ah sorry one more nit. This should be moved to the "Deprecated arguments" section

msau42 · 2019-04-02T19:55:29Z

deploy/kubernetes/rbac.yaml

@@ -51,7 +51,7 @@ roleRef:
  apiGroup: rbac.authorization.k8s.io

 ---
-# Attacher must be able to work with config map in current namespace


Also add a comment that a driver only needs to include one of these rules depending on which leader election method it chose

msau42 · 2019-04-02T19:55:50Z

README.md

-* `--leader-election-namespace <namespace>`: Namespace where the external-attacher runs and where leader election object will be created. It is recommended that this parameter is populated from Kubernetes DownwardAPI.
-
-* `--leader-election-identity <id>`: Unique identity of this replica of external-attacher container. It must be unique among all running replicas of the external-attacher. Pod name populated from Kubernetes DownwardAPI is recommended.
+* `--leader-election-type`: The resource type to use for leader election, options are 'configmaps' (default) or 'leases' (recommended)


Should we deprecate the "configmap" option?

msau42 · 2019-04-02T19:56:44Z

Vendor commit msg needs to be updated.

Also, could you add an entry to the Changelog file?

msau42 · 2019-04-02T20:39:28Z

CHANGELOG-1.1.md

@@ -16,6 +16,8 @@

 ## Other notable changes

+* Add support for Lease based leader election. Enable this by setting `--leader-election-type=leases` ([#135](https://github.com/kubernetes-csi/external-attacher/pull/135))


This can probably go under "Notable features"

msau42 · 2019-04-02T20:39:38Z

CHANGELOG-1.1.md

@@ -16,6 +16,8 @@

 ## Other notable changes

+* Add support for Lease based leader election. Enable this by setting `--leader-election-type=leases` ([#135](https://github.com/kubernetes-csi/external-attacher/pull/135))
+* --leader-election-identity is now deprecated. Setting this flag will do nothing now. ([#135](https://github.com/kubernetes-csi/external-attacher/pull/135))


This should go under "Deprecations"

msau42 · 2019-04-02T21:11:25Z

deploy/kubernetes/deployment.yaml

@@ -38,17 +38,11 @@ spec:
            - "--v=5"
            - "--csi-address=$(ADDRESS)"
            - "--leader-election"
-            - "--leader-election-namespace=$(MY_NAMESPACE)"


should this example use lease?

Yeah it should. Originally left that out since I didn't want anyone re-deploying this on an existing deployment but I don't think that'll be a problem

msau42 · 2019-04-02T21:12:10Z

CHANGELOG-1.1.md

@@ -3,6 +3,7 @@
 # Deprecations

 * Command line flag `-connection-timeout` is deprecated and has no effect.
+* --leader-election-identity is now deprecated. Setting this flag will have no effect on leader election. ([#135](https://github.com/kubernetes-csi/external-attacher/pull/135))


Can you follow the style of the above?

msau42 · 2019-04-02T21:19:15Z

CHANGELOG-1.1.md

@@ -3,6 +3,7 @@
 # Deprecations

 * Command line flag `-connection-timeout` is deprecated and has no effect.
+* Command line flag `--leader-election-identity` is deprecated and has no effect.


Can we also deprecate the "configmaps" option?

pohly · 2019-04-03T07:17:13Z

The current code still has to support the old leaderelection method for backwards compatibility. So we still need rules for both ways.

It would be useful to document that in the RBAC file: someone might want to remove the parts that they don't need because they only use one mode.

msau42 · 2019-04-03T18:54:41Z

cmd/csi-attacher/main.go

+		// Name of config map with leader election lock
+		lockName := "external-attacher-leader-" + csiAttacher
+		if *leaderElectionType == "configmaps" {
+			le = leaderelection.NewLeaderElectionWithConfigMaps(clientset, lockName, run)


Can you also print out a warning message saying that the configmap option is deprecated.

msau42 · 2019-04-03T18:55:17Z

cmd/csi-attacher/main.go

@@ -60,14 +62,20 @@ var (
 	timeout           = flag.Duration("timeout", 15*time.Second, "Timeout for waiting for attaching or detaching the volume.")

 	enableLeaderElection    = flag.Bool("leader-election", false, "Enable leader election.")
-	leaderElectionNamespace = flag.String("leader-election-namespace", "", "Namespace where this attacher runs.")
-	leaderElectionIdentity  = flag.String("leader-election-identity", "", "Unique identity of this attacher. Typically name of the pod where the attacher runs.")
+	leaderElectionType      = flag.String("leader-election-type", "configmap", "the type of leader election, options are 'configmaps' (default) or 'leases' (recommended)")


Also add a note here that configmaps is deprecated.

xing-yang · 2019-04-03T19:17:48Z

Gopkg.toml

  name = "github.com/container-storage-interface/spec"
-  version = "1.0.0"
+  version = "=1.0.0"


This should be updated to 1.1.0 to be consistent with csi-lib-utils 0.6.1.
"override" should be changed to "constraint".

Upgrading to 1.1.0 breaks a bunch of tests which I don't want to be fixing in this PR, hence the override

go test `go list ./... | grep -v 'vendor' ` # github.com/kubernetes-csi/external-attacher/vendor/github.com/kubernetes-csi/csi-test/driver vendor/github.com/kubernetes-csi/csi-test/driver/mock.go:41:5: cannot use servers.Controller (type *MockControllerServer) as type csi.ControllerServer in field value: *MockControllerServer does not implement csi.ControllerServer (missing ControllerExpandVolume method) vendor/github.com/kubernetes-csi/csi-test/driver/mock.go:42:5: cannot use servers.Node (type *MockNodeServer) as type csi.NodeServer in field value: *MockNodeServer does not implement csi.NodeServer (missing NodeExpandVolume method) ? github.com/kubernetes-csi/external-attacher/cmd/csi-attacher [no test files] FAIL github.com/kubernetes-csi/external-attacher/pkg/attacher [build failed] ok github.com/kubernetes-csi/external-attacher/pkg/controller 1.451s make: *** [test-go] Error 2

I can fix this in a follow-up PR

That should go away if you also update csi-test to 2.0.0.

ah, thanks, updated now

msau42 · 2019-04-03T23:58:27Z

Gopkg.toml

@@ -1,16 +1,16 @@
 # List of dependecies for CSI attacher

-[[constraint]]
+[[override]]


This can be constraint now?

msau42 · 2019-04-04T00:16:14Z

CHANGELOG-1.1.md

@@ -3,6 +3,8 @@
 ## Deprecations

 * Command line flag `-connection-timeout` is deprecated and has no effect.
+* Command line flag `--leader-election-identity` is deprecated and has no effect.
+* Command line argument `configmaps` for flag `--leader-election-type` is deprecated in favor of `--leader-election-type=leases`. The default is still `configmaps` for backwards compatibility reasons but will be switched to `leases` in a future release.


I thought of another way to document this. What do you think of this?

Looks much better, I'll update :)

…ease based leader election

msau42 · 2019-04-04T18:25:07Z

README.md

@@ -57,10 +57,6 @@ Note that the external-attacher does not scale with more replicas. Only one exte

 * `--leader-election`: Enables leader election. This is useful when there are multiple replicas of the same external-attacher running for one CSI driver. Only one of them may be active (=leader). A new leader will be re-elected when current leader dies or becomes unresponsive for ~15 seconds.

-* `--leader-election-namespace <namespace>`: Namespace where the external-attacher runs and where leader election object will be created. It is recommended that this parameter is populated from Kubernetes DownwardAPI.


We still need to keep the namespace argument right? Do we have to deprecate this as well?

oops, yeah we need to keep this here. I'm not sure about deprecation though since you can run external-attacher outside the cluster.

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>

msau42 · 2019-04-04T21:15:23Z

/lgtm
/approve

k8s-ci-robot · 2019-04-04T21:15:30Z

[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:

~~OWNERS~~ [msau42]

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

msau42 · 2019-04-05T02:26:43Z

/hold cancel

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 requested review from davidz627 and sbezverk March 29, 2019 18:55

k8s-ci-robot assigned msau42 and xing-yang Mar 29, 2019

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 29, 2019

andrewsykim force-pushed the leaderelection branch 3 times, most recently from e737962 to 066e5ac Compare March 29, 2019 19:21

andrewsykim commented Mar 29, 2019

View reviewed changes

andrewsykim force-pushed the leaderelection branch from 066e5ac to 70e3507 Compare March 29, 2019 19:47

k8s-ci-robot assigned jsafrane Mar 29, 2019

jsafrane reviewed Apr 1, 2019

View reviewed changes

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

andrewsykim force-pushed the leaderelection branch from 70e3507 to f55b92b Compare April 2, 2019 19:16

andrewsykim force-pushed the leaderelection branch from f55b92b to 597f4a9 Compare April 2, 2019 19:20

msau42 reviewed Apr 2, 2019

View reviewed changes

andrewsykim force-pushed the leaderelection branch from 597f4a9 to f36eab7 Compare April 2, 2019 19:59

msau42 reviewed Apr 2, 2019

View reviewed changes

andrewsykim force-pushed the leaderelection branch 2 times, most recently from df8bbcc to 58cd406 Compare April 2, 2019 20:59

msau42 reviewed Apr 2, 2019

View reviewed changes

andrewsykim force-pushed the leaderelection branch from 58cd406 to 8809605 Compare April 2, 2019 21:16

msau42 reviewed Apr 2, 2019

View reviewed changes

andrewsykim force-pushed the leaderelection branch from 8809605 to 0228ef9 Compare April 3, 2019 16:13

msau42 reviewed Apr 3, 2019

View reviewed changes

xing-yang reviewed Apr 3, 2019

View reviewed changes

andrewsykim force-pushed the leaderelection branch 2 times, most recently from 42e663b to 1bd3b71 Compare April 3, 2019 23:43

msau42 reviewed Apr 3, 2019

View reviewed changes

msau42 reviewed Apr 4, 2019

View reviewed changes

andrewsykim force-pushed the leaderelection branch from 1bd3b71 to fb07c0d Compare April 4, 2019 15:08

andrewsykim added 2 commits April 4, 2019 11:20

update vendor csi-lib-utils@v0.6.1

4b601b9

use leader election lib from csi-lib-utils, also adds option to use l…

1720e97

…ease based leader election

andrewsykim force-pushed the leaderelection branch from fb07c0d to 9bd541a Compare April 4, 2019 15:20

msau42 reviewed Apr 4, 2019

View reviewed changes

andrewsykim force-pushed the leaderelection branch from 9bd541a to 45ca4f6 Compare April 4, 2019 19:48

andrewsykim added 3 commits April 4, 2019 15:49

update RBAC and README to use leases based leaderelection

8d020d2

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>

update deployment example with new leader election flags

358440e

update CHANGELOG with leader election changes

eddfb81

Signed-off-by: Andrew Sy Kim <kiman@vmware.com>

andrewsykim force-pushed the leaderelection branch from 45ca4f6 to eddfb81 Compare April 4, 2019 19:49

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

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

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 5, 2019

k8s-ci-robot merged commit 70a1411 into kubernetes-csi:master Apr 5, 2019

		@@ -16,6 +16,8 @@

		## Other notable changes

		* Add support for Lease based leader election. Enable this by setting `--leader-election-type=leases` ([#135](https://github.com/kubernetes-csi/external-attacher/pull/135))

		@@ -57,10 +57,6 @@ Note that the external-attacher does not scale with more replicas. Only one exte

		* `--leader-election`: Enables leader election. This is useful when there are multiple replicas of the same external-attacher running for one CSI driver. Only one of them may be active (=leader). A new leader will be re-elected when current leader dies or becomes unresponsive for ~15 seconds.

		* `--leader-election-namespace <namespace>`: Namespace where the external-attacher runs and where leader election object will be created. It is recommended that this parameter is populated from Kubernetes DownwardAPI.

Use leader election library from csi-lib-utils #135

Use leader election library from csi-lib-utils #135

Conversation

andrewsykim commented Mar 29, 2019 • edited by jsafrane Loading

andrewsykim commented Mar 29, 2019

andrewsykim Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Mar 29, 2019

jsafrane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim commented Apr 1, 2019

pohly commented Apr 2, 2019

msau42 commented Apr 2, 2019

pohly commented Apr 2, 2019

msau42 commented Apr 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Apr 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Apr 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Apr 4, 2019

k8s-ci-robot commented Apr 4, 2019

msau42 commented Apr 5, 2019

andrewsykim commented Mar 29, 2019 •

edited by jsafrane

Loading

andrewsykim Mar 29, 2019 •

edited

Loading