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

nodecontroller: Fix log message on successful update #26207

Merged
merged 1 commit into from
May 27, 2016
Merged

nodecontroller: Fix log message on successful update #26207

merged 1 commit into from
May 27, 2016
+3 −2

Conversation

zmerlynn
Copy link
Member

@zmerlynn zmerlynn commented May 24, 2016 •
edited
Loading

Seen in logs as:

E0524 20:05:01.102671      16 nodecontroller.go:379] Failed while updating Node.Spec.PodCIDR : <nil>

Analytics

@zmerlynn zmerlynn added the sig/node Categorizes an issue or PR as relevant to SIG Node. label May 24, 2016
@zmerlynn
Copy link
Member Author

cc @kubernetes/sig-node

@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 24, 2016
@zmerlynn
Copy link
Member Author

Amusingly, on a 1000 node cluster this was easy to see. On a 2000 node, the nodecontroller is in some crazy death spiral.

@zmerlynn zmerlynn 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 24, 2016
@zmerlynn
Copy link
Member Author

@k8s-bot test this, issue #26187

@zmerlynn
Copy link
Member Author

@k8s-bot test this, issue #IGNORE (not sure, still looking)

@zmerlynn
Copy link
Member Author

@k8s-bot e2e test this, issue #26187

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

@k8s-bot e2e test this, issue #26187

bprashanth
bprashanth reviewed May 25, 2016
View reviewed changes
pkg/controller/node/nodecontroller.go
@@ -375,7 +375,7 @@ func (nc *NodeController) allocateOrOccupyCIDR(obj interface{}) {
glog.V(4).Infof("Assigning node %s CIDR %s", node.Name, podCIDR)
for rep := 0; rep < podCIDRUpdateRetry; rep++ {
node.Spec.PodCIDR = podCIDR.String()
if _, err := nc.kubeClient.Core().Nodes().Update(node); err == nil {
if _, err := nc.kubeClient.Core().Nodes().Update(node); err != nil {
glog.Errorf("Failed while updating Node.Spec.PodCIDR : %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, if i run into error on update (like resource version conflict) I'd want to re-get and retry the update till podCIDRUpdateRetry. This will break on first error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. The code looks messed up both directions (there's a break below as well). Trying to fix it.

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, the bug here is just the log message, nothing else, I just realized.

Copy link
Member Author

Choose a reason for hiding this comment

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

PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make it clearer that this isn't an actual failure but one that we might retry? i.e just will retry %d times, (podCIDRUpdateRetry-rep).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. PTAL.

@zmerlynn zmerlynn removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2016
@zmerlynn zmerlynn force-pushed the fix-unneeded-updated branch 2 times, most recently from c44068b to 70469f8 Compare May 25, 2016 06:41
@zmerlynn zmerlynn changed the title nodecontroller: Fix error check on update nodecontroller: Fix log message on successful update May 25, 2016
@dchen1107
Copy link
Member

Is this the fix for #26211? cc/ @Random-Liu

@zmerlynn
Copy link
Member Author

@dhen1107: No, it's unrelated log spam I found along the way. I thought there was a bug here and it turned out to just be a bug in the logging of of the update case.

@wojtek-t
Copy link
Member

@k8s-bot e2e test this please github issue: #IGNORE

@zmerlynn
Copy link
Member Author

@k8s-bot e2e test this, issue #26270

@wojtek-t wojtek-t mentioned this pull request May 25, 2016
Closed
@zmerlynn
Copy link
Member Author

@k8s-bot e2e test this, issue #IGNORE

@zmerlynn
Copy link
Member Author

Can I get an LGTM on this? (Out the next two days.)

@zmerlynn zmerlynn force-pushed the fix-unneeded-updated branch from 70469f8 to 5876e6d Compare May 25, 2016 19:02
@zmerlynn
Copy link
Member Author

@k8s-bot e2e test this, issue #IGNORE (PR builder was being bad)

@zmerlynn zmerlynn force-pushed the fix-unneeded-updated branch from 5876e6d to cb69960 Compare May 25, 2016 21:44
@zmerlynn zmerlynn added this to the v1.3 milestone May 25, 2016
@bprashanth bprashanth added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2016
@bprashanth
Copy link
Contributor

LGTM, thanks

@zmerlynn
Copy link
Member Author

@k8s-bot e2e test this, issue #26131

@k8s-bot
Copy link

k8s-bot commented May 26, 2016

GCE e2e build/test passed for commit cb69960.

@a-robinson
Copy link
Contributor

Manually merging small PRs while submit queue is struggling

@a-robinson a-robinson merged commit 7522389 into kubernetes:master May 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bprashanth bprashanth

Labels
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. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
10 participants
@zmerlynn @dchen1107 @wojtek-t @bprashanth @k8s-bot @a-robinson @gmarek @googlebot @yujuhong @k8s-github-robot