Enable support for taints, annotations and labels #256

hardikdr · 2019-04-21T09:19:45Z

What this PR does / why we need it: This PR implements a feature to propagate and maintain the taints, annotations and labels from machine-api-objects till node-objects.
Proposal doc: https://docs.google.com/document/d/1yNTCJ7hwAhYPmBwACZvnKqO10G_aZ8kotcuSkiN4NAA/edit

Which issue(s) this PR fixes:
Fixes #174

Special notes for your reviewer:

Release note:

Enables support for propagating and maintaining the taints, annotations and labels from machine-api objects to node-objects.

…ith machineset objects

…ine-objects

…ith node-objects

prashanth26

Some minor comments can choose to ignore wherever it is not applicable.

prashanth26 · 2019-04-22T08:41:53Z

pkg/controller/machine.go

@@ -199,6 +210,11 @@ func (c *controller) reconcileClusterMachine(machine *v1alpha1.Machine) error {
 	Machine controller - nodeToMachine
 */
 func (c *controller) addNodeToMachine(obj interface{}) {
+	if !cache.WaitForCacheSync(nil, c.nodeSynced) {


Do we need this?

As discussed offline, we can avoid it here I guess, thanks for bringing it up.

prashanth26 · 2019-04-22T08:45:28Z

pkg/controller/machine_util.go

+// It ensures, that any nodeTemplate element available on Machine should be available on node-object.
+// Although there could be more elements already available on node-object which will not be touched.
+func (c *controller) syncMachineNodeTemplates(machine *v1alpha1.Machine) error {
+	if machine.Status.Node == "" {


If a machine object doesn't have a node object associated with it due to some error while creation. Could it be a possibility that the object doesn't get deleted? as syncMachineNodeTemplates are tried before syncing machine creation/deletion/update? or maybe I didn't get it.

Yes, this is a very good catch, thanks.
Basically if, for any reason, node-object is available and Status.Node has not been updated then the controller can get stuck into the loop, taking care of it.

prashanth26 · 2019-04-22T08:51:21Z

pkg/controller/machine_util.go

+		}
+		nCopy[mkey] = mvalue
+	}
+	// Update is not required.


Just a suggestion for better code readability. Can choose to ignore it.

if updateNeeded { node.Labels = nCopy } return updateNeeded

Suggestion looks good to me, taking it in.

prashanth26 · 2019-04-22T08:52:37Z

pkg/controller/machine_util.go

+		}
+		nCopy[mkey] = mvalue
+	}
+	// Update is not required.


Same as above

Just a suggestion for better code readability. Can choose to ignore it.

if updateNeeded { node.Annotations = nCopy } return updateNeeded

Suggestion looks good to me, taking it in.

prashanth26 · 2019-04-22T08:54:57Z

pkg/controller/machine_util.go

+			updateNeeded = true
+		}
+	}
+	node.Spec.Taints = nTaintsCopy


Might need this to maintain consistency?

if updateNeeded { node.Spec.Taints = nTaintsCopy } return updateNeeded

Suggestion looks good to me, taking it in.

amshuman-kr

Suggested some changes. Label/Annotation/Taint deletion is not yet supported, right?

amshuman-kr · 2019-04-23T04:27:59Z

pkg/controller/deployment_util.go

+	if newIS.Annotations == nil {
+		newIS.Annotations = make(map[string]string)
+	}
+	oldRevision, ok := newIS.Annotations[RevisionAnnotation]


Would it not be better to use the status section to store the old revision rather than in annotations? Please ignore if this was already used for other MachineSet controller logic.

Yes, this logic is being used at other places in controllers.

amshuman-kr · 2019-04-23T04:28:45Z

pkg/controller/deployment_util.go

