Modify Machine Creation flow to make sure node label is updated before initialization of VM. Modify Deletion flow to call DeleteMachine even if VM is not found. #940

thiyyakat · 2024-09-05T06:59:55Z

What this PR does / why we need it:

The PR changes triggerCreationFlow to update machine labels before proceeding to initialization. This was done to make sure that the labels are always updated even in the case of initialization failures.

It also changes getVMStatus to update the labels in the following way:

list all the nodes and find matching machine name label ( effective when Introduce a feature to propagate the machine name to user data and as label to the node #919 and Introduce node-agent-authorizer webhook to authenticate gardener-node-agents in shoots gardener#10014 are released)
If the above does not find a node name, do a GetMachineStatus call and populate the node name.

In addition, getVMStatus will always redirect to initiateDrain.

A change was also made to remove error logs when InitializeMachine is not implemented by a provider.

Which issue(s) this PR fixes:
Fixes #934 #936
Fixes part of #933

Special notes for your reviewer:

The changes in the PR were tested by doing the following:
Manually returned an error from machine-controller-manager-provider-azure's CreateMachine code after NIC is created, such that the VM is not created. The machine was then marked for deletion. The logs showed that the NIC was deleted successfully.

Manually returned codes.Uninitialized error code from InitializeMachine for AWS. On triggering creation of machine, found that initialization is tried in a loop after shortRetry.

Manually returned codes.Uninitialized from initializeMachine after the call to driver.initializeMachine. The logs show that the machine state update is successful. In the next reconciliation, since the machine is found to be initialized, machine goes to running.

Release note:

`getVMStatus` always redirects to `InitiateDrain`. It also populates the node label on the machine object by checking `node.gardener.cloud/machine-name` label on the nodes.

Fixed a bug where failure of machine initialization caused label updates to not happen.

gardener-robot · 2024-09-05T07:00:11Z

@thiyyakat Thank you for your contribution.

gardener-robot-ci-2 · 2024-09-05T07:00:23Z

Thank you @thiyyakat for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

gardener-robot · 2024-09-06T07:11:30Z

@thiyyakat You need rebase this pull request with latest master branch. Please check.

…VM not found

unmarshall · 2024-09-09T06:21:17Z

pkg/util/provider/machinecontroller/machine.go

+			return retryPeriod, err
+		}
+		// Return error even when machine object is updated
+		err = fmt.Errorf("Machine creation in process. Machine UPDATE successful")


is this error msg correct? If the initializeMachine call has succeeded and there is no error then this error is created with a msg that perhaps is not very clear. What is intended here?

If we look at the call stack, this error message is propagated to reconcileClusterMachineKey which calls enqueueMachineAfter where this message is used to print a log specifying the reason for the enqueue operation.

This is not something new that has been introduced. We are preserving the original way. Changing the message to Machine creation in process. Machine initialization (if required) and label update successful

unmarshall · 2024-09-09T06:25:45Z

pkg/util/provider/machinecontroller/machine.go

 		}
