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

Change default clusterCIDRs from /16 to /14 in GCE configs allowing 1000 Node clusters by default. #25350

Merged
merged 1 commit into from
May 11, 2016
Merged

Change default clusterCIDRs from /16 to /14 in GCE configs allowing 1000 Node clusters by default. #25350

merged 1 commit into from
May 11, 2016

Conversation

gmarek
Copy link
Contributor

@gmarek gmarek commented May 9, 2016

@gmarek gmarek added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label May 9, 2016
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels May 9, 2016
roberthbailey
roberthbailey reviewed May 9, 2016
View reviewed changes
cluster/gce/config-test.sh
@@ -49,7 +49,7 @@ CLUSTER_NAME="${CLUSTER_NAME:-${INSTANCE_PREFIX}}"
MASTER_NAME="${INSTANCE_PREFIX}-master"
MASTER_TAG="${INSTANCE_PREFIX}-master"
NODE_TAG="${INSTANCE_PREFIX}-minion"
CLUSTER_IP_RANGE="${CLUSTER_IP_RANGE:-10.245.0.0/16}"
CLUSTER_IP_RANGE="${CLUSTER_IP_RANGE:-10.245.0.0/14}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same as 10.244.0.0/14. If you want test to remain a distinct range from regular clusters we should change this to 10.248.0.0/14.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely missed it. Do we actually care?

Copy link
Member

Choose a reason for hiding this comment

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

In theory someone can run tests on the same GCE network as their main cluster. (They have to override the NETWORK, obviously.)

At the very least, the code should use the canonical address of 10.244.0.0./14, but I tend to agree with @roberthbailey

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer that we change it to 10.248.0.0/16 so that it maintains the current intent of having a separate range for test/non-test by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to 180

@roberthbailey
Copy link
Contributor

@wojtek-t -- release note?

@k8s-bot
Copy link

k8s-bot commented May 10, 2016

GCE e2e build/test passed for commit 5dc3d50.

@roberthbailey
Copy link
Contributor

LGTM, but please add a release note.

@therc
Copy link
Member

therc commented May 10, 2016

It would be nice to know the why. Are legacy clusters with a /16 at risk? Etc.

@gmarek
Copy link
Contributor Author

gmarek commented May 10, 2016

@therc - depends on the size. We'll start respecting CIDRs soon, which means that /16 clusters would be able to handle only 255 Nodes.

@luxas luxas added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 10, 2016
@roberthbailey
Copy link
Contributor

@therc see #19242

@therc
Copy link
Member

therc commented May 10, 2016

@roberthbailey I commented on #19242 back in January. :-) What I meant is that it's not obvious from the PR (or the docs as they exist now) why the change is needed, let alone labelled P0, etc. Even if I keep getting mails for the other issue, I hadn't connected the dots until gmarek and you commented. Thanks!

@gmarek
Copy link
Contributor Author

gmarek commented May 11, 2016

@therc - sorry for that. We're going to communicate this change publicly before it goes live.

@gmarek gmarek changed the title Change default clusterCIDRs to /14 Change default clusterCIDRs to /14 in GCE configs. May 11, 2016
@gmarek gmarek changed the title Change default clusterCIDRs to /14 in GCE configs. Change default clusterCIDRs from /16 to /14 in GCE configs allowing 1000 Node clusters by default. May 11, 2016
@gmarek
Copy link
Contributor Author

gmarek commented May 11, 2016

@roberthbailey - changed the PR title to make a better release note. PTAL.

@zmerlynn zmerlynn added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2016
@roberthbailey
Copy link
Contributor

lgtm

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented May 11, 2016

GCE e2e build/test passed for commit 5dc3d50.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 91daebd into kubernetes:master May 11, 2016
@gmarek gmarek deleted the configs-cidr branch August 30, 2016 09:50
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Aug 19, 2020
@k8s-publishing-bot
Merge pull request kubernetes#25350 from damemi/3.11-upstream-label-s…
7ee9643 
…elector

[release-3.11] Bug 1760807: UPSTREAM: <carry>: Enforce label selector validation in scheduler podAffinity

Origin-commit: dc045cb38e8a8a12314fe7e4a1f00edf214a7cf8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@zmerlynn zmerlynn

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@gmarek @roberthbailey @k8s-bot @therc @k8s-github-robot @zmerlynn @googlebot @luxas