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

Remove deprecated flag --resource-container from kube-proxy #78294

Merged
merged 1 commit into from
Jun 22, 2019
Merged

Remove deprecated flag --resource-container from kube-proxy #78294

merged 1 commit into from
Jun 22, 2019

Conversation

vllry
Copy link
Contributor

@vllry vllry commented May 24, 2019 •
edited by thockin
Loading

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Removes a long-deprecated field (--resource-container) from kube-proxy. This also removes the resourceContainer code, as kube-proxy was the last component to use it.

Which issue(s) this PR fixes:
Fixes #78287

Special notes for your reviewer:

  • If the user keeps the flag (EG as part of their bootstrap process), kube-proxy will fail to start, but the result may not be obvious. Is it okay to yank it all at once, or should we give a stronger warning when the flag is used and wait another release?
  • Should we worry about the API change compatibility? As far as I can tell, removing a deprecated admin flag/v1alpha API field is fine.

Does this PR introduce a user-facing change?:

ACTION REQUIRED: Removed deprecated flag `--resource-container` from kube-proxy.

The deprecated `--resource-container` flag has been removed from kube-proxy, and specifying it will now cause an error.  The behavior is now as if you specified `--resource-container=""`.  If you previously specified a non-empty `--resource-container`, you can no longer do so as of kubernetes 1.16.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 24, 2019
@vllry
Copy link
Contributor Author

vllry commented May 24, 2019

/priority backlog

@k8s-ci-robot k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels May 24, 2019
@vllry vllry force-pushed the kp-remove-resource-container branch 2 times, most recently from 8f7f092 to 318b3fa Compare May 24, 2019 14:15
@danwinship
Copy link
Contributor

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from danwinship May 24, 2019 14:43
@vllry
Copy link
Contributor Author

vllry commented May 24, 2019

/uncc @bowei

Sorry for the spam :C (Of course I get this command wrong several times...)

@k8s-ci-robot k8s-ci-robot removed the request for review from bowei May 24, 2019 15:24
@vllry vllry force-pushed the kp-remove-resource-container branch from 65124af to e00ed66 Compare May 24, 2019 17:00
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 24, 2019
@vllry vllry force-pushed the kp-remove-resource-container branch from f266200 to 6667837 Compare May 26, 2019 07:12
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 26, 2019
@vllry vllry mentioned this pull request May 27, 2019
Closed
johscheuer
johscheuer reviewed May 28, 2019
View reviewed changes
pkg/proxy/apis/config/types.go
@@ -138,9 +138,6 @@ type KubeProxyConfiguration struct {
// portRange is the range of host ports (beginPort-endPort, inclusive) that may be consumed
// in order to proxy service traffic. If unspecified (0-0) then ports will be randomly chosen.
PortRange string
// resourceContainer is the absolute name of the resource-only container to create and run
// the Kube-proxy in (Default: /kube-proxy).
ResourceContainer string
Copy link
Contributor

Choose a reason for hiding this comment

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

You must also remove the default value for ResourceContainer (https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/apis/config/v1alpha1/defaults.go#L61-L63)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I spotted that but I'm still missing something. All unit tests are fine, but the system doesn't start properly in e2e.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2019
@k8s-ci-robot k8s-ci-robot added area/kubeadm and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 16, 2019
@vllry vllry force-pushed the kp-remove-resource-container branch from 3829fc6 to 3fd1807 Compare June 16, 2019 06:20
@vllry
Copy link
Contributor Author

vllry commented Jun 16, 2019

/test pull-kubernetes-bazel-build

1 similar comment
@vllry
Copy link
Contributor Author

vllry commented Jun 16, 2019

/test pull-kubernetes-bazel-build

@vllry vllry changed the title WIP: Remove deprecated flag --resource-container from kube-proxy Remove deprecated flag --resource-container from kube-proxy Jun 16, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 16, 2019
@vllry vllry force-pushed the kp-remove-resource-container branch from 3fd1807 to dc0f143 Compare June 16, 2019 15:37
@vllry
Copy link
Contributor Author

vllry commented Jun 16, 2019

/assign @bowei
/assign @thockin

@thockin
Copy link
Member

thockin commented Jun 19, 2019

Thanks!

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin, vllry

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. 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 19, 2019
@nikhita
Copy link
Member

nikhita commented Jun 22, 2019

FYI added ```release-note in the release note block to fix the release-note label.

@k8s-ci-robot k8s-ci-robot merged commit eee3e97 into kubernetes:master Jun 22, 2019
@justinsb
Copy link
Member

justinsb commented Jul 8, 2019

Can we change the release note flag to be more instructive?

I suggest:

ACTION REQUIRED: Removed deprecated flag `--resource-container` from kube-proxy.

The --resource-container flag has been removed from kube-proxy.  The behaviour is now as if you specified `--resource-container=""`.  However, you must stop specifying `--resource-container=""`.  If you previously specified a non-empty `--resource-container`, you can no longer do so as of kubernetes 1.16.

justinsb added a commit to justinsb/kops that referenced this pull request Jul 8, 2019
@justinsb
Remove kube-proxy resource-container flag
5f5f37a 
Removed in kubernetes/kubernetes#78294

(A k/k breaking change: `--resource-container=""` is now the default!)
@justinsb justinsb mentioned this pull request Jul 8, 2019
Merged
@thockin
Copy link
Member

thockin commented Jul 8, 2019

Updated

@zouyee
Copy link
Member

zouyee commented Jul 21, 2019

#66614

@zouyee zouyee mentioned this pull request Jul 22, 2019
Merged
@SataQiu SataQiu mentioned this pull request Oct 29, 2019
Closed
@tfussell tfussell mentioned this pull request May 27, 2020
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johscheuer johscheuer johscheuer left review comments

Assignees

@bowei bowei

@thockin thockin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/network Categorizes an issue or PR as relevant to SIG Network. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove deprecated kube-proxy flag —resource-container
10 participants
@vllry @danwinship @fejta-bot @thockin @k8s-ci-robot @nikhita @justinsb @zouyee @johscheuer @bowei