✨ machine: Introduce Deletion status field and add timestamps for drain and volumeDetach instead of using the condition #11166
Conversation
chrischdi
added
the
tide/merge-method-squash
k8s-ci-robot
added
area/machine
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
size/L
|
/test help |
|
@chrischdi: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test pull-cluster-api-e2e-main |
chrischdi
changed the title
… and volumeDetach instead of using the condition
chrischdi
force-pushed
the
pr-deletion-status
branch
from
0451196
to
b2ddb80
Compare
|
/test pull-cluster-api-e2e-main |
|
/retest |
|
/retest different flake |
chrischdi
changed the title
|
/assign sbueringer fabriziopandini |
|
/test help |
|
@chrischdi: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test pull-cluster-api-e2e-main |
There was a problem hiding this comment.
Nice, also qualifies as a backport candidate IMO
Also changed the PR type to give some more visibility
/lgtm
cc @vincepri @JoelSpeed for the API changes
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 5d3b081c89d004ac999478da2bc5bf7fc9f47730
|
fabriziopandini
changed the title
There was a problem hiding this comment.
I'm slightly confused by the premise of this PR given my understanding of @fabriziopandini's recent update to status.
A condition should be a perfectly suitable way to represent the drain state, but you mention there have been issues, do we have any of those written up that you can link to?
Both are used to track the starting time and used to check if we reached the timeouts instead of relying on the clusterv1.Condition's lastTransitionTime.
LastTransitionTime should only be changed when the status is change from true to false or vice versa, does that cause an issue here? Has it been behaving in that way?
This is to get rid of the issue that the lastTransitionTime get's updated due to changes to the condition and to prepare for the changes for v1beta2, where conditions don't have a lastTransitionTime anymore.
It sounds like yes, it sounds like any change to the condition has resulted in the transition time being updated, which is incorrect condition behaviour.
Also, lastTransitionTime is not going anywhere, see the docs for metav1.Condition here. The current implementation is compatible with the future set out in Fabrizio's plan.
api/v1beta1/machine_types.go
Outdated
| @@ -230,10 +230,22 @@ type MachineStatus struct { | |||
| // Conditions defines current service state of the Machine. | |||
| // +optional | |||
| Conditions Conditions `json:"conditions,omitempty"` | |||
|
|
|||
| // Deletion is the deletion state of the Machine. | |||
There was a problem hiding this comment.
Where did we land with getting the godocs to start with the serialised versions?
Perhaps a little more context might be useful?
| // Deletion is the deletion state of the Machine. | |
| // deletion contains information relating to removal of the Machine. | |
| // Only present when the Machine has a deletionTimestamp and is being removed from the cluster. |
There was a problem hiding this comment.
Where did we land with getting the godocs to start with the serialised versions?
First time I'm hearing about this :). godoc conventions suggest it should be upper case, right? (and that's what we do today everywhere in core CAPI)
There was a problem hiding this comment.
I can't see it explicitly called out in the API conventions from upstream, but, take a look at the godoc across the core types (and I know the upstream API reviewers enforce this from experience), https://github.com/kubernetes/api/blob/master/core/v1/types.go, all of the godocs on fields start with the json tag version of the field name, so that when docs are generated, they represent the serialised versions of the strings
There was a problem hiding this comment.
Makes sense. Thank you!
@chrischdi Can you please change it here and also open a help-wanted issue so we change it everywhere ?
| // MachineStatusDeletion is the deletion state of the Machine. | ||
| type MachineStatusDeletion struct { | ||
| // NodeDrainStartTime is the time when the drain of the node started. | ||
| NodeDrainStartTime *metav1.Time `json:"nodeDrainStartTime,omitempty"` |
There was a problem hiding this comment.
Do we also need a finish time? Same for the other field?
There was a problem hiding this comment.
I personally don't think that drain or detach finish time is an important info to have in the API (users & SRE mostly care about what is going on now and eventually why it is stuck, rarely they care about at what happened and for this log a more exhaustive)
But no strong opinion.
There was a problem hiding this comment.
If I know that these have started, how do I know that they have finished if I don't have some field to tell me? 🤔 I realise eventually the machine is going away, but, what if it gets stuck terminating the instance, will that show up somewhere and will I know that drain and volume detach are done?
There was a problem hiding this comment.
how do I know that they have finished if I don't have some field to tell me? 🤔
From controller-perspective we (at least currently) do not care:
- either the controller tries to drain again which should be a no-op (happy path)
- or the drain is skipped because the timeout is reached.
From the user perspective: the information where the deletion is at should be part of the Deleting condition I'd say.
There was a problem hiding this comment.
I think in general the new Deleting condition should make clear at which phase of the deletion workflow we are (including making clear which parts are already completed)
api/v1beta1/machine_types.go
Outdated
| } | ||
|
|
||
| // ANCHOR_END: MachineStatus | ||
|
|
||
| // MachineStatusDeletion is the deletion state of the Machine. | ||
| type MachineStatusDeletion struct { | ||
| // NodeDrainStartTime is the time when the drain of the node started. |
There was a problem hiding this comment.
I feel like we can probably add some more context to this, what does it mean when it's not present for example, what does it mean if it has elapsed for some period and the Machine is still here, what hints can we give to end users?
There was a problem hiding this comment.
Tried to add some more context on both keys.
api/v1beta1/machine_types.go
Outdated
| } | ||
|
|
||
| // ANCHOR_END: MachineStatus | ||
|
|
||
| // MachineStatusDeletion is the deletion state of the Machine. | ||
| type MachineStatusDeletion struct { |
There was a problem hiding this comment.
Should we include information here related to the drain, such as the configuration for the timeout?
Since the drain fields are optional in the spec, it would be good perhaps to show the configured values here so that you can correlate between start time and expected end time just by looking at the status?
There was a problem hiding this comment.
This is an interesting idea, not really sure how we can represent we are waiting forever in a clear way (not showing timeout 0).
There was a problem hiding this comment.
A value of -1 potentially? But you're right, timeout 0 is awkward 🤔
There was a problem hiding this comment.
I think the status that it is still waiting should then be part of the condition Deleting condition message? 🤔 (or as long as that one is not around, the DrainSucceeded condition message.
There was a problem hiding this comment.
Yep, having this in the conditions seems the easiest way to address this
k8s-ci-robot
removed
the
lgtm
|
First of all, thanks for the great review!
Yes that's the current behavior in the utils, but will be fixed with the v1beta2 conditions.
That's true, but from the proposal: the target conditions for the final state of the Machine's do not contain the The information about the deletion status they showed should be moved / available at the Deleted condition instead. |
api/v1beta1/machine_types.go
Outdated
| type MachineStatusDeletion struct { | ||
| // NodeDrainStartTime is the time when the drain of the node started. | ||
| // Only present when the Machine has a deletionTimestamp, is being removed from the cluster | ||
| // and draining the node had been started. |
There was a problem hiding this comment.
Why would drain not have been started when there's a deletion timestamp?
There was a problem hiding this comment.
Because of waiting for pre-drain hooks is one thing.
There was a problem hiding this comment.
Note: the best pointer to overview the deletion process may be this: https://main.cluster-api.sigs.k8s.io/tasks/automated-machine-management/machine_deletions
There was a problem hiding this comment.
I was thinking it might be useful to point a user to, or at least give them a hint to why node draining might not have started, can we include a line?
There was a problem hiding this comment.
Another one may be the node is excluded from the drain entirely (there is an annotation for it).
I shortened it to:
// NodeDrainStartTime is the time when the drain of the node started and is used to determine
// if the NodeDrainTimeout is exceeded.
// Only present when the Machine has a deletionTimestamp and draining the node had been started.
Happy to get other suggestions though :-)
One way would be to say:
// NodeDrainStartTime is the time when the drain of the node started and is used to determine
// if the NodeDrainTimeout is exceeded.
// Only present when the Machine has a deletionTimestamp and draining the node had been started.
// NodeDrainStartTime may not be set because the deletion is blocked by a pre-drain hook or draining is skipped for the machine.
But doing the same for WaitForNodeVolumeDetachStartTime would get very verbose 🤔 :
// WaitForNodeVolumeDetachStartTime is the time when waiting for volume detachment started
// and is used to determine if the NodeVolumeDetachTimeout is exceeded.
// Detaching volumes from nodes is usually done by CSI implementations and the current state
// is observed from the node's `.Status.VolumesAttached` field.
// Only present when the Machine has a deletionTimestamp and waiting for volume detachments had been started.
// WaitForNodeVolumeDetachStartTime may not be set because the deletion is blocked by a pre-drain hook, stuck in drain or waiting for volume detachment is skipped for the machine.
```
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 2d7929ac4d700f1cf20b236e8e815c0509393408
|
|
@JoelSpeed @fabriziopandini Fine to merge from your side as well? |
| // +optional | ||
| NodeDrainStartTime *metav1.Time `json:"nodeDrainStartTime,omitempty"` | ||
|
|
||
| // waitForNodeVolumeDetachStartTime is the time when waiting for volume detachment started |
There was a problem hiding this comment.
| // waitForNodeVolumeDetachStartTime is the time when waiting for volume detachment started | |
| // WaitForNodeVolumeDetachStartTime is the time when waiting for volume detachment started |
What this PR does / why we need it:
Machine.status.deletionnodeDrainStartTimenodeVolumeDetachStartTimeBoth are used to track the starting time and used to check if we reached the timeouts instead of relying on the clusterv1.Condition's lastTransitionTime.
This is to get rid of the issue that the lastTransitionTime get's updated due to changes to the condition and to prepare for the changes for v1beta2, where conditions don't have a lastTransitionTime anymore.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #11126
/area machine