-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
📖 CAEP: ControlPlane #1613
📖 CAEP: ControlPlane #1613
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: detiber 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 |
/milestone v0.3.0 |
71dc5fb
to
a25cb15
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.
EOD here, but I have a couple of initial comments. Will try to review more tomorrow.
73c86a8
to
940b5a3
Compare
/kind proposal |
2575b61
to
de893a9
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.
Thank you for putting this proposal together, it's a great step in the right direction.
The comments outlined below are a mix of major and minor questions, given the amount of comments, we should probably go over them on Zoom later today, or it's also fine to address them async if you all prefer.
docs/proposals/images/controlplane/controlplane-init-2.plantuml
Outdated
Show resolved
Hide resolved
docs/proposals/images/controlplane/controlplane-init-3.plantuml
Outdated
Show resolved
Hide resolved
docs/proposals/images/controlplane/controlplane-init-6.plantuml
Outdated
Show resolved
Hide resolved
docs/proposals/images/controlplane/controlplane-init-7.plantuml
Outdated
Show resolved
Hide resolved
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.
Thanks for the proposal, really nice work.
One question though:
Should we explicitly define the minimal contract which xyz-control-plane-controller should adhere to, as part of this CAEP?
- Eg, homogeneous status/or-something across
XyzControlPlane
CRDs, which should be initialized/maintained by respective control-plane-controllers. - This proposal does talk many details of kubeadm-cp-controller, but I was also hoping for one generic-behavioral contract.
- Or is it already mentioned and I missed it?
2c04462
to
42eea5f
Compare
@ncdc Added a current non-goal for the followup work related to stable api endpoints, we already have the current mechanism for using cluster.Status.APIEndpoints as part of the Constraints and Assumptions |
@chuckha @akutz @jaypipes @alexbrand @pablochacin @randomvariable @rsmitty @dlipovetsky @jzhoucliqr @sbueringer @CecileRobertMichon @hardikdr I believe all outstanding comments have been addressed. Please take a look, I'm hoping that we can go ahead and get this merged to unblock work to start on the initial implementation. |
We would like to go with lazy consensus and plan to merge this on Wednesday as long as there are no blocking comments (will discuss at the community meeting) |
My only comment is to change this from an annotation to a label, so that it can be used in labelSelector |
Are you okay if we address this after merging as a followup issue? |
Yes |
Created #1764 to address the feedback on the label vs annotation for upgrade selection. |
What this PR does / why we need it:
Adds a CAEP for ControlPlane Management