✨ Add MachinePool workers support in ClusterClass #9016

willie-yao · 2023-07-19T22:58:29Z

What this PR does / why we need it:
This PR adds MachinePool workers support in ClusterClass. This is currently a WIP with the following work items still in progress:

Determine fields for DockerMachinePoolTemplate and implement patch
Template cleanup
Unit tests
E2E tests

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #5991

willie-yao · 2023-07-19T23:11:02Z

/hold for squash

sbueringer · 2023-07-21T11:49:53Z

@willie-yao Just ping me when I should do a first review

api/v1alpha4/conversion.go

api/v1beta1/cluster_types.go

api/v1beta1/clusterclass_types.go

api/v1beta1/condition_consts.go

test/extension/handlers/topologymutation/handler.go

test/framework/kubernetesversions/template.go

test/infrastructure/docker/exp/api/v1beta1/dockermachinepooltemplate_types.go

test/infrastructure/docker/templates/cluster-template-mp-development.yaml

sbueringer · 2023-08-10T17:42:18Z

@willie-yao Really really nice work!!!

Most of my comments come down to:

nits
re-sync because we changed parts of the MD implemention while you were working on this PR

I would suggest the following:

Let's fix the open findings then do another round of review
Then let's only adjust the existing unit tests so they are green again
Let's then consider to get some quick & easy e2e test coverage (I would take a look how we can achieve this once we're done with 1. and 2.

We have to discuss this but I think I'm in favor of getting this PR with its current scope in place and then follow-up with e2e test, unit test coverage, ...

I think the PR as is is still reviewable (at least for folks familiar with ClusterClass) and the easiest way to make progress is to get this one in as is and then follow-up with more tests. The existing tests ensure that existing ClusterClass + MD functionality doesn't break. Splitting the implementation in multiple PRs including unit tests makes everything more complicated in my opinion and also harder to tell if everything works. With unit tests I expect that this PR would be at least 3-4x the size. Also this PR mostly comes down to replicating the same logic we have for MD. I would like to merge this relatively quickly so that we don't have to continuously rebase when changes are made to the ClusterClass implementation

@CecileRobertMichon @fabriziopandini ^^ Does this sound like an acceptable way forward for you?

willie-yao · 2023-08-10T18:41:56Z

/hold cancel

CecileRobertMichon

Really great stuff @willie-yao

Also this PR mostly comes down to replicating the same logic we have for MD

My overall comment is that there seems to be a lot of code that was adapted directly from the MachineDeployment implementation and it seems it's already outdated (from @sbueringer's earlier comment) so I'm wondering how we can reduce duplicate code so we don't have the same logic in two places, one for MachineDeployments and one for MachinePools

Does this sound like an acceptable way forward for you?

yes although without unit tests there is even greater risk of the two diverging in the meantime and MachinePool regressing because of it...

for e2e tests for now if we add a MachinePool worker to the clusterclass quickstart test template would that be enough to at least create one in tests?

Overall +1 to not expanding scope too much in this PR and getting it merged then iterating on tests, etc.

internal/controllers/topology/cluster/blueprint.go

internal/controllers/topology/cluster/current_state.go

internal/controllers/topology/cluster/desired_state.go

fabriziopandini · 2023-08-11T14:15:50Z

@CecileRobertMichon @fabriziopandini ^^ Does this sound like an acceptable way forward for you?

+1 from me
Please, start immediately tracking the next steps and pending TODO in an issue assigned to @willie-yao (unfortunately in some cases in the past we lost track of those action items, and rebuilding those lists after some time is always both time-consuming and risky, and I would like to avoid this)

willie-yao · 2023-08-11T17:40:44Z

Thanks for all the helpful comments and feedback! Before moving forward, I wanted to make sure that my current approach is the best path forward.

Addressing @CecileRobertMichon's comment

My overall comment is that there seems to be a lot of code that was adapted directly from the MachineDeployment implementation and it seems it's already outdated (from @sbueringer's earlier comment) so I'm wondering how we can reduce duplicate code so we don't have the same logic in two places, one for MachineDeployments and one for MachinePools

I agree that this PR does contain a lot of duplicated code from MachineDeployments. This could pose a problem if someone wants to make changes to one of the topologies, they will have to duplicate the efforts for the other. Would it be worth considering refactoring this PR to use generics or to reduce the amount of repeated code somehow? If our priority is to land this feature soon, we may also consider merging this as-is and re-factoring the approach later. @sbueringer @fabriziopandini wdyt?

CecileRobertMichon · 2023-08-11T22:47:38Z

To be clear, I'm +1 on moving forward with this PR as-is and refactoring as a follow-up (perhaps before even spending time writing unit tests for the new code). A single PR with the refactor and the feature would be too large for review.

