Improved NIC creation and deletion logic #594

prashanth26 · 2021-03-15T09:28:34Z

NIC creation now adopts any existing NICs with matching names
NIC deletion confirms deletion by performing GET on the deletion
Improved creation logs to give more details

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes https://github.com/gardener/machine-controller-manager/issues/544

Special notes for your reviewer:

Release note:

Azure: Improved NIC creation and deletion logic to handle NIC creation and deletions more gracefully.

prashanth26 · 2021-03-15T09:29:23Z

pkg/driver/driver_azure.go

+	// Change to something like below, however I am unable to find the helper method for this.
+	// if err != nil && management.IsResourceNotFoundError(err) {


I am having some trouble with validating the error code when NIC doesn't exist. Any suggestions?

cc: @AxiomSamarth , @amshuman-kr

https://github.com/Azure/go-autorest/blob/c7f947c0610de1bc279f76e6d453353f95cd1bfa/autorest/error.go#L41 might be worth a shot.

Based on your suggestion i have updated the logic here to this - https://github.com/gardener/machine-controller-manager/pull/594/files#diff-36365e28193d28c39f36ac4d9172fbf8eac69553361f6179fa3184f9cad9eb0cR1329-R1342. I hope this is more appropriate now.

prashanth26 · 2021-03-16T07:02:49Z

/invite @MSSedusch

amshuman-kr

Thanks @prashanth26! The PR itself looks good. Some optional nit-picks and design pattern suggestions below.

amshuman-kr · 2021-03-16T07:11:06Z

pkg/driver/driver_azure.go

@@ -1035,20 +1047,64 @@ func (clients *azureDriverClients) checkOrphanDisks(ctx context.Context, resourc
 }

 func (clients *azureDriverClients) deleteNIC(ctx context.Context, resourceGroupName string, nicName string) error {
+	var (
+		nicDeletionTimeout     = 10 * time.Minute


Done - https://github.com/gardener/machine-controller-manager/pull/594/files#diff-36365e28193d28c39f36ac4d9172fbf8eac69553361f6179fa3184f9cad9eb0cR1077-R1083.

amshuman-kr · 2021-03-16T07:11:57Z

pkg/driver/driver_azure.go

+		sleepInterval    = 10 * time.Second
+		maxSleepInterval = 3 * time.Minute
+		currentSleepTime = 0 * time.Second


Constants? Or even better, why not use client-go/backoff? https://github.com/kubernetes/client-go/blob/master/util/flowcontrol/backoff.go#L32-L38

I was actually thinking of using a backoff client, didn't know which one to use. Yes, this makes sense. Let me check it out.

I looked through using the client-go/backoff mechanism. However, it looks like it's usecase is a little different compared to ours. It seems like it's keeping a bucket for objects with different names that are being backed off. Refer below

https://github.com/kubernetes/client-go/blob/master/util/flowcontrol/backoff.go#L37

https://github.com/kubernetes/kubernetes/blob/019080fc4d32bc7acc994605afbc2fac416188d7/pkg/controller/daemon/daemon_controller.go#L820-L838

So looked up more appropriate libraries for our use-case. I found this backoff library that seems to be used by a lot of projects. And usage also seems simple and works well in our case. The Licence however is MIT, I hope that should be fine as I see other vendors using similar licenses - https://github.com/cenkalti/backoff/blob/v4/LICENSE.

pkg/driver/driver_azure.go

prashanth26 · 2021-03-19T09:45:29Z

/ok-to-test

prashanth26 · 2021-03-19T12:15:05Z

/ok-to-test

prashanth26 · 2021-03-22T05:06:20Z

/needs review

prashanth26 · 2021-03-22T10:30:11Z

@AxiomSamarth - We will probably need to incorporate this change prior to the MCM azure provider release.

AxiomSamarth · 2021-03-24T14:46:15Z

pkg/driver/driver_azure.go

-	/*
-		NIC creation
-	*/
+	NIC, err := clients.nic.Get(ctx, resourceGroupName, nicName, "")


Hey Prashanth,

If this Get() call returns an error apart from 404 Not Found, it will result in nil pointer error at line 634 VMParameters := d.getVMParameters(vmName, vmImageRef, *NIC.ID) as it will have no reference to NIC.

Found this issue while I was incorporating this in OOT and wrote a UT for the same. Please let me know your thoughts about it.

Nice catch. This is why we need tests. :)

I have tried to fix this scenario with this commit. Please let me know if it makes sense. The commit looks a little confusing, however in reality it is only one extra if condition. Not sure why GitHub shows the diff changes differently. Locally it looks much less.

Perfect! This works like a charm! Thank you :)

AxiomSamarth · 2021-03-24T17:28:52Z

pkg/driver/driver_azure.go

+				return nil
+			}
+
+			klog.V(4).Infof("NIC doesn't existance for %q", nicName)


Can we keep the log as simple as NIC not found? The current one does not seem to be so good. Just a thought.

Done. Here is the change - 3c0e156.

amshuman-kr

Thanks a lot for the change @prashanth26! and sorry for the delay in approving the PR. LGTM

