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

📖 Cluster API Add-on Orchestration proposal #6905

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

Jont828
Copy link
Contributor

@Jont828 Jont828 commented Jul 12, 2022

What this PR does / why we need it:

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 #

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 12, 2022
@Jont828 Jont828 marked this pull request as ready for review August 9, 2022 00:48
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 9, 2022
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this generally makes sense and reads well to me, i just have a question about the API

Copy link
Contributor

@g-gaston g-gaston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, just a few nit picky suggestions and questions 🙏🏻

docs/proposals/20220712-cluster-api-addon-orchestration.md Outdated Show resolved Hide resolved
docs/proposals/20220712-cluster-api-addon-orchestration.md Outdated Show resolved Hide resolved
docs/proposals/20220712-cluster-api-addon-orchestration.md Outdated Show resolved Hide resolved
docs/proposals/20220712-cluster-api-addon-orchestration.md Outdated Show resolved Hide resolved
docs/proposals/20220712-cluster-api-addon-orchestration.md Outdated Show resolved Hide resolved
docs/proposals/20220712-cluster-api-addon-orchestration.md Outdated Show resolved Hide resolved
docs/proposals/20220712-cluster-api-addon-orchestration.md Outdated Show resolved Hide resolved
docs/proposals/20220712-cluster-api-addon-orchestration.md Outdated Show resolved Hide resolved

However, considering the fact that the link between Cluster lifecycle and Cluster add-on lifecycle is simpler than e.g the link between a Cluster and its infrastructure provider, in this case it is possible to adopt a simplified interaction model between Cluster API and HelmChartProxy (vs an integration model based on CRD with a set of well known field defined in the CAPI contract).

In the first iteration, we will implement this as a single, concrete HelmChartProxy CRD, with a controller watching Cluster API resources, mainly the Cluster and eventually the ControlPlane object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, it would help to clarify by making a clear separate section for Helm Implementation

Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this captures the overall problem statement and solution design well- great work!

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 24, 2022
@sbueringer
Copy link
Member

Great work so far. Thx for pushing this forward!

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jackfrancis
Copy link
Contributor

(I am biased, but) lgtm

@Jont828
Copy link
Contributor Author

Jont828 commented Sep 28, 2022

Squashed and pushed again!

@sbueringer
Copy link
Member

Thank you!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2022
@CecileRobertMichon
Copy link
Contributor

/lgtm

@Jont828
Copy link
Contributor Author

Jont828 commented Oct 3, 2022

@sbueringer @fabriziopandini @CecileRobertMichon It looks like the discussion is wrapping up. Should we start a lazy consensus timer to get this merged?

@CecileRobertMichon
Copy link
Contributor

+1 on starting lazy consensus

Let's set the lazy consensus for 7 days from now, Wednesday, October 12, 2022?

/approve
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 5, 2022
@fabriziopandini
Copy link
Member

/lgtm

@Jont828
Copy link
Contributor Author

Jont828 commented Oct 6, 2022

Sounds good to me! Thanks to everyone who helped review this doc!

@fabriziopandini
Copy link
Member

Lazy consensus expired,
Great work!
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, 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:
  • OWNERS [CecileRobertMichon,fabriziopandini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sbueringer
Copy link
Member

/hold cancel
:)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2022
@k8s-ci-robot k8s-ci-robot merged commit 5774dcc into kubernetes-sigs:main Oct 12, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.3 milestone Oct 12, 2022
@Jont828 Jont828 deleted the addon-proposal branch October 15, 2022 09:27
@ossfellow
Copy link

While a nice solution, in my opinion this proposal suffers from 2 problems:

  1. It doesn’t integrate with the GitOps ecosystem, and in that sense is only useful for bootstrapping a GitOps engine in the newly created cluster.
  2. As the reference implementation shows, the concrete implementations either cannot or would require substantial work (including CRD changes) to capture or employ the richness of combining multiple package management tools (e.g. Helm and Kustomize), which the community has spent years to develop.

The Runtime Hooks proposal avoids both issues, allowing the existing user solutions to get triggered as needed (e.g. bootstrap a CNI and a GitOps engine and let it take over), and let the Kubernetes community invest in other work areas, instead of recreating solutions the community has spent years to develop and improve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.