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

Predicate cacheing and cleanup #33763

Merged
merged 1 commit into from Oct 20, 2016
Merged

Predicate cacheing and cleanup #33763

merged 1 commit into from Oct 20, 2016

Conversation

jayunit100
Copy link
Member

@jayunit100 jayunit100 commented Sep 29, 2016 •
edited

Fix to #31795

First pass @ cleanup and caching of the CheckServiceAffinity function.

The cleanup IMO is necessary because the logic around the pod listing and the use of the "implicit selector" (which is reverse engineered to enable the homogenous pod groups).

Should still pass the E2Es.

@timothysc @wojtek-t

This change is Reviewable

@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch from bd1866a to a2bc2c7 Compare September 29, 2016 16:18
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Sep 29, 2016
@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch 2 times, most recently from d876b28 to 32c4431 Compare September 29, 2016 16:21
@jayunit100
Copy link
Member Author

@k8s-bot gke e2e test this #33175

@jayunit100
Copy link
Member Author

@k8s-bot gke e2e test this.

1 similar comment
@jayunit100
Copy link
Member Author

@k8s-bot gke e2e test this.

@spxtr
Copy link
Contributor

spxtr commented Sep 29, 2016

@k8s-bot gke test this kubernetes/test-infra#709

@jayunit100
Copy link
Member Author

still not sure why e2es are dying here

@timothysc
Copy link
Member

So instead of breaking it up into functional objects, why not create a series of well defined utility functions? I see most of those f(n)'s as being generally useful and it would also be easier to read.

@timothysc timothysc added sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. labels Oct 3, 2016
@timothysc timothysc added this to the v1.5 milestone Oct 3, 2016
@timothysc timothysc added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 3, 2016
@jayunit100
Copy link
Member Author

Broader context for Utils, data access patterns and so on. Bee have something like a dao wrapper query lib in e2e/federated/... maybe time to bump it to core if the same thing doesn't already exist there?

@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch from 1e81963 to d47d3fb Compare October 5, 2016 01:01
@k8s-github-robot k8s-github-robot mentioned this pull request Oct 5, 2016
Closed
@jayunit100
Copy link
Member Author

heres another impl. Shud be passing assume failures are flakes

@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch 2 times, most recently from d9ef54b to db7e0f2 Compare October 5, 2016 14:53
@jayunit100
Copy link
Member Author

ok i added unit tests

@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch from 43fc84f to 1174f4c Compare October 5, 2016 16:59
@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch from 3f519c4 to b96cf3a Compare October 19, 2016 15:44
@jayunit100
Copy link
Member Author

jayunit100 commented Oct 19, 2016 •
edited

@wojtek-t

  • Rather then adding the GetPodServices check inside checkServiceAffinity, Ive added a second commit which is separate (update 3:33pm - its now sqaushed :)), which adds a unit test that verifies predicate behaviour with / without precomputation. That is a good stub for eventually making a decision on wether or not we want predicates to bootstrap their own data.
  • Other comments addressed - good catch on moving the pFactory() up, you are right that will speed things up also. Regarding the EmptyMetadata change, i also reverted it - i thought it made casting logic less amibguous but probably adds no value.

jayunit100
jayunit100 commented Oct 19, 2016
View reviewed changes
plugin/pkg/scheduler/algorithm/listers.go
@@ -91,10 +91,6 @@ func (f FakeServiceLister) GetPodServices(pod *api.Pod) (services []*api.Service
services = append(services, service)
}
}
if len(services) == 0 {
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes the if err == nil check which woj suggested less noisy,and more importantly makes it so semantics conform to the other GetPodsServices tests.

@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch 3 times, most recently from b5bad40 to 751fa4f Compare October 19, 2016 20:00
@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch from 751fa4f to 6fe77cb Compare October 19, 2016 20:18
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 6fe77cb. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 6fe77cb. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 6fe77cb. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@jayunit100 jayunit100 mentioned this pull request Oct 20, 2016
Closed
@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch from 6fe77cb to cf833be Compare October 20, 2016 03:09
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit cf833be. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@jayunit100
Copy link
Member Author

im giong to dig into the right semantics to keep for now wrt serices/ for now the unit test wont pass.

wojtek-t
wojtek-t reviewed Oct 20, 2016
View reviewed changes
Copy link
Member

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

Few more (mostly minor) comments.

plugin/pkg/scheduler/algorithm/predicates/predicates.go Show resolved Hide resolved
plugin/pkg/scheduler/algorithm/predicates/predicates.go Show resolved Hide resolved
plugin/pkg/scheduler/algorithm/predicates/predicates.go Show resolved Hide resolved
@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch from cf833be to e7fe19d Compare October 20, 2016 10:59
@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch 2 times, most recently from 4ae15dd to 9aa982f Compare October 20, 2016 11:36
@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch from 9aa982f to 9b1c14e Compare October 20, 2016 12:22
@jayunit100
PredicateMetadata factory and optimization, Cleaned up some comments,
08cff01 
Comments addressed, Make emptyMetadataProducer a func to avoid casting,
FakeSvcLister: remove error return for len(svc)=0.  New test for
predicatePrecomp to make method semantics explictly enforced when meta
is missing. Precompute wrapper.
@jayunit100 jayunit100 force-pushed the sched-checkservice-predicateCache branch from 9b1c14e to 08cff01 Compare October 20, 2016 12:27
@wojtek-t
Copy link
Member

/lgtm

(I'm assuming that tests will pass)

@wojtek-t wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit b0a4216 into kubernetes:master Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wojtek-t wojtek-t wojtek-t left review comments

Assignees

@davidopp davidopp

@wojtek-t wojtek-t

Labels
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/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jayunit100 @spxtr @timothysc @wojtek-t @k8s-ci-robot @k8s-github-robot @googlebot @davidopp