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

Make controller Run methods consistent #44408

Merged
merged 2 commits into from
Apr 14, 2017
Merged

Make controller Run methods consistent #44408

merged 2 commits into from
Apr 14, 2017

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented Apr 12, 2017

  • startup/shutdown logging
  • wait for cache sync logging
  • defer utilruntime.HandleCrash()
  • wait for stop channel before exiting

Please review for accuracy and consistency.

cc @deads2k @wojtek-t @smarterclayton @sttts @mikedanese @liggitt @rrati @derekwaynecarr

@ncdc ncdc added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 12, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 12, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@ncdc ncdc mentioned this pull request Apr 12, 2017
Merged
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 12, 2017
@wojtek-t
Copy link
Member

I can take this if you're fine with waiting until tomorrow for review - I really like this PR (from the description).

@ncdc
Copy link
Member Author

ncdc commented Apr 12, 2017

@wojtek-t that's fine - no need to rush this in, nor should we. Thanks!

@guangyouyu
Copy link
Contributor

@ncdc nodeController in the cloud-controller-manager should also run in goroutine because the controller.WaitForCacheSync in nodeController.run will block(after removing the anonymous function)

@ncdc
Copy link
Member Author

ncdc commented Apr 13, 2017

@guangyouyu thanks, fixing now

@ncdc
Copy link
Member Author

ncdc commented Apr 13, 2017

@guangyouyu actually that's a different type (from k8s.io/kubernetes/pkg/controller/cloud)

@ncdc ncdc force-pushed the controller-run-consistency branch from 5555759 to c4132b2 Compare April 13, 2017 01:55
@guangyouyu
Copy link
Contributor

@ncdc sorry，my mistake
In addition, CloudNodeController seems to refactoring also

@wojtek-t wojtek-t self-assigned this Apr 13, 2017
@wojtek-t
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 13, 2017
@ncdc
Make controller Run methods consistent
e63fcf7 
- startup/shutdown logging
- wait for cache sync logging
- defer utilruntime.HandleCrash()
- wait for stop channel before exiting
@ncdc
Update bazel
a81e580
@ncdc ncdc force-pushed the controller-run-consistency branch from c4132b2 to a81e580 Compare April 14, 2017 11:38
@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 Apr 14, 2017
@ncdc
Copy link
Member Author

ncdc commented Apr 14, 2017

@wojtek-t rebased. The only conflict was with your changes to pkg/proxy/config. Please review and make sure it still looks ok in there. Thanks!

@wojtek-t
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc, wojtek-t

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@ncdc
Copy link
Member Author

ncdc commented Apr 14, 2017

@k8s-bot cvm gce e2e test this

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 14, 2017 •
edited

@ncdc: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins GCI GCE e2e a81e580 link @k8s-bot gci gce e2e test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit ce860e1 into kubernetes:master Apr 14, 2017
@ncdc ncdc mentioned this pull request Apr 17, 2017
Merged
@ncdc ncdc deleted the controller-run-consistency branch April 18, 2017 17:14
@aveshagarwal aveshagarwal mentioned this pull request Jul 5, 2017
Merged
@joelsmith joelsmith mentioned this pull request Sep 27, 2017
Merged
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Sep 29, 2017
@openshift-merge-robot
Merge pull request #16575 from joelsmith/release-3.6
f7a367c 
Automatic merge from submit-queue.

Cherry-pick #15067 endpoints fixes.

cc @sjenning @eparis @derekwaynecarr

This is a cherry-pick of #15067 to prevent too-frequent processing of endpoint updates.

https://bugzilla.redhat.com/show_bug.cgi?id=1481603

Here's the original text of #15067 as written by @aveshagarwal:

> Fixes #14710
> 
> Note: I had to manually merge changes and due to this, the commits might not match exactly. 
> 
> It has following 2 complete upstream PRs: 
> kubernetes/kubernetes#47731  (with some modifications/additions).
> kubernetes/kubernetes#47788
> 
> And one partial PR to make above PRs compile:
> kubernetes/kubernetes#44408

(Edited to fix bz link -- originally sent with wrong bug)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jsafrane jsafrane

@goltermann goltermann

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@ncdc @k8s-reviewable @wojtek-t @guangyouyu @k8s-github-robot @k8s-ci-robot @jsafrane @goltermann