refactor(manager): set machinepool true as default #5146

KOREAN139 · 2024-09-29T10:28:01Z

MachinePool feature flag is true as default since CAPI v1.7.0, but capz-contoller-manager deployment still uses false as default.
It causes error spec: Forbidden: can be set only if the Cluster API 'MachinePool' feature flag is enabled when user does not specify MachinePool feature flag true in capz-controller-manager.
As CAPI v1.7.0 has been GA around six months, It should set MachinePool feature flag as true to make seamless UX.
I left manager manifest in test as it is since it set MachinePool feature flag as true using EXP_MACHINE_POOL.

references
https://github.com/kubernetes-sigs/cluster-api/releases/tag/v1.7.0
kubernetes-sigs/cluster-api#10141

related slack thread
https://kubernetes.slack.com/archives/CEX9HENG7/p1727200579137939

What type of PR is this?

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):
Closes #4911

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

refactor(manager): set machinepool true as default

linux-foundation-easycla · 2024-09-29T10:28:08Z

The committers listed above are authorized under a signed CLA.

✅ login: KOREAN139 / name: Sanggu Han (aef33ac)

k8s-ci-robot · 2024-09-29T10:28:10Z

Welcome @KOREAN139!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-azure 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-azure has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-09-29T10:28:10Z

Hi @KOREAN139. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

mboersma · 2024-09-29T21:19:19Z

/ok-to-test

KOREAN139 · 2024-09-29T23:59:54Z

/retest

mboersma · 2024-09-30T14:06:01Z

/retest

codecov · 2024-09-30T14:20:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.36%. Comparing base (abd8819) to head (7860aa3).
Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5146   +/-   ##
=======================================
  Coverage   51.36%   51.36%           
=======================================
  Files         273      273           
  Lines       24676    24676           
=======================================
  Hits        12674    12674           
  Misses      11212    11212           
  Partials      790      790

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mboersma · 2024-09-30T14:23:17Z

Thanks @KOREAN139 – we had #4911 but it was never updated.

Could you make a similar change to hack/observability/opentelemetry/controller-manager-patch.yaml and update any related docs to match? See the comment from #4911:

Can we add a note in the docs that this is now enabled by default? https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/docs/book/src/topics/managedcluster.md?plain=1#L59

There are probably a few other places in the docs that need updating as well.

KOREAN139 · 2024-09-30T15:34:02Z

@mboersma Sure! I'll check the #4911 as well, and update hack/observability/opentelemetry/controller-manager-patch.yaml & related docs.

willie-yao · 2024-09-30T21:43:39Z

/retest

KOREAN139 · 2024-10-01T16:58:51Z

Just updated following documents.

docs/book/src/developers/development.md
Add note that its default value is true, need to set false if user wants to disable this
docs/book/src/managed/managedcluster.md
Add note that MachinePool feature flag is enabled by default
docs/book/src/self-managed/machinepools.md
Add note that MachinePool feature is in Beta, and enabled by default. Add reference for this too.

Let me know if there's any document left to update! @mboersma

P.S. Force-pushed some commits to make it verified / fix some commit messages 😅

willie-yao

Looks good! Just a minor wording change and remember to squash your commits!

docs/book/src/developers/development.md

- MachinePool feature flag is true as default since CAPI v1.7.0, but capz-contoller-manager deployment still uses false as default. - It causes error `spec: Forbidden: can be set only if the Cluster API 'MachinePool' feature flag is enabled` when user does not specify MachinePool feature flag true in capz-controller-manager. - As CAPI v1.7.0 has been GA around six months, It should set MachinePool feature flag as true to make seamless UX. references https://github.com/kubernetes-sigs/cluster-api/releases/tag/v1.7.0 kubernetes-sigs/cluster-api#10141 refactor(hack): set machinepool true in default - set machinepool feature flag as true for controller in observability hack stack doc(machinepool): add note of enabled per default - add note about MachinePool feature gate's promotion to beta - add note about MachinePool feature gate's default value (true) doc(machinepool): restore feature requirement part - as the information "MachinePool feature gate need to be set as true" must be shown, restore the part - modify the "need to turn on" sentence to "it's already on, but you can set it again" doc(machinepool): accept wording change - changed the wording as willie-yao suggested in PR Co-authored-by: Willie Yao <[email protected]>

