Add ClusterOperator Scheduling #650

michaelgugino · 2021-02-12T22:11:17Z

No description provided.

openshift-ci-robot · 2021-02-12T22:11:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign russellb after the PR has been reviewed.
You can assign the PR to them by writing /assign @russellb in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

wking · 2021-02-12T22:29:31Z

enhancements/scheduling/clusteroperator-scheduling.md

+## Summary
+
+To ensure system components, especially ClusterOperators, have the correct
+set of tolerations.


Possibly PR with adjustments to this existing enhancements content, instead of creating a new, parallel enhancement?

I should have used grep! I looked in the scheduling folder but didn't find this obviously.

smarterclayton · 2021-02-12T22:41:46Z

enhancements/scheduling/clusteroperator-scheduling.md

+draining a node can complete.  This results in hot-looping between draining
+and the scheduler.
+
+Any other NoSchedule toleration is fine.


In the spirit of Chesterton fence, I’d like to see a review of the components that thought they should set this and why (representative sample).

That ensures we correct misconceptions about when the toleration is appropriate.

If it was accidental, what would have prevented the accident?

The one I know of for sure is the CVO. I don't have data if anything else is doing it or not. I will take a look.

Looks like the CVO is the only-non daemonset pod that has this toleration, at least from a typical CI run. The daemonsets get that toleration dynamically, so everything else looks correct.

There was a discussion sometime ago on whether CVO should tolerate NoSchedule taint and the answer was yes. Has something changed now? I was always in favour of specific tolerations instead of blanket tolerations. How about adding a taint called drain-in-progress to node with NoSchedule effect. That won't be tolerated by CVO which should help in proceeding with draining smoothly.

Thanks for sharing that discussion. There is more than one 'NoSchedule' taint. Based on the discussion you linked, it seems node.kubernetes.io/unschedulable was included by mistake. This is not discussed directly, the discussion revolved around unreachable nodes and unready nodes. node.kubernetes.io/unschedulable does not affect those scenarios and is only used for administrative cordons (eg, node.spec.unschedulable=True).

I am not entire sure of that, IIUC I got a firm yes on unschedulable however if you think this was not needed, I can change the tolerations of CVO.

There are many taints that have the effect 'NoSchedule' but there is one 'node.kubernetes.io/unschedulable' taint with the effect 'NoSchedule'. It appears in the previous discussion only NoSchedule was considered rather than the specific taints (at the least, node.kubernetes.io/unschedulable was not itself considered directly).

any NoSchedule taint != unschedulable condition. unschedulable condition == node.kubernetes.io/unschedulable taint. unschedulable is a node condition which is being represented via a special well known taint called node.kubernetes.io/unschedulable.

I'm trying to use the k8s precise language. There are taints, and those taints have effects. The effect we're discussing is NoSchedule. That effect is caused by one or more taints.

I did not see the taint node.kubernetes.io/unschedulable discussed in the link you provided. There were only discussions around the NoSchedule effect.

smarterclayton · 2021-02-12T22:42:12Z

enhancements/scheduling/clusteroperator-scheduling.md

+
+
+### Test Plan
+


I assumed we would enforce this via a test? Or did I miss that above?

I think we do

Looks like that ensure control plane operators do not make themselves unevictable does not cover node.kubernetes.io/unschedulable today? Just unreachable and not-ready.

ravisantoshgudimetla

Thank you for initiating this discussion @michaelgugino. This looks like a step in right direction.

ravisantoshgudimetla · 2021-02-15T04:02:47Z

enhancements/scheduling/clusteroperator-scheduling.md

+draining a node can complete.  This results in hot-looping between draining
+and the scheduler.
+
+Any other NoSchedule toleration is fine.


There was a discussion sometime ago on whether CVO should tolerate NoSchedule taint and the answer was yes. Has something changed now? I was always in favour of specific tolerations instead of blanket tolerations. How about adding a taint called drain-in-progress to node with NoSchedule effect. That won't be tolerated by CVO which should help in proceeding with draining smoothly.

ravisantoshgudimetla · 2021-02-15T04:04:29Z

enhancements/scheduling/clusteroperator-scheduling.md

