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

Jump to bottom

Provide ability to add labels to Cluster object #809

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Provide ability to add labels to Cluster object #809

wants to merge 1 commit into from

Conversation

wahabmk
Copy link
Contributor

@wahabmk wahabmk commented Dec 18, 2024 •
edited
Loading

Description

Provide ability to add labels to the Cluster object created when installing a clusterTemplate by:

  1. Adding .Values.ClusterLabels to all clusterTemplate helm charts.
  2. Using ManagedCluster's own labels via the HMC controller if none have been defined in .Values.ClusterLabels.

Testing

As can be seen from the output below, the .metadata.labels from ManagedCluster have been added to the Cluster object:

➜  ~ kubectl -n hmc-system get managedclusters.hmc.mirantis.com wali-dev-1 --show-labels
NAME         READY   STATUS                    LABELS
wali-dev-1   True    ManagedCluster is ready   labelA=A,labelB=B
➜  ~
➜  ~
➜  ~ kubectl -n hmc-system get cluster --show-labels                                
NAME         CLUSTERCLASS   PHASE         AGE   VERSION   LABELS
wali-dev-1                  Provisioned   13m             app.kubernetes.io/managed-by=Helm,helm.toolkit.fluxcd.io/name=wali-dev-1,helm.toolkit.fluxcd.io/namespace=hmc-system,labelA=A,labelB=B,sveltos-agent=present

@wahabmk wahabmk force-pushed the custom-labels-managed-cluster branch 2 times, most recently from 805f843 to f080795 Compare December 18, 2024 18:12
@wahabmk wahabmk changed the title WIP Provide ability to add labels to Cluster object Dec 18, 2024
@wahabmk wahabmk self-assigned this Dec 18, 2024
@wahabmk wahabmk linked an issue Dec 18, 2024 that may be closed by this pull request
Set labels on Cluster object via ManagedCluster #801
Open
@wahabmk wahabmk force-pushed the custom-labels-managed-cluster branch from f080795 to 3475d9c Compare December 18, 2024 18:23
@wahabmk wahabmk marked this pull request as ready for review December 18, 2024 18:42
@wahabmk wahabmk force-pushed the custom-labels-managed-cluster branch from 3475d9c to 44b336c Compare December 18, 2024 23:09
zerospiel
zerospiel reviewed Dec 19, 2024
View reviewed changes
Copy link
Member

@zerospiel zerospiel left a comment

Choose a reason for hiding this comment

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

lgtm, 2 charts-related-mostly-nit comments

templates/cluster/vsphere-standalone-cp/templates/cluster.yaml Outdated Show resolved Hide resolved
templates/cluster/vsphere-standalone-cp/values.yaml Outdated Show resolved Hide resolved
@wahabmk wahabmk force-pushed the custom-labels-managed-cluster branch 2 times, most recently from 651f907 to b5a7452 Compare December 19, 2024 21:48
@wahabmk wahabmk requested a review from zerospiel December 19, 2024 22:39
@wahabmk wahabmk force-pushed the custom-labels-managed-cluster branch from b5a7452 to 1f07334 Compare December 20, 2024 14:34
Kshatrix
Kshatrix reviewed Dec 23, 2024
View reviewed changes
internal/controller/managedcluster_controller.go

if _, ok := values["clusterLabels"]; !ok {
// Use the ManagedCluster's own labels if not defined.
values["clusterLabels"] = mc.GetObjectMeta().GetLabels()
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't seem reliable since there's no guarantee that all cluster templates have this field (unless you want to make this field required).

Copy link
Contributor Author

@wahabmk wahabmk Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, in this approach we are depending on the helm chart for cluster templates to use the clusterLabels value. We do need some way to assign labels to Cluster objects. Maybe the better approach would be?

  1. In the HMC controller, use the values defined in clusterLabels in the ManagedCluster spec and patch Cluster to apply those labels to it.
  2. Or if clusterLabels is not defined, then use the ManagedCluster's own labels.

internal/controller/managedcluster_controller.go
values["clusterIdentity"] = cred.Spec.IdentityRef

if _, ok := values["clusterLabels"]; !ok {
// Use the ManagedCluster's own labels if not defined.
Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding was that clusterSelector in MultiClusterService is supposed to refer hmc ManagedCluster instead of capi Cluster objects. In that case propagation should not be optional. Is my assumption valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now the ClusterSelector metav1.LabelSelector field is fed directly into the the .spec.clusterSelector field for the Sveltos ClusterProfile object. So the label selectors are matching Cluster objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kshatrix Kshatrix Kshatrix left review comments

@zerospiel zerospiel zerospiel approved these changes

@eromanova eromanova Awaiting requested review from eromanova eromanova is a code owner

@a13x5 a13x5 Awaiting requested review from a13x5 a13x5 is a code owner

@gmlexx gmlexx Awaiting requested review from gmlexx

@kylewuolle kylewuolle Awaiting requested review from kylewuolle

@BROngineer BROngineer Awaiting requested review from BROngineer

Assignees

@wahabmk wahabmk

Labels
None yet
Projects
Project 2A
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set labels on Cluster object via ManagedCluster
3 participants
@wahabmk @zerospiel @Kshatrix