Upgrade to latest Azure Go SDK #105

unmarshall · 2023-08-31T11:59:28Z

What this PR does / why we need it:
Azure SDK go-autorest is now out of support. This PR revamps the codebase and uses the latest SDK.

Which issue(s) this PR fixes:
Fixes #91, #41

Special notes for your reviewer:

Release note:

Uses new Azure SDK as the older go-autorest is out of support. 
Adds 2 new metrics which compute driver API call duration and Azure API call duration for all successful API calls.
Recently introduced Azure fakes are used extensively for unit tests.
Driver.GetMachineStatus now only gets the status from the Machine and not from associated NIC(s).
Deletion of a machine now cascade deletes NIC(s) and Disk(s) (OSDisk and DataDisk(s)) as well. Previously it was a 2 step process of detatch followed by a delete.
In the API following have been marked as deprecated:
- Constants: [api.AzureClientID, api.AzureClientSecret, api.AzureSubscriptionID, api.AzureTenantID, api.AzureAlternativeClientID, api.AzureAlternativeClientSecret, api.AzureAlternativeSubscriptionID, api.AzureAlternativeTenantID, api.MachineSetKindVMO and api.MachineSetKindAvailabilitySet]
- AzureVirtualMachineProperties.MachineSet has been marked as deprecated

…letes of disks and nics

…tatus

himanshu-kun

9th set of review done

Pls also update the following:

update go version in Dockerfile
MCM_VERSION -> v0.50.1

himanshu-kun · 2023-11-09T06:26:57Z

pkg/azure/validation/validation_test.go

+		{"should forbid empty subnetName and vnetName",
+			"", "", 2,
+			ConsistOf(
+				PointTo(MatchFields(IgnoreExtras, Fields{"Type": Equal(field.ErrorTypeRequired), "Field": Equal("providerSpec.subnetInfo.vnetName")})),
+				PointTo(MatchFields(IgnoreExtras, Fields{"Type": Equal(field.ErrorTypeRequired), "Field": Equal("providerSpec.subnetInfo.subnetName")})),
+			),
+		},


the one with description should forbid empty subnetName and vnetName. because you are already checking them individually before, so why both now?

Makefile

pkg/azure/provider/provider_test.go

himanshu-kun · 2023-11-09T06:57:51Z

pkg/azure/provider/provider_test.go

+	g.Expect(err).ToNot(BeNil())
+	var statusErr *status.Status
+	g.Expect(errors.As(err, &statusErr)).Should(BeTrue())
+	g.Expect(statusErr.Code()).To(Equal(codes.InvalidArgument))


We only wanted a validation for errorMessage, but if you feel its not correct, then its fine. We think the earlier way of validation was much simpler and easy to understand. Could you pls revert.

pkg/azure/provider/provider_test.go

NewTokenCredetial from fake package upstream

himanshu-kun

/lgtm
Thanks for the clean PR , it was pleasant reviewing it :)

rishabh-11 · 2023-11-17T10:18:37Z

/lgtm
Finally, the review is over.

unmarshall and others added 30 commits August 31, 2023 17:17

wip commit for sdk changes

245cd00

second set of changes

39f708e

wip changes

3ccf9a9

wip- added metric to capture duration, misc refactoring

448275f

added resourcegroup and disk helper

13fd2de

wip - added code to delete machine

4ef5a60

minor changes

00ad67c

corrected runparallel

c49802a

fixed concurrentruns and added tests

ea6177f

adapted metric name change

7134f14

added code to check if disk/nic is managed by a non-existing vm

541c8fb

fake tests wip

be3c08f

corrections, added code for GetMachineStatus

a80b6f5

WIP: fake simpleunit test

e546292

full test for delete virtual machine

4a45541

fixed small typo

8b070f8

naming changes, added tokencredentialprovider, changed unit tests

c700e05

refactored unit tests, introduced fakes package

8ac41b9

added fakes, code to create nic

98cf657

additional helper methods added

df40a7b

refactored code for fake unit tests

57e0b6f

added DeleteMachine table tests

0b15062

added tests for delete machine, corrected fakes, change concurrent de…

70aa5e3

…letes of disks and nics

added unit tests for GetMachineStatus

66c491e

moved to latest beta version and minor change in test for GetMachineS…

b981479

…tatus

added support to test with injecting errors

8396504

added additional tests for DeleteMachine covering error scenarios

1bc43fa

added unit tests for ListMachines

29a1f48

added unit test to cover panic handling during delete of resources

331caa6

introduce functions for vm creation, general refactoring

c2a30b4

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 Nov 8, 2023

himanshu-kun suggested changes Nov 9, 2023

View reviewed changes

addressed review comments

4253351

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 Nov 9, 2023

Incremented versions of dependencies, adopted to removal of

8c9b033

NewTokenCredetial from fake package upstream

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 Nov 16, 2023

changed log and added new log in delete pending nic

b316051

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 Nov 16, 2023

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 Nov 16, 2023

unmarshall requested a review from himanshu-kun November 17, 2023 06:48

himanshu-kun approved these changes Nov 17, 2023

View reviewed changes

gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review needs/second-opinion Needs second review by someone else labels Nov 17, 2023

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 Nov 17, 2023

gardener-robot-ci-2 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 Nov 17, 2023

unmarshall 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 Nov 17, 2023

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 Nov 17, 2023

rishabh-11 approved these changes Nov 17, 2023

View reviewed changes

unmarshall merged commit 02e0c9f into gardener:master Nov 17, 2023
7 checks passed

gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Nov 17, 2023

rishabh-11 mentioned this pull request Mar 26, 2024

Orphaned Azure NICs block subnet deletion afterwards #41

Closed

rishabh-11 mentioned this pull request Jul 3, 2024

Enhance MCM metrics gardener/machine-controller-manager#872

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to latest Azure Go SDK #105

Upgrade to latest Azure Go SDK #105

unmarshall commented Aug 31, 2023 •

edited by rishabh-11

Loading

himanshu-kun left a comment

himanshu-kun Nov 9, 2023

himanshu-kun Nov 9, 2023

himanshu-kun left a comment

rishabh-11 commented Nov 17, 2023 •

edited

Loading

Upgrade to latest Azure Go SDK #105

Upgrade to latest Azure Go SDK #105

Conversation

unmarshall commented Aug 31, 2023 • edited by rishabh-11 Loading

himanshu-kun left a comment

Choose a reason for hiding this comment

himanshu-kun Nov 9, 2023

Choose a reason for hiding this comment

himanshu-kun Nov 9, 2023

Choose a reason for hiding this comment

himanshu-kun left a comment

Choose a reason for hiding this comment

rishabh-11 commented Nov 17, 2023 • edited Loading

unmarshall commented Aug 31, 2023 •

edited by rishabh-11

Loading

rishabh-11 commented Nov 17, 2023 •

edited

Loading