prashanth26 · 2021-03-25T07:11:55Z

Thanks @amshuman-kr . Will squash all changes, run the changes once more and then merge it.

- NIC creation now, adopts any existing NICs with matching name - NIC deletion confirms deletion by performing GET on deletion - Improved creation logs to give more details - Revendored additional libraries

prashanth26 · 2021-03-25T11:42:59Z

Made a final round of test. Looks good to me. Merging now.

prashanth26 requested review from amshuman-kr and AxiomSamarth March 15, 2021 09:28

prashanth26 requested a review from a team as a code owner March 15, 2021 09:28

gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Mar 15, 2021

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 15, 2021

prashanth26 commented Mar 15, 2021

View reviewed changes

gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 15, 2021

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 16, 2021

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 16, 2021

gardener-robot requested a review from MSSedusch March 16, 2021 07:02

amshuman-kr reviewed Mar 16, 2021

View reviewed changes

AxiomSamarth approved these changes Mar 16, 2021

View reviewed changes

gardener-robot added size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else and removed size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Mar 19, 2021

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 19, 2021

prashanth26 force-pushed the fix/azure-orphan-nics branch from e461713 to 69a2814 Compare March 19, 2021 08:57

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 19, 2021

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 19, 2021

prashanth26 force-pushed the fix/azure-orphan-nics branch from c3e3b06 to 4ff6c55 Compare March 19, 2021 09:29

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 19, 2021

gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 19, 2021

prashanth26 force-pushed the fix/azure-orphan-nics branch from 4ff6c55 to 99eb67d Compare March 19, 2021 09:46

gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 19, 2021

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 19, 2021

prashanth26 force-pushed the fix/azure-orphan-nics branch from 99eb67d to 398d725 Compare March 19, 2021 10:47

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 19, 2021

prashanth26 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 19, 2021

gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 19, 2021

AxiomSamarth reviewed Mar 24, 2021

View reviewed changes

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 24, 2021

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 24, 2021

AxiomSamarth reviewed Mar 24, 2021

View reviewed changes

gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 25, 2021

amshuman-kr approved these changes Mar 25, 2021

View reviewed changes

Improved NIC creation and deletion logic

570486e

- NIC creation now, adopts any existing NICs with matching name - NIC deletion confirms deletion by performing GET on deletion - Improved creation logs to give more details - Revendored additional libraries

prashanth26 force-pushed the fix/azure-orphan-nics branch from 3c0e156 to 570486e Compare March 25, 2021 10:43

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 25, 2021

gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Mar 25, 2021

prashanth26 merged commit 85382aa into gardener:master Mar 25, 2021

prashanth26 deleted the fix/azure-orphan-nics branch April 28, 2021 03:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved NIC creation and deletion logic #594

Improved NIC creation and deletion logic #594

prashanth26 commented Mar 15, 2021 •

edited

prashanth26 Mar 15, 2021 •

edited

amshuman-kr Mar 16, 2021 •

edited

prashanth26 Mar 19, 2021

prashanth26 commented Mar 16, 2021

amshuman-kr left a comment

amshuman-kr Mar 16, 2021

prashanth26 Mar 19, 2021

amshuman-kr Mar 16, 2021

prashanth26 Mar 16, 2021

prashanth26 Mar 19, 2021

prashanth26 commented Mar 19, 2021

prashanth26 commented Mar 19, 2021

prashanth26 commented Mar 22, 2021

prashanth26 commented Mar 22, 2021

AxiomSamarth Mar 24, 2021

prashanth26 Mar 24, 2021 •

edited

AxiomSamarth Mar 24, 2021 •

edited

AxiomSamarth Mar 24, 2021

prashanth26 Mar 25, 2021

amshuman-kr left a comment

prashanth26 commented Mar 25, 2021

prashanth26 commented Mar 25, 2021

		// Change to something like below, however I am unable to find the helper method for this.
		// if err != nil && management.IsResourceNotFoundError(err) {

Improved NIC creation and deletion logic #594

Improved NIC creation and deletion logic #594

Conversation

prashanth26 commented Mar 15, 2021 • edited

prashanth26 Mar 15, 2021 • edited

Choose a reason for hiding this comment

amshuman-kr Mar 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashanth26 commented Mar 16, 2021

amshuman-kr left a 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

prashanth26 commented Mar 19, 2021

prashanth26 commented Mar 19, 2021

prashanth26 commented Mar 22, 2021

prashanth26 commented Mar 22, 2021

Choose a reason for hiding this comment

prashanth26 Mar 24, 2021 • edited

Choose a reason for hiding this comment

AxiomSamarth Mar 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshuman-kr left a comment

Choose a reason for hiding this comment

prashanth26 commented Mar 25, 2021

prashanth26 commented Mar 25, 2021

prashanth26 commented Mar 15, 2021 •

edited

prashanth26 Mar 15, 2021 •

edited

amshuman-kr Mar 16, 2021 •

edited

prashanth26 Mar 24, 2021 •

edited

AxiomSamarth Mar 24, 2021 •

edited