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

Cleanup of preinstall assertions #11901

Merged
merged 9 commits into from
Jan 23, 2025
Merged

Cleanup of preinstall assertions #11901

merged 9 commits into from
Jan 23, 2025

Conversation

VannTen
Copy link
Contributor

@VannTen VannTen commented Jan 20, 2025

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

  • Various cleanup in kubernetes/preinstall/task/0040-verify-settings.yml
  • Drop old assert for removed stuff which should be out of users inventories by now
  • Drop compatibility with etcd_kubeadm_enabled (which was deprecated) and assert it's not defined for one release

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

action required
`etdc_kubeadm_enabled` (was deprecated) is removed. You should remove it from your inventory

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jan 20, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VannTen

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 20, 2025
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2025
@k8s-ci-robot k8s-ci-robot requested a review from yankay January 20, 2025 16:36
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 20, 2025
@VannTen
Copy link
Contributor Author

VannTen commented Jan 20, 2025

/label tide/merge-method-merge

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges. label Jan 20, 2025
yankay
yankay reviewed Jan 23, 2025
View reviewed changes
roles/kubernetes/preinstall/defaults/main.yml Outdated
# Minimal memory requirement in MB for safety checks
minimal_node_memory_mb: 1024
minimal_master_memory_mb: 1500

Copy link
Member

Choose a reason for hiding this comment

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

Hi @VannTen

Why is the minimal check of memory removed.
Is it because kubeadm already has a check?
Ref to https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/constants/constants.go#L396

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's exactly the point, as well as the kernel version check. Since kubeadm is already doing validation, (which we're not ignoring anymore 😆 ), let's not maintain our own.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't better to fail early than mid-way through the kubeadm install ?

I would prefer to fail at this stage rather than to 10 min later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'd still like to defer to kubeadm, but I think we need a better story on download first, to have kubeadm available sufficiently early than we can use it for validation (assuming that we can run the preflight independantly, but I think that's possible).

I'll remove that commit for now.

ant31
ant31 reviewed Jan 23, 2025
View reviewed changes
roles/kubernetes/preinstall/tasks/0040-verify-settings.yml Outdated
@@ -160,13 +132,6 @@
- dashboard_enabled
- not ignore_assert_errors

- name: Stop if kernel version is too low
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same that the mem requirements, this is checked by kubeadm. I'll drop this as well per above.

ant31
ant31 reviewed Jan 23, 2025
View reviewed changes
roles/kubernetes/preinstall/tasks/0040-verify-settings.yml Outdated
@@ -193,13 +158,6 @@
- cloud-provider
- facts

- name: "Check that kube_service_addresses is a network range"
Copy link
Contributor

Choose a reason for hiding this comment

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

why to remove this check ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a leftover from a removed commit, messed up my rebase. I'll clean this up.

@VannTen VannTen force-pushed the cleanup/verify_settings branch from 7520fb4 to cd60262 Compare January 23, 2025 13:16
@VannTen
verify_settings: fix bad task name + remove redundant conditions
277ab73
@VannTen
verify_settings: Consolidate assert loop in one task
f53552e
@VannTen
verify settings: fix etcd assertion when implicity etcd group
98cdb53 
When the etcd group is not specified we assume it's kube_control_plane.
In that case, etcd still can't be even, so instead of only checking the
etcd group we need to default to kube_control_plane
@VannTen
Don't check address space when using 'none' network plugin
85d9e3e 
Since 'none' can be, for instance, a manual calico deployment, don't
check whether there is enough ip for pods on a node, because the plugin
can use another mechanism than the podCIDR to allocate IPs.
@VannTen
etcd_kubeadm simplify assert
ed789c9
@VannTen
verify_settings: consolidate choices validation
12a2c5e
@VannTen
Drop compatibility for etcd_kubeadm_enabled
7753612 
This has been deprecated for a long time, time to pull the plug.
We leave an assert for one release to have a straightforward failure if
some users were still using the variable.
@VannTen
Remove containerd_config assert
fba31be 
This assert is present since 2021, we can assume now users have removed
it from their inventories.
@VannTen
Remove deprecation checks admission plugins list
67a7376 
This assertion is present since 2022, users inventories' should be clean
from it now.
@VannTen VannTen force-pushed the cleanup/verify_settings branch from cd60262 to 67a7376 Compare January 23, 2025 13:32
@ant31
Copy link
Contributor

ant31 commented Jan 23, 2025

Thanks @VannTen
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2025
@k8s-ci-robot k8s-ci-robot merged commit cbd0b7b into kubernetes-sigs:master Jan 23, 2025
43 checks passed
@VannTen VannTen mentioned this pull request Jan 28, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yankay yankay yankay left review comments

@ant31 ant31 ant31 left review comments

@MrFreezeex MrFreezeex Awaiting requested review from MrFreezeex

Assignees

@ant31 ant31

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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VannTen @k8s-ci-robot @ant31 @yankay