+
+	oldRevisionInt, err := strconv.ParseInt(oldRevision, 10, 64)
+	if err != nil {
+		if oldRevision != "" {


Why not check for oldRevision == "" before the parsing?

I think this is being tackled at very initial steps in ParseInt, not sure if required to check again here.
Basically, the template for the core logic of this method is taken from the existing code, wanted to keep it consistence.

amshuman-kr · 2019-04-23T04:30:37Z

pkg/controller/deployment_util.go

+		//If the RS annotation is empty then initialise it to 0
+		oldRevisionInt = 0
+	}
+	newRevisionInt, err := strconv.ParseInt(newRevision, 10, 64)


Why not pass newRevisionInt as a parameter and avoid a parsing?

The template is adapted from an existing method, just wanted to keep it consistent.

amshuman-kr · 2019-04-23T04:34:37Z

pkg/controller/deployment_util.go

+	isNodeTemplateChanged := false
+
+	deploymentNodeTemplateCopy := deployment.Spec.Template.Spec.NodeTemplateSpec.DeepCopy()
+	machineSetNodeTemplateCopy := is.Spec.Template.Spec.NodeTemplateSpec.DeepCopy()


Is this required? We will be overriding is.Spec.Template.Spec.NodeTemplateSpec later anyway, right?

Yes, just wanted to be safe as deployment is a pointer. Though might not be required really for machineset, again wanted to keep the behavior similar for both parameters in a method.

amshuman-kr · 2019-04-23T04:36:07Z

pkg/controller/deployment_util.go

+func copyMachineDeploymentNodeTemplatesToMachineSet(deployment *v1alpha1.MachineDeployment, is *v1alpha1.MachineSet) bool {
+	isNodeTemplateChanged := false
+
+	deploymentNodeTemplateCopy := deployment.Spec.Template.Spec.NodeTemplateSpec.DeepCopy()


Can't we postpone this until after the isNodeTemplateChanged check?

I think, isNodeTemplateChanged is using the copy, so not sure if we can really postpone it.

No. The point was that you do not need a copy for DeepEqual. If isNodeTemplateChanged returns false, we can skip a spurious DeepCopy.

amshuman-kr · 2019-04-23T04:37:29Z

pkg/controller/deployment_util.go

+	isNodeTemplateChanged = !(apiequality.Semantic.DeepEqual(deploymentNodeTemplateCopy, machineSetNodeTemplateCopy))
+
+	if isNodeTemplateChanged {
+		is.Spec.Template.Spec.NodeTemplateSpec = deployment.Spec.Template.Spec.NodeTemplateSpec


Shouldn't there be a DeepCopy of deployment.Spec.Template.Spec.NodeTemplateSpec here?

It's mainly an assignment to the machineset, so not sure we necessarily need to use the deep copy here.

We are assigning a subtree of the MachineDeployment struct (which might have references/pointers) as a subtree of MachineSet. Is that a good idea?

amshuman-kr · 2019-04-23T06:24:02Z

pkg/controller/machine_util.go

+		if _, ok := nCopy[mkey]; !ok || mvalue != nCopy[mkey] {
+			updateNeeded = true
+		}
+		nCopy[mkey] = mvalue


Here nCopy is not really a copy, is it? It will actually modify the node.Labels? Is this expected? According to this line, it seems that nCopy is treated as a true (possibly shallow) copy.

I see, thanks this is a really good catch. I think we don't need to explicitly assign the nCopy to node.Labels.

amshuman-kr · 2019-04-23T06:24:47Z

pkg/controller/machine_util.go

+
+// SyncMachineAnnotations syncs the annotations of the machine with node-objects.
+// It returns true if update is needed else false.
+func SyncMachineAnnotations(machine *v1alpha1.Machine, node *v1.Node) bool {


Same comments as SyncMachineLabels.

Yes, taking care of it, thanks.

amshuman-kr · 2019-04-23T06:25:27Z

pkg/controller/machine_util.go

+
+// SyncMachineTaints syncs the annotations of the machine with node-objects.
+// It returns true if update is needed else false.
+func SyncMachineTaints(machine *v1alpha1.Machine, node *v1.Node) bool {


Same comments as SyncMachineLabels.

Yes, taking care of it, thanks.

amshuman-kr · 2019-04-23T06:27:59Z

pkg/controller/machineset_util.go

+// copyMachineSetNodeTemplatesToMachines copies machineset's nodeTemplate to machine's nodeTemplate,
+// and returns true if machine's nodeTemplate is changed.
+// Note that apply and revision nodeTemplates are not copied.
+func copyMachineSetNodeTemplatesToMachines(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool {


Same comments as copyMachineDeploymentNodeTemplatesToMachineSet.

Basically, the same answer, hope though I didn't miss anything in there.

hardikdr · 2019-04-23T10:11:09Z

@amshuman-kr Yes, deletion is not supported in this logic, although not sure if we need to according to proposal discussed.

amshuman-kr · 2019-04-24T09:30:33Z

@hardikdr It is fine to keep out deletion from the scope. I have responded to a couple of comments. PTAL.

prashanth26

Just take care of header changes.

hardikdr added 4 commits April 21, 2019 14:37

Update APIs to manage taints, labels and annotations on node-objects

72a16fd

Instrument machine-deployment to maintain taints/annotations/labels w…

62248d3

…ith machineset objects

Instrument machineset to maintain taints/annotations/labels with mach…

5b7e51e

…ine-objects

Instrument machine-controller to maintain taints/annotations/labels w…

1178e41

…ith node-objects

hardikdr requested review from ggaurav10 and a team as code owners April 21, 2019 09:19

gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 21, 2019

hardikdr changed the title ~~Sync metadata~~ Enable support for taints, annotations and labels Apr 21, 2019

gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 21, 2019

prashanth26 reviewed Apr 22, 2019

View reviewed changes

amshuman-kr suggested changes Apr 23, 2019

View reviewed changes

Made changes according review comments

5a78cea

rfranzke mentioned this pull request Apr 24, 2019

Support for labels, annotations, taints for worker pools gardener/gardener#952

Merged

prashanth26 approved these changes May 2, 2019

View reviewed changes

hardikdr force-pushed the sync_metadata branch from ee0d588 to 6f46afe Compare May 2, 2019 09:29

hardikdr added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 2, 2019

gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 2, 2019

hardikdr force-pushed the sync_metadata branch from 6f46afe to 3e8ad07 Compare May 2, 2019 12:03

hardikdr added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 2, 2019

gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 2, 2019

Add tests and made few changes based on review

99a19a7

hardikdr force-pushed the sync_metadata branch from 3e8ad07 to 99a19a7 Compare May 3, 2019 05:11

hardikdr removed the needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 3, 2019

hardikdr added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label May 3, 2019

gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels May 3, 2019

prashanth26 merged commit 9e544fe into gardener:master May 3, 2019

hardikdr mentioned this pull request Aug 26, 2019

Few negative unit-tests for the feature supporting taints, annotations and labels #327

Merged

Enable support for taints, annotations and labels #256

Enable support for taints, annotations and labels #256

Conversation

hardikdr commented Apr 21, 2019 • edited Loading

prashanth26 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashanth26 Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

hardikdr Apr 23, 2019 • edited Loading

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

amshuman-kr 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

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

amshuman-kr Apr 24, 2019 • edited Loading

Choose a reason for hiding this comment

amshuman-kr Apr 23, 2019 • edited Loading

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

hardikdr commented Apr 23, 2019

amshuman-kr commented Apr 24, 2019

prashanth26 left a comment

Choose a reason for hiding this comment

hardikdr commented Apr 21, 2019 •

edited

Loading

prashanth26 left a comment •

edited

Loading

prashanth26 Apr 22, 2019 •

edited

Loading

hardikdr Apr 23, 2019 •

edited

Loading

amshuman-kr Apr 24, 2019 •

edited

Loading

amshuman-kr Apr 23, 2019 •

edited

Loading