+	}
+	if uninitializedMachine {
+		retryPeriod, err := c.initializeMachine(ctx, createMachineRequest.Machine, createMachineRequest.MachineClass, createMachineRequest.Secret)


Before calling initializeMachine we now update the labels. This is as we discussed. If there is an update done then the genration of the resource changes, right? Then later you take the machine from the request object (which is now old) and then that is passed into initializeMachine which also attempt to update the status by making a clone of the now old object in case of errors. This could result in conflict. Have you tested this?

unmarshall · 2024-09-09T06:27:57Z

pkg/util/provider/machinecontroller/machine_util.go

+		nodes, err = c.nodeLister.List(labels.Everything())
+		if err == nil {
+			for _, node := range nodes {
+				if node.Labels["node.gardener.cloud/machine-name"] == getMachineStatusRequest.Machine.Name {


in Oliver's PR is there a constant created for this label?

unmarshall · 2024-09-09T06:28:51Z

pkg/util/provider/machinecontroller/machine_util.go

+	// Node label is required for drain of node, therefore we try to update machine object before proceeding to drain.
+	isNodeLabelUpdated := false
+	//check if node name label is already present in machine object
+	nodeName := getMachineStatusRequest.Machine.Labels[v1alpha1.NodeLabelKey]


declare this as a variable instead of short assign.

unmarshall · 2024-09-09T06:31:04Z

pkg/util/provider/machinecontroller/machine_util.go

+		// get all nodes and check if any node has the machine name as label
+		var nodes []*v1.Node
+		nodes, err = c.nodeLister.List(labels.Everything())
+		if err == nil {


if there is an error it is totally ignored. I would suggest that you at least log the error for better debuggability.

unmarshall · 2024-09-09T06:33:04Z

pkg/util/provider/machinecontroller/machine_util.go

+	} else {
+		// Figure out node label either by checking all nodes for label matching machine name or retrieving it using GetMachineStatus
+		// get all nodes and check if any node has the machine name as label
+		var nodes []*v1.Node


extract the logic to look for nodeName matching the machine name into a separate function. That function should just find the matching nodeName and the update should happen in the caller.

unmarshall · 2024-09-09T06:33:29Z

pkg/util/provider/machinecontroller/machine_util.go

@@ -944,77 +944,94 @@ func (c *controller) setMachineTerminationStatus(ctx context.Context, deleteMach
 	return machineutils.ShortRetry, err
 }

-// getVMStatus tries to retrive VM status backed by machine
+// getVMStatus tries to retrieve VM status backed by machine
 func (c *controller) getVMStatus(ctx context.Context, getMachineStatusRequest *driver.GetMachineStatusRequest) (machineutils.RetryPeriod, error) {


the function is named getVMStatus but it does not return any status. This is a bit weird. Shouldn't it be called updateMachineStatusAndNodeLabel instead?

unmarshall · 2024-09-09T06:45:19Z

pkg/util/provider/machinecontroller/machine_util.go

+	isNodeLabelUpdated := false
+	//check if node name label is already present in machine object
+	nodeName := getMachineStatusRequest.Machine.Labels[v1alpha1.NodeLabelKey]
+	if isValidNodeName(nodeName) {


this function is actually quite useless. It only checks if the node Name is empty. I would just remove this function.

unmarshall · 2024-09-09T06:55:35Z

pkg/util/provider/machinecontroller/machine_util.go

 			}
 		}
-
+		if !isNodeLabelUpdated {


It would be better to rewrite this function.
An idea:

func (c *controller) updateMachineNodeLabelAndStatus(ctx context.Context, machine *v1alpha1.Machine) { // calls getNodeName // if there is no error then updates the node label // if there is an error either in calling getNodeName or updating the node label then updates the status accordingly } func (c *controller) getNodeName(ctx context.Context, machine *v1alpha1.Machine) (string, error) { nodeName := machine.Labels[v1alpha1.NodeLabelKey] if len(strings.TrimSpace(nodeName)) > 0 { return nodeName, nil } matchingNode, err := c.fetchMatchingNode(ctx, machine.Name) if err == nil && matchingNode != nil { return matchingNode.Name, nil } if err != nil { klog.Errorf("Error trying to get node matching machine %s: %v. Will try to get the node name by calling driver.GetMachineStatus instead.", machine.Name, err) } // call GetMachineStatus to get the node name }

unmarshall · 2024-09-11T11:24:31Z

pkg/util/provider/machinecontroller/machine.go

 	return machineutils.LongRetry, nil
 }

+func (c *controller) updateLabelsAndInitializeMachine(ctx context.Context, createMachineRequest *driver.CreateMachineRequest, nodeName, providerID string, shouldInitializeMachine bool) (retryPeriod machineutils.RetryPeriod, err error) {
+	_, machineNodeLabelPresent := createMachineRequest.Machine.Labels[v1alpha1.NodeLabelKey]


you could have used:

metav1.HasAnnotation(createMachineRequest.Machine.ObjectMeta, v1alpha1.NodeLabelKey)

pkg/util/provider/machinecontroller/machine.go

unmarshall · 2024-09-11T11:35:25Z

pkg/util/provider/machinecontroller/machine.go

+		}
+	}
+	if shouldInitializeMachine {
+		retryPeriod, err = c.initializeMachine(ctx, clone, createMachineRequest.MachineClass, createMachineRequest.Secret)


why have initializeMachine call from here? I would have just renamed this method to updateMachineLabels since we already have initializeMachine. The just call these functions from triggerCreationFlow. This also removes the need to pass shouldInitializeMachine as a method argument.

unmarshall · 2024-09-11T11:37:48Z

pkg/util/provider/machinecontroller/machine.go

+			return retryPeriod, err
+		}
+		// Return error even when machine object is updated
+		err = fmt.Errorf("Machine creation in process. Machine initialization (if required) is successful.")


messages given to fmt.Errorf should not start with a Capital letter and should not end with a dot. You will see warnings shown in the IDE. Please correct it. There are other places as well where i see this issue. Can you at least correct it in the functions that you are touching?

…ty code

unmarshall

/lgtm

unmarshall · 2024-09-13T08:28:54Z

pkg/util/provider/machinecontroller/machine.go

@@ -497,6 +492,8 @@ func (c *controller) triggerCreationFlow(ctx context.Context, createMachineReque
 		case codes.Uninitialized:
 			uninitializedMachine = true
 			klog.Infof("VM instance associated with machine %s was created but not initialized.", machine.Name)
+			nodeName = getMachineStatusResponse.NodeName


We discussed that we will go with this even though this is quite dirty. We add a comment to improve this further.
The easier way to do this would be to add a bool pointer indicating the status of initialization status. If it is nil, then no initialization needs to be done (ignored). If it is false/true then we act upon it. Essentially do not return an error as initialization becomes part of the VM status.

thiyyakat requested a review from a team as a code owner September 5, 2024 06:59

gardener-robot added the needs/review Needs review label Sep 5, 2024

gardener-robot added the size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) label Sep 5, 2024

thiyyakat 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 Sep 5, 2024

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 Sep 5, 2024

gardener-robot added the needs/rebase Needs git rebase label Sep 6, 2024

thiyyakat added 8 commits September 6, 2024 16:12

Change order in which labels are updated and machine is initialized

47ab69d

Change how node label is updated in getVMStatus. Proceed to drain if …

a507c1b

…VM not found

Remove error log when initializeMachine is unimplemented

1bf7fbb

Change where initializeMachine is called in triggerCreationFlow

bed6a1c

Correct GetVMStatus flow from previous commit

bd56014

Remove unnecessary comments

567b225

Initialize VM and update labels in the same reconciliation

2123a90

Fix make check errors

99cf01d

thiyyakat force-pushed the delete-orphan-nics branch from 1970a2a to 99cf01d Compare September 6, 2024 11:18

rishabh-11 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 Sep 6, 2024

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 Sep 6, 2024

unmarshall requested changes Sep 9, 2024

View reviewed changes

gardener-robot added the needs/changes Needs (more) changes label Sep 9, 2024

thiyyakat changed the title ~~Change Machine Creation and Deletion to Handle Transient Issues in Cloud Provider Calls~~ Modify Machine Creation flow to make sure node label is updated before initialization of VM. Modify Deletion flow to call DeleteMachine even if VM is not found. Sep 9, 2024

Address review comments

946d73e

unmarshall requested changes Sep 11, 2024

View reviewed changes

Address review comments part 2

e548bb6

rishabh-11 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 Sep 12, 2024

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 Sep 12, 2024

gardener-robot-ci-1 added the needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 12, 2024

thiyyakat added 2 commits September 13, 2024 13:59

Add comment for future changes to triggerCreationFlow to clean up dir…

138a929

…ty code

Correct err variable assignment

759e60c

unmarshall approved these changes Sep 13, 2024

View reviewed changes

gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/rebase Needs git rebase needs/review Needs review labels Sep 13, 2024

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 Sep 13, 2024

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 Sep 13, 2024

Shift comment to the right place

c8eef6b

rishabh-11 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 Sep 13, 2024

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 Sep 13, 2024

rishabh-11 merged commit 9bded0e into gardener:master Sep 13, 2024
8 checks passed

gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Sep 13, 2024

rishabh-11 mentioned this pull request Sep 13, 2024

Ensure that DeleteMachine call is made even in case of a NotFound error from Driver.GetMachineStatus in the deletion flow #936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify Machine Creation flow to make sure node label is updated before initialization of VM. Modify Deletion flow to call DeleteMachine even if VM is not found. #940

Modify Machine Creation flow to make sure node label is updated before initialization of VM. Modify Deletion flow to call DeleteMachine even if VM is not found. #940

thiyyakat commented Sep 5, 2024 •

edited by rishabh-11

Loading

gardener-robot commented Sep 5, 2024

gardener-robot-ci-2 commented Sep 5, 2024

gardener-robot commented Sep 6, 2024

unmarshall Sep 9, 2024

thiyyakat Sep 9, 2024

unmarshall Sep 9, 2024

unmarshall Sep 9, 2024

unmarshall Sep 9, 2024

unmarshall Sep 9, 2024

unmarshall Sep 9, 2024

unmarshall Sep 9, 2024

unmarshall Sep 9, 2024

unmarshall Sep 9, 2024

unmarshall Sep 11, 2024

unmarshall Sep 11, 2024

unmarshall Sep 11, 2024

unmarshall left a comment

unmarshall Sep 13, 2024

Modify Machine Creation flow to make sure node label is updated before initialization of VM. Modify Deletion flow to call DeleteMachine even if VM is not found. #940

Modify Machine Creation flow to make sure node label is updated before initialization of VM. Modify Deletion flow to call DeleteMachine even if VM is not found. #940

Conversation

thiyyakat commented Sep 5, 2024 • edited by rishabh-11 Loading

gardener-robot commented Sep 5, 2024

gardener-robot-ci-2 commented Sep 5, 2024

gardener-robot commented Sep 6, 2024

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

unmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiyyakat commented Sep 5, 2024 •

edited by rishabh-11

Loading