Add leader election code to MIC #277

kkmsft · 2019-07-05T19:48:55Z

Reason for Change:

Add leader election code to MIC to run it as ReplicaSet with multiple replicas which can come up and continue in case of a failure of the active MIC. Also added are deployment yamls which configures the ReplicaSet appropriately under deploy/master/replicaset.

Issue Fixed:

Notes for Reviewers:

TODO:

Add tests in e2e

kkmsft · 2019-07-06T00:55:26Z

Ran 10 of 10 Specs in 2939.239 seconds
SUCCESS! -- 10 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAADPodIdentity (2939.24s)

aramase · 2019-07-17T16:14:43Z

cmd/mic/main.go

 )

 func main() {
 	defer glog.Flush()
+	hostName, err := os.Hostname()
+	if err != nil {
+		glog.Fatalf("Get hostname failure.")


Need to include the error in the err msg

aramase · 2019-07-17T16:20:46Z

cmd/mic/main.go

 	flag.StringVar(&kubeconfig, "kubeconfig", "", "Path to the kube config")
 	flag.StringVar(&cloudconfig, "cloudconfig", "", "Path to cloud config e.g. Azure.json file")
 	flag.BoolVar(&forceNamespaced, "forceNamespaced", false, "Forces namespaced identities, binding, and assignment")
 	flag.BoolVar(&versionInfo, "version", false, "Prints the version information")
 	flag.DurationVar(&syncRetryDuration, "syncRetryDuration", 3600*time.Second, "The interval in seconds at which sync loop should periodically check for errors and reconcile.")

+	// Leader election parameters
+	flag.StringVar(&leaderElectionCfg.ResourceName, "leader-election-name", hostName, "leader name. default is 'hostname'")
+	flag.StringVar(&leaderElectionCfg.Namespace, "leader-election-namespace", "default", "name space to create leader election objects. default is 'default' namesapce")


We don't need to explicitly say the default value in the usage. Since the default value is defined, pflag will show that in the output.

For instance -

-syncRetryDuration duration The interval in seconds at which sync loop should periodically check for errors and reconcile. (default 1h0m0s)

aramase · 2019-07-17T16:22:42Z

deploy/infra/master/replicaset/deployment-rbac.yaml

+      serviceAccountName: aad-pod-id-mic-service-account
+      containers:
+      - name: mic
+        image: "mcr.microsoft.com/k8s/aad-pod-identity/mic:1.3"


Image should be 1.5-rc1 image here instead of 1.3

aramase · 2019-07-17T16:23:45Z

deploy/infra/master/replicaset/deployment-rbac.yaml

+        name: iptableslock
+      containers:
+      - name: nmi
+        image: "mcr.microsoft.com/k8s/aad-pod-identity/nmi:1.4"


Image should be 1.5-rc1 instead of 1.4

aramase · 2019-07-17T16:23:59Z

deploy/infra/master/replicaset/deployment.yaml

+        name: iptableslock
+      containers:
+      - name: nmi
+        image: "mcr.microsoft.com/k8s/aad-pod-identity/nmi:master"


same as above

@kkmsft looks like the image version is still master here?

aramase · 2019-07-17T16:24:07Z

deploy/infra/master/replicaset/deployment.yaml

+    spec:
+      containers:
+      - name: mic
+        image: "mcr.microsoft.com/k8s/aad-pod-identity/mic:master"


same as above

@kkmsft looks like the image version is still master here instead of rc?

aramase · 2019-07-17T16:27:24Z

pkg/mic/mic.go

+			Identity:      c.ResourceName,
+			EventRecorder: recorder})
+	if err != nil {
+		glog.Errorf("Resource lock creation for leadeer election failed with error : %v", err)


s/leadeer/leader

aramase · 2019-07-17T16:29:14Z

pkg/mic/mic.go

+	}
+
+	leaderElector, err = leaderelection.NewLeaderElector(config)
+	if err != nil {


We can skip the err != nil check here and return from here

aramase · 2019-07-17T16:31:34Z

test/e2e/template/deployment-rbac.yaml

@@ -140,6 +140,9 @@ rules:
 - apiGroups: [""]
  resources: ["events"]
  verbs: ["create", "patch"]
+- apiGroups: [""]
+  resources: ["endpoints"]
+  verbs: ["create", "get", "list", "watch", "update", "patch"]


Why do we need create, update and patch?

hmm we need create and get for sure, the rest i am not so sure about.

khenidak · 2019-07-17T18:17:49Z

cmd/mic/main.go

 	flag.StringVar(&kubeconfig, "kubeconfig", "", "Path to the kube config")
 	flag.StringVar(&cloudconfig, "cloudconfig", "", "Path to cloud config e.g. Azure.json file")
 	flag.BoolVar(&forceNamespaced, "forceNamespaced", false, "Forces namespaced identities, binding, and assignment")
 	flag.BoolVar(&versionInfo, "version", false, "Prints the version information")
 	flag.DurationVar(&syncRetryDuration, "syncRetryDuration", 3600*time.Second, "The interval in seconds at which sync loop should periodically check for errors and reconcile.")

+	// Leader election parameters
+	flag.StringVar(&leaderElectionCfg.ResourceName, "leader-election-name", hostName, "leader name. default is 'hostname'")


default shouldn't be host name, default should be a well known string. Mic pods will get different host names.

isn't that for ID ? Shouldn't the resource name be unique ?

leaderElectionCfg.ResourceName throw me off. i thought it is for endpoint name, not for the identity of the leader. Let us try to avoid the same for future users.

rename the field ResourceName to leader identity or something of that sort.

rename field ID to Identity

ResourceName ==> Instance
ID ==> Name

khenidak · 2019-07-17T18:18:21Z

cmd/mic/main.go

 	flag.StringVar(&kubeconfig, "kubeconfig", "", "Path to the kube config")
 	flag.StringVar(&cloudconfig, "cloudconfig", "", "Path to cloud config e.g. Azure.json file")
 	flag.BoolVar(&forceNamespaced, "forceNamespaced", false, "Forces namespaced identities, binding, and assignment")
 	flag.BoolVar(&versionInfo, "version", false, "Prints the version information")
 	flag.DurationVar(&syncRetryDuration, "syncRetryDuration", 3600*time.Second, "The interval in seconds at which sync loop should periodically check for errors and reconcile.")

+	// Leader election parameters
+	flag.StringVar(&leaderElectionCfg.ResourceName, "leader-election-name", hostName, "leader name. default is 'hostname'")
+	flag.StringVar(&leaderElectionCfg.Namespace, "leader-election-namespace", "default", "name space to create leader election objects. default is 'default' namesapce")


nit namespace not name space

khenidak · 2019-07-17T18:19:40Z

pkg/mic/mic.go

+	}
+	leaderElector, err := c.NewLeaderElector(clientSet, recorder, leaderElectionConfig)
+	if err != nil {
+		return nil, err


please log here.

khenidak · 2019-07-17T18:20:53Z

test/e2e/template/deployment-rbac.yaml

@@ -140,6 +140,9 @@ rules:
 - apiGroups: [""]
  resources: ["events"]
  verbs: ["create", "patch"]
+- apiGroups: [""]
+  resources: ["endpoints"]
+  verbs: ["create", "get", "list", "watch", "update", "patch"]


hmm we need create and get for sure, the rest i am not so sure about.

khenidak · 2019-07-17T18:24:30Z

pkg/mic/mic.go

+			OnStoppedLeading: func() {
+				glog.Errorf("Lost Leader Lease")
+				glog.Flush()
+				os.Exit(1)


nit the case of log line consistent. don't use title case, and generally avoid caps all together.

Changed. Only first one is caps now.

khenidak · 2019-07-17T18:26:44Z

deploy/infra/master/replicaset/deployment-rbac.yaml

@@ -0,0 +1,208 @@
+apiVersion: v1


Why do we have these new files, are they new?

right now the deployment file (https://github.com/Azure/aad-pod-identity/blob/master/deploy/infra/deployment-rbac.yaml) under the infra has the extra parameter with the binary name, because of a bug in the Docker packaging which we fixed later. That file needs to be remain as is so that anyone using the stable images can use that deployment file. These file contains the right yaml for the images from the master. When we do next release - 1.5 this file will move to (https://github.com/Azure/aad-pod-identity/blob/master/deploy/infra/deployment-rbac.yaml) so that stable images can be obtained by using the same deployment files and also have replicaset configuration.

kkmsft · 2019-07-24T20:13:26Z

Ran 12 of 12 Specs in 2723.302 seconds
SUCCESS! -- 12 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAADPodIdentity (2723.30s)
PASS
ok github.com/Azure/aad-pod-identity/test/e2e 2723.340s

…RBAC permissions

kkmsft requested review from cpuguy83 and aramase and removed request for cpuguy83 July 5, 2019 19:55

kkmsft changed the title ~~[WIP] Add leader election code to MIC - initial commit~~ [WIP] Add leader election code to MIC Jul 6, 2019

kkmsft force-pushed the leader_election branch 2 times, most recently from f4f27b3 to 4c928bb Compare July 16, 2019 05:03

kkmsft changed the title ~~[WIP] Add leader election code to MIC~~ Add leader election code to MIC Jul 16, 2019

kkmsft requested a review from khenidak July 16, 2019 05:33

kkmsft added this to the v1.5 milestone Jul 16, 2019

aramase reviewed Jul 17, 2019

View reviewed changes

khenidak suggested changes Jul 17, 2019

View reviewed changes

kkmsft force-pushed the leader_election branch 2 times, most recently from a31f846 to 56c65b0 Compare July 24, 2019 18:00

kkmsft added 8 commits July 24, 2019 14:47

Add leader election code to MIC - initial commit

1863801

Add leader election configuration

28ea418

Add deployment which works with master images and has leaderelection …

cf3f342

…RBAC permissions

Add leaderelection permissons and replicas to e2e deployment

841c1be

Reduce the refresh time for leader election

5a7ec74

Merge fixes and variable name simplification

872f2c6

Address review comments

fb3cb4b

Update the endpoint RBAC and point to the rc images

d831d46

kkmsft force-pushed the leader_election branch from afb8f26 to d831d46 Compare July 24, 2019 21:47

kkmsft merged commit 1aa529b into Azure:master Jul 24, 2019

kkmsft deleted the leader_election branch July 24, 2019 21:52

kkmsft mentioned this pull request Aug 6, 2019

Add leader election logic to MIC and make it a ReplicaSet #280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add leader election code to MIC #277

Add leader election code to MIC #277

kkmsft commented Jul 5, 2019 •

edited

Loading

kkmsft commented Jul 6, 2019

aramase Jul 17, 2019

kkmsft Jul 22, 2019

aramase Jul 17, 2019

kkmsft Jul 22, 2019

aramase Jul 17, 2019

aramase Jul 17, 2019

aramase Jul 17, 2019

aramase Jul 24, 2019

aramase Jul 17, 2019

aramase Jul 24, 2019

aramase Jul 17, 2019

kkmsft Jul 17, 2019

aramase Jul 17, 2019

aramase Jul 17, 2019

khenidak Jul 17, 2019

khenidak Jul 17, 2019

kkmsft Jul 17, 2019

khenidak Jul 17, 2019

kkmsft Jul 22, 2019

khenidak Jul 17, 2019

kkmsft Jul 22, 2019

khenidak Jul 17, 2019

kkmsft Jul 22, 2019

khenidak Jul 17, 2019

khenidak Jul 17, 2019

kkmsft Jul 17, 2019

khenidak Jul 17, 2019

kkmsft Jul 17, 2019

kkmsft commented Jul 24, 2019

Add leader election code to MIC #277

Add leader election code to MIC #277

Conversation

kkmsft commented Jul 5, 2019 • edited Loading

kkmsft commented Jul 6, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkmsft commented Jul 24, 2019

kkmsft commented Jul 5, 2019 •

edited

Loading