Provide the ability to perform actions before termination of the VM

[CaptainIRS]

How to categorize this issue?

/area control-plane
/kind enhancement

What would you like to be added:

As an operator, I would like to have the ability to perform some actions after the draining of the node and before the deletion of the VM. For example, in a self-hosted control plane whose VMs are managed by MCM, there should be a mechanism to remove the etcd member associated with the VM from the etcd cluster before MCM deletes the VM as part of replacement/rolling updates.

Proposed changes:

• A phase in the Machine lifecycle to signify to external actors that the drain is complete and that it is safe to perform the out-of-band operations necessary before termination of the VM. Suggestion: a WaitingForPreTerminationHooks phase.
• An annotation, whose absence could be used by MCM as a signal to proceed with the termination of the VM. Suggestion: pre-terminate.delete.hook.machine.sapcloud.io/<name>=true where <name> is set by the external actor for identification. So, once the external actor is done with the actions (like removal of the etcd member), it will delete the annotation from the Machine object, after which MCM can continue with the deletion flow. This annotation shall be set by the external actor as part of the MachineTemplate while creating the MachineDeployment.

Tasks:

• Confirm the naming of the phase and annotation
• Implement the handling of the introduced phase and annotation

Why is this needed:

Part of gardener/gardener#15298

[elankath]

Team recommended removing the custom phase and stick to existing "stage" based check in triggerDeletionFlow

Proposed changes:

• An annotation, whose absence could be used by MCM as a signal to proceed with the termination of the VM.
  Suggestion: node.machine.sapcloud.io/suspend-instance-deletion/<purpose>=<owner> where <purpose> and <owner> is set by the external actor for identification.
• Once the external actor is done with the actions (like removal of the etcd member), it will delete the annotation from the Machine object, after which MCM can proceed with the VM deletion flow.
• This annotation shall be set by the external actor as part of the MachineTemplate while creating the MachineDeployment. ( The MCM will copy this to the Machine object (if present) when creating a new Machine in the machine set controller)
• This annotation can also be set on a specific Machine object directly (at any time before actual VM deletion starts)
• The Machine Controller adds a new stage Instance Deletion Suspended to the Machine.Status.LastOperation.Description. (This is logically a "sub-stage" of Termanting phase). This is used by the Machine Controller for switching internal logic in triggerDeletionFlow.
• We propose adding a NodeCondition on Machine.Status.Conditions to indicate that VM/Instance is suspended to external actors. This will be set by the Machine Controller.
  • Condition Type will be InstanceDeletionSuspended.
  • Condition Message will hold a human readable message containing purpose(s) and owner(s). Ex: Instance Deletion suspended by <ownerX> for <purposeA>, <ownerY> for <purposeB>, ...

• (optional) We can also add a label on the Machine object as well as Node object for easy observability: node.machine.sapcloud.io/instance-deletion-suspended=true

[rfranzke]

@elankath Your proposal is fine (basically, that's what @CaptainIRS already described in the second bullet point) just the naming is different which is okay.

However, what about the first bullet point?

• A phase in the Machine lifecycle to signify to external actors that the drain is complete and that it is safe to perform the out-of-band operations necessary before termination of the VM. Suggestion: a WaitingForPreTerminationHooks phase.

How can the external actor know that MCM completed it's drain (and whatever else it is doing, basically everything that happens before it finally terminates the VM) and is now waiting for the annotation to be removed?

[elankath]

@rfranzke
Adding a new MachinePhase is unsuitable since the MachinePhase is Terminating right from the time the Machine object's deletion timestamp is set by the machine set controller till the end of the VM deletion. A fair bit of code depends on this behavior and it is risky to change this.

• The Machine Controller adds a new stage Instance Deletion Suspended to the Machine.Status.LastOperation.Description. (This is logically a "sub-stage" of Termanting phase). This is used by the MCM for switching internal logic in triggerDeletionFlow
• For ease of external actors, we propose adding a NodeCondition on Machine.Status.Conditions to indicate that VM/Instance is suspended to external actors. This will be set by the Machine Controller. Condition Type will be InstanceDeletionSuspended. Condition Message will hold a human readable message containing purpose(s) and owner(s). Ex: Instance Deletion suspended by <ownerX> for <purposeA>, <ownerY> for <purposeB>, ...
• (optional) We can also add a label on the Machine object as well as Node object for easy observability: node.machine.sapcloud.io/instance-deletion-suspended=true

[marc1404]

For ease of external actors, we propose adding a NodeCondition on Machine.Status.Conditions to indicate that VM/Instance is suspended to external actors. This will be set by the Machine Controller. Condition Type will be InstanceDeletionSuspended. Condition Message will hold a human readable message containing purpose(s) and owner(s). Ex: "Instance Deletion suspended by: for , for , ..."

While not merged yet, in the light of this PR:

• Should this be a constraint rather than a condition?
• With the name InstanceDeletionSuspended a Status: True would mean the instance deletion is suspended. What do you think about something like InstanceDeletionPossible instead?

[elankath]

@marc1404 Hmm.. I am not sure this is a constraint since the proposed NodeCondition in Machine.Status.Conditions is set by the Machine Controller to reflect that the annotation has been processed. It is a result and hence just by itself is not logically a constraint . Or maybe I am misunderstanding what you are saying.

Unhappy with InstanceDeletionPossible. It sounds like "is it technically possible to delete this?" rather than the more clear "the instance deletion has been suspended". The semantics are muddied.

[rfranzke]

Fair enough, I can also live with the condition type InstanceDeletionSuspended.

[elankath]

@rfranzke One question - how long should this hook last ? The Cluster API hook (from which this was inspired) can block deletion indefinitely if the annotation is never removed. But should we follow same behavior ? It doesn't feel right.

[rfranzke]

It should last until the annotation gets removed - that sounds perfectly reasonable to me. Otherwise, you risk breaking the ETCD cluster if you prematurely decide to terminate a VM after some time. WDYT?

[CaptainIRS]

An annotation, whose absence could be used by MCM as a signal to proceed with the termination of the VM.
Suggestion: node.machine.sapcloud.io/suspend-instance-deletion/<purpose>=<owner> where and is set by the external actor for identification.

@elankath the proposed name is problematic since it violates the rules for a label key (https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) as only one prefix is allowed. So, if I try setting it via .spec.provider.workers[*].annotations of the Shoot spec, it results in the following error:

Invalid value: "node.machine.sapcloud.io/suspend-instance-deletion/etcd-member-removal": a valid label key must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName')

Could you please let us know if there is an alternative format we could use for the annotation key?

[rfranzke]

Can we move on with this issue?