Drop unused labels from installer machineSets #182

vikaschoudhary16 · 2019-03-21T09:59:47Z

https://jira.coreos.com/browse/CLOUD-407

ingvagabund · 2019-03-21T12:37:15Z

examples/machine-set.yaml

    spec:
+      metadata:
+        labels:
+          node-role.kubernetes.io/master: "infra"


Is node-role.kubernetes.io/master: "infra" a valid role label? I am familiar only with node-role.kubernetes.io/master: "" and node-role.kubernetes.io/worker: "" forms.

Since value is string type and i have seen forms like node-role.kubernetes.io/master: "true" as well, I think it depends on implementation that what is treated as valid.

Alternative could be to use two labels: node-role.kubernetes.io/master: "" and node-role.kubernetes.io/infra: "" . But since data type allows arbitrary strings, thought of using it instead of increasing label count.

it should be node-role.kubernetes.io/master and node-role.kubernetes.io/infra.

ingvagabund · 2019-03-21T12:40:37Z

examples/machine-set.yaml

@@ -6,8 +6,6 @@ metadata:
  namespace: test
  labels:
    machine.openshift.io/cluster-api-cluster: tb-asg-35
-    machine.openshift.io/cluster-api-machine-role: infra
-    machine.openshift.io/cluster-api-machine-type: master
 spec:


Machine HC controller is still using the labels to filter out master nodes [1]. Do we have any replacement for it?

[1] https://github.com/openshift/machine-api-operator/blob/c4a348a011c07db7e3f864380b0134b318112d19/pkg/controller/machinehealthcheck/machinehealthcheck_controller.go#L335-L336

will update mao as well. Thanks!

machine HC already validates from node role https://github.com/openshift/machine-api-operator/blob/c4a348a011c07db7e3f864380b0134b318112d19/pkg/controller/machinehealthcheck/machinehealthcheck_controller.go#L340-L360

vikaschoudhary16 · 2019-03-22T08:52:29Z

/test e2e-aws-operator

enxebre · 2019-03-27T07:58:53Z

pkg/actuators/machine/utils.go

@@ -189,7 +189,7 @@ func providerConfigFromMachine(client client.Client, machine *machinev1.Machine,

 // isMaster returns true if the machine is part of a cluster's control plane
 func isMaster(machine *machinev1.Machine) bool {
-	if machineType, exists := machine.ObjectMeta.Labels[providerconfigv1.MachineTypeLabel]; exists && machineType == "master" {
+	if _, exists := machine.Spec.ObjectMeta.Labels["node-role.kubernetes.io/master"]; exists {


can we fetch from node here?

@enxebre done!

https://jira.coreos.com/browse/CLOUD-407

vikaschoudhary16 · 2019-03-27T13:15:31Z

/test e2e

vikaschoudhary16 · 2019-03-27T15:50:05Z

/test e2e-aws

ingvagabund · 2019-03-28T08:08:17Z

/retest

ingvagabund · 2019-03-28T08:08:25Z

/approve

openshift-ci-robot · 2019-03-28T08:08:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

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

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

enxebre · 2019-03-29T10:20:17Z

/lgtm

frobware

I know this has merged but I had some comments having looked at the description for:

Drop unused labels machine-api-operator#255

frobware · 2019-04-02T07:10:59Z

pkg/actuators/machine/stubs.go

+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					"node-role.kubernetes.io/master": "",
+					"node-role.kubernetes.io/infra":  "",


Looking through the PR I got the impression we were removing infra.

frobware · 2019-04-02T07:11:44Z

pkg/actuators/machine/utils.go

-		return true
+func (a *Actuator) isMaster(machine *machinev1.Machine) (bool, error) {
+	if machine.Status.NodeRef == nil {
+		glog.Errorf("NodeRef not found in machine %s", machine.Name)


As this can be transient should we log at warning level?

frobware · 2019-04-02T07:15:04Z

pkg/actuators/machine/utils.go

+		return false, fmt.Errorf("failed to get node from machine %s", machine.Name)
+	}
+
+	if _, exists := node.Labels["node-role.kubernetes.io/master"]; exists {


The rest of this can collapse to:

_, exists := node.Labels["node-role.kubernetes.io/master"] return exists

Signed-off-by: Naadir Jeewa <[email protected]>

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 21, 2019

openshift-ci-robot requested review from frobware and michaelgugino March 21, 2019 10:00

vikaschoudhary16 force-pushed the drop-unused-labels branch 3 times, most recently from 6119232 to f79bd3f Compare March 21, 2019 11:27

ingvagabund reviewed Mar 21, 2019

View reviewed changes

vikaschoudhary16 force-pushed the drop-unused-labels branch 2 times, most recently from 189a7ae to b16436b Compare March 25, 2019 12:55

vikaschoudhary16 mentioned this pull request Mar 25, 2019

Drop unused labels openshift/machine-api-operator#255

Merged

enxebre reviewed Mar 27, 2019

View reviewed changes

vikaschoudhary16 mentioned this pull request Mar 27, 2019

Drop unused machine labels openshift/installer#1474

Merged

Drop unused labels from installer machineSets

67ac39b

https://jira.coreos.com/browse/CLOUD-407

vikaschoudhary16 force-pushed the drop-unused-labels branch from b16436b to 67ac39b Compare March 27, 2019 12:02

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

openshift-ci-robot assigned enxebre Mar 29, 2019

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

openshift-merge-robot merged commit 4d95324 into openshift:master Mar 29, 2019

vikaschoudhary16 deleted the drop-unused-labels branch March 29, 2019 11:59

frobware reviewed Apr 2, 2019

View reviewed changes

michaelgugino pushed a commit to mgugino-upstream-stage/cluster-api-provider-aws that referenced this pull request Feb 12, 2020

services/ec2/instances: Fix NPE (openshift#182)

34aaa09

Signed-off-by: Naadir Jeewa <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop unused labels from installer machineSets #182

Drop unused labels from installer machineSets #182

vikaschoudhary16 commented Mar 21, 2019

ingvagabund Mar 21, 2019 •

edited

Loading

vikaschoudhary16 Mar 22, 2019

derekwaynecarr Mar 25, 2019

ingvagabund Mar 21, 2019

vikaschoudhary16 Mar 22, 2019

enxebre Mar 27, 2019

vikaschoudhary16 commented Mar 22, 2019

enxebre Mar 27, 2019

vikaschoudhary16 Mar 27, 2019

vikaschoudhary16 commented Mar 27, 2019

vikaschoudhary16 commented Mar 27, 2019

ingvagabund commented Mar 28, 2019

ingvagabund commented Mar 28, 2019

openshift-ci-robot commented Mar 28, 2019

enxebre commented Mar 29, 2019

frobware left a comment

frobware Apr 2, 2019

frobware Apr 2, 2019

frobware Apr 2, 2019

Drop unused labels from installer machineSets #182

Drop unused labels from installer machineSets #182

Conversation

vikaschoudhary16 commented Mar 21, 2019

ingvagabund Mar 21, 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

vikaschoudhary16 commented Mar 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikaschoudhary16 commented Mar 27, 2019

vikaschoudhary16 commented Mar 27, 2019

ingvagabund commented Mar 28, 2019

ingvagabund commented Mar 28, 2019

openshift-ci-robot commented Mar 28, 2019

enxebre commented Mar 29, 2019

frobware 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

ingvagabund Mar 21, 2019 •

edited

Loading