Add Startup Taint Removal Feature

[ConnorJC3]

Is this a bug fix or adding new feature?

New feature

What is this PR about? / Why do we need it?

Implements a feature to remove a taint on driver startup to alleviate potential race conditions. Supercedes #1581, all credit for the design and initial implementation to @gtxu.

This PR differs from the original in a few meaningful ways:

• The CLI option is removed, users do not need to enable this feature (it is always enabled)
• Because of this, failing to connect to the k8s API is a soft failure that only logs on log level 4+ (because otherwise it would inaccurately log an error in scenarios such as using alternative COs like Nomad)

What testing is done?

CI/Manual/New unit tests added

[rdpsin]

@torredil mentioned this, but can we use https://pkg.go.dev/k8s.io/kubernetes/pkg/util/taints#RemoveTaint instead of JSON patch?

[ConnorJC3]

I originally thought that was possible, but sadly, not really:

Firstly, RemoveTaint requires a specific effect, so we'd have to repeat it for all three Effects (and the code wouldn't be very future proof, we'd have to update it whenever a new taint effect is added).

Secondly, and more importantly, RemoveTaint only updates the local representation of the node, it doesn't actually make any calls to the k8s API. Thus, you then need to push the entire node object, which (1) is way less efficient than just patching the taints and (2) possibly even introduces a race condition (if the node is modified by something else between us downloading it and attempting to update it).

[torredil]

Are there concerns with this implementation?

node.Spec.Taints = taintsToKeep
err = clientset.CoreV1().Nodes().Update(context.Background(), node, metav1.UpdateOptions{})
if err != nil {
	return err
}
return nil

[ConnorJC3]

Yes, see my comment above:

Thus, you then need to push the entire node object, which (1) is way less efficient than just patching the taints and (2) possibly even introduces a race condition (if the node is modified by something else between us downloading it and attempting to update it).

[torredil]

(2) possibly even introduces a race condition (if the node is modified by something else between us downloading it and attempting to update it).

K8s requires that objects submitted in update requests contain a resourceVersion, the K8s API server verifies this field and will reject the request if there is a conflict which should prevent race conditions.

(1) is way less efficient than just patching the taints.

👍

/lgtm

[rdpsin]

Instead of creating the patch manually, can we DeepCopy the Node object, delete the taints, and use strategicpatch.CreateTwoWayMergePatch to create the patch data?

[ConnorJC3]

We could although I'd rather not unless we have a strong reason to because it eats an unnecessary amount of RAM and CPU creating a second copy of the node, and transforming both to JSON for the patch.

That said, I'm open to either option: I'll leave this open for other feedback.

[rdpsin]

Why did we remove this CLI option?

[ConnorJC3]

Why did we remove this CLI option?

Adding a knob to enable/disable this feature is unnecessary, because it has no impact (besides a single call to the k8s API on startup, which we already make for node metadata anyways) on users that don't use it.

[rdpsin]

I'd still prefer an option for the feature, even if the default is behavior is true. There may be cases where customers may want to explicitly opt out of the feature.

[ConnorJC3]

/retest

[ConnorJC3]

/retest

[torredil]

Are there concerns with this implementation?

node.Spec.Taints = taintsToKeep
err = clientset.CoreV1().Nodes().Update(context.Background(), node, metav1.UpdateOptions{})
if err != nil {
	return err
}
return nil

[torredil]

Regarding the CLI option, this is a good mental model to follow:

@wmesard: Every configuration option represents a failure of the software to do the right thing automatically. Every configuration option needs to be documented and protected by unit tests, thereby increasing the cognitive load of user and developer alike. Sometimes they are necessary, but only as a last resort.

[ConnorJC3]

/retest

[hanyuel]

/lgtm

[torredil]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: torredil

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [torredil]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ConnorJC3]

/retest