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.

Sign up for GitHub

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

Jump to bottom

🌱 Decouple MachinePool reconciler from Kubeadm #4931

Merged
merged 1 commit into from
Jun 28, 2024
Merged

🌱 Decouple MachinePool reconciler from Kubeadm #4931

merged 1 commit into from
Jun 28, 2024

Conversation

Danil-Grigorev
Copy link
Member

@Danil-Grigorev Danil-Grigorev commented Jun 20, 2024 •
edited
Loading

What type of PR is this?

What this PR does / why we need it:

This change removes static dependency on kubeadm to establish watches for MachinePool reconciler, allowing to specify a different <Bootstrap>Config GVK on user demand.

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: #4854

Special notes for your reviewer:

  • cherry-pick candidate

TODOs:

  • verify RBAC can be extended for the alternative resource ✨ Add aggregated role support #4933
  • check logic on the token retrieval part - works based on resource hash.
  • includes documentation
  • adds unit tests

Release note:

- Decoupled kubeadm dependency from MachinePool reconciler by supplying alternative GVK for reconciler.
- Provided CLI flag: `--bootstrap-config-gvk` allowing to optionally supply alternative to Kubeadm bootstrap config group in the form of `<Kind>.<version>.<group>`. Example: `KubeadmConfig.v1beta1.bootstrap.cluster.x-k8s.io`

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 20, 2024
@Danil-Grigorev Danil-Grigorev changed the title 🌱 Decouple MachinePool reconciler from Kubeadm 🌱 [WIP] Decouple MachinePool reconciler from Kubeadm Jun 20, 2024
nojnhuh
nojnhuh reviewed Jun 20, 2024
View reviewed changes
config/rbac/role.yaml Outdated
@@ -3,6 +3,18 @@ apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: manager-role
aggregationRule:
Copy link
Contributor

Choose a reason for hiding this comment

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

I was actually also planning to make this RBAC change soon. Depending on how long you think the rest of this PR might take, a separate PR with this particular change would be welcome!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @nojnhuh, thanks, I'm also thinking about opening aggregated role PR separately, as the current state passes CI. Do you know how the codecov job can be retriggered? The failure there seems like a flake.

@Danil-Grigorev Danil-Grigorev force-pushed the decouple-kubeadm branch 2 times, most recently from 45fffb9 to befc216 Compare June 20, 2024 18:46
@codecov Codecov
Copy link

codecov bot commented Jun 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 61.90476% with 8 lines in your changes missing coverage. Please review.

Project coverage is 62.21%. Comparing base (a52056d) to head (c1472a8).
Report is 19 commits behind head on main.

Files Patch % Lines
exp/controllers/helpers.go 14.28% 6 Missing ⚠️
exp/controllers/azuremachinepool_controller.go 85.71% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4931      +/-   ##
==========================================
+ Coverage   62.19%   62.21%   +0.02%     
==========================================
  Files         201      201              
  Lines       16878    16879       +1     
==========================================
+ Hits        10497    10502       +5     
+ Misses       5591     5586       -5     
- Partials      790      791       +1

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

@Danil-Grigorev
Decouple MachinePool reconciler from Kubeadm
c1472a8 
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
@Danil-Grigorev
Copy link
Member Author

/retest codecov

@k8s-ci-robot
Copy link
Contributor

@Danil-Grigorev: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test pull-cluster-api-provider-azure-apiversion-upgrade
  • /test pull-cluster-api-provider-azure-build
  • /test pull-cluster-api-provider-azure-ci-entrypoint
  • /test pull-cluster-api-provider-azure-e2e
  • /test pull-cluster-api-provider-azure-e2e-aks
  • /test pull-cluster-api-provider-azure-test
  • /test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-provider-azure-apidiff
  • /test pull-cluster-api-provider-azure-capi-e2e
  • /test pull-cluster-api-provider-azure-conformance
  • /test pull-cluster-api-provider-azure-conformance-custom-builds
  • /test pull-cluster-api-provider-azure-conformance-dual-stack-with-ci-artifacts
  • /test pull-cluster-api-provider-azure-conformance-ipv6-with-ci-artifacts
  • /test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
  • /test pull-cluster-api-provider-azure-e2e-optional
  • /test pull-cluster-api-provider-azure-e2e-workload-upgrade
  • /test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts-serial-slow
  • /test pull-cluster-api-provider-azure-windows-custom-builds
  • /test pull-cluster-api-provider-azure-windows-with-ci-artifacts

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-provider-azure-apidiff
  • pull-cluster-api-provider-azure-build
  • pull-cluster-api-provider-azure-e2e
  • pull-cluster-api-provider-azure-e2e-aks
  • pull-cluster-api-provider-azure-test
  • pull-cluster-api-provider-azure-verify

