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

WIP: Remove the legacy networking mode #34906

Merged
merged 4 commits into from
Oct 19, 2016
Merged

WIP: Remove the legacy networking mode #34906

merged 4 commits into from
Oct 19, 2016

Conversation

luxas
Copy link
Member

@luxas luxas commented Oct 16, 2016 •
edited by thockin
Loading

What this PR does / why we need it:
Removes the deprecated configure-cbr0 flag and networking mode to avoid having untested and maybe unstable code in kubelet, see: #33789

Which issue this PR fixes (optional, in fixes #<issue number>(, #<issue_number>, ...) format, will close that issue when PR gets merged):
fixes #30589
fixes #31937

Special notes for your reviewer: There are a lot of deployments who rely on this networking mode. Not sure how we deal with that: force switch to kubenet or just delete the old deployment?

But please review the code changes first (the first commit)

Release note:

Removed the deprecated kubelet --configure-cbr0 flag, and with that the "classic" networking mode as well

PTAL @kubernetes/sig-network @kubernetes/sig-node @mikedanese

This change is Reviewable

@luxas luxas added this to the v1.5 milestone Oct 16, 2016
@luxas luxas added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Oct 16, 2016
@luxas luxas changed the title Remove the legacy networking mode WIP: Remove the legacy networking mode Oct 16, 2016
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2016
@luxas
Copy link
Member Author

luxas commented Oct 16, 2016

Will run hack/update-all.sh later...

This was referenced Oct 16, 2016
Closed
Closed
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit e3207bf. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@thockin
Copy link
Member

thockin commented Oct 17, 2016

I love it. This may be the first time we actually remove anything from componentconfig - @mikedanese can you think of any problems this will cause?

I am editing the subject for better relnote

@luxas a bunch of verify scripts failed - run update-all, please

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 17, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 72c36c8. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit 72c36c8. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 72c36c8. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 72c36c8. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 72c36c8. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 72c36c8. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

danwinship
danwinship reviewed Oct 17, 2016
View reviewed changes
cmd/kubelet/app/options/options.go
@@ -200,10 +198,10 @@ func (s *KubeletServer) AddFlags(fs *pflag.FlagSet) {
fs.Float64Var(&s.ChaosChance, "chaos-chance", s.ChaosChance, "If > 0.0, introduce random client errors and latency. Intended for testing. [default=0.0]")
fs.BoolVar(&s.Containerized, "containerized", s.Containerized, "Experimental support for running kubelet in a container. Intended for testing. [default=false]")
fs.Int64Var(&s.MaxOpenFiles, "max-open-files", s.MaxOpenFiles, "Number of files that can be opened by Kubelet process. [default=1000000]")
fs.BoolVar(&s.ReconcileCIDR, "reconcile-cidr", s.ReconcileCIDR, "Reconcile node CIDR with the CIDR specified by the API server. No-op if register-node or configure-cbr0 is false. [default=true]")
fs.BoolVar(&s.ReconcileCIDR, "reconcile-cidr", s.ReconcileCIDR, "Reconcile node CIDR with the CIDR specified by the API server. Won't have any effect if register-node is false. [default=true]")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this was previously a no-op if configure-cbr0 was false, and configure-cbr0 is now always false, this is now always a no-op. (Actually... it looks like it may have been a no-op already; reconcileCBR0 in kubelet_network.go always got called if configure-cbr0 was true, regardless of the value of reconcile-cidr.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually my initial draft of this was removing reconcile-cidr as well, but I think we should take it separately.

If we agree to remove that as well, I'm fine, but please in another PR to make it easier for reviewing.

Another concern is that we didn't mark reconcile-cidr as deprecated...

Copy link
Contributor

Choose a reason for hiding this comment

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

well, if you don't remove it, the doc string that's there now is wrong; it implies that reconcile-cidr still does something if register-node is true, but it doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

ping @luxas on this

@luxas luxas mentioned this pull request Oct 17, 2016
Closed
@luxas
Copy link
Member Author

luxas commented Oct 17, 2016

Will run hack/update-all.sh later...

@thockin :)

Ok, so here's what we should sort out (in order):

  • Is the Go code change here?
    • I'll prefer to deal with reconcile-cidr separately
  • There are a lot of deployments who rely on this networking mode.
    • Not sure how we should deal with that: force switch to kubenet or just delete the old deployment?
  • Is the provisioning cluster/ changes ok?

If this is ok, I'm gonna rebase (will probably be needed at that time), fix up the commits and remove WIP.

However, I'm traveling to London the rest of this week and will be totally offline, so some time next week I can come back and update this. But please comment on it anyway :)

@yujuhong
Copy link
Contributor

/cc @freehan

@thockin
Copy link
Member

thockin commented Oct 18, 2016

@freehan is out on leave, assign to me to @matchstick

On Mon, Oct 17, 2016 at 10:17 AM, Yu-Ju Hong notifications@github.com
wrote:

/cc @freehan https://github.com/freehan

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#34906 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVArhbspFHmYUlrBqy8eagC46JK5_ks5q062OgaJpZM4KYC8K
.

@luxas luxas unassigned vishh Oct 18, 2016
@chrislovecnm chrislovecnm mentioned this pull request Oct 19, 2016
Closed
@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2016
@thockin
Copy link
Member

thockin commented Oct 19, 2016

I am LGTM on this. Consider also removing the second flag

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 61e0113 into kubernetes:master Oct 19, 2016
@luxas luxas mentioned this pull request Oct 25, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Oct 27, 2016
Merge pull request #35523 from luxas/remove_reconcile
10061ac 
Automatic merge from submit-queue

Deprecate the --reconcile-cidr flag

<!--  Thanks for sending a pull request!  Here are some tips for you:
1. If this is your first time, read our contributor guidelines https://github.com/kubernetes/kubernetes/blob/master/CONTRIBUTING.md and developer guide https://github.com/kubernetes/kubernetes/blob/master/docs/devel/development.md
2. If you want *faster* PR reviews, read how: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/faster_reviews.md
3. Follow the instructions for writing a release note: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/pull-requests.md#release-notes
-->

**What this PR does / why we need it**: Follows up #34906

**Special notes for your reviewer**: I'm not sure why coreos had set `--reconcile-cidr` to `false` and what the implications are now.

**Release note**:
<!--  Steps to write your release note:
1. Use the release-note-* labels to set the release note state (if you have access) 
2. Enter your extended release note in the below block; leaving it blank means using the PR title as the release note. If no release note is required, just write `NONE`. 
-->
```release-note
Deprecate the --reconcile-cidr kubelet flag because it has no function anymore
```

PTAL @thockin @freehan @justinsb @yujuhong 
@kubernetes/sig-node @kubernetes/sig-network 

**I will add `hack/update-all.sh` contents soon to fix builds**
@ChenLingPeng ChenLingPeng mentioned this pull request Feb 15, 2017
Closed
@dcbw dcbw mentioned this pull request Apr 10, 2017
Closed
@islinwb islinwb mentioned this pull request Feb 3, 2018
Closed
@islinwb islinwb mentioned this pull request Feb 26, 2018
Merged
mtaufen added a commit to mtaufen/kubernetes that referenced this pull request Jun 22, 2018
@mtaufen
remove incorrect comment referencing removed functionality
6e9e306 
The cbr0 configuration behavior this comment references was removed in kubernetes#34906
mtaufen added a commit to mtaufen/kubernetes that referenced this pull request Jul 16, 2018
@mtaufen
remove incorrect comment referencing removed functionality
d245e72 
The cbr0 configuration behavior this comment references was removed in kubernetes#34906
tanshanshan pushed a commit to tanshanshan/kubernetes that referenced this pull request Aug 10, 2018
@mtaufen
remove incorrect comment referencing removed functionality
652c610 
The cbr0 configuration behavior this comment references was removed in kubernetes#34906
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thockin thockin thockin left review comments

@danwinship danwinship danwinship left review comments

Assignees

@thockin thockin

@matchstick matchstick

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
9 participants
@luxas @k8s-ci-robot @thockin @yujuhong @k8s-github-robot @danwinship @googlebot @vishh @matchstick