Add proposal for temporary preservation of machines

[thiyyakat]

What this PR does / why we need it:

This PR introduces a proposal to support the temporary preservation of machines.

The use cases have been discussed here.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Added proposal for the temporary preservation of machines.

[unmarshall]

Suggested change

	* Users can request MCM to delete a preserved `Failed` machine, even before the timeout expires
	* Users can request MCM to release a preserved `Failed` machine, even before the timeout expires, so that MCM can transition the machine to `Terminating` phase and trigger its deletion.

[unmarshall]

Suggested change

	## Solution Design
	## Proposal

[unmarshall]

Allow user/operator to explicitly request for preservation of a machine if it moves to Failed phase with the use of an annotation

If the machine already moves to FAILED state and if there is still capacity to preserve MCM will automatically preserve it and if there is no capacity to reserve then it will swiftly move it to termination and trigger its deletion, giving user no chance to influence. I think you meant provide a user an option to preserve specific machines which are not yet in FAILED state, right?

[thiyyakat]

Yes. Will reword and disambiguate.

[unmarshall]

This needs to be reworded and brought out as this is an open question.
Open point:
Should MCM provide an ability to the consumer to trigger drain on the preserved FAILED machine?

[unmarshall]

not clear why User removes node.machine.sapcloud.io/preserve-when-failed=true] → (Running) is here

[thiyyakat]

For completeness. If the annotation is removed from a healthy machine, the state transitions to (Running) from (Running+Requested), making it a candidate for auto-preservation by MCM on failure, and also allowing users to re-annotate.

[unmarshall]

This case only talks about Operator suspects a machine might fail and wants to ensure preservation for analysis.
Should it also include explicit release of the preserved-failed machine? Is that not covered in Use Case 4 (early release)?

[etiennnr]

Feel free to reach out to me directly for precision!

[etiennnr]

TLDR: I would not restrict this feature to failing node.

Eg. it can happen that we detect problems with essentially all pods on a node but that this node doesn't report any condition failures (aka the node/machine will not be in failed state).

From an SRE perspective, we want to be as available as possible. Thus, in these kind of cases, we would cordon / drain the pods (except daemonSets) and start investigating the node. Furthermore, since expertise is spread around the globe, we sometimes need to keep a node in a cordoned state for 24-28 hours in order to investigate the root cause with the right area's expert. However, if a node is cordoned with not workload on it, it has very high chances to be scheduled for scale down by CA first.

Thus, this feature should also work for non failing nodes in order to cover all cases.

[thiyyakat]

We will extend the feature to preserve non-failing nodes as well. Will update the proposal.

[ashwani2k]

I feel, we should not generalize non-failing node, as it will introduce more complexity to the state management. We should only enable for cases where the node is cordoned and drained, also I'm guessing we need to do some changes in CA handling for unneeded machines as well as I don't see mentioned in this proposal.

[etiennnr]

FYI, it's rare that many nodes need to be investigated. Since configuration is pretty standard across all node of a same worker group, investigating 1 node is usually enough. That being said, I don't have anything against this protection!

[etiennnr]

I'd put the default of that to 48 or 72 hours as a default. We require expertise from around the world. We also have to include weekends. Since this is going to be a setting shoot-wide, setting it to 3h would in many cases render this feature useless since we can't really change settings in the shoot yaml without the shoot-owner approval.

That being said, if the shoot-owner chooses to set a low amount like 3h on purpose, well then they made the choice to limit support on problematic nodes.

[ashwani2k]

I'll also agree that if we are preserving we preserve it for duration of 72hrs as default for preserveTimeout and PreserveMax to be 1 as default if set.
We should also make it explicit that this setting is per worker pool in the doc.

[etiennnr]

Making the annotation be available on both machine and node object is important IMHO, so that any shoot-operator can investigate a failing node by themselves (self troubleshoot)

[ashwani2k]

Why have it at machine level at all, if you plan to have it at node level?
Won't just having it at node level suffice, like any other annotation we expose for the end user to play with the nodes.

[unmarshall]

Not all machine's will have backing Node. So if there is a machine that has not initialized properly to become a K8S Node and if you wish to wish to preserve the backing VM then a provision should be provided to annotate the machine instead. Ofcourse this can only be done by an operator who has access to the Machine objects.

[ashwani2k]

Got it.

[etiennnr]

Why not call the status Preserving instead of PreserveFailed (aka as I said above, this would be useful for machines / nodes not detected as failed too)

[ashwani2k]

I believe we need a new state diagram here -- Healthy --> Perserved as well Failed --> Preserved.
I'm guessing removal of preserved should bring back the machine to either failed or healthy state as it be the case at the point in time ?

[etiennnr]

This can be risky of having many nodes in PreserveFailed mainly if we use a relatively high failedMachinePreserveTimeout, even with failedMachinePreserveMax 'cause then a failing worker pool could make multiple nodes getting in PreserveFailed. And depending on how MCM chooses which node to keep / replace in PreserveFailed, could have undesired side effects.

Maybe make that available as an option in the shoot yaml (but defaults to false)?

[etiennnr]