sbueringer · 2023-08-14T10:56:10Z

yes although without unit tests there is even greater risk of the two diverging in the meantime and MachinePool regressing because of it...

I think duplicating the unit test from MD to MP won't significantly reduce the risk of MD code changing in the meantime. Getting this PR merged as soon as possible will help though :)

for e2e tests for now if we add a MachinePool worker to the clusterclass quickstart test template would that be enough to at least create one in tests?

Yup that is pretty much what I was thinking

I think we have to be careful about de-duplication as it can introduce bugs. I would suggest we explore this topic once we have the unit test coverage in place for MPs as well

exp/api/v1alpha3/conversion.go

internal/controllers/topology/cluster/patches/inline/json_patch_generator.go

internal/controllers/topology/cluster/patches/engine.go

internal/controllers/topology/cluster/scope/blueprint.go

internal/controllers/topology/cluster/scope/state.go

internal/controllers/topology/cluster/desired_state.go

internal/controllers/topology/cluster/reconcile_state.go

sbueringer · 2023-08-14T16:11:24Z

A quick round this time. Let's try to address the open comments + linter + verify + test. Then I would take another closer look

willie-yao · 2023-08-15T21:00:56Z

/test all

sbueringer · 2023-08-24T04:50:32Z

/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-workload-upgrade-1-28-latest-main

chrischdi

One more nit,

but as of that

/lgtm

internal/controllers/topology/cluster/desired_state.go

k8s-ci-robot · 2023-08-24T07:30:47Z

LGTM label has been added.

Git tree hash: 2d8512a22504156e49ba30bc4395bd238ad0fad3

sbueringer · 2023-08-24T11:13:50Z

Delta looks fine. I'll try to do a last full review either on Friday or Monday. But at this stage I only expect nits or follow-up tasks.

sbueringer · 2023-08-24T17:56:32Z

/test pull-cluster-api-e2e-full-main

sbueringer · 2023-08-25T18:20:42Z

Really really nice work!!

I'll open a small follow-up PR on Monday to fix some nits (just fyi)

/lgtm
/approve

k8s-ci-robot · 2023-08-25T18:20:48Z

LGTM label has been added.

Git tree hash: ad08bc42e6ec991f2bea7b2ee1f3b2913d1f0621

k8s-ci-robot · 2023-08-25T18:20:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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~~ [sbueringer]

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

chrischdi · 2023-08-26T08:36:49Z

Awesome work @willie-yao ! 🎉

sbueringer · 2023-08-28T10:09:03Z

Yup, absolute great work!

@willie-yao Now we have a lot of unit tests in front of us :)

I would suggest to go over the list I added to the umbrella issue and then open PRs for maybe around 2-3 files at a time? (so that we get the unit tests quickly merged but the PRs are not too big) WDYT?

willie-yao · 2023-08-28T17:18:24Z

I would suggest to go over the list I added to the umbrella issue and then open PRs for maybe around 2-3 files at a time? (so that we get the unit tests quickly merged but the PRs are not too big) WDYT?

Perfect! I'll get get to work right away. Should we prioritize unit tests first or the E2E test?

sbueringer · 2023-08-29T05:54:09Z

Unit tests for now would be good. I want to take a look at our current e2e tests first

sbueringer · 2023-08-29T05:55:11Z

@willie-yao btw I ordered the unit tests by priority on the issue

sbueringer · 2023-08-29T10:04:45Z

@willie-yao Another note. Please let me know on the issue on which unit tests you're working before you do it. Just so that we can potentially can get help from others if someone has time and avoid duplicate work.

g-gaston · 2023-10-23T15:45:42Z

/area machinepool

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. labels Jul 19, 2023

k8s-ci-robot requested review from elmiko and jackfrancis July 19, 2023 22:58

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 19, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 19, 2023

willie-yao force-pushed the cc-mp-squashed branch from 98276e2 to 2439fb9 Compare July 25, 2023 00:32

sbueringer reviewed Aug 10, 2023

View reviewed changes

willie-yao changed the title ~~WIP: ✨ Add MachinePool workers support in ClusterClass~~ ✨ Add MachinePool workers support in ClusterClass Aug 10, 2023

willie-yao force-pushed the cc-mp-squashed branch from 2439fb9 to 017cd16 Compare August 10, 2023 18:41

k8s-ci-robot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 10, 2023

willie-yao force-pushed the cc-mp-squashed branch from f5d13f8 to f415da6 Compare August 10, 2023 22:08

CecileRobertMichon reviewed Aug 11, 2023

View reviewed changes

willie-yao force-pushed the cc-mp-squashed branch 2 times, most recently from fbc594a to 74ff719 Compare August 11, 2023 19:49

sbueringer reviewed Aug 14, 2023

View reviewed changes

