✨ Apply labels to Machines if they're part of a MD or MS #1701
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
tahsinrahman
force-pushed
the
add-machine-labels
branch
from
6a3b68e to
e9d3063
Compare
|
/assign |
tahsinrahman
force-pushed
the
add-machine-labels
branch
2 times, most recently
from
f1cb7da to
aabc18a
Compare
tahsinrahman
force-pushed
the
add-machine-labels
branch
from
aabc18a to
b1a0e14
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tahsinrahman, vincepri 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 |
k8s-ci-robot
added
the
approved
|
/milestone v0.3.0 |
controllers/machine_controller.go
Outdated
| @@ -181,6 +182,37 @@ func (r *MachineReconciler) reconcile(ctx context.Context, cluster *clusterv1.Cl | |||
| return res, kerrors.NewAggregate(errs) | |||
| } | |||
|
|
|||
| func (r *MachineReconciler) reconcileLables(ctx context.Context, m *clusterv1.Machine) error { | |||
There was a problem hiding this comment.
I'm wondering if these labels should be added at the same time we are adding the ownerreferences in the MachineSet/MachineDeployment.
For example, on the MachineDeployment, we could add the label for the machinedeployment to the MachineSet, but also update the labelselector and the labels applied on the MachineSet. We could do similar in the MachineSet as well.
I'm thinking that if we go this route, since we are already adding the cluster-name label, then we could potentially even remove the need for a user-specified label selector.
@vincepri thoughts?
There was a problem hiding this comment.
That was my first idea as well, it's quite different from what this PR is doing though. We can possibly follow-up to improve this in a different PR and move the logic in the MS/MD controllers?
There was a problem hiding this comment.
@detiber @vincepri thanks for your suggestions! lets close/redo it
We can actually add these following lines in getNewMachine(), it'll be a lot simpler than now!
machine.Labels[clusterv1.MachineSetLabelName] = machineSet.Name
if md := metav1.GetControllerOf(machineSet); md != nil {
machine.Labels[clusterv1.MachineDeploymentLabelName] = md.Name
}
There was a problem hiding this comment.
filed this draft pr!
#1711
|
closing in favor of #1711 |
|
@tahsinrahman: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #1695