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

MESOS: fix panic in controller-manager when apiserver lookups fail #18017

Merged
merged 1 commit into from
Dec 2, 2015
Merged

MESOS: fix panic in controller-manager when apiserver lookups fail #18017

merged 1 commit into from
Dec 2, 2015

Conversation

s-urbaniak
Copy link
Contributor

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 1, 2015
@s-urbaniak
Copy link
Contributor Author

/cc @sttts PTAL

sttts
sttts reviewed Dec 1, 2015
View reviewed changes
contrib/mesos/pkg/node/statusupdater.go

// having returns a slice of node objects matching
// the given slaves as their Spec.ExternalIDs.
func (f filter) having(slaves []string) []api.Node {
Copy link
Contributor

Choose a reason for hiding this comment

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

whichAreIn ? having doesn't make sense for two sets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as discussed we chose intersect

@k8s-bot
Copy link

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit 6079cf7cb258ac0dbe48d5b9bb86b9d0233d2578.

@s-urbaniak
Copy link
Contributor Author

PTAL @sttts @jdef

@k8s-bot
Copy link

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit 44e6f3276ee607186c8450321dd8b65342eeea8f.

@k8s-bot
Copy link

k8s-bot commented Dec 1, 2015

GCE e2e build/test failed for commit 0f08d1b41ff90ada5f4bdd765d470386faf62244.

@k8s-bot
Copy link

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit 5f34ea2b4a01e2f6637c1e92da3548bc7b2dc162.

@k8s-bot
Copy link

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit d7aa860f815e76006220e3df6e956537d069bae6.

@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?

@s-urbaniak
Copy link
Contributor Author

While fixing the bug me and @sttts just recognized that querying via the listwatch is inefficient in the current implementation:
https://github.com/kubernetes/kubernetes/blob/master/contrib/mesos/pkg/node/statusupdater.go#L78

The suggestion is to use nodeStore instead. This makes the unit tests of the set-jazz pretty unnecessary.

The following commit implements the suggestion mentioned here.

@jdef
Copy link
Contributor

jdef commented Dec 1, 2015

🎷

@jdef jdef assigned sttts and unassigned karlkfi Dec 1, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 1, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit f572bedac9359a317468674900963e9670362dfd.

@k8s-github-robot
Copy link

Labelling this PR as size/S

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 2, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 2, 2015

GCE e2e test build/test passed for commit 98148ffeb445fcd4514a573b90cf2a83f4c0047a.

@s-urbaniak
Copy link
Contributor Author

@jdef PTAL
Additionally i smoke-tested this patch locally using:

$ NUM_NODES=3 ./cluster/kube-up.sh
$ kubectl describe no

On two machines a kubelet has been started marked as KubeletReady, one machine was being left idle and correctly marked as SlaveReady which is what the code in this commit does.

@jdef
Copy link
Contributor

jdef commented Dec 2, 2015

lgtm. please squash

@s-urbaniak
Copy link
Contributor Author

@jdef squashed :-)

@sttts sttts added e2e-not-required lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Dec 2, 2015
@k8s-github-robot
Copy link

Travis continuous integration appears to have missed, closing and re-opening to trigger it

@k8s-bot
Copy link

k8s-bot commented Dec 2, 2015

GCE e2e test build/test passed for commit 91cbd5f.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Dec 2, 2015
Merge pull request #18017 from mesosphere/sur-k8sm-656-panic
cd76ca9 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit cd76ca9 into kubernetes:master Dec 2, 2015
@jdef jdef changed the title WIP/MESOS: fix panic in controller-manager when apiserver lookups fail MESOS: fix panic in controller-manager when apiserver lookups fail Dec 2, 2015
@sttts sttts mentioned this pull request Dec 8, 2015
Closed
@s-urbaniak s-urbaniak deleted the sur-k8sm-656-panic branch December 9, 2015 15:15
s-urbaniak pushed a commit to mesosphere-backup/kubernetes that referenced this pull request Dec 10, 2015
Merge pull request kubernetes#18017 from mesosphere/sur-k8sm-656-panic
975eeeb 
Auto commit by PR queue bot
jdef pushed a commit to mesosphere-backup/kubernetes that referenced this pull request Jan 25, 2016
Merge pull request kubernetes#18017 from mesosphere/sur-k8sm-656-panic
0573dd5 
Auto commit by PR queue bot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sttts sttts

Labels
area/platform/mesos lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@s-urbaniak @k8s-github-robot @k8s-bot @jdef @sttts @k8s-teamcity-mesosphere @karlkfi @googlebot