-
Notifications
You must be signed in to change notification settings - Fork 1.5k
KEP4322: add some clusterProfile property #5185
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
base: master
Are you sure you want to change the base?
Conversation
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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
f6f24af
to
5390b8a
Compare
|
||
##### count.node.k8s.io | ||
It contains the total number of nodes in the k8s clusters | ||
* The value is dynamic but not changing fast. |
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.
If the nodes can be of different types, does a total count have much value?
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 am not sure what percentage of k8s clusters have heterogenous vs homogeneous node so it might be useful for homogeneous clusters
Here is the corresponding ClusterProfile API | ||
```yaml | ||
properties: | ||
- name: group.customResource.k8s.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.
should we include the version? consumer may need to know what's the latest version installed.
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.
sure, but CRD version might not show the real version. i.e argo CD version has been stuck in v1alpha1 forever.
labels: | ||
multicluster.x-k8s.io/clusterProfile: true | ||
spec: | ||
value: ["100.3.3.4:5683","qs-oar7gr9p.azmk8s.io:443"] |
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 it be quoted to not be treated as an array?
value: ["100.3.3.4:5683","qs-oar7gr9p.azmk8s.io:443"] | |
value: '["100.3.3.4:5683","qs-oar7gr9p.azmk8s.io:443"]' |
name: count.node.k8s.io | ||
labels: | ||
multicluster.x-k8s.io/clusterProfile: true | ||
Spec: |
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.
There are other cases with upper case Spec
Spec: | |
spec: |
|
||
Here is an example of a cluster-entrypoints.k8s.io ClusterProperty | ||
```yaml | ||
apiVersion: about.k8s.io/v1 |
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.
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.
my bad, I think it's still v1alpha1
labels: | ||
multicluster.x-k8s.io/clusterProfile: true | ||
Spec: | ||
value: 120 |
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.
similar as the previous comment.
value: 120 | |
value: '120' |
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.
Thank you! Overall it looks good. Just a couple of minor comments.
different cluster managers. | ||
Properties defines cluster characteristics through a list of Property objects. | ||
Each Property can be one of: | ||
1. A ClusterProperty resource (as defined in KEP-2149) |
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.
nit: could you please add a link to https://github.com/kubernetes/enhancements/tree/master/keps/sig-multicluster/2149-clusterid#clusterproperty-crd
@@ -1135,7 +1325,7 @@ This is creeping pretty far towards excessive cluster proliferation (and cross-r | |||
|
|||
 | |||
|
|||
This is the model most suited to a cluster-scoped ClusterProfile resource. In contrast to the prior models discussed, in this approach the ClusterProfile CRD would be written directly to each "member" cluster. ClusterSet membership would either be established through peer-to-peer relationships, or managed by an external control plane. For ClusterSet security and integrity, a two-way handshake of some sort would be needed between the local cluster and each peer or the external control plane to ensure it is properly authorized to serve endpoints for exported services or import services from other clusters. While these approaches could be implemented with a namespace-scoped ClusterProfile CRD in the `default` or a designated namespace, misuse is most likely in this model, because the resource would be more likely to be authored by a human if using the peer-to-peer model. Due to the complexity and fragility concerns of managing clusterset membership in a peer-to-peer topology, an external control plane would likely be preferable. Assuming the external control plane does not support Kubernetes APIs (if it did, any of the "hub" models could be applied instead), it could still be possible to implement this model with a namespace-scoped ClusterProfile resource, but it is _not_ recommended. | |||
This is the model most suited to a cluster-scoped ClusterProfile resource. In contrast to the prior models discussed, in this approach the ClusterProfile CRD would be written directly to each "member" cluster. ClusterSet membership would either be established through peer-to-peer relationships, or managed by an external control plane. For ClusterSet security and integrity, a two-way handshake of some sort would be needed between the local cluster and each peer or the external control plane to ensure it is properly authorized to serve entrypoints for exported services or import services from other clusters. While these approaches could be implemented with a namespace-scoped ClusterProfile CRD in the `default` or a designated namespace, misuse is most likely in this model, because the resource would be more likely to be authored by a human if using the peer-to-peer model. Due to the complexity and fragility concerns of managing clusterset membership in a peer-to-peer topology, an external control plane would likely be preferable. Assuming the external control plane does not support Kubernetes APIs (if it did, any of the "hub" models could be applied instead), it could still be possible to implement this model with a namespace-scoped ClusterProfile resource, but it is _not_ recommended. |
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 hadn't seen this paragraph/model before and I understand that it is not new in this PR, but I just wanted to note that this model has so many un-addressed implementation issues that makes it practically infeasible for most real scenarios. It is ok to keep it, but maybe we should add a note that this model is not suitable for most production scenarios.
Triage note: This PR is still open for discussion on the properties proposed themselves, though as a matter of process/governance it is blocked pending decisions made around the quorum/voting/government/etc process being discussed separately in #5255 (comment) |
Add the discussed cluster properties to the KEP4322