Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop unused labels from installer machineSets #182

Merged
merged 1 commit into from
Mar 29, 2019
+53 −24
Merged

Drop unused labels from installer machineSets #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions examples/machine-set.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will update mao as well. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

replicas: 2
selector:
Expand All @@ -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
Expand Down
6 changes: 4 additions & 2 deletions examples/machine-with-filters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions examples/machine-with-user-data.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions examples/machine.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions examples/master-machine.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion pkg/actuators/machine/actuator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/actuators/machine/actuator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 7 additions & 3 deletions pkg/actuators/machine/stubs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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": "",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
},
ProviderSpec: *providerSpec,
},
}
Expand Down
23 changes: 19 additions & 4 deletions pkg/actuators/machine/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of this can collapse to:

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

return true, nil
}
return false, nil
}

// updateConditionCheck tests whether a condition should be updated from the
Expand Down
4 changes: 1 addition & 3 deletions pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!
Expand Down