馃摉 Docs cluster controller #5611
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
| @@ -11,6 +11,17 @@ The Cluster controller's main responsibilities are: | |||
|
|
|||
| ## Contracts | |||
|
|
|||
| ### Relationship to other Cluster API types | |||
There was a problem hiding this comment.
What are you trying to surface, and this is super useful, but I'm wondering if this is there right place to do it given that here we are talking about the Cluster controller, while those informations most probably are important when trying to understand the InfrastructureCluster controller (down in this page) and the control plane controller (which is documented on another page).
What about adding a "Relationship with other controllers" paragraph under those, so we split control plane and infrastructure?
There was a problem hiding this comment.
I was trying to have an analogous to https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/book/src/developer/architecture/controllers/control-plane.md#relationship-to-other-cluster-api-types.
The way the Cluster CR coordinates with infraCluster and controlPlane and its dependencies are not particularly intuitive for people with little context, so I thought it'd be useful to have them clarified in one single place.
What about adding a "Relationship with other controllers" paragraph under those, so we split control plane and infrastructure?
you mean having two sections one here another one under control-plane.md? sgtm though I think having this to be read as above in a single place somewhere it'd be illustrative for new joiners to understand how all pieces play together.
There was a problem hiding this comment.
Ok, I understand the value of providing an overall documentation of Cluster controller interactions, but my concern is that the main consumers of this documentation are providers developers, and we are not covering their perspective given they start from control plane or infrastructure cluster docs.
What about:
- in this paragraph (or better immediately under Contracts without a sub title) just state that the Cluster controller interacts with InfrastructureCluster and the control plane controller, linking the corresponding paragraphs in the the two controllers for details.
- in the corresponding paragraphs for InfrastructureCluster and the control plane controller, provide details of the interactions each controller has with the Cluster/Controller; this will allow to extend those details e.g. by adding interaction between control plane and MHC etc. etc. thus providing a single point where the providers developers can find relevant info for each controller type
There was a problem hiding this comment.
Adding a little bit more context, also CABPK and probably other controllers interact with the Cluster for ownership/infrastructure ready etc. etc. and I'm a little bit of conflating all those infos in a single paragraph; this is another reason for moving details where their are consumed.
There was a problem hiding this comment.
ok, updated to complement both Infrastructure Provider and Control Plane Provider sections.
PTAL.
|
|
||
| The Cluster controller bubbles up `spec.controlPlaneEndpoint` and `status.infrastructureReady` from the infraCluster. | ||
|
|
||
| Kubeadm control plane implementation will exit reconciliation until it sees `cluster.spec.controlPlaneEndpoint` populated. |
There was a problem hiding this comment.
Let's keep this generic and speak about control plane controller instead of KCP
enxebre
force-pushed
the
docs-cluster-controller
branch
2 times, most recently
from
01ad2aa
to
054f5fc
Compare
| @@ -5,6 +5,7 @@ | |||
| The Cluster controller's main responsibilities are: | |||
|
|
|||
| * Setting an OwnerReference on the infrastructure object referenced in `Cluster.Spec.InfrastructureRef`. | |||
| * Setting an OwnerReference on the control plane object referenced in `Cluster.Spec.ControlPlaneRef`. | |||
There was a problem hiding this comment.
| * Setting an OwnerReference on the control plane object referenced in `Cluster.Spec.ControlPlaneRef`. | |
| * Setting an OwnerReference on the controlPlane object referenced in `Cluster.Spec.ControlPlaneRef`. |
nits - these names are better aligning with the way the structs are named (+/- capitaliztion) to prevent any confusion.
There was a problem hiding this comment.
I would probably use:
| * Setting an OwnerReference on the control plane object referenced in `Cluster.Spec.ControlPlaneRef`. | |
| * Setting an OwnerReference on the ControlPlane object referenced in `Cluster.spec.controlPlaneRef`. |
Changed the path, not sure if we have any consistency in the docs regarding "control plane object" vs "ControlPlane object". If we go with ControlPlane I would prefer capitalizing it.
There was a problem hiding this comment.
It seems It's referred across this file and also in control-plane.md as "control plane" so I'd rather keep it and consider renaming everywhere in a different PR.
There was a problem hiding this comment.
That's fine by me - control plane is easy to understand written as plain English. For InfrastructureCluster and others though it tends to be clearer to make the fact it's a type very explicit.
There was a problem hiding this comment.
@enxebre WDYT about correcting the JSON path? Cluster.Spec.ControlPlaneRef => Cluster.spec.controlPlaneRef
(I don't insist on it)
There was a problem hiding this comment.
I had missed it, done! updated the existing one a the one this is introducing.
| @@ -39,6 +39,12 @@ Kubernetes control plane consisting of the following services: | |||
|
|
|||
| ### Relationship to other Cluster API types | |||
|
|
|||
| The Cluster controller will set an OwnerReference on the Control Plane CR. The Control Plane controller should normally exit reconciliation until it sees the owner reference. | |||
There was a problem hiding this comment.
| The Cluster controller will set an OwnerReference on the Control Plane CR. The Control Plane controller should normally exit reconciliation until it sees the owner reference. | |
| The Cluster controller will set an OwnerReference on the controlPlane. The controlPlane controller should normally take no action during reconciliation until it sees the ownerReference. |
| @@ -39,6 +39,12 @@ Kubernetes control plane consisting of the following services: | |||
|
|
|||
| ### Relationship to other Cluster API types | |||
|
|
|||
| The Cluster controller will set an OwnerReference on the Control Plane CR. The Control Plane controller should normally exit reconciliation until it sees the owner reference. | |||
|
|
|||
| A Control Plane controller implementation should exit reconciliation until it sees `cluster.spec.controlPlaneEndpoint` populated. | |||
There was a problem hiding this comment.
| A Control Plane controller implementation should exit reconciliation until it sees `cluster.spec.controlPlaneEndpoint` populated. | |
| A controlPlane controller implementation should take no action during reconciliation until it sees `cluster.spec.controlPlaneEndpoint` populated. |
|
|
||
| A Control Plane controller implementation should exit reconciliation until it sees `cluster.spec.controlPlaneEndpoint` populated. | ||
|
|
||
| The Cluster controller bubbles up `status.ready` into `status.controlPlaneReady` and `status.initialized` into a `controlPlaneInitialized` condition from the Control Plane CR. |
There was a problem hiding this comment.
| The Cluster controller bubbles up `status.ready` into `status.controlPlaneReady` and `status.initialized` into a `controlPlaneInitialized` condition from the Control Plane CR. | |
| The Cluster controller bubbles up `status.ready` into `status.controlPlaneReady` and `status.initialized` into a `controlPlaneInitialized` condition from the controlPlane. |
enxebre
force-pushed
the
docs-cluster-controller
branch
from
054f5fc
to
be0a8f1
Compare
|
thanks @killianmuldoon @sbueringer! I think I addressed all but the ones for control plane which we can address separately. |
enxebre
force-pushed
the
docs-cluster-controller
branch
from
be0a8f1
to
2e2580f
Compare
|
/lgtm (pending one tiny final nit on capitalization) |
k8s-ci-robot
added
the
lgtm
enxebre
force-pushed
the
docs-cluster-controller
branch
from
2e2580f
to
e286980
Compare
k8s-ci-robot
removed
the
lgtm
enxebre
force-pushed
the
docs-cluster-controller
branch
from
e286980
to
005294b
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
Add Relationship to other Cluster API types for cluster controller
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #