add explicit machineset label #519
Conversation
openshift-ci-robot
added
the
size/XS
There was a problem hiding this comment.
Please add a description to the PR and improve the commit message (https://github.com/openshift/installer/blob/master/CONTRIBUTING.md).
| @@ -53,12 +53,12 @@ items: | |||
| replicas: {{$instance.Replicas}} | |||
| selector: | |||
| matchLabels: | |||
| sigs.k8s.io/cluster-api-machineset: worker | |||
| sigs.k8s.io/cluster-api-machineset: {{$c.ClusterName}}-worker-{{$index}} | |||
There was a problem hiding this comment.
Do we really want to use the index here? This implies that the order of the worker machine pools in the install config is important, which I don't think that it is. Shouldn't we be using the name of the machine pool from the install config instead?
There was a problem hiding this comment.
Hey @staebler this is just being consistent with the machineset name https://github.com/openshift/installer/pull/519/files#diff-a416ed747e927b21b9c26916a1129cdfR46
To reduce possible side effects for machines matching more than one owner label
https://github.com/kubernetes-sigs/cluster-api/blob/master/pkg/controller/machineset/controller.go#L304
There was a problem hiding this comment.
I'm not sure what the purpose of having a name in the install-config.yml for each worker machine pool is if we are not going to use it for the name of the machine set.
|
ping @abhinavdahiya @csrwng |
There was a problem hiding this comment.
@enxebre can you also make sure this is correct for other objects... Like the master, libvirt worker, openstack worker.
openshift-ci-robot
added
size/S
|
Not sure about the CI error. I'm seeing same here openshift/cluster-api-provider-aws#81 |
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
Add explicit machineset name for matchLabel selector
This is being consistent with the machineset name https://github.com/openshift/installer/pull/519/files#diff-a416ed747e927b21b9c26916a1129cdfR46
To reduce possible side effects for machines matching more than one owner label
https://github.com/kubernetes-sigs/cluster-api/blob/master/pkg/controller/machineset/controller.go#L304