Improve calculation of NodeDrainTimeout & NodeVolumeDetachTimeout exceeded

[sbueringer]

Today, we check if the NodeDrainTimeout & NodeVolumeDetachTimeout are exceeded by comparing time.Now with the LastTransitionTime of the corresponding conditions.

This is fragile for multiple reasons:

• LastTransitionTime is updated when the condition flickers between true & false
• LastTransitionTime is updated every time anything changes in the condition (e.g. message), xref:

  • cluster-api/util/conditions/setter.go

    Line 54 in 4f1637e

    if !hasSameState(&existingCondition, condition) {

  • cluster-api/util/conditions/setter.go

    Line 199 in 4f1637e

    func hasSameState(i, j *clusterv1.Condition) bool {

It would be already a good improvement if the following is implemented:

existing conditions utils changes lastTranistionTime whenever something changes in the condition, while future/upstream aligned conditions utils are changing lastTranistionTime only when status change
#11033 (comment)

But I think instead of relying on the LastTransitionTime we should have additional status fields tracking when we started to drain & when we started to wait for volume detach and then calculate if the timeouts are exceeded based on that.

(Note: this is not relevant for NodeDeletionTimeout because for this one we use the deletionTimestamp field)

[sbueringer]

/triage accepted

[sbueringer]

/cc @fabriziopandini @chrischdi @enxebre

[fabriziopandini]

But I think instead of relying on the LastTransitionTime we should have additional status fields tracking when we started to drain & when we started to wait for volume detach and then calculate if the timeouts are exceeded based on that.

+1
I think that we should add a status.Termination struct with NodeDrainStartTime & NodeVolumeDetachStartTime fields. This will be somehow "consistent" with the Initialization struct proposed by #10897.

[chrischdi]

/assign

[sbueringer]

One small note, wondering if we should call it status.deletion instead of termination. I think in CAPI we usually always talk about "deletion"

+ NodeVolumeDetachStartTime should be WaitForNodeVolumeDetachStartTime

[fabriziopandini]

No strong opinions about the name.
Termination makes a good pair to Initialization, it also reminds of stuff like gracetermination period etc etc

[sbueringer]

Yup, we just use deletion in a lot of places and we don't have things like terminationGracePeriod for Machines (but we have a deletionTimestamp)

(we're also always talking about Machine deletion, never really used/heard Machine termination)