[Feature]: Do not uncordon nodes cordoned independently of NVSentinel

[natherz97]

Prerequisites

• I searched existing issues

Code of Conduct

• I agree to follow NVSentinel's Code of Conduct

Feature Summary

This feature would allow nodes cordoned prior to NVSentinel triggering the breakfix pipeline to remain cordoned after recovering from a fatal unhealthy event when NVSentinel releases ownership of the node.

Problem/Use Case

Currently, the fault-quarantine module does not track whether a node was cordoned prior to a fatal unhealthy event arriving. As a result, if a node was cordoned by an external system and then experiences a fatal unhealthy event which recovers, the node will be uncordoned by fault-quarantine. This can result in nodes isolated for maintenance or faults not detected by NVSentinel becoming schedulable.

Proposed Solution

Add a new node annotation as part of the fault-quarantine module which indicates whether a node was cordoned prior to fault-quarantine receiving a fatal unhealthy event. If this annotation is set when the last unhealthy event is cleared in fault-quarantine, it should release ownership of the node but keep the node cordoned.

Component

Health Monitor

[natherz97]

Maintaining pre-existing cordons and taints

We'll want to make sure the behavior is consistent for both cordons + taints applied by fault-quarantine.

1. Maintaining cordons: we can extend the existing annotation from quarantineHealthEventIsCordoned=True to either a JSON structure or add a new value which indicates whether the node was cordoned prior fault-quarantine adding the quarantineHealthEvent annotation. To ensure this change is backwards compatible with any existing quarantineHealthEventIsCordoned annotation (and to avoid needing to try unmarshaling the annotation to multiple types), we can support the values True or PreExisting. If True, we'll remove the cordon after the last event in quarantineHealthEvent removed. If PreExisting, we'll maintain the cordon on the node. Alternatively, we could add a completely new annotation called quarantineHealthEventCordonPreExisting which we can apply alongside the existing annotation if the node was cordoned prior to NVSentinel taking action.
2. Maintaining taints: for quarantineHealthEventAppliedTaints to inherit an existing taint, the key, value, and effect derived from fault-quarantine rule sets needs to match an existing taint. We can make a similar change to the quarantineHealthEventAppliedTaints struct which tracks at a per taint level whether the taint should be maintained when the quarantineHealthEvent is removed. Since we already use a JSON structure, we can add the new field to the existing struct which is backwards compatible:

type Taint struct {
    Key        string `json:"key"`
    Value      string `json:"value"`
    Effect     string `json:"effect"`
    PreExisting bool  `json:"preExisting"`
}

If both matching taints and a node cordon exist prior to the node being isolated, they will both be allowed to persist after the quarantineHealthEvent annotation is removed.

Existing issues with cordon and taint management

As part of researching this task, I noticed two behavior issues with our cordon and taint management which we may want to separately fix.

1. Dynamic cordon or taint updates: it looks like we only compute the set of cordons + taints from the initial rule set evaluation of the first unhealthy event which resulted in a quarantined event. Any additional unhealthy events will be added to the quarantineHealthEvent annotation, however, any cordon or taint updates from the rule-set evaluation will not be added. For example, if we only tainted a node from the first event, a second event which says to cordon the node or add a second taint will not apply to the node and only its unhealthy event will be processed. A similar issue exists when these events recover where we do not remove cordon or taint state as their corresponding events recover. Removing these events is less critical and would require significant complexity to map the health event to the taints/cordons it owns (which may overlap with multiple events), however, it seems reasonable for us to continually add new taints/cordons as new unhealthy events are processed.
2. Cancelling breakfix manually: if a user manually uncordons a node when quarantineHealthEventIsCordoned is set or manually removes a taint tracked in quarantineHealthEventAppliedTaints, the node will be released and the quarantineHealthEvent annotation will be removed. This behavior is sufficient if we only have 1 cordon or taint, however, if we have a combination of a cordon and taints or multiple taints, it can result in fault-quarantine orphaning a cordon or taint that it previously added. We have 3 options:

1. Existing behavior: allow breakfix to be cancelled if any cordon/taint is removed + orphan the existing cordon/taints
2. Require all cordon/taint state to be removed prior to cancelling breakfix
3. Allow breakfix to be cancelled if any cordon or taint is removed, however, automatically remove any existing cordon/taints. This solution could be integrated with our primary change above where manual breakfix cancellations could also preserve cordon/taint state (as long as the cordon/taint wasn't itself removed to cancel breakfix).

[lalitadithya]

we can extend the existing annotation from quarantineHealthEventIsCordoned=True to either a JSON structure or add a new value which indicates whether the node was cordoned prior fault-quarantine adding the quarantineHealthEvent annotation.

My preference would be to add a new annotation, since this is a new use case. I'm unsure if we have any consumers who currently depend on the quarantineHealthEventIsCordoned for their use cases, so I'd like to err on the side of caution and add a new annotation for this use case.

Dynamic cordon or taint updates

IMO this is a bug, and we should open an issue to fix this. @KaivalyaMDabhadkar , do you agree?

Cancelling breakfix manually:

Require all cordon/taint state to be removed prior to cancelling breakfix

This one was a concisions decision that we made while this feature was implemented which was that if the user undoes even a single cordon/taint, the breakfix will be cancelled. The main rational behind this was that in most scripts multiple taints are typically removed one a time, so we don't want to be in a scenario where we end up remediating a node while the user was in between removing taint N1 and taint N2.

Existing behavior

I would prefer to keep the existing behaviour in this case because we don't know what the user's intent is here. For example, if a user has configured multiple different taints, then they are likely power users. One possible use case could be that the user applies taint X for NIC faults and taint Y for GPU faults and workloads that require only GPUs will tolerate taint X. There could be a scenario where there is a node with a faulty NIC that the user wants to keep tainted, but at some point, there was a false positive GPU error. In this case they would prefer to remove the GPU taint but keep the NIC taint. If we were to automatically remove the NIC taint as well, then we would end up opening up the node for all types of jobs which would be incorrect.