+
+
+### Test Plan
+


I think we do

ravisantoshgudimetla · 2021-02-17T20:30:32Z

CONVENTIONS.md

@@ -222,6 +225,10 @@ spec:
      - operator: Exists
 ```

+Tolerating all taints should be reserved for DaemonSets and static


Which static pods are you referring to?

Is there more than one type?

I meant which specific static pods in OpenShift. IIUC, static pods should be tolerating all NoExecute taints not NoSchedule as their manifests are directly present on the node, there is no need for them to go through scheduling cycle.

There isn't an option to tolerate all taints of a certain effect, just "All" generally.

dhellmann · 2021-03-02T20:45:12Z

CONVENTIONS.md

@@ -189,6 +189,9 @@ following taints if doing so is necessary to form a functional Kubernetes node:
 Operators should not specify tolerations in their manifests for any of the taints in
 the above list without an explicit and credible justification.

+Operators should never specify the following toleration:


The other guidelines in this section include justification, so that someone coming to the doc without all of the background can learn why we have each restriction. This addition would benefit from a sentence or two explaining why this value is so special that it should never be used.

Thanks, that looks good.

smarterclayton · 2021-05-18T15:40:33Z

/lgtm
/approve

In the future we may come up with exceptions, which are reasonable to propose as new enhancements. We may also wish to take this feedback back to sig-scheduling and gather other perspectives on whether tolerations are missing something in between "list" and "all".

openshift-ci · 2021-05-18T15:40:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

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~~ [smarterclayton]

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

Add ClusterOperator Scheduling

6a04c90

openshift-ci-robot requested review from abhinavdahiya and ironcladlou February 12, 2021 22:11

wking reviewed Feb 12, 2021

View reviewed changes

smarterclayton reviewed Feb 12, 2021

View reviewed changes

ravisantoshgudimetla reviewed Feb 15, 2021

View reviewed changes

michaelgugino force-pushed the co-sched branch from 84f03d6 to 1fc41ea Compare February 16, 2021 23:25

ravisantoshgudimetla reviewed Feb 17, 2021

View reviewed changes

wking mentioned this pull request Feb 23, 2021

Remove toleration of unschedulable openshift/cluster-version-operator#526

Merged

dhellmann reviewed Mar 2, 2021

View reviewed changes

Add unschedulable taint to CONVENTIONS

8b9f4a8

michaelgugino force-pushed the co-sched branch from 1fc41ea to 8b9f4a8 Compare March 3, 2021 13:19

openshift-ci bot assigned smarterclayton May 18, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2021

openshift-merge-robot merged commit fe57853 into openshift:master May 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ClusterOperator Scheduling #650

Add ClusterOperator Scheduling #650

michaelgugino commented Feb 12, 2021

openshift-ci-robot commented Feb 12, 2021

wking Feb 12, 2021

michaelgugino Feb 12, 2021

smarterclayton Feb 12, 2021

michaelgugino Feb 12, 2021

michaelgugino Feb 12, 2021

ravisantoshgudimetla Feb 15, 2021

michaelgugino Feb 16, 2021

ravisantoshgudimetla Feb 17, 2021

michaelgugino Feb 22, 2021

ravisantoshgudimetla Feb 22, 2021

michaelgugino Feb 23, 2021

smarterclayton Feb 12, 2021

ravisantoshgudimetla Feb 15, 2021

wking Feb 17, 2021

ravisantoshgudimetla left a comment

ravisantoshgudimetla Feb 15, 2021

ravisantoshgudimetla Feb 15, 2021

ravisantoshgudimetla Feb 17, 2021

michaelgugino Feb 17, 2021

ravisantoshgudimetla Feb 22, 2021

michaelgugino Feb 23, 2021

dhellmann Mar 2, 2021

dhellmann Mar 3, 2021

smarterclayton commented May 18, 2021

openshift-ci bot commented May 18, 2021

Add ClusterOperator Scheduling #650

Add ClusterOperator Scheduling #650

Conversation

michaelgugino commented Feb 12, 2021

openshift-ci-robot commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented May 18, 2021

openshift-ci bot commented May 18, 2021