Skip to content

Commit

Permalink
addressed Rishab's review comments part 2
Browse files Browse the repository at this point in the history
  • Loading branch information
@himanshu-kun
himanshu-kun committed Dec 15, 2023
1 parent b73c147 commit 9d41bf7
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 51 deletions.
28 changes: 12 additions & 16 deletions pkg/util/provider/machinecontroller/machine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func (c *controller) updateMachine(oldObj, newObj interface{}) {
return
}

if oldMachine.ResourceVersion != newMachine.ResourceVersion && oldMachine.Generation == newMachine.Generation {
if oldMachine.Generation == newMachine.Generation {
klog.V(3).Infof("Skipping non-spec updates for machine %s", oldMachine.Name)
return
}
Expand Down Expand Up @@ -342,6 +342,7 @@ func addedOrRemovedEssentialTaints(oldNode, node *corev1.Node, taintKeys []strin
_, oldNodeHasTaint := mapOldNodeTaintKeys[tk]
_, newNodeHasTaint := mapNodeTaintKeys[tk]
if oldNodeHasTaint != newNodeHasTaint {
klog.V(2).Infof("Taint with key %q has been added/removed from the node %q", tk, node.Name)
return true
}
}
Expand Down Expand Up @@ -443,7 +444,7 @@ func (c *controller) triggerCreationFlow(ctx context.Context, createMachineReque
}

// machine obj marked Failed for double security
updateErr := c.machineStatusUpdate(
updateRetryRequired, updateErr := c.machineStatusUpdate(
ctx,
machine,
v1alpha1.LastOperation{
Expand All @@ -458,10 +459,9 @@ func (c *controller) triggerCreationFlow(ctx context.Context, createMachineReque
},
machine.Status.LastKnownState,
)
if apierrors.IsConflict(updateErr) {
return machineutils.ConflictRetry, err
} else if updateErr != nil {
return machineutils.ShortRetry, updateErr

if updateErr != nil {
return updateRetryRequired, updateErr
}

klog.V(2).Infof("Machine %q marked Failed as VM was referring to a stale node object", machine.Name)
Expand All @@ -475,7 +475,7 @@ func (c *controller) triggerCreationFlow(ctx context.Context, createMachineReque
case codes.Unknown, codes.DeadlineExceeded, codes.Aborted, codes.Unavailable:
// GetMachineStatus() returned with one of the above error codes.
// Retry operation.
updateErr := c.machineStatusUpdate(
updateRetryRequired, updateErr := c.machineStatusUpdate(
ctx,
machine,
v1alpha1.LastOperation{
Expand All @@ -490,16 +490,14 @@ func (c *controller) triggerCreationFlow(ctx context.Context, createMachineReque
},
machine.Status.LastKnownState,
)
if apierrors.IsConflict(updateErr) {
return machineutils.ConflictRetry, err
} else if updateErr != nil {
return machineutils.ShortRetry, updateErr
if updateErr != nil {
return updateRetryRequired, updateErr
}

return machineutils.ShortRetry, err

default:
updateErr := c.machineStatusUpdate(
updateRetryRequired, updateErr := c.machineStatusUpdate(
ctx,
machine,
v1alpha1.LastOperation{
Expand All @@ -514,10 +512,8 @@ func (c *controller) triggerCreationFlow(ctx context.Context, createMachineReque
},
machine.Status.LastKnownState,
)
if apierrors.IsConflict(updateErr) {
return machineutils.ConflictRetry, err
} else if updateErr != nil {
return machineutils.MediumRetry, updateErr
if updateErr != nil {
return updateRetryRequired, updateErr
}

return machineutils.MediumRetry, err
Expand Down
4 changes: 2 additions & 2 deletions pkg/util/provider/machinecontroller/machine_safety.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ func (c *controller) reconcileClusterMachineSafetyAPIServer(key string) error {
klog.V(2).Infof("SafetyController: Reinitializing machine health check for machine: %q with backing node: %q and providerID: %q", machine.Name, getNodeName(machine), getProviderID(machine))
}

// En-queue after 30 seconds, to ensure all machine phases are reconciled to their actual value
// as they are currently reset to `Running`
// En-queue after 30 seconds, to ensure all machine phases are reconciled to their actual value
// as they are currently reset to `Running`
c.enqueueMachineAfter(machine, 30*time.Second, "kube-api-servers are up again, so reconcile of machine phase is needed")
}

Expand Down
64 changes: 31 additions & 33 deletions pkg/util/provider/machinecontroller/machine_util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ func (c *controller) machineCreateErrorHandler(ctx context.Context, machine *v1a
lastKnownState = createMachineResponse.LastKnownState
}

updateErr := c.machineStatusUpdate(
updateRetryRequired, updateErr := c.machineStatusUpdate(
ctx,
machine,
v1alpha1.LastOperation{
Expand All @@ -498,10 +498,9 @@ func (c *controller) machineCreateErrorHandler(ctx context.Context, machine *v1a
},
lastKnownState,
)
if apierrors.IsConflict(updateErr) {
return machineutils.ConflictRetry, err
} else if updateErr != nil {
return retryRequired, updateErr

if updateErr != nil {
return updateRetryRequired, updateErr
}

return retryRequired, err
Expand All @@ -513,15 +512,15 @@ func (c *controller) machineStatusUpdate(
lastOperation v1alpha1.LastOperation,
currentStatus v1alpha1.CurrentStatus,
lastKnownState string,
) error {
) (machineutils.RetryPeriod, error) {
clone := machine.DeepCopy()
clone.Status.LastOperation = lastOperation
clone.Status.CurrentStatus = currentStatus
clone.Status.LastKnownState = lastKnownState

if isMachineStatusSimilar(clone.Status, machine.Status) {
klog.V(3).Infof("Not updating the status of the machine object %q, as the content is similar", clone.Name)
return nil
return machineutils.ShortRetry, nil
}

_, err := c.controlMachineClient.Machines(clone.Namespace).UpdateStatus(ctx, clone, metav1.UpdateOptions{})
Expand All @@ -532,7 +531,11 @@ func (c *controller) machineStatusUpdate(
klog.V(2).Infof("Machine/status UPDATE for %q", machine.Name)
}

return err
if apierrors.IsConflict(err) {
return machineutils.ConflictRetry, err
}

return machineutils.ShortRetry, err
}

// isMachineStatusSimilar checks if the status of 2 machines is similar or not.
Expand Down Expand Up @@ -966,7 +969,7 @@ func (c *controller) getVMStatus(ctx context.Context, getMachineStatusRequest *d
}
}

updateErr := c.machineStatusUpdate(
updateRetryRequired, updateErr := c.machineStatusUpdate(
ctx,
getMachineStatusRequest.Machine,
v1alpha1.LastOperation{
Expand All @@ -981,10 +984,9 @@ func (c *controller) getVMStatus(ctx context.Context, getMachineStatusRequest *d
getMachineStatusRequest.Machine.Status.CurrentStatus,
getMachineStatusRequest.Machine.Status.LastKnownState,
)
if apierrors.IsConflict(updateErr) {
return machineutils.ConflictRetry, err
} else if updateErr != nil {
return retry, updateErr

if updateErr != nil {
return updateRetryRequired, updateErr
}

return retry, err
Expand Down Expand Up @@ -1170,7 +1172,7 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver
}
}

updateErr := c.machineStatusUpdate(
updateRetryRequired, updateErr := c.machineStatusUpdate(
ctx,
machine,
v1alpha1.LastOperation{
Expand All @@ -1185,10 +1187,9 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver
machine.Status.CurrentStatus,
machine.Status.LastKnownState,
)
if apierrors.IsConflict(updateErr) {
return machineutils.ConflictRetry, err
} else if updateErr != nil {
return machineutils.ShortRetry, updateErr

if updateErr != nil {
return updateRetryRequired, updateErr
}

return machineutils.ShortRetry, err
Expand Down Expand Up @@ -1239,7 +1240,7 @@ func (c *controller) deleteNodeVolAttachments(ctx context.Context, deleteMachine
}
now := metav1.Now()
klog.V(4).Infof("(deleteVolumeAttachmentsForNode) For node %q, machine %q, set LastOperation.Description: %q", nodeName, machine.Name, description)
updateErr := c.machineStatusUpdate(
updateRetryRequired, updateErr := c.machineStatusUpdate(
ctx,
machine,
v1alpha1.LastOperation{
Expand All @@ -1251,10 +1252,9 @@ func (c *controller) deleteNodeVolAttachments(ctx context.Context, deleteMachine
machine.Status.CurrentStatus,
machine.Status.LastKnownState,
)
if apierrors.IsConflict(updateErr) {
return machineutils.ConflictRetry, err
} else if updateErr != nil {
return retryPeriod, updateErr

if updateErr != nil {
return updateRetryRequired, updateErr
}

return retryPeriod, err
Expand Down Expand Up @@ -1308,7 +1308,7 @@ func (c *controller) deleteVM(ctx context.Context, deleteMachineRequest *driver.
lastKnownState = deleteMachineResponse.LastKnownState
}

updateErr := c.machineStatusUpdate(
updateRetryRequired, updateErr := c.machineStatusUpdate(
ctx,
machine,
v1alpha1.LastOperation{
Expand All @@ -1323,10 +1323,9 @@ func (c *controller) deleteVM(ctx context.Context, deleteMachineRequest *driver.
machine.Status.CurrentStatus,
lastKnownState,
)
if apierrors.IsConflict(updateErr) {
return machineutils.ConflictRetry, err
} else if updateErr != nil {
return retryRequired, updateErr

if updateErr != nil {
return updateRetryRequired, updateErr
}

return retryRequired, err
Expand Down Expand Up @@ -1365,7 +1364,7 @@ func (c *controller) deleteNodeObject(ctx context.Context, machine *v1alpha1.Mac
err = fmt.Errorf("Machine deletion in process. No node object found")
}

updateErr := c.machineStatusUpdate(
updateRetryRequired, updateErr := c.machineStatusUpdate(
ctx,
machine,
v1alpha1.LastOperation{
Expand All @@ -1380,10 +1379,9 @@ func (c *controller) deleteNodeObject(ctx context.Context, machine *v1alpha1.Mac
machine.Status.CurrentStatus,
machine.Status.LastKnownState,
)
if apierrors.IsConflict(updateErr) {
return machineutils.ConflictRetry, err
} else if updateErr != nil {
return machineutils.ShortRetry, updateErr

if updateErr != nil {
return updateRetryRequired, updateErr
}

return machineutils.ShortRetry, err
Expand Down

0 comments on commit 9d41bf7

Please sign in to comment.