KOREAN139 · 2024-10-03T05:04:40Z

Just applied your suggestion, and squashed commits @willie-yao !

mboersma

/lgtm

Thank you!

k8s-ci-robot · 2024-10-03T14:43:04Z

LGTM label has been added.

Git tree hash: a2b5271f712e6f3e026971ccf546a5722fdcdebb

nawazkh · 2024-10-03T16:28:57Z

🚢 it!
lgtm!

willie-yao · 2024-10-03T18:56:01Z

/lgtm
/approve

Thank you! 🚀

k8s-ci-robot · 2024-10-03T18:56:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: willie-yao

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~~ [willie-yao]

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

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 29, 2024

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 29, 2024

k8s-ci-robot requested review from marosset and nojnhuh September 29, 2024 10:28

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 29, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 29, 2024

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 30, 2024

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 1, 2024

willie-yao reviewed Oct 2, 2024

View reviewed changes

docs/book/src/developers/development.md Outdated Show resolved Hide resolved

k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. and removed do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels Oct 3, 2024

mboersma approved these changes Oct 3, 2024

View reviewed changes

k8s-ci-robot assigned mboersma Oct 3, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2024

k8s-ci-robot assigned willie-yao Oct 3, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2024

k8s-ci-robot merged commit 80fca66 into kubernetes-sigs:main Oct 3, 2024
13 checks passed

k8s-ci-robot added this to the v1.18 milestone Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(manager): set machinepool true as default #5146

refactor(manager): set machinepool true as default #5146

KOREAN139 commented Sep 29, 2024 •

edited by mboersma

Loading

linux-foundation-easycla bot commented Sep 29, 2024 •

edited

Loading

k8s-ci-robot commented Sep 29, 2024

k8s-ci-robot commented Sep 29, 2024

mboersma commented Sep 29, 2024

KOREAN139 commented Sep 29, 2024

mboersma commented Sep 30, 2024

codecov bot commented Sep 30, 2024 •

edited

Loading

mboersma commented Sep 30, 2024 •

edited

Loading

KOREAN139 commented Sep 30, 2024 •

edited

Loading

willie-yao commented Sep 30, 2024

KOREAN139 commented Oct 1, 2024 •

edited

Loading

willie-yao left a comment

KOREAN139 commented Oct 3, 2024

mboersma left a comment

k8s-ci-robot commented Oct 3, 2024

nawazkh commented Oct 3, 2024

willie-yao commented Oct 3, 2024

k8s-ci-robot commented Oct 3, 2024

refactor(manager): set machinepool true as default #5146

refactor(manager): set machinepool true as default #5146

Conversation

KOREAN139 commented Sep 29, 2024 • edited by mboersma Loading

linux-foundation-easycla bot commented Sep 29, 2024 • edited Loading

k8s-ci-robot commented Sep 29, 2024

k8s-ci-robot commented Sep 29, 2024

mboersma commented Sep 29, 2024

KOREAN139 commented Sep 29, 2024

mboersma commented Sep 30, 2024

codecov bot commented Sep 30, 2024 • edited Loading

Codecov Report

mboersma commented Sep 30, 2024 • edited Loading

KOREAN139 commented Sep 30, 2024 • edited Loading

willie-yao commented Sep 30, 2024

KOREAN139 commented Oct 1, 2024 • edited Loading

willie-yao left a comment

Choose a reason for hiding this comment

KOREAN139 commented Oct 3, 2024

mboersma left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 3, 2024

nawazkh commented Oct 3, 2024

willie-yao commented Oct 3, 2024

k8s-ci-robot commented Oct 3, 2024

KOREAN139 commented Sep 29, 2024 •

edited by mboersma

Loading

linux-foundation-easycla bot commented Sep 29, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

mboersma commented Sep 30, 2024 •

edited

Loading

KOREAN139 commented Sep 30, 2024 •

edited

Loading

KOREAN139 commented Oct 1, 2024 •

edited

Loading