In response to this:

/retest codecov

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.

@Danil-Grigorev Danil-Grigorev changed the title 🌱 [WIP] Decouple MachinePool reconciler from Kubeadm 🌱 Decouple MachinePool reconciler from Kubeadm Jun 21, 2024
@k8s-ci-robot
Copy link
Contributor

@Danil-Grigorev: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test pull-cluster-api-provider-azure-apiversion-upgrade
  • /test pull-cluster-api-provider-azure-build
  • /test pull-cluster-api-provider-azure-ci-entrypoint
  • /test pull-cluster-api-provider-azure-e2e
  • /test pull-cluster-api-provider-azure-e2e-aks
  • /test pull-cluster-api-provider-azure-test
  • /test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-provider-azure-apidiff
  • /test pull-cluster-api-provider-azure-capi-e2e
  • /test pull-cluster-api-provider-azure-conformance
  • /test pull-cluster-api-provider-azure-conformance-custom-builds
  • /test pull-cluster-api-provider-azure-conformance-dual-stack-with-ci-artifacts
  • /test pull-cluster-api-provider-azure-conformance-ipv6-with-ci-artifacts
  • /test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
  • /test pull-cluster-api-provider-azure-e2e-optional
  • /test pull-cluster-api-provider-azure-e2e-workload-upgrade
  • /test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts-serial-slow
  • /test pull-cluster-api-provider-azure-windows-custom-builds
  • /test pull-cluster-api-provider-azure-windows-with-ci-artifacts

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-provider-azure-apidiff
  • pull-cluster-api-provider-azure-build
  • pull-cluster-api-provider-azure-e2e
  • pull-cluster-api-provider-azure-e2e-aks
  • pull-cluster-api-provider-azure-test
  • pull-cluster-api-provider-azure-verify

In response to this:

/test codecov

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.

@Danil-Grigorev
Copy link
Member Author

/retest

1 similar comment
@Danil-Grigorev
Copy link
Member Author

/retest

@Danil-Grigorev Danil-Grigorev mentioned this pull request Jun 21, 2024
Merged
4 tasks
@mboersma mboersma mentioned this pull request Jun 24, 2024
Closed
3 tasks
@k8s-ci-robot 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 Jun 25, 2024
mboersma
mboersma approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm

Closes #4868

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 54dff72ba9ac1feced41848a23c424779d2b9be6

@nojnhuh
Copy link
Contributor

nojnhuh commented Jun 27, 2024

/test pull-cluster-api-provider-azure-e2e-optional

nojnhuh
nojnhuh approved these changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

/hold for optional tests

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nojnhuh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 27, 2024
@nojnhuh
Copy link
Contributor

nojnhuh commented Jun 27, 2024

tests are green!
/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 Jun 27, 2024
@nojnhuh
Copy link
Contributor

nojnhuh commented Jun 27, 2024

CI should be healthier now.
/retest

@nojnhuh
Copy link
Contributor

nojnhuh commented Jun 27, 2024

/test pull-cluster-api-provider-azure-e2e-aks

@k8s-ci-robot k8s-ci-robot merged commit e699f04 into kubernetes-sigs:main Jun 28, 2024
22 checks passed
@salasberryfin salasberryfin mentioned this pull request Jul 2, 2024
Open
3 tasks
@Danil-Grigorev Danil-Grigorev mentioned this pull request Aug 7, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mboersma mboersma mboersma approved these changes

@nojnhuh nojnhuh nojnhuh approved these changes

@jsturtevant jsturtevant Awaiting requested review from jsturtevant

@marosset marosset Awaiting requested review from marosset

Assignees

@mboersma mboersma

@nojnhuh nojnhuh

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
CAPZ Planning
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove dependency on Kubeadm when enabling MachinePools
4 participants
@Danil-Grigorev @k8s-ci-robot @nojnhuh @mboersma