Performance Improvement - reduce identity assignment time #219

aramase · 2019-05-17T20:44:49Z

Batch process id assignments to node on each sync cycle (pooling delete and add ids, issuing one CreateOrUpdate call for each sync cycle)
Reduces identity assignment for 44 pods from 29m to ~3m

Add states to the assigned identities

Created - assigned identity will be in created state when the crd is created.
Assigned - once the identity has been successfully assigned to underlying node, the state of the assigned identity will be changed to Assigned.
Unassigned - the assigned identity will be in this state when the identity has been successfully removed from the underlying node.

Note

Assigned identities will be created and then the identity will be tried to be assigned to the node. If assignment fails, then the identity will be deleted. No change in this behavior compared to previous version.
Assigned identities will be deleted only after the identity has been successfully removed from the node. This is change in behavior from previous version to ensure we successfully remove the identities from the underlying node.

kkmsft · 2019-05-21T17:23:11Z

Thank you @aramase . A good e2e could be validating that #145 is resolved and the identities are assigned in a reasonable amount of time.

pkg/cloudprovider/identity.go

pkg/cloudprovider/vm.go

cpuguy83 · 2019-05-22T18:33:16Z

pkg/mic/mic.go

+						fmt.Sprintf("Lookup of node %s for pod %s resulted in error %v", createID.Spec.NodeName, createID.Name, err))
+					continue
+				}
+				err = c.CloudClient.CheckUserMSI(id.Spec.ResourceID, node)


Do we need the new interface? Can remove just do the right thing?

aramase · 2019-05-22T18:35:00Z

@kkmsft I was able to validate #145 with my changes. Created close to 44 pods with 2 assigned identities. With version 1.3 it took about 29m to create all assigned identities and assign msi to node. With my changes, it takes only about a minute since it's batching all the updates with a single call. I've a lot of refactoring of my PR still though.

kkmsft · 2019-05-22T18:39:45Z

Thank you @aramase. Good to know that it's moving in the right direction. After our initial discussion on the approach have not looked deeply into the changes. Please let me know when you want me to start looking at the changes.

Also, any chance (I know I am being greedy here ;-)) you can convert that test to an e2e test so that we can track any regression in future.

aramase · 2019-05-22T18:41:51Z

@kkmsft Sure, I'll add that as an e2e. Still working on testing a couple of things. I'll need to refactor the PR. I'll let you know when it's ready for review.

kkmsft · 2019-05-24T20:16:51Z

pkg/cloudprovider/cloudprovider.go

@@ -112,6 +115,59 @@ func withInspection() autorest.PrepareDecorator {
 	}
 }

