Sync taints from machine spec to the node in additive fashion #154

vikaschoudhary16 · 2018-12-20T15:40:08Z

enxebre · 2018-12-20T15:46:47Z

cmd/nodelink-controller/main.go

@@ -393,8 +393,22 @@ func (c *Controller) processNode(node *corev1.Node) error {
 		modNode.Labels[k] = v
 	}

-	// Taints are to be an authoritative list on the machine spec per cluster-api comments:


Nice!
Can you add a PR upstream even if it's to update the field comment and why we think should be additive?
Can you please add a validation for this machine/node taint reconciliation here https://github.com/openshift/machine-api-operator/blob/master/test/e2e/operator_expectations.go?

sure. how about handling e2e in follow-up PR?

please let's include it in this one, the point of having it in the same PR is that it prevent us from merging code which does not work as expected rather than having to fix later

derekwaynecarr · 2018-12-20T15:56:20Z

Change looks good. Link to upstream feature for why it cannot be authoritative. See https://kubernetes.io/docs/concepts/configuration/taint-and-toleration/#taint-nodes-by-condition

enxebre · 2018-12-20T15:56:41Z

cmd/nodelink-controller/main.go

-	modNode.Spec.Taints = matchingMachine.Spec.Taints
+	// Taints are to be an authoritative list on the machine spec per cluster-api comments.
+	// However, we believe many components can directly taint a node and there is no direct source of truth that should enforce a single writer of taints
+	for _, mTaint := range matchingMachine.Spec.Taints {


It'd be nice to create a function and unit test it

enxebre · 2018-12-20T16:00:02Z

cmd/nodelink-controller/main.go

+			}
+		}
+		if !alreadyPresent {
+			modNode.Spec.Taints = append(modNode.Spec.Taints, mTaint)


we are appending here when nTaint.Key == mTaint.Key && nTaint.Effect != mTaint.Effect, so the final Taints
list will have duplicated keys. What will be the behaviour?

This is list of taints and for a taint to be unique key and effect as a pair must be unique. PTAL at example here:
https://kubernetes.io/docs/concepts/configuration/taint-and-toleration/#concepts

vikaschoudhary16 · 2018-12-20T17:48:03Z

/hold

vikaschoudhary16 · 2018-12-26T09:42:55Z

/hold cancel

vikaschoudhary16 · 2018-12-26T12:04:02Z

/hold cancel

vikaschoudhary16 · 2018-12-26T12:05:06Z

/retest

enxebre · 2019-01-02T09:37:31Z

cmd/nodelink-controller/main.go

+		glog.V(3).Infof("machine taint: %v", mTaint)
+		alreadyPresent := false
+		for _, nTaint := range node.Spec.Taints {
+			if nTaint.Key == mTaint.Key && nTaint.Effect == mTaint.Effect {


Why don't we want to take into consideration the .value here?

enxebre · 2019-01-02T09:37:35Z

test/e2e/operator_expectations.go

+	}
+	machineList := capiv1alpha1.MachineList{}
+
+	if err := tc.client.List(context.TODO(), &listOptions, &machineList); err != nil {


this should be wrapped into wait.PollImmediate 186-195

enxebre · 2019-01-02T09:37:38Z

test/e2e/operator_expectations.go

+	}
+	node := &corev1.Node{}
+
+	if err := tc.client.Get(context.TODO(), nodeKey, node); err != nil {


this should be wrapped into wait.PollImmediate

enxebre · 2019-01-02T09:37:41Z

test/e2e/operator_expectations.go

+	}
+	glog.Info("Updated machine object with taint")
+	var expectedTaints = sets.NewString("not-from-machine", "from-machine")
+	err := wait.PollImmediate(1*time.Second, waitLong, func() (bool, error) {


we could just return inline here

enxebre · 2019-01-02T09:38:13Z

/approve

openshift-ci-robot · 2019-01-02T09:39:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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

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

enxebre · 2019-01-02T12:05:50Z

/test e2e-aws-operator

enxebre · 2019-01-02T12:21:04Z

/test e2e-aws-gate

enxebre · 2019-01-02T16:40:00Z

/test all

frobware

One nit, but LGTM.

frobware · 2019-01-03T08:31:17Z

test/e2e/operator_expectations.go

+		for _, taint := range node.Spec.Taints {
+			observedTaints.Insert(taint.Key)
+		}
+		if expectedTaints.Difference(observedTaints).HasAny("not-from-machine", "from-machine") == false {


Use !<expression> and not == false.

enxebre · 2019-01-03T08:42:02Z

/lgtm

Update namespace -> openshift-machine-api

openshift-ci-robot requested review from derekwaynecarr and enxebre December 20, 2018 15:40

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 20, 2018

enxebre reviewed Dec 20, 2018

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2018

vikaschoudhary16 changed the title ~~Sync taints from machine spec to the node in additive fashion~~ [WIP]Sync taints from machine spec to the node in additive fashion Dec 20, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 20, 2018

Sync taints frommachine spec to the node in additive fashion

3da28b4

vikaschoudhary16 force-pushed the taints-additive branch from 1f05d8b to 3da28b4 Compare December 26, 2018 09:41

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 26, 2018

vikaschoudhary16 changed the title ~~[WIP]Sync taints from machine spec to the node in additive fashion~~ Sync taints from machine spec to the node in additive fashion Dec 26, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 26, 2018

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 26, 2018

enxebre reviewed Jan 2, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 2, 2019

enxebre added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 2, 2019

enxebre removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 3, 2019

frobware reviewed Jan 3, 2019

View reviewed changes

openshift-ci-robot assigned enxebre Jan 3, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2019

openshift-merge-robot merged commit f24b3ef into openshift:master Jan 3, 2019

enxebre mentioned this pull request Jan 3, 2019

The machine-api-operator overwrites taints written by either API or oc/kubectl commands #144

Closed

ingvagabund pushed a commit to ingvagabund/machine-api-operator that referenced this pull request Jul 11, 2019

Merge pull request openshift#154 from enxebre/namespace

90be5eb

Update namespace -> openshift-machine-api

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync taints from machine spec to the node in additive fashion #154

Sync taints from machine spec to the node in additive fashion #154

vikaschoudhary16 commented Dec 20, 2018

enxebre Dec 20, 2018

vikaschoudhary16 Dec 20, 2018

enxebre Dec 20, 2018

derekwaynecarr commented Dec 20, 2018

enxebre Dec 20, 2018

vikaschoudhary16 Dec 20, 2018

enxebre Dec 20, 2018 •

edited

vikaschoudhary16 Dec 20, 2018

vikaschoudhary16 commented Dec 20, 2018

vikaschoudhary16 commented Dec 26, 2018 •

edited

vikaschoudhary16 commented Dec 26, 2018

vikaschoudhary16 commented Dec 26, 2018

enxebre Jan 2, 2019

enxebre Jan 2, 2019

enxebre Jan 2, 2019

enxebre Jan 2, 2019

enxebre commented Jan 2, 2019

openshift-ci-robot commented Jan 2, 2019

enxebre commented Jan 2, 2019

enxebre commented Jan 2, 2019

enxebre commented Jan 2, 2019

frobware left a comment

frobware Jan 3, 2019

enxebre commented Jan 3, 2019

Sync taints from machine spec to the node in additive fashion #154

Sync taints from machine spec to the node in additive fashion #154

Conversation

vikaschoudhary16 commented Dec 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Dec 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Dec 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikaschoudhary16 commented Dec 20, 2018

vikaschoudhary16 commented Dec 26, 2018 • edited

vikaschoudhary16 commented Dec 26, 2018

vikaschoudhary16 commented Dec 26, 2018

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

enxebre commented Jan 2, 2019

openshift-ci-robot commented Jan 2, 2019

enxebre commented Jan 2, 2019

enxebre commented Jan 2, 2019

enxebre commented Jan 2, 2019

frobware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Jan 3, 2019

enxebre Dec 20, 2018 •

edited

vikaschoudhary16 commented Dec 26, 2018 •

edited