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.

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

Placement by cluster name requires managedcluster be labeled name=<cluster name> #16

Open
hatfieldbrian opened this issue Jul 6, 2021 · 12 comments
Assignees

Comments

@hatfieldbrian
Copy link

I wonder why a managedcluster needs to be labeled with its name, when its metadata.name could be used?

// MCM Assumption: clusters are always labeled with name

If ocm requires a managedcluster be labeled with its name for placement, I wonder why ocm does not so label each managedcluster?

@mikeshng
Copy link
Member

@xiangjingli do you know the history of this label requirement?
As the author mentioned, if we have the name of the managedcluster it's probably good enough to just do a GET with the name.

@xiangjingli
Copy link
Collaborator

@mikeshng The managedCluster CR has the name label by default in midstream. So placementRule leverages the matchLabel functionality to get the clusters by cluster name.

Has the name label been removed from managedCluster CR in upstream? If so, we can switch to get the clusters by matching the managedCluster meta.name.

@mikeshng
Copy link
Member

@xiangjingli the label doesn't seem to be there using just the upstream registration code.

I think we can try doing label lookup first then if there are no results use a GET as a fallback? What do you think?

/assign mikeshng

@xiangjingli
Copy link
Collaborator

@mikeshng yeah, it sounds good. There are two ways

  • get all managed clusters one time, return the managed clusters where the meta.name matches the cluster name list in placementRule spec.clusters.
  • parse all the cluster names from the placementRule spec.clusters list. for each cluster name, get the managed cluster CR

It seems the first solution may get better performance. there is only one GET

@hatfieldbrian hatfieldbrian changed the title Placement by cluster name requies managedcluster be labeled name=<cluster name> Placement by cluster name requires managedcluster be labeled name=<cluster name> Jul 19, 2021
@ivan-cai
Copy link
Contributor

ivan-cai commented Nov 23, 2021

This problem still exists~, i think the ways said by @xiangjingli are good. It seems better if both support solution 1 and 2?
if solution 2 is supported, we can create one PlacementRule for every application, no matter what method(e.g. clusters, or cluster selector) for placement.

@mikeshng
Copy link
Member

Hi @ivan-cai thank you for your feedback. We are currently in the process of adopting OCM's Placement API https://github.com/open-cluster-management-io/placement which is the native Placement API for OCM across the whole project.
The current Subscription native PlacementRule API is designated for deprecation and will no longer be supported soon. Hence, we are unlikely to work on this item.

If the community is interested in this enhancement for the PlacementRule API, please feel free to submit a fix. Sorry for the lack of update until now.

@ivan-cai
Copy link
Contributor

Hi @mikeshng,adopting Placement API is a good choice. But what time can it be done?

@mikeshng
Copy link
Member

@ivan-cai we are targeting before end of this year.

@mikeshng
Copy link
Member

We have implemented majority of the placement API integration. Follow steps in https://github.com/open-cluster-management-io/OCM/blob/main/solutions/deploy-a-helm-chart/READEME.md to give it a try.

More docs and other misc changes to come later but for now the integration should work fine.

@nirs
Copy link

nirs commented Mar 24, 2024

@mikeshng, We replaced usage of PlacementRule with Placement, do we still need to keep the workaround labeling the managed cluster?

@mikeshng
Copy link
Member

@mikeshng, We replaced usage of PlacementRule with Placement, do we still need to keep the workaround labeling the managed cluster?

No, it shouldn't be needed but it's highly recommended you give it a try first.

Also, it might depend on your Placement spec. If your Placement spec contains the label criteria then it needs to be there.

@xiangjingli could you please confirm? Thanks.

@nirs
Copy link

nirs commented Mar 24, 2024

Also, it might depend on your Placement spec. If your Placement spec contains the label criteria then it needs to be there.

Thanks, will check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants