Merge pull request #182 from vikaschoudhary16/drop-unused-labels

Drop unused labels from installer machineSets
openshift · Mar 29, 2019 · 4d95324 · 4d95324
2 parents 82a961f + 67ac39b
commit 4d95324
Show file tree

Hide file tree

Showing 10 changed files with 53 additions and 24 deletions.
diff --git a/examples/machine-set.yaml b/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:
   replicas: 2
   selector:
@@ -19,9 +17,11 @@ spec:
       labels:
         machine.openshift.io/cluster-api-machineset: test-master
         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:
+      metadata:
+        labels:
+          node-role.kubernetes.io/master: ""
+          node-role.kubernetes.io/infra: ""
       providerSpec:
         value:
           apiVersion: awsproviderconfig.k8s.io/v1alpha1

diff --git a/examples/machine-with-filters.yaml b/examples/machine-with-filters.yaml
@@ -7,9 +7,11 @@ metadata:
   generateName: vs-worker-
   labels:
     machine.openshift.io/cluster-api-cluster: tb-asg-35
-    machine.openshift.io/cluster-api-machine-role: compute
-    machine.openshift.io/cluster-api-machine-type: worker
 spec:
+  metadata:
+    labels:
+      node-role.kubernetes.io/compute: ""
+      node-role.kubernetes.io/worker: ""
   providerSpec:
     value:
       apiVersion: awsproviderconfig.k8s.io/v1alpha1

diff --git a/examples/machine-with-user-data.yaml b/examples/machine-with-user-data.yaml
@@ -7,9 +7,11 @@ metadata:
   generateName: vs-master-
   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:
+  metadata:
+    labels:
+      node-role.kubernetes.io/master: ""
+      node-role.kubernetes.io/infra: ""
   providerSpec:
     value:
       apiVersion: awsproviderconfig.k8s.io/v1alpha1

diff --git a/examples/machine.yaml b/examples/machine.yaml
@@ -7,9 +7,11 @@ metadata:
   generateName: vs-master-
   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:
+  metadata:
+    labels:
+      node-role.kuberntes.io/master: ""
+      node-role.kuberntes.io/infra: ""
   providerSpec:
     value:
       apiVersion: awsproviderconfig.k8s.io/v1alpha1

diff --git a/examples/master-machine.yaml b/examples/master-machine.yaml
@@ -7,9 +7,11 @@ metadata:
   generateName: vs-master-
   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:
+  metadata:
+    labels:
+      node-role.kubernetes.io/master: ""
+      node-role.kubernetes.io/infra: ""
   providerSpec:
     value:
       apiVersion: awsproviderconfig.k8s.io/v1alpha1

diff --git a/pkg/actuators/machine/actuator.go b/pkg/actuators/machine/actuator.go
@@ -191,7 +191,11 @@ func (a *Actuator) CreateMachine(cluster *machinev1.Cluster, machine *machinev1.
 	}
 
 	// We explicitly do NOT want to remove stopped masters.
-	if !isMaster(machine) {
+	isMaster, err := a.isMaster(machine)
+	if err != nil {
+		return nil, a.handleMachineError(machine, apierrors.CreateMachine("error determining if machine is master: %v", err), createEventAction)
+	}
+	if !isMaster {
 		// Prevent having a lot of stopped nodes sitting around.
 		err = removeStoppedMachine(machine, awsClient)
 		if err != nil {

diff --git a/pkg/actuators/machine/actuator_test.go b/pkg/actuators/machine/actuator_test.go
@@ -56,7 +56,7 @@ func TestMachineEvents(t *testing.T) {
 	machineInvalidProviderConfig.Spec.ProviderSpec.ValueFrom = nil
 
 	workerMachine := machine.DeepCopy()
-	workerMachine.Labels[providerconfigv1.MachineTypeLabel] = "worker"
+	workerMachine.Spec.Labels["node-role.kubernetes.io/worker"] = ""
 
 	cases := []struct {
 		name                    string

diff --git a/pkg/actuators/machine/stubs.go b/pkg/actuators/machine/stubs.go
@@ -114,9 +114,7 @@ func stubMachine() (*machinev1.Machine, error) {
 			Name:      "aws-actuator-testing-machine",
 			Namespace: defaultNamespace,
 			Labels: map[string]string{
-				providerconfigv1.ClusterIDLabel:   clusterID,
-				providerconfigv1.MachineRoleLabel: "infra",
-				providerconfigv1.MachineTypeLabel: "master",
+				providerconfigv1.ClusterIDLabel: clusterID,
 			},
 			Annotations: map[string]string{
 				// skip node draining since it's not mocked
@@ -125,6 +123,12 @@ func stubMachine() (*machinev1.Machine, error) {
 		},
 
 		Spec: machinev1.MachineSpec{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					"node-role.kubernetes.io/master": "",
+					"node-role.kubernetes.io/infra":  "",
+				},
+			},
 			ProviderSpec: *providerSpec,
 		},
 	}

diff --git a/pkg/actuators/machine/utils.go b/pkg/actuators/machine/utils.go
@@ -188,11 +188,26 @@ 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" {
-		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)
+		return false, nil
 	}
-	return false
+	node := &corev1.Node{}
+	nodeKey := types.NamespacedName{
+		Namespace: machine.Status.NodeRef.Namespace,
+		Name:      machine.Status.NodeRef.Name,
+	}
+
+	err := a.client.Get(context.Background(), nodeKey, node)
+	if err != nil {
+		return false, fmt.Errorf("failed to get node from machine %s", machine.Name)
+	}
+
+	if _, exists := node.Labels["node-role.kubernetes.io/master"]; exists {
+		return true, nil
+	}
+	return false, nil
 }
 
 // updateConditionCheck tests whether a condition should be updated from the

diff --git a/pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go b/pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go
@@ -25,9 +25,7 @@ import (
 const (
 	// ClusterIDLabel is the label that a machineset must have to identify the
 	// cluster to which it belongs.
-	ClusterIDLabel   = "machine.openshift.io/cluster-api-cluster"
-	MachineRoleLabel = "machine.openshift.io/cluster-api-machine-role"
-	MachineTypeLabel = "machine.openshift.io/cluster-api-machine-type"
+	ClusterIDLabel = "machine.openshift.io/cluster-api-cluster"
 )
 
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!