-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
KEP-4322: Add cluster inventory definition #4620
base: master
Are you sure you want to change the base?
Conversation
Welcome @ryanzhang-oss! |
Hi @ryanzhang-oss. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
72bba06
to
04f03c3
Compare
/cc |
/ok-to-test |
@ryanzhang-oss please address the issue |
Summary of the error Table of content not up to date. Did you run 'make update-toc' ?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix build
Table of content not up to date. Did you run 'make update-toc' ?
(nit) |
/test pull-enhancements-verify |
of cluster manager are projects like [OCM](https://open-cluster-management.io/), | ||
[Clusternet](https://clusternet.io/) or [Kubernetes Fleet Manager](https://github.com/Azure/fleet) | ||
|
||
- **Cluster Inventory**: A collection of clusters governed by a single cluster manager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- **Cluster Inventory**: A collection of clusters governed by a single cluster manager. | |
- **Cluster Inventory**: A conceptual term referring to a collection of clusters managed by a single cluster manager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we need to give a quick mention to ClusterSet? https://github.com/kubernetes/enhancements/tree/master/keps/sig-multicluster/1645-multi-cluster-services-api#terminology
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused why ClusterInventory is linked to a cluster Manager and not linked to "All cluster profiles in a given tuple {HubCluster, Namespace}" or "ClusterInventory is the List of ClusterProfile in a namespace". A hub cluster can hold multiple inventories.
Why do we need to talk about ClusterManager here? I feel that's adding stuff to an api not needing it at the moment.
Also, +1 to mikesng@'s "Conceptual term" edit.
|
||
- **Inventory Cluster Consumer**: the person running the cluster managers | ||
or the person developing extensions for cluster managers for the purpose of | ||
workload distribution, operation management etc. | ||
|
||
- **Member Cluster**: A kubernetes cluster that is managed by the cluster | ||
manager. A cluster manager SHOULD have sufficient permission to access | ||
- **Member Cluster**: A kubernetes cluster that is part of the cluster Inventory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- **Member Cluster**: A kubernetes cluster that is part of the cluster Inventory. | |
- **Member Cluster**: A kubernetes cluster that is part of the Cluster Inventory. |
cluster Inventory
should be either Cluster Inventory
or cluster inventory
. Same comment for the recurrences below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use the term "cluster inventory"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to finalizing tomorrow!
I think an inventory isClusterProfileList(Namespace)
.
Which means that it's not only a concept but it's API-tied to ClusterProfile and Namespace on a hub cluster.
Then let's clarify the ClusterSet rules. I think we're on the same page.
An inventory can be at most a single cluster set or there is no clusterset concept within the inventory.
of cluster manager are projects like [OCM](https://open-cluster-management.io/), | ||
[Clusternet](https://clusternet.io/) or [Kubernetes Fleet Manager](https://github.com/Azure/fleet) | ||
|
||
- **Cluster Inventory**: A collection of clusters governed by a single cluster manager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused why ClusterInventory is linked to a cluster Manager and not linked to "All cluster profiles in a given tuple {HubCluster, Namespace}" or "ClusterInventory is the List of ClusterProfile in a namespace". A hub cluster can hold multiple inventories.
Why do we need to talk about ClusterManager here? I feel that's adding stuff to an api not needing it at the moment.
Also, +1 to mikesng@'s "Conceptual term" edit.
- **Member Cluster**: A kubernetes cluster that is managed by the cluster | ||
manager. A cluster manager SHOULD have sufficient permission to access | ||
- **Member Cluster**: A kubernetes cluster that is part of the cluster Inventory. | ||
A cluster manager SHOULD have sufficient permission to access |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it need to be specified? How else would a ClusterManager be able to do its job?
### Notes/Constraints/Caveats (Optional) | ||
### Notes/Constraints/Caveats | ||
#### What's the relationship between the clusterProfile API and cluster Inventory? | ||
The clusterProfile API represents a single member cluster in a cluster Inventory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(new to kep writing here) Do kep usually refer to CRDs as API?
A ClusterProfile CR/Instance = a Cluster
A ClusterInventory = The list of Clusters in the same target hub Cluster, Namespace.
Is that similar to what you're saying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's the same. I think KEP usually lays out the concepts and rules instead of going to describe system design.
The clusterProfile API represents a single member cluster in a cluster Inventory. | ||
|
||
#### What's the relationship between a cluster Inventory and clusterSet? | ||
A cluster inventory is considered a clusterSet if all its member clusters adhere to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is automatic like that. I think it should be a conscious decision, available to consumers for proper assumptions.
A cluster inventory is considered a clusterSet if all its member clusters adhere to the | |
A cluster inventory may or may not represent a ClusterSet. If it does, only one clusterset should be represented in the inventory. (In other terms, a ClusterInventory can map to 0 or 1 ClusterSet). | |
As a result of representing a ClusterSet, member clusters would adhere to sameness rules among themselves. |
[namespace sameness](https://github.com/kubernetes/community/blob/master/sig-multicluster/namespace-sameness-position-statement.md) principle. | ||
|
||
#### How should the API be consumed? | ||
We recommend that all clusterProfile objects within the same cluster inventory reside on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with my definition above, they do since the inventory IS the {cluster, namespace} tuple, it's not a recommendation, it's by definition.
I think inventory is an actual ListClusterProfile(Namespace), not really a concept?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cluster inventory is a concept. I am not sure if we force the ClusterProfile to be CRDs in this KEP.
controller can be run on the hub cluster to offer high-level functionalities over this inventory of clusters. | ||
|
||
#### How should we organize clusterProfile objects on a hub cluster? | ||
While there are no strict requirements, we suggest making the clusterProfile API a namespace-scoped object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While there are no strict requirements, we suggest making the clusterProfile API a namespace-scoped object. | |
The ClusterProfile CRD is a namespace-scoped object, this approach allows to maintain separate cluster inventories on the same cluster, making for easier testing, a multi-inventory feature such multi-environment deployment support and other use cases that may require crossing inventory boundaries. It also allows RBAC to be set at an inventory level, providing some stronger isolation between inventories. |
I think there is at this point. We should be less ambiguous.
Let's say that ClusterProfile CRD is made namespace-scoped
This approach allows users to leverage Kubernetes' native namespace-based RBAC if they wish to restrict access to | ||
certain clusters within the inventory. | ||
|
||
However, if the cluster inventory is a clusterSet, all the clusterProfile objects MUST be placed in the same namespace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's true.
I think clusterset would be a flag on the inventory at this point. So all clusters of the inventory are in the clusterset. I don't think it guarantees completeness of the clusterset. It means that consumers of the inventory can leverage sameness within that inventory safely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what's the benefit to allow splitting one clusterSet into multiple namespaces? Also, what will be the name of the namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the namespace doesn't really matter if you have the label.
I think what's important with clusterset is guaranteeing that all clusters can be considered "having sameness" for a user, because that enables a lot of management simplicity. The requirement is that all clusters are part of the same cluster, not necessarily that they are all there.
use cases I can think of:
- regional management of clusterset; if one wanted localized management of clusters. Example: you have 3 clusters in europe and 3 clusters in us. They are part of the same clusterset but you want a regional hub-cluster. You would have 2 hub cluster that each manage 1 inventory of 3 clusters. The two regional clusters would have to be guaranteed by something global that they wont break sameness (maybe a global hub-cluster?).
- isolation of management: let's say we've given a inventory consumer the ability to manage N namespaces within some clusterProfiles, we could have an inventory of clusters thats a subset of the ClusterSet
- migration: if we needed to migrate a clusterset, we will need to move one cluster at a time to the new representation of the clusterset (but without breaking the ClusterSet rules)
- timing of ON/OFF: clusters may come and go and join/leave the set as they are managed, so the completeness of the set may vary
Obviously a consumer of the API would be limited by the set of profiles they see and it may impact behaviors if the clusterset is not complete, but I think the main rule that we care about here is that the consumer can treat the clusters as being part of the same clusterset.
certain clusters within the inventory. | ||
|
||
However, if the cluster inventory is a clusterSet, all the clusterProfile objects MUST be placed in the same namespace. | ||
In addition, the namespace must have a label with the key "clusterset.multicluster.x-k8s.io" and the value as the name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to the first sentence about labelling. It's a bit opiniated but has the advantage of being specific :)
I don't understand the second part RBAC, it sounds like another piece you have in mind. What is "native namespace-based RBAC" here? why is it different for a namespace with clusterset label?
While there are no strict requirements, we recommend that there is only one clusterProfile object representing a cluster | ||
on a hub cluster. | ||
|
||
However, the clusterProfile objects in two namespaces that contain two clusterSets must be disjoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not only about sameness, its about clusterset rules.
You can just say:
A Cluster can only be in one ClusterSet and therefore its ClusterProfiles can only be in namespaces of that same clusterset or no clusterset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am explaining the reason behind the rule
f4bfa1b
to
6dc38e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@mikeshng: changing LGTM is restricted to collaborators 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-sigs/prow repository. |
/lgtm with only a minor comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there :)
I think it defines most of what we need and my comments are on some details at this point. Thank you!
controller can be run on the hub cluster to offer high-level functionalities over this inventory of clusters. | ||
|
||
#### How should we organize ClusterProfile objects on a hub cluster? | ||
While there are no strict requirements, we suggest making the ClusterProfile API a namespace-scoped object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"we recommend" instead of "we suggest"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we strongly recommend
This approach allows users to leverage Kubernetes' native namespace-based RBAC if they wish to restrict access to | ||
certain clusters within the inventory. | ||
|
||
However, if the cluster inventory is a clusterSet, all the clusterProfile objects MUST be placed in the same namespace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the namespace doesn't really matter if you have the label.
I think what's important with clusterset is guaranteeing that all clusters can be considered "having sameness" for a user, because that enables a lot of management simplicity. The requirement is that all clusters are part of the same cluster, not necessarily that they are all there.
use cases I can think of:
- regional management of clusterset; if one wanted localized management of clusters. Example: you have 3 clusters in europe and 3 clusters in us. They are part of the same clusterset but you want a regional hub-cluster. You would have 2 hub cluster that each manage 1 inventory of 3 clusters. The two regional clusters would have to be guaranteed by something global that they wont break sameness (maybe a global hub-cluster?).
- isolation of management: let's say we've given a inventory consumer the ability to manage N namespaces within some clusterProfiles, we could have an inventory of clusters thats a subset of the ClusterSet
- migration: if we needed to migrate a clusterset, we will need to move one cluster at a time to the new representation of the clusterset (but without breaking the ClusterSet rules)
- timing of ON/OFF: clusters may come and go and join/leave the set as they are managed, so the completeness of the set may vary
Obviously a consumer of the API would be limited by the set of profiles they see and it may impact behaviors if the clusterset is not complete, but I think the main rule that we care about here is that the consumer can treat the clusters as being part of the same clusterset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments in line with @corentone
Looks good we are almost there
controller can be run on the hub cluster to offer high-level functionalities over this inventory of clusters. | ||
|
||
#### How should we organize ClusterProfile objects on a hub cluster? | ||
While there are no strict requirements, we suggest making the ClusterProfile API a namespace-scoped object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we strongly recommend
dc4d0f0
to
340745d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now.
the namespacing and clusterSet rule were the points I cared about, I think they are reasonably described.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: corentone, mikeshng, ryanzhang-oss The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
and keeps their status up-to-date. Each cluster manager MUST be identified with a unique name. | ||
Each ClusterProfile resource SHOULD be managed by only one cluster manager. A cluster manager SHOULD | ||
have sufficient permission to access the member cluster to fetch the information so it can update the status | ||
of the ClusterProfile API resource. Examples of cluster manager are projects like [OCM](https://open-cluster-management.io/), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these examples of a "Cluster Manager" which "creates ClusterProfile API objects"? I don't think any of them use "ClusterProfile API" yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, we haven't implemented those yet (since the clusterProfile API is still not well defined). However, we do have plans to emit clusterProfile API after it's established in the sig. Here are some of the issues
OCM
ClusterNet
FleetManager
|
||
#### How should the API be consumed? | ||
We recommend that all ClusterProfile objects within the same cluster inventory reside on | ||
a dedicated hub Kubernetes cluster. This approach allows consumers to have a single integration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we defined "hub Kubernetes cluster" somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Not yet, maybe we don't need to define what is the hub Kubernetes cluster.
Given the ClusterProfile is created by the cluster manager
, so I guess here we can say the cluster manager organizes the ClusterProfile.
However, if a cluster inventory represents a ClusterSet, all its ClusterProfile objects MUST be part of the same clusterSet | ||
and namespace must be used as the grouping mechanism. In addition, the namespace must have a label with the key "clusterset.multicluster.x-k8s.io" | ||
and the value as the name of the clusterSet. It's important to note that this means users would lose the ability to apply Kubernetes' | ||
native namespace-based RBAC within a clusterSet since Kubernetes does not support nested namespaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence is confusing. ClusterSet and ClusterProfiles are just objects in a namespace. Native RBAC can be applied to the objects. I think this sentence is trying to say that native namespace RBAC in the hub cluster cannot be applied to the clusters that these objects refer to. Is that right?
New changes are detected. LGTM label has been removed. |
Thanks, @ryanzhang-oss! I don't have approval powers, but it LGTM now. |
Add the definition of a cluster Inventory and some notes on how it should be consumed.