Add support for Preservation of Machines and Backing nodes

[thiyyakat]

What this PR does / why we need it:

This PR introduces a feature that allows operators and endusers to preserve a machine/node and the backing VM for diagnostic purposes.

The expected behaviour, use cases and usage are detailed in the proposal that can be found here

Which issue(s) this PR fixes:
Fixes #1008

Special notes for your reviewer:

Note: This section will be changed once I raise the PR

1. The code changes have been manually tested using the mcm-provider-virtual.
2. Some lines have been introduced in machine.go for simulating failure and recovery of nodes with the help of label "test-failed". These will be removed when I raise the PR.
3. All the unit tests pass
4. The following cases have been tested manually:

• When random value assigned to preserve on machine:
  • if node exists with annotation, node value synced
  • print log ()
• When preserve=now is set on node and machine has no annotations:
  • CA scaledown disable annotations must be set on node
  • Node condition must be set for Preserved
  • annotation should be synced to MC
  • machine status.currentstatus.preserveexpirytime should be set
  • when changed to preserve=false:
    • annotation synced to MC
    • node condition should be set to false
    • CA annotation should be removed
    • machine status.currentstatus.preserveexpirytime should be set to nil (removed)
• When preserve=now is set on machine and node has no annotations:
  • CA scaledown disable annotations must be set on node
  • Node condition must be set for Preserved
  • Annotations must not be synced to node
  • machine status.currentstatus.preserveexpirytime should be set
  • when changed to preserve=false:
    • annotation should not be synced to node
    • node condition should be set to false
    • CA annotation should be removed
    • machine status.currentstatus.preserveexpirytime should be set to nil (removed)
• When preserve=when-failed is set on machine:
  • annotation should not be set on the node
  • check if status.currentstatus.phase== failed, if yes:
    • machine status.currentstatus.preserveexpirytime should be set on machine
    • CA annotation should be added on node
    • node condition should be added on node
  • when changed to preserve=false:
    • machine status.currentstatus.preserveexpirytime should be set to nil,if preserved
    • CA annotations should be removed from node and machine
    • if machine still in Failed phase, terminate machine
• When annotation value for preserve changes from now to when-failed:
  • annotation should be changed on node too
  • check if phase is Failed:
    • if yes, only change preserveexpirytime
    • if no, set preserveexpirytime to nil
• When annotation value changes from when-failed to now:
  • annotation should be changed on node too
  • check if machine already preserved:
    • update preserveexpirytime if yes
    • if no,
      • set preserveexpirytime
      • CA annotations should be added to node
• deletion should be de-prioritised in case of k scale mcd , and if machine is preserved
  • in case of scale down anyway, deletion must go through to maintain replica count
• If a preserved Failed Machine moves to Running, pods must get scheduled on to it

5. Additionally, I have manually tested if the code interferes with RollingUpdates - it doesn't seem to. I have also checked if drain works, only with the virtual-provider though.

TODO:

1. If autoscaler annotation is modified by user, it should be reset to true
2. Add unit tests
3. Test behaviour if isMachineCandidateForPreservation() in manageReplicas() returns an error.

Release note:

[gardener-robot]

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

[thiyyakat]

Questions that remain unanswered:

1. On recovery of a preserved machine, it transitions from Failed to Running. However, if the preserve annotation was when-failed, then the node continues to be preserved in Running even though the annotation says when-failed - is that okay? The node needs to be preserved so that pods can get scheduled onto it without CA scaling it down.
2. drain timeout is checked currently by calculating time from LastUpdateTime (from when machine moved to Failed) to now. Is there a better way to do it?
   timeOutOccurred = utiltime.HasTimeOutOccurred(machine.Status.CurrentStatus.LastUpdateTime, timeOutDuration)
   In the normal drain, it is checked wrt DeletionTimestamp
3. In some parts of the code, checks are performed to see if the returned error is due to a Conflict, and ConflictRetry rather than ShortRetry is returned. When should these checks be performed? The preservation flow has a lot of update calls.

[thiyyakat]

Note: A review meeting was held today for this PR. The comments were given during the meeting.

During the meeting, we revisited the decision to move drain to Failed state for preserved machine. The reason discussed previously was that it didn't make sense semantically to move the machine to Terminating and then do the drain, because there is a possibility that the machine may recover. Since Terminating is a final state, the drain (separate from the drain in triggerDeletionFlow) will be performed in Failed phase. There was no change proposed during the meeting. This design decision was only reconfirmed.

[takoverflow]

Have only gone through half of the PR, have some suggestions PTAL.

[takoverflow]

IsPreserveExpiryTimeSet already checks that the time is non-zero, then only HasPreservationTimedOut is called.
Is there any reason to perform the redundant IsZero check for PreserveExpiryTime again in HasPreservationTimedOut?
I don't see the function being called elsewhere as well.

If the zero check is removed, it could just be simplified to

func HasPreservationTimedOut(m *v1alpha1.Machine) bool {
	return !m.Status.CurrentStatus.PreserveExpiryTime.After(time.Now())
}

[takoverflow]

Consider renaming this to preservedConditionFalse?

[takoverflow]

Is there a reason why there are two get and update calls made for a node, can these not be combined into a single atomic node object update?

And I know this is not part of your PR but can we update this RemoveAnnotation function, it's needlessly complicated.
All you have to do after fetching the object and checking that annotations are non-nil is

delete(obj.Annotations, annotationKey)

Creating a dummy annotation map, then passing it and then creating a new map which doesn't have the key. All of this complication can be avoided.

[thiyyakat]

By 2 Get() calls are you referring to the call within AddOrUpdateConditionsOnNode and the following Get() here:
latestNode, err := c.targetCoreClient.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{})?

The first one can be avoided if we didn't use the function. The second one is required because step 1 adds conditions to the node object, and the function does not return the updated node object. Fetching from the cache doesn't guarantee an up-to-date node object (tested this out empirically). I could potentially avoid fetching the objects if I didn't use the function. Will test it out.

The two update calls cannot be combined since step 1 requires an UpdateStatus() call, and step 2 updates the Spec, and requires an Update() call.

I will update the RemoveAnnotation function as recommended by you.

Edit: The RemoveAnnotation function returns a boolean indicating whether or not an update is needed. This value is being used in other usages of the function. The function cannot be updated. I will use your suggestion instead of using the function since the boolean value is not required in this case.

[takoverflow]

Can there be scenarios where the preserveExpiryTime hasn't been set but the node has preserve conditions
and scale-down disabled annotation added to it? If so, then the removal will never proceed right?

Please let me know if it's not a possible scenario.

[thiyyakat]

The setting of the preserveExpiryTime is the first step in machine preservation. Node conditions and the CA annotation are added only if the step 1 completes successfully. However, if a user manually adds the CA annotation and the node condition, but not the preserveExpiryTime then the case you described may occur. I'm not sure we should handle that case though.

[takoverflow]

What happens when a machine doesn't have the nodeName set i.e. nodeName is "" and isExpirySet is false.
Wouldn't this always fail? Why even try to get the node in that case?

Why not move the check above outside of this function i.e. inside preserveMachine, fetch the nodeName and
use isExpirySet. WDYT?

	if nodeName == "" && isExpirySet {
		return true, nil
	}
    if isExpirySet {
        isComplete, err := c.isMachinePreservationComplete(machine, nodeName)
	    if err != nil {
		    return err
	    }
    }

[takoverflow]

Please revisit this case, the comments and the code seem to contradict each other, if you wish to compare oldMachine annotation value with the newMachine
to make decisions to stop preservation etc, consider utilising updateMachine which would have both objects available.