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

add CIDR allocator for NodeController #19242

Merged
merged 4 commits into from
May 20, 2016
Merged

add CIDR allocator for NodeController #19242

merged 4 commits into from
May 20, 2016

Conversation

mqliang
Copy link
Contributor

@mqliang mqliang commented Jan 4, 2016 •
edited by mikedanese
Loading

This PR:

  • use pkg/controller/framework to watch nodes and reduce lists when allocate CIDR for node
  • decouple the cidr allocation logic from monitoring status logic

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 4, 2016
@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

derekwaynecarr
derekwaynecarr reviewed Jan 4, 2016
View reviewed changes
pkg/controller/node/cidr_allocator.go
@@ -0,0 +1,175 @@
/*
Copyright 2014 The Kubernetes Authors All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

update year

@mqliang mqliang force-pushed the node-controller branch 2 times, most recently from 38effe8 to 9fdd970 Compare January 5, 2016 02:44
@mqliang
Copy link
Contributor Author

mqliang commented Jan 5, 2016

@derekwaynecarr Updated. PTAL

@derekwaynecarr
Copy link
Member

lgtm, thanks

@derekwaynecarr derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2016
@mqliang
Copy link
Contributor Author

mqliang commented Jan 12, 2016

@derekwaynecarr rebased.

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 12, 2016
@mqliang mqliang mentioned this pull request Jan 13, 2016
Merged
therc
therc reviewed Jan 13, 2016
View reviewed changes
pkg/controller/node/cidr_allocator.go

var errCIDRRangeNoCIDRsRemaining = errors.New("CIDR allocation failed; there are no remaining CIDRs left to allocate in the accepted range")

// CIDRAllocator ss an interface implemented by things that know how to allocate/recycle CIDR for nodes.
Copy link
Member

Choose a reason for hiding this comment

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

typo: ss -> is

@mqliang
Copy link
Contributor Author

mqliang commented Jan 18, 2016

@gmarek Could you please kindly review this and give me some feedback?

@gmarek
Copy link
Contributor

gmarek commented Jan 18, 2016

Sure, but I'm currently struggling with some P0 issues - I'm not sure I'll be able to do this this week (and I'm OOO in the next one).

@gmarek
Copy link
Contributor

gmarek commented May 20, 2016

I'll do it in half an hour

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

gmarek commented May 20, 2016

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 20, 2016

GCE e2e build/test passed for commit 552a247.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit f935507 into kubernetes:master May 20, 2016
@mikedanese
Copy link
Member

This may have broken GKE CI.

@roberthbailey
Copy link
Contributor

@andyzheng0831 confirmed that it broke gke ci

@wojtek-t
Copy link
Member

@roberthbailey @mikedanese - I think it was kind of expected that this PR may break things, because we finally implemented feature that we thought we have. So those may be configuration issues with our suites.

@andyzheng0831
Copy link

It also broke k8s ci running on GCI. I am checking what is wrong. So far it looks like controller-manager process is in crash loop

@roberthbailey
Copy link
Contributor

That's consistent with what Jenkins shows: All cluster resources were brought up, but the cluster API is reporting that component "controller-manager" is unhealthy.

@andyzheng0831 andyzheng0831 mentioned this pull request May 20, 2016
Merged
@andyzheng0831
Copy link

The breakage is fixed by #25984

@wojtek-t
Copy link
Member

@mikedanese - it probably also broke kubemark (at least kubemark-5 started failing around that time, all others didn't finish since the yet, so probably they will also start failing)
I think the problem might b very similar, but I cannot take a look now, can you please take a look?

@lavalamp
Copy link
Member

This PR is no longer automatically revertable...

@ghost ghost mentioned this pull request May 20, 2016
Closed
This was referenced May 20, 2016
Merged
Closed
@mqliang mqliang deleted the node-controller branch May 21, 2016 00:04
@euank euank mentioned this pull request May 21, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request May 21, 2016
Merge pull request #26006 from euank/service-cluster-ip-range
5c08302 
Automatic merge from submit-queue

cluster/gce/coreos: Set service-cluster-ip-range

Broken by #19242 

See also #26002 

This is necessary to kube-up for me, but depending on how #26002 plays out, this PR might not be necessary. Happy to close this or merge or whatever depending on what's best.

cc @yifan-gu @sjpotter @mikedanese
@zmerlynn zmerlynn mentioned this pull request May 24, 2016
Merged
@erictune erictune mentioned this pull request Jul 2, 2016
Closed
@erictune
Copy link
Member

erictune commented Jul 2, 2016

@mqliang Does this PR require action by the user when upgrading from 1.2.x to 1.3.0? (Think about non-developer users.) If so, please edit your first comment to have a release-note block, like in #28132. If it is just an optional feature, please change the label to just release-note. If it is not a complete feature by itself, then apply "release-note-none" label instead.

@mqliang
Copy link
Contributor Author

mqliang commented Jul 3, 2016

@erictune This is a transparent change for users, it does not require any actions by the user when upgrading. And unfortunately, it seems that I have no permission to change the label.

@gmarek gmarek added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Jul 4, 2016
@bprashanth bprashanth mentioned this pull request Jul 16, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@gmarek gmarek

Labels
area/nodecontroller 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. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. 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.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.