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

fixes the way the informers are started in sample controller pkg #69636

Merged
merged 1 commit into from
Oct 22, 2018
Merged

fixes the way the informers are started in sample controller pkg #69636

merged 1 commit into from
Oct 22, 2018

Conversation

p0lyn0mial
Copy link
Contributor

What this PR does / why we need it: there is no need to run informerFactory.Start method in a separate goroutine. The method is non-blocking and runs all registered informers in a dedicated goroutine.

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 10, 2018
@p0lyn0mial
Copy link
Contributor Author

/assign @deads2k

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 10, 2018
nikhita
nikhita reviewed Oct 11, 2018
View reviewed changes
staging/src/k8s.io/sample-controller/main.go
go kubeInformerFactory.Start(stopCh)
go exampleInformerFactory.Start(stopCh)
kubeInformerFactory.Start(stopCh)
exampleInformerFactory.Start(stopCh)
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link

Choose a reason for hiding this comment

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

we should add comments here explaining this doesnt need to go in a go routine for so and so reason, more so because lot of first timers come here to copy code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sgtm

nikhita
nikhita reviewed Oct 11, 2018
View reviewed changes
staging/src/k8s.io/sample-controller/main.go
go kubeInformerFactory.Start(stopCh)
go exampleInformerFactory.Start(stopCh)
kubeInformerFactory.Start(stopCh)
exampleInformerFactory.Start(stopCh)

if err = controller.Run(2, stopCh); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be run as a go routine though?

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 think it is okay, the main func will stop at this method as it essentially runs the controller and blocks until the stopCh is closed.

@nikhita
Copy link
Member

nikhita commented Oct 11, 2018

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 11, 2018
@jennybuckley
Copy link

/assign @yliaog

@p0lyn0mial p0lyn0mial force-pushed the sample_controller_factory_start branch from a2f5bb9 to e55ca64 Compare October 22, 2018 18:11
@p0lyn0mial
Copy link
Contributor Author

added the comment. PTAL

@yliaog
Copy link
Contributor

yliaog commented Oct 22, 2018

/lgtm

@p0lyn0mial
Copy link
Contributor Author

@stts, @munnerz can one of you approve this pull :) ?

@nikhita
Copy link
Member

nikhita commented Oct 22, 2018

Going to close + reopen so tests run again.

/close

@munnerz
Copy link
Member

munnerz commented Oct 22, 2018

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz, p0lyn0mial

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 22, 2018
@k8s-ci-robot
Copy link
Contributor

@nikhita: Closing this PR.

In response to this:

Going to close + reopen so tests run again.

/close

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.

@nikhita
Copy link
Member

nikhita commented Oct 22, 2018

/reopen

@k8s-ci-robot
Copy link
Contributor

@nikhita: Reopening this PR.

In response to this:

/reopen

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.

@k8s-ci-robot k8s-ci-robot reopened this Oct 22, 2018
@k8s-ci-robot k8s-ci-robot merged commit 8e7e226 into kubernetes:master Oct 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikhita nikhita nikhita left review comments

@krmayankk krmayankk krmayankk left review comments

@munnerz munnerz Awaiting requested review from munnerz

Assignees

@munnerz munnerz

@deads2k deads2k

@yliaog yliaog

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. kind/bug Categorizes issue or PR as related to a bug. 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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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.

None yet
8 participants
@p0lyn0mial @nikhita @jennybuckley @yliaog @munnerz @k8s-ci-robot @krmayankk @deads2k