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

vSphere kube-up: Wait for cbr0 configuration to complete before setting up routes. #35232

Merged
merged 1 commit into from
Oct 21, 2016
Merged

vSphere kube-up: Wait for cbr0 configuration to complete before setting up routes. #35232

merged 1 commit into from
Oct 21, 2016

Conversation

kerneltime
Copy link

@kerneltime kerneltime commented Oct 20, 2016 •
edited by jessfraz
Loading 

vSphere kube-up: Wait for cbr0 configuration to complete before setting up routes.

What this PR does / why we need it:
Fixes routing setup when deploying via kube-up.sh on vSphere.
Remove optimizations for salt status check till flakyness of install with optimization
is addressed.

Which issue this PR fixes
fixes #34248, #31310

Special notes for your reviewer:
Ref PR with a similar fix #31672

This change is Reviewable

@kerneltime kerneltime mentioned this pull request Oct 20, 2016
Closed
@kerneltime
Copy link
Author

kerneltime commented Oct 20, 2016 •
edited
Loading

Will be requesting for this to be cherry picked to release-1.4 as well

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Oct 20, 2016
@kerneltime kerneltime force-pushed the fix-dashboard.kerneltime branch from cbd52c8 to 4ff1b89 Compare October 20, 2016 21:05
@eparis
Copy link
Contributor

eparis commented Oct 20, 2016

@kubernetes/sig-network

@kerneltime kerneltime changed the title Wait for cbr0 configuration to complete before setting up routes. vSphere kube-up: Wait for cbr0 configuration to complete before setting up routes. Oct 20, 2016
@kerneltime
Copy link
Author

@eparis to clarify this does not change any of the networking code within kubernetes and only changes the kube-up.sh script for deploying on top of vSphere.

@imkin
Copy link

imkin commented Oct 20, 2016

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

cluster/vsphere/util.sh, line 331 at r1 (raw file):

    network=""
    top2_octets_final=$(echo $NODE_IP_RANGES | awk -F "." '{ print $1 "." $2 }') # Assume that a 24 bit mask per node

We could identify the mask from network we setup in config-default.sh

Comments from Reviewable

@kerneltime kerneltime mentioned this pull request Oct 20, 2016
Closed
@kerneltime
Copy link
Author

kerneltime commented Oct 20, 2016 •
edited
Loading

@imkin This is quick and mainly a reliable way to insure that the cbr0 interface has been configured correctly before setting up the routes based on the information read from cbr0. The limitation is that the the mask possible is limited to 255 IPs per node. Without this fix overall networking is broken. As discussed we can look at improvements, for now we need to make sure that this fix gets into 1.4.4 release.

@imkin
Copy link

imkin commented Oct 20, 2016

If the code will fail predictably lets document it clearly. both for the developer and the user. Please add comments to the config-default and config-common that sing \16 or higher prefixes are stable.

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

@kerneltime
Copy link
Author

Will do. Adding documentation for the limitation.

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

cluster/vsphere/util.sh, line 331 at r1 (raw file):

Previously, imkin (Dhawal Yogesh Bhanushali) wrote…

We could identify the mask from network we setup in config-default.sh

This is quick and mainly a reliable way to insure that the cbr0 interface has been configured. For now we will document the limitation and move to an alternate scheme for detecting initialization of network information.

Comments from Reviewable

Wait for cbr0 configuration to complete before setting up routes.
a71dc97 
- fixes kubernetes#34248, kubernetes#31310
- Remove optimizations for salt status check till flakyness of install is addressed
- fix indentation
@kerneltime kerneltime force-pushed the fix-dashboard.kerneltime branch from 4ff1b89 to a71dc97 Compare October 21, 2016 00:43
@imkin
Copy link

imkin commented Oct 21, 2016

:lgtm:

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

@imkin
Copy link

imkin commented Oct 21, 2016

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@kerneltime
Copy link
Author

cc @abrarshivani @AlainRoy @luomiao

@kerneltime
Copy link
Author

kerneltime commented Oct 21, 2016 •
edited
Loading

Can this PR be automatically cherry picked to release-1.4 else I can file a new PR.

@eparis eparis added this to the v1.4 milestone Oct 21, 2016
@eparis
Copy link
Contributor

eparis commented Oct 21, 2016

it will not be automaticaly picked in any way. You will have to follow the cherry-pick process. See Individual Cherry Picks https://github.com/kubernetes/kubernetes/blob/master/docs/devel/cherry-picks.md#how-do-cherrypick-candidates-make-it-to-the-release-branch

Also be sure the cc @ jessfraz on your pick

After this merges. I'll also point out that it looks like you are just doing what the flannel host gateway does. Why not use that? A lot of other providers use flannel...

@eparis eparis added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2016
@eparis
Copy link
Contributor

eparis commented Oct 21, 2016

@k8s-bot test this

@eparis
Copy link
Contributor

eparis commented Oct 21, 2016 •
edited
Loading

adding retest-not-required as the tests are not going to run vShpere code anyway.

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Oct 21, 2016
@kerneltime
Copy link
Author

@eparis can you adjust the release note label?

@eparis eparis added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Oct 21, 2016
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 0dbd954 into kubernetes:master Oct 21, 2016
@kerneltime
Copy link
Author

@eparis yup agreed. Right now the goal is to make sure that we get release branch fixed and then look into improving the kube-up deployment. There are a few other things we want to do with the kube-up script to reduce time taken to install in addition to looking into flannel. The bigger question is will kube-up be there in 1.5? We are also working on supporting vSphere within kube-anywhere.

@kerneltime kerneltime deleted the fix-dashboard.kerneltime branch October 21, 2016 16:36
@kerneltime kerneltime mentioned this pull request Oct 21, 2016
Merged
@jessfraz jessfraz added release-note Denotes a PR that will be considered when it comes time to generate release notes. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 21, 2016
k8s-github-robot pushed a commit that referenced this pull request Oct 25, 2016
Merge pull request #35308 from vmware/automated-cherry-pick-of-#35232…
68ba01d 
…-upstream-release-1.4

Automatic merge from submit-queue

Wait for cbr0 configuration to complete before setting up routes. - f…

Automated cherry pick of #35232 , this is needed for kube-up to work on vSphere, need this in the next 1.4 release.

fixes #34248, #31310 
- Remove optimizations for salt status check till flakyness of install is addressed - fix indentation
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#35308 from vmware/automated-cherry-pick…
78f9cbe 
…-of-#35232-upstream-release-1.4

Automatic merge from submit-queue

Wait for cbr0 configuration to complete before setting up routes. - f…

Automated cherry pick of kubernetes#35232 , this is needed for kube-up to work on vSphere, need this in the next 1.4 release.

fixes kubernetes#34248, kubernetes#31310 
- Remove optimizations for salt status check till flakyness of install is addressed - fix indentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eparis eparis

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.

vSphere Deployment container communication broken
6 participants
@kerneltime @eparis @imkin @k8s-github-robot @jessfraz @googlebot