Skip to content

kubelet: clean up cloud provider code because cloud provider only supports empty or external and cloud is nil #130161

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
Merged
merged 1 commit into from
May 17, 2025
Merged

kubelet: clean up cloud provider code because cloud provider only supports empty or external and cloud is nil #130161

merged 1 commit into from
May 17, 2025

Conversation

carlory
Copy link
Member

@carlory carlory commented Feb 14, 2025 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

A follow-up of #124519

the --cloud-provider flag of kubelet only supports empty or external, and cloud is nil

Which issue(s) this PR fixes:

Related to kubernetes/enhancements#2395

Special notes for your reviewer:

Does this PR introduce a user-facing change?

ACTION REQUIRED:
kubelet:  removed the deprecated flag `--cloud-config` from the command line.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/issues/2395

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 14, 2025
@carlory
Copy link
Member Author

carlory commented Feb 14, 2025

/cc @dims @elmiko

@k8s-ci-robot k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. labels Feb 14, 2025
@k8s-ci-robot k8s-ci-robot requested review from dims and elmiko February 14, 2025 10:24
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Feb 14, 2025
@carlory carlory force-pushed the kep-2395-kubelet-cleanup branch from e6ba9e7 to a1fe8fe Compare February 14, 2025 10:30
@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 release-note-none Denotes a PR that doesn't merit a release note. labels Feb 14, 2025
@carlory carlory force-pushed the kep-2395-kubelet-cleanup branch from a1fe8fe to 66000d8 Compare February 17, 2025 03:37
@bart0sh
Copy link
Contributor

bart0sh commented Feb 17, 2025

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 17, 2025
elmiko
elmiko reviewed Feb 26, 2025
View reviewed changes
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.

it looks like this is removing the --cloud-provider flag, how will we know to set the kubelet into external provider mode?

@carlory
Copy link
Member Author

carlory commented Feb 26, 2025

@elmiko it only remove the --cloud-config flag, not remove others. I only change the description of those flags.

@elmiko
Copy link
Contributor

elmiko commented Feb 26, 2025

ah! thank you @carlory , i misread the commit diff.

@carlory
Copy link
Member Author

carlory commented Feb 27, 2025

/test pull-kubernetes-cmd

elmiko
elmiko reviewed Feb 27, 2025
View reviewed changes
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.

i think this is making more sense to me now, i would like to have someone with more experience on the network parts review as well.

also, i have one minor suggestion for the updated flag help text.

@carlory carlory force-pushed the kep-2395-kubelet-cleanup branch from 66000d8 to e62734c Compare February 28, 2025 06:31
elmiko
elmiko reviewed Feb 28, 2025
View reviewed changes
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.

/lgtm

would like to get another review, especially for the networking related changes.

@k8s-ci-robot k8s-ci-robot requested a review from danwinship May 12, 2025 01:52
@danwinship
Copy link
Contributor

networking parts lgtm

@carlory
Copy link
Member Author

carlory commented May 13, 2025

/cc @elmiko
for lgtm
/assign @mborsz @sjenning
for approval

elmiko
elmiko reviewed May 13, 2025
View reviewed changes
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.

/lgtm

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

LGTM label has been added.

Git tree hash: 98aae9f19dd4fc9ff5959cf69ff3690b5211cdf4

@carlory
Copy link
Member Author

carlory commented May 14, 2025

/assign @SergeyKanzhelev @derekwaynecarr @wojtek-t
for approval (sig-node)
/assign @dims
for approval (sig-cloud-provider)

@dims
Copy link
Member

dims commented May 15, 2025

@carlory do ping me after the sig-node folks take a shot at it please

@carlory
Copy link
Member Author

carlory commented May 16, 2025

/cc @tallclair @klueska

Can you have a look?

@k8s-ci-robot k8s-ci-robot requested review from klueska and tallclair May 16, 2025 01:52
SergeyKanzhelev
SergeyKanzhelev approved these changes May 16, 2025
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev 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

@carlory
Copy link
Member Author

carlory commented May 17, 2025

/cc @dims

@dims
Copy link
Member

dims commented May 17, 2025

/lgtm
/approve

thanks @carlory

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlory, dims, SergeyKanzhelev

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 May 17, 2025
@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

1 similar comment
@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@k8s-ci-robot k8s-ci-robot merged commit 2125fd6 into kubernetes:master May 17, 2025
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.34 milestone May 17, 2025
@github-project-automation github-project-automation bot moved this from Needs Reviewer to Done in SIG Node: code and documentation PRs May 17, 2025
@carlory carlory deleted the kep-2395-kubelet-cleanup branch May 18, 2025 07:10
pohly
pohly reviewed May 19, 2025
View reviewed changes
cmd/kubelet/app/options/options.go
@@ -320,8 +320,6 @@ func (f *KubeletFlags) AddFlags(mainfs *pflag.FlagSet) {
fs.MarkDeprecated("register-schedulable", "will be removed in a future version")
fs.StringVar(&f.ExperimentalMounterPath, "experimental-mounter-path", f.ExperimentalMounterPath, "[Experimental] Path of mounter binary. Leave empty to use the default mount.")
fs.MarkDeprecated("experimental-mounter-path", "will be removed in 1.25 or later. in favor of using CSI.")
fs.StringVar(&f.CloudConfigFile, "cloud-config", f.CloudConfigFile, "The path to the cloud provider configuration file. Empty string for no configuration file.")
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to warning in the release notes, please also check for usage in-tree. hack/local-up-cluster.sh still uses it and now fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

=> #131841

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for that.

@pohly pohly mentioned this pull request May 19, 2025
Merged
This was referenced May 20, 2025
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly left review comments

@elmiko elmiko elmiko left review comments

@aojea aojea aojea left review comments

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@hashim21223445 hashim21223445 hashim21223445 approved these changes

@dims dims Awaiting requested review from dims

@ffromani ffromani Awaiting requested review from ffromani

@mborsz mborsz Awaiting requested review from mborsz

@sjenning sjenning Awaiting requested review from sjenning

@danwinship danwinship Awaiting requested review from danwinship

@klueska klueska Awaiting requested review from klueska

@tallclair tallclair Awaiting requested review from tallclair

Assignees

@dims dims

@elmiko elmiko

@sjenning sjenning

@mborsz mborsz

@derekwaynecarr derekwaynecarr

@SergeyKanzhelev SergeyKanzhelev

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider area/kubelet 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node: code and documentation PRs
Archived in project
Milestone
v1.34
Development

Successfully merging this pull request may close these issues.