willie-yao force-pushed the cc-mp-squashed branch 2 times, most recently from c13efb9 to 6158492 Compare August 15, 2023 20:45

willie-yao force-pushed the cc-mp-squashed branch from 6158492 to a5b08df Compare August 15, 2023 22:45

This comment was marked as resolved.

Sign in to view

chrischdi reviewed Aug 24, 2023

View reviewed changes

internal/controllers/topology/cluster/desired_state.go Show resolved Hide resolved

k8s-ci-robot assigned chrischdi Aug 24, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2023

Add machinepool support to clusterclass

8ae27c1

willie-yao force-pushed the cc-mp-squashed branch from 8fae422 to 8ae27c1 Compare August 24, 2023 17:53

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2023

k8s-ci-robot requested a review from chrischdi August 24, 2023 17:53

k8s-ci-robot assigned sbueringer Aug 25, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2023

k8s-ci-robot merged commit f28fc73 into kubernetes-sigs:main Aug 25, 2023

k8s-ci-robot added this to the v1.6 milestone Aug 25, 2023

sbueringer mentioned this pull request Aug 25, 2023

🌱 Minor fixes for CC+MP implementation #9318

Merged

k8s-ci-robot added the area/machinepool Issues or PRs related to machinepools label Oct 23, 2023

richardcase mentioned this pull request Nov 4, 2024

Add ClusterClass support machine pools kubernetes-sigs/cluster-api-provider-aws#4658

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add MachinePool workers support in ClusterClass #9016

✨ Add MachinePool workers support in ClusterClass #9016

willie-yao commented Jul 19, 2023 •

edited by sbueringer

Loading

willie-yao commented Jul 19, 2023

sbueringer commented Jul 21, 2023

sbueringer commented Aug 10, 2023 •

edited

Loading

willie-yao commented Aug 10, 2023

CecileRobertMichon left a comment •

edited

Loading

fabriziopandini commented Aug 11, 2023

willie-yao commented Aug 11, 2023

CecileRobertMichon commented Aug 11, 2023

sbueringer commented Aug 14, 2023 •

edited

Loading

sbueringer commented Aug 14, 2023

willie-yao commented Aug 15, 2023

This comment was marked as resolved.

sbueringer commented Aug 24, 2023

chrischdi left a comment

k8s-ci-robot commented Aug 24, 2023

sbueringer commented Aug 24, 2023 •

edited

Loading

sbueringer commented Aug 24, 2023

sbueringer commented Aug 25, 2023

k8s-ci-robot commented Aug 25, 2023

k8s-ci-robot commented Aug 25, 2023

chrischdi commented Aug 26, 2023

sbueringer commented Aug 28, 2023

willie-yao commented Aug 28, 2023

sbueringer commented Aug 29, 2023 •

edited

Loading

sbueringer commented Aug 29, 2023

sbueringer commented Aug 29, 2023

g-gaston commented Oct 23, 2023

✨ Add MachinePool workers support in ClusterClass #9016

✨ Add MachinePool workers support in ClusterClass #9016

Conversation

willie-yao commented Jul 19, 2023 • edited by sbueringer Loading

willie-yao commented Jul 19, 2023

sbueringer commented Jul 21, 2023

sbueringer commented Aug 10, 2023 • edited Loading

willie-yao commented Aug 10, 2023

CecileRobertMichon left a comment • edited Loading

Choose a reason for hiding this comment

fabriziopandini commented Aug 11, 2023

willie-yao commented Aug 11, 2023

CecileRobertMichon commented Aug 11, 2023

sbueringer commented Aug 14, 2023 • edited Loading

sbueringer commented Aug 14, 2023

willie-yao commented Aug 15, 2023

This comment was marked as resolved.

sbueringer commented Aug 24, 2023

chrischdi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 24, 2023

sbueringer commented Aug 24, 2023 • edited Loading

sbueringer commented Aug 24, 2023

sbueringer commented Aug 25, 2023

k8s-ci-robot commented Aug 25, 2023

k8s-ci-robot commented Aug 25, 2023

chrischdi commented Aug 26, 2023

sbueringer commented Aug 28, 2023

willie-yao commented Aug 28, 2023

sbueringer commented Aug 29, 2023 • edited Loading

sbueringer commented Aug 29, 2023

sbueringer commented Aug 29, 2023

g-gaston commented Oct 23, 2023

willie-yao commented Jul 19, 2023 •

edited by sbueringer

Loading

sbueringer commented Aug 10, 2023 •

edited

Loading

CecileRobertMichon left a comment •

edited

Loading

sbueringer commented Aug 14, 2023 •

edited

Loading

sbueringer commented Aug 24, 2023 •

edited

Loading

sbueringer commented Aug 29, 2023 •

edited

Loading