✨Adds a new controller to synchronize labels between BMHs and K Nodes #152

Arvinderpal · 2021-01-11T16:10:31Z

Introduces a new controller in CAPM3 to synchronize a specific set of labels placed on a BMH with those on the corresponding Kubernetes Node running on that BMH.

Design Proposal: metal3-io/metal3-docs#149

Example 1: Labels on BMH are added to the K Node

Add prefix annotation to Metal3Cluster:

 kubectl annotate metal3cluster test1 metal3.io/metal3-label-sync-prefixes=my-prefix.metal3.io,foo.bar -n=metal3 --overwrite

Add labels to BareMetalHost:

kubectl label baremetalhosts node-0 my-prefix.metal3.io/rack=xyz-123 -n=metal3
kubectl label baremetalhosts node-0 foo.bar/xyz=abc -n=metal3

You should now see labels show up on the K Node:

kubectl get baremetalhosts --show-labels -n=metal3
kubectl get nodes --show-labels

Example 2: Label changes directly on K Node will be reconciled:

Add label directly to node. The label will be removed at the next reconcile cycle:

kubectl label node test1-h4vjt my-prefix.metal3.io/color=blue

Delete label from Node. The label will be re-added at next reconcile cycle:

kubectl label node test1-h4vjt my-prefix.metal3.io/rack-

metal3-io-bot · 2021-01-11T16:10:48Z

Hi @Arvinderpal. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

maelk · 2021-01-11T16:39:58Z

/ok-to-test
Can you please add a condition to not reconcile paused objects ?

Arvinderpal · 2021-01-22T18:18:00Z

/ok-to-test
Can you please add a condition to not reconcile paused objects ?

@maelk I added the pause check for the BareMetalHost, Cluster and Metal3Cluster. Thanks.

Arvinderpal · 2021-01-22T18:21:51Z

@dhellmann @maelk @zaneb
This PR is ready for an initial review. PTAL.
I have also updated the description with how to try out this feature.

prefix, between BareMetalHost and Kubernetes Nodes.

maelk · 2021-02-27T16:27:19Z

controllers/metal3labelsync_controller.go

+// +kubebuilder:rbac:groups=metal3.io,resources=baremetalhosts,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=metal3.io,resources=baremetalhosts/status,verbs=get;update;patch
+// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=metal3machines,verbs=get;list;watch
+// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=metal3machines/status,verbs=get
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines;machines/status,verbs=get;list;watch
+// +kubebuilder:rbac:groups="",resources=events,verbs=get;list;watch;create;update;patch
+// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch;delete


do you need all those permissions ? I would highly recommend to trim them down

Yes, you're right. I will only keep the read operations -- they are needed to fetch the kubeconfig of the target cluster.

maelk · 2021-02-27T16:28:26Z

controllers/metal3labelsync_controller.go