+// CheckUserMSI checks if the identity has been assigned to the node
+func (c *Client) CheckUserMSI(userAssignedMSIID string, node *corev1.Node) bool {
+	idH, _, err := c.getIdentityResource(node)


Arent' these calls costly ? in the sense that they have to ARM every time ?

@kkmsft You're right! we can avoid the multiple calls to ARM by just getting the list once and then validating existence/non-existence of all assigned ids based on that list.

kkmsft · 2019-05-24T20:22:29Z

pkg/mic/mic.go

@@ -261,24 +275,48 @@ func (c *Client) Sync(exit <-chan struct{}) {

 				glog.V(5).Infof("Initiating assigned id creation for pod - %s, binding - %s", createID.Spec.Pod, binding.Name)

-				err = c.createAssignedIdentityDeps(&createID, id, node)
+				err = c.createAssignedIdentity(&createID)


Is there a way we can perform this create when we start looking at the node map ? Since we don't track state of the assigned identities, we could have a large time gap between the time the assignment is created and then the node assignment was made. Is that a possibility ? If so, can we mitigate it some how without additional state ?

@kkmsft This mimics the current logic. Currently, we create the assigned identity, then make the request for node assignment. The timegap still remains the same with this change because we are batching all the assignments and making the single call. So the time for assignment still remains the same. There shouldn't be a long time gap here.

kkmsft · 2019-06-17T20:12:11Z

pkg/mic/mic_test.go

@@ -159,9 +163,11 @@ func (c *TestCloudClient) CompareMSI(nodeName string, userIDs []string) bool {

 func (c *TestCloudClient) PrintMSI() {
 	for key, val := range c.ListMSI() {
+		fmt.Printf("\nNode name: %s\n", key)


Is this debugging logs ?

Yeah, I'll clean that up.

aramase · 2019-06-17T21:18:49Z

Tests passed -

Ran 8 of 9 Specs in 1339.441 seconds
SUCCESS! -- 8 Passed | 0 Failed | 0 Pending | 1 Skipped
--- PASS: TestAADPodIdentity (1339.44s)
PASS
ok  	github.com/Azure/aad-pod-identity/test/e2e	1339.459s

@kkmsft

aramase · 2019-06-17T21:23:07Z

pkg/mic/mic.go

+				glog.V(5).Infof("Initiating assigned id creation for pod - %s, binding - %s", createID.Spec.Pod, binding.Name)
+
+				if isUserAssignedMSI {
+					err := c.createAssignedIdentity(&createID)


The assigned identity will only be created once the identity assignment on the node is successfully complete.

aramase · 2019-06-17T21:23:39Z

pkg/mic/mic.go

+					fmt.Sprintf("Binding %s removed from node %s for pod %s", removedBinding.Name, delID.Spec.NodeName, delID.Spec.Pod))
+
+				// remove assigned identity crd from cluster
+				if err := c.removeAssignedIdentity(&delID); err != nil {


Similarly, removing the assigned identity only when the identity has been successfully removed from the node

aramase · 2019-06-17T21:24:59Z

test/e2e/aadpodidentity_test.go

@@ -231,7 +231,10 @@ var _ = Describe("Kubernetes cluster using aad-pod-identity", func() {
 			identityValidatorName := fmt.Sprintf("identity-validator-%d", i)

 			setUpIdentityAndDeployment(keyvaultIdentity, fmt.Sprintf("%d", i))
-			time.Sleep(5 * time.Second)
+


Removed the sleep, so the test can actually proceed once the assigned identity exists.

kkmsft · 2019-06-20T16:09:19Z

pkg/cloudprovider/cloudprovider.go

+}
+
+// AppendOrRemoveUserMSI will batch process the removal and addition of ids
+func (c *Client) AppendOrRemoveUserMSI(addUserAssignedMSIIDs []string, removeUserAssignedMSIIDs []string, node *corev1.Node) error {


Is it possible to split this into two separate functions, one which adds and another which removes and then we can add individually add UTs for them ?

Or may be naming can be UpdateUserMSI - AppendOrRemove feels like CreateOrUpdate which is doing a different thing.

Renamed the method

pkg/cloudprovider/cloudprovider.go

pkg/mic/mic.go

kkmsft · 2019-06-20T21:41:21Z

pkg/mic/mic.go

+
+func (c *Client) appendToRemoveListForNode(nodeMap map[string]trackUserAssignedMSIIds, resourceID string, node *corev1.Node) {
+	if trackList, ok := nodeMap[node.Name]; ok {
+		if trackList.removeUserAssignedMSIIDs != nil {


Is this check necessary ?

kkmsft · 2019-06-20T21:47:49Z

pkg/mic/mic.go

+			if err != nil {
+				// check which all identity assignment failed
+				// remove those assigned identities
+				// TODO check with identity team if CreateOrUpdate results in error, is it all or some failed


Can we do this before the PR gets merged.

Sent out an email.

pkg/mic/mic.go

kkmsft · 2019-06-20T21:54:43Z

pkg/mic/mic.go

+			continue
+		}
+		idList, err := c.getUserMSIListForNode(node)
+		if err != nil {


What is the plan here. If we just log and continue then idList can be having random values..?

getUserMSIListForNode returns nil on error. So we'll fail all of them.

kkmsft · 2019-06-20T22:00:39Z

pkg/mic/mic.go


-		err = c.CloudClient.AssignUserMSI(id.Spec.ResourceID, node)
+	nodeIdentityList := make(map[string][]string)
+	for _, n := range nodesWithError {


What happens if we crash or get restarted at this time ?

aramase · 2019-06-21T17:19:08Z

test/e2e/aadpodidentity_test.go

@@ -626,6 +626,8 @@ func setUpIdentityAndDeployment(azureIdentityName, suffix, replicas string) {
 	ok, err = daemonset.WaitOnReady(nmiDaemonSet)
 	Expect(err).NotTo(HaveOccurred())
 	Expect(ok).To(Equal(true))
+
+	time.Sleep(30 * time.Second)


we can remove this sleep in the states PR because there we can start waiting for n assigned identities with desired state.

aramase

Ran 9 of 10 Specs in 2221.232 seconds
SUCCESS! -- 9 Passed | 0 Failed | 0 Pending | 1 Skipped
--- PASS: TestAADPodIdentity (2221.23s)
PASS
ok github.com/Azure/aad-pod-identity/test/e2e 2221.249s

wip batch process createorupdate update logic update events fix events generate set to use unique values update interface reorder check flow update to using one get per node update tests add e2e test for testing scale perf create assigned identity before assignment add unit tests for UpdateUserMSI interface

aramase · 2019-06-25T19:41:15Z

kkmsft · 2019-06-28T23:32:44Z

pkg/mic/mic.go

+}
+
+// getListOfIdsToDelete will go over the delete list to determine if the id is required to be deleted
+// only user assigned identity not in use are added to the remove list for the node


Nit: two sentences.

aramase · 2019-07-01T03:47:16Z

Ran 10 of 10 Specs in 2353.337 seconds
SUCCESS! -- 10 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAADPodIdentity (2353.34s)
PASS
ok github.com/Azure/aad-pod-identity/test/e2e 2353.353s

aramase changed the title ~~[WIP] Taking care of some todos~~ [WIP] Performance Improvement - reduce identity assignment time May 20, 2019

cpuguy83 suggested changes May 22, 2019

View reviewed changes

aramase force-pushed the refactor branch from 2f60fa0 to c7bf5dc Compare May 23, 2019 17:41

kkmsft reviewed May 24, 2019

View reviewed changes

aramase force-pushed the refactor branch from f048a9e to 84f7ba7 Compare June 17, 2019 19:18

kkmsft reviewed Jun 17, 2019

View reviewed changes

aramase force-pushed the refactor branch from c2120e8 to 04eebb1 Compare June 17, 2019 21:18

aramase changed the title ~~[WIP] Performance Improvement - reduce identity assignment time~~ Performance Improvement - reduce identity assignment time Jun 17, 2019

aramase commented Jun 17, 2019

View reviewed changes

kkmsft reviewed Jun 20, 2019

View reviewed changes

pkg/cloudprovider/cloudprovider.go Outdated Show resolved Hide resolved

kkmsft reviewed Jun 20, 2019

View reviewed changes

pkg/mic/mic.go Outdated Show resolved Hide resolved

kkmsft reviewed Jun 20, 2019

View reviewed changes

pkg/mic/mic.go Outdated Show resolved Hide resolved

kkmsft reviewed Jun 20, 2019

View reviewed changes

aramase commented Jun 21, 2019

View reviewed changes

aramase force-pushed the refactor branch from c4de5ea to 0089aa6 Compare June 21, 2019 17:20

aramase commented Jun 21, 2019

View reviewed changes

aramase added this to the v1.5 milestone Jun 21, 2019

aramase force-pushed the refactor branch 2 times, most recently from 5ecf29d to 20e4e14 Compare June 25, 2019 05:54

aramase added 4 commits June 25, 2019 08:38

remove assigned identity on failure

3a6207b

remove debug logs

92288bf

reorder assigned identity cleanup

6aac224

aramase force-pushed the refactor branch from 2e19b1d to 6aac224 Compare June 25, 2019 17:47

add states and reorganize sync loop

b2e8352

This was referenced Jun 27, 2019

Identity assigned to only 1 pod #227

Closed

[WIP] Add states to assigned identity #255

Closed

kkmsft reviewed Jun 28, 2019

View reviewed changes

refactor batch processing

7baedac

update error scope

7649b3d

kkmsft merged commit 09028ea into Azure:master Jul 1, 2019

aramase deleted the refactor branch July 1, 2019 17:13

This was referenced Jul 3, 2019

Add States for CRD #256

Closed

User Assigned Identity Not Removed from VM #217

Closed

Identity assignment performance issues resulting in slow scale out #145

Closed

aramase self-assigned this Jul 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance Improvement - reduce identity assignment time #219

Performance Improvement - reduce identity assignment time #219

aramase commented May 17, 2019 •

edited

Loading

kkmsft commented May 21, 2019

cpuguy83 May 22, 2019

aramase commented May 22, 2019

kkmsft commented May 22, 2019 •

edited by aramase

Loading

aramase commented May 22, 2019

kkmsft May 24, 2019

aramase May 29, 2019

kkmsft May 24, 2019

aramase May 29, 2019

kkmsft Jun 17, 2019

aramase Jun 17, 2019

aramase commented Jun 17, 2019

aramase Jun 17, 2019

aramase Jun 17, 2019

aramase Jun 17, 2019

kkmsft Jun 20, 2019

kkmsft Jun 20, 2019

aramase Jun 21, 2019

kkmsft Jun 20, 2019

kkmsft Jun 20, 2019

aramase Jun 21, 2019

kkmsft Jun 20, 2019

aramase Jun 21, 2019

kkmsft Jun 20, 2019

aramase Jun 21, 2019

aramase left a comment

aramase commented Jun 25, 2019

kkmsft Jun 28, 2019

aramase commented Jul 1, 2019

Performance Improvement - reduce identity assignment time #219

Performance Improvement - reduce identity assignment time #219

Conversation

aramase commented May 17, 2019 • edited Loading

kkmsft commented May 21, 2019

Choose a reason for hiding this comment

aramase commented May 22, 2019

kkmsft commented May 22, 2019 • edited by aramase Loading

aramase commented May 22, 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

aramase commented Jun 17, 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

aramase left a comment

Choose a reason for hiding this comment

aramase commented Jun 25, 2019

Choose a reason for hiding this comment

aramase commented Jul 1, 2019

aramase commented May 17, 2019 •

edited

Loading

kkmsft commented May 22, 2019 •

edited by aramase

Loading