FYI changing values directly in the shoot YAML is not always a possibility for operators (E.g. no permissions to edit the shoot YAML (but have admin permission in the cluster). That being said, this is also a valid use case

[ezadik]

@thiyyakat
I thought that the values of the "failedMachinePreserveMax" and "failedMachinePreserveTimeout" can be defined in yaml per worker-group?

[thiyyakat]

@thiyyakat I thought that the values of the "failedMachinePreserveMax" and "failedMachinePreserveTimeout" can be defined in yaml per worker-group?

Yes, it will be defined per worker pool.

[ashwani2k]

This again makes me substantiate that we don't need maxPreservedMachine as this will be done outside of shoot spec and will play conflict.
We can have it as defaultPreserveMachineCount from 1 to any value the shoot owner decides.

The operator can override this value by directly annotating the machine for preservation.

[unmarshall]

If that is the case then, we can simplify this functionality and completely remove preserveMachineMax or what its currently called failedMachinePreserveMax (proposal has been iterated several times now but not yet reflected here).
The user can just use annotations on Node/Machine to indicate machines that they would like to preserve.
But do remember that it is now the responsibility of whosoever adds the annotations to keep in mind that annotated machines are counted as part of MCD replicas. This means if they do not pay attention and mark multiple machines to be preserved then availability of machines in a MCD can potentially get impacted. The onus of that will solely lie on the operator who made this change.

[etiennnr]

kubectl drain? I don't see this as something that should be done my MCM IMHO. The exact purpose of this feature is to be able to test a node even if it's failing, thus workload is sometimes needed in order to troubleshoot. I'd add a warning instead in the documentation to do the drain yourself if it's required.

[ashwani2k]

I would rather make it a pre-requisite to mark a machine to be preserved only if it has been drained/scheduling disabled. This will ensure that removal of preserve state directly moves the machine to terminating phase removing unnecessary state mgmt. wdyt?

[unmarshall]

I would rather make it a pre-requisite to mark a machine to be preserved only if it has been drained/scheduling disabled.

Not clear. Today MCM will only drain the machine when it reaches a FAILED state. Once it reaches that state then apart from cases where the drain takes a long time the transition to Terminating and removal of the VM is pretty quick. So your suggestion will then only apply for machines that have not transitioned to FAILED state and are manually drained.

But why should preservation of a machine be tied to draining?
In subsequent discussions the following was agreed upon:

• If the machine is not in FAILED state and has preserve annotation, then MCM will never drain these nodes. It is upto the operator to take the call if a drain is required and then use kubectl drain command to do that.
• If the machine has transitioned to FAILED state then MCM can drain the node. Today it does when a VM transitions to Terminating phase but there is hardly any time difference between FAILED and Terminating, that is the reason we proposed that draining can also be tied to FAILED machines only.

However there was also a question what happens when a VM of a machine in FAILED is rebooted from the CSP console? If it becomes healthy now will it go back to Running? Today that is not the case but since we are preserving FAILED machines this transition now becomes possible. Keeping that in mind maybe we do not have to drain at all, unless it reaches Terminating state (the current behavior).

[etiennnr]

This is confusing to me, but I might not understand the details also... Do you mean that failedMachinePreserveMax is global to all machines / machineDeployment but the maximum amount of nodes in a given MD still takes precedence ?

[thiyyakat]

Yes. Similar to the way Max for worker pools is enforced. I will update the proposal with an example.

[ashwani2k]

I'll propose to remove the concept of maxPreserve ensure default is set to 1 and anything more than 1 needs to be done manually by the operator as just perserveMachineCount.
As this is not a standard operation we don't need to have additional logic of max/min.
If timeout is set then default is set to 1 per machine deployment, which can be increased by the operator as the case be.

[ashwani2k]

Thanks for the proposal. I've put some comments, while reading it I felt we have not considered all the personas who can consume this feature.

1. Operators who will manipulate the machine/node objects.
2. Stakholders who will maniuplate the shoot spec.
3. Stakeholder who will manipulate the node objects.

Another dimension which I don't see mentioned is how is this state conveyed as part of shoot status and the dashboard.

[ashwani2k]

I'll also agree that if we are preserving we preserve it for duration of 72hrs as default for preserveTimeout and PreserveMax to be 1 as default if set.
We should also make it explicit that this setting is per worker pool in the doc.

[ashwani2k]

Shouldn't this be agnostic to the end user. A value of 1 is spread to all machineDeployments and the distribution is implicitly taken. The end user doesn't need to know the semantics of setting he/she just sets how many machines they would like to preserve. If its 1 then 1 machine per zone will be preserved implicitly. If its 2 then 2 machines per zone will be preserved.

[elankath]

If its 1 then 1 machine per zone will be preserved implicitly. If its 2 then 2 machines per zone will be preserved.

The convention in gardener is that min/max values for the worker pool are distributed across zones. So we would be breaking the current convention, if we say that failedMachinePreserveMax is per zone of the worker pool, instead of across the worker pool.

[ashwani2k]

Why have it at machine level at all, if you plan to have it at node level?
Won't just having it at node level suffice, like any other annotation we expose for the end user to play with the nodes.

[ashwani2k]

I believe we need a new state diagram here -- Healthy --> Perserved as well Failed --> Preserved.
I'm guessing removal of preserved should bring back the machine to either failed or healthy state as it be the case at the point in time ?

[ashwani2k]

I would rather make it a pre-requisite to mark a machine to be preserved only if it has been drained/scheduling disabled. This will ensure that removal of preserve state directly moves the machine to terminating phase removing unnecessary state mgmt. wdyt?

[ashwani2k]

I'll propose to remove the concept of maxPreserve ensure default is set to 1 and anything more than 1 needs to be done manually by the operator as just perserveMachineCount.
As this is not a standard operation we don't need to have additional logic of max/min.
If timeout is set then default is set to 1 per machine deployment, which can be increased by the operator as the case be.

[ashwani2k]

This again makes me substantiate that we don't need maxPreservedMachine as this will be done outside of shoot spec and will play conflict.
We can have it as defaultPreserveMachineCount from 1 to any value the shoot owner decides.

The operator can override this value by directly annotating the machine for preservation.

[ashwani2k]

I feel, we should not generalize non-failing node, as it will introduce more complexity to the state management. We should only enable for cases where the node is cordoned and drained, also I'm guessing we need to do some changes in CA handling for unneeded machines as well as I don't see mentioned in this proposal.