+		}
+		return ctrl.Result{}, err
+	}
+	if host.Annotations != nil && host.Annotations[bmh.PausedAnnotation] == baremetal.PausedAnnotationKey {


You should check on the presence of the annotation, not on the value. There might be other values set by the user (or empty string) so we should not care for it when checking for pause (like in capi)

Arvinderpal

@maelk Thanks for the review. Please take another look.

Arvinderpal · 2021-03-03T14:18:27Z

controllers/metal3labelsync_controller.go

+// +kubebuilder:rbac:groups=metal3.io,resources=baremetalhosts,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=metal3.io,resources=baremetalhosts/status,verbs=get;update;patch
+// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=metal3machines,verbs=get;list;watch
+// +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=metal3machines/status,verbs=get
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines;machines/status,verbs=get;list;watch
+// +kubebuilder:rbac:groups="",resources=events,verbs=get;list;watch;create;update;patch
+// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch;delete


Yes, you're right. I will only keep the read operations -- they are needed to fetch the kubeconfig of the target cluster.

Arvinderpal · 2021-03-03T14:27:09Z

controllers/metal3labelsync_controller.go

+		}
+		return ctrl.Result{}, err
+	}
+	if host.Annotations != nil && host.Annotations[bmh.PausedAnnotation] == baremetal.PausedAnnotationKey {


Arvinderpal · 2021-03-03T14:39:34Z

@dhellmann @zaneb PTAL.
Much appreciated.

digambar15 · 2021-03-09T15:32:51Z

Hey, @Arvinderpal Can you fix unit test issues ? Otherwise this patch looks good.

Arvinderpal · 2021-03-09T16:05:41Z

/retest

Arvinderpal · 2021-03-15T15:07:24Z

@fmuyassarov PTAL at this PR.
Also, I can't seem to issue the /retest command. I wonder if you can allow that?

furkatgofurov7 · 2021-03-15T15:12:31Z

Seems you inserted the word inside the code block, and it did not work.

/retest

fmuyassarov

Looks good to me, just some nits

controllers/metal3labelsync_controller.go

maelk

One last comment, otherwise looks good to me

maelk · 2021-03-29T09:36:51Z

controllers/metal3labelsync_controller.go

+		capm3Machine := &capm3.Metal3Machine{}
+		if err := r.Client.Get(context.TODO(), name, capm3Machine); err != nil {
+			log.Error(err, "failed to get Metal3Machine")
+			return nil


I'm wondering if we should rather use "continue" here to not block the reconciliation on one machine. For example a user has scaled out a machine deployment but does not have enough BMH. If that machine is first in the list, it will always return on line 311. Using continue, would allow us to get past that.

@maelk That's a good point. I had thought about this too and took the more conservative route. In addition to line 311, should we also use continue for 316 and 321?

yes, I think for all exit conditions, we should instead use "continue"

what about line 306. There could be situations (though hopefully not frequent) where a machine was created without metal3machine (yet ?). We should probably use continue there too, no ?

@maelk The if statement above "if m.Spec.InfrastructureRef.Name == nil", should catch the case where Machine exists but Metal3Machine does not. I suppose there is a rare corner case where InfrastructureRef is updated but Metal3Machine is not yet created. I will make the change.

kashifest · 2021-03-29T10:05:02Z

/lgtm

Arvinderpal · 2021-03-31T05:06:17Z

@fmuyassarov Can you issue /test-integration? Seems to get ignored when I issue it.

/test-integration

kashifest · 2021-03-31T05:42:11Z

/test-integration
/test golint

fmuyassarov

/lgtm
/cc @maelk @kashifest

maelk · 2021-04-01T08:06:09Z

@Arvinderpal I have a nit, however, I think it could be good to go as is, and fixed afterwards, as you wish. I will approve the PR and put a hold. if you want to merge it and fix after, that's ok by me, since it's a detail, then jut remove the hold. But if you fix it in this PR, you'll just need another lgtm.
/approve
/hold

metal3-io-bot · 2021-04-01T08:06:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Arvinderpal, digambar15, maelk

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

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

maelk · 2021-04-01T08:06:57Z

Hum, my comment is not easily visible, it is regarding lne 306, where we should probably use a continue too.

Metal3Cluster that defines prefix set. Only labels that fall within prefix set will be synchronized.

Arvinderpal · 2021-04-01T18:34:14Z

/retest

Arvinderpal · 2021-04-06T01:21:32Z

Can someone kick off /test-integration for me?
I have addressed all the open issues, so we should be in good shape to merge.

kashifest · 2021-04-06T05:23:55Z

/test-integration

kashifest · 2021-04-06T06:10:46Z

/lgtm

Arvinderpal · 2021-04-07T15:23:06Z

@maelk @fmuyassarov PTAL. Should be ready to merge.

fmuyassarov · 2021-04-08T08:32:49Z

/hold cancel

fmuyassarov · 2021-04-08T08:36:20Z

/test golint

metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 11, 2021

Arvinderpal force-pushed the sync-labels-bmh-node branch from 1dd77b9 to 04aa457 Compare January 22, 2021 17:56

Arvinderpal changed the title ~~WIP: ✨Adds a new controller to synchronize labels between BMHs and K Nodes~~ ✨Adds a new controller to synchronize labels between BMHs and K Nodes Jan 22, 2021

metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2021

Arvinderpal mentioned this pull request Jan 25, 2021

Define mechanism for managing lifecycle of Labels, Taints post deployment via BMH or provider artifacts airshipit/airshipctl#377

Closed

Arvinderpal mentioned this pull request Feb 8, 2021

Add design proposal for label sync mechanism between BMHs and K Nodes metal3-io/metal3-docs#149

Merged

Arvinderpal force-pushed the sync-labels-bmh-node branch from 04aa457 to d05cc34 Compare February 25, 2021 21:06

metal3-io-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 25, 2021

Adds a new controler to synchronize labels, matching a specific

4ac4b41

prefix, between BareMetalHost and Kubernetes Nodes.

Arvinderpal force-pushed the sync-labels-bmh-node branch from d05cc34 to 3c3fd2b Compare February 25, 2021 22:07

metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 25, 2021

maelk reviewed Feb 27, 2021

View reviewed changes

Arvinderpal force-pushed the sync-labels-bmh-node branch from 3c3fd2b to 1121d0b Compare March 3, 2021 14:27

Arvinderpal commented Mar 3, 2021

View reviewed changes

fmuyassarov reviewed Mar 17, 2021

View reviewed changes

controllers/metal3labelsync_controller.go Outdated Show resolved Hide resolved

controllers/metal3labelsync_controller.go Outdated Show resolved Hide resolved

controllers/metal3labelsync_controller.go Outdated Show resolved Hide resolved

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2021

maelk reviewed Mar 29, 2021

View reviewed changes

Arvinderpal force-pushed the sync-labels-bmh-node branch from e9bdf05 to 5b3bab1 Compare March 30, 2021 23:27

metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2021

fmuyassarov reviewed Mar 31, 2021

View reviewed changes

metal3-io-bot requested a review from maelk March 31, 2021 17:49

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2021

metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2021

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2021

For label synchronization, use an annotation defined on the

2ca12f2

Metal3Cluster that defines prefix set. Only labels that fall within prefix set will be synchronized.

Arvinderpal force-pushed the sync-labels-bmh-node branch from 5b3bab1 to 2ca12f2 Compare April 1, 2021 15:47

metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2021

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2021

metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2021

metal3-io-bot merged commit 0440379 into metal3-io:master Apr 8, 2021

lb4368 mentioned this pull request Jul 13, 2021

Spike: Validate Node Label changes can be made through Metal3 BMH airshipit/airshipctl#615

Closed

kashifest deleted the sync-labels-bmh-node branch December 9, 2022 07:15

✨Adds a new controller to synchronize labels between BMHs and K Nodes #152

✨Adds a new controller to synchronize labels between BMHs and K Nodes #152

Conversation

Arvinderpal commented Jan 11, 2021 • edited Loading

metal3-io-bot commented Jan 11, 2021

maelk commented Jan 11, 2021

Arvinderpal commented Jan 22, 2021

Arvinderpal commented Jan 22, 2021

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

Arvinderpal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arvinderpal commented Mar 3, 2021

digambar15 commented Mar 9, 2021

Arvinderpal commented Mar 9, 2021

Arvinderpal commented Mar 15, 2021

furkatgofurov7 commented Mar 15, 2021

fmuyassarov left a comment

Choose a reason for hiding this comment

maelk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashifest commented Mar 29, 2021

Arvinderpal commented Mar 31, 2021

kashifest commented Mar 31, 2021

fmuyassarov left a comment

Choose a reason for hiding this comment

maelk commented Apr 1, 2021

metal3-io-bot commented Apr 1, 2021

maelk commented Apr 1, 2021

Arvinderpal commented Apr 1, 2021

Arvinderpal commented Apr 6, 2021

kashifest commented Apr 6, 2021

kashifest commented Apr 6, 2021

Arvinderpal commented Apr 7, 2021

fmuyassarov commented Apr 8, 2021

fmuyassarov commented Apr 8, 2021

Arvinderpal commented Jan 11, 2021 •

edited

Loading