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

Fix TestServiceAlloc flakes #37487

Merged
merged 1 commit into from
Nov 30, 2016
Merged

Fix TestServiceAlloc flakes #37487

merged 1 commit into from
Nov 30, 2016
+4 −3

Conversation

wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Nov 25, 2016 •
edited by jessfraz
Loading

Fix #37040

Fix TestServiceAlloc flakes

This change is Reviewable

@wojtek-t wojtek-t added kind/flake Categorizes issue or PR as related to a flaky test. release-note-none Denotes a PR that doesn't merit a release note. labels Nov 25, 2016
@wojtek-t wojtek-t added this to the v1.5 milestone Nov 25, 2016
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 25, 2016
@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 25, 2016
@wojtek-t wojtek-t mentioned this pull request Nov 25, 2016
Closed
@smarterclayton
Copy link
Contributor

Lgtm

@wojtek-t wojtek-t assigned smarterclayton and unassigned deads2k Nov 28, 2016
@wojtek-t
Copy link
Member Author

@k8s-bot unit test this, issue: #IGNORE (Waiting for status to be reported)

deads2k
deads2k reviewed Nov 28, 2016
View reviewed changes
pkg/registry/generic/registry/store.go
@@ -203,7 +203,8 @@ func (e *Store) List(ctx api.Context, options *api.ListOptions) (runtime.Object,
// ListPredicate returns a list of all the items matching m.
func (e *Store) ListPredicate(ctx api.Context, p storage.SelectionPredicate, options *api.ListOptions) (runtime.Object, error) {
if options == nil {
options = &api.ListOptions{ResourceVersion: "0"}
// By default we should serve the request from etcd.
options = &api.ListOptions{ResourceVersion: ""}
Copy link
Contributor

Choose a reason for hiding this comment

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

@liggitt @ncdc

Copy link
Member

Choose a reason for hiding this comment

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

Does this need backporting?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so.

@wojtek-t
Copy link
Member Author

@deads2k @smarterclayton - if you're fine with this, can one of you apply label?

liggitt
liggitt reviewed Nov 28, 2016
View reviewed changes
pkg/registry/core/service/ipallocator/controller/repair.go
@@ -105,7 +105,7 @@ func (c *Repair) runOnce() error {
// the service collection. The caching layer keeps per-collection RVs,
// and this is proper, since in theory the collections could be hosted
// in separate etcd (or even non-etcd) instances.
list, err := c.registry.ListServices(ctx, nil)
Copy link
Member

Choose a reason for hiding this comment

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

With the fix in the generic store, why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

We just should avoid passing nil - it seems there are just few such places in the code and we should forbid it.

pkg/registry/generic/registry/store.go
@@ -203,7 +203,8 @@ func (e *Store) List(ctx api.Context, options *api.ListOptions) (runtime.Object,
// ListPredicate returns a list of all the items matching m.
func (e *Store) ListPredicate(ctx api.Context, p storage.SelectionPredicate, options *api.ListOptions) (runtime.Object, error) {
if options == nil {
options = &api.ListOptions{ResourceVersion: "0"}
// By default we should serve the request from etcd.
options = &api.ListOptions{ResourceVersion: ""}
Copy link
Member

Choose a reason for hiding this comment

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

Does this need backporting?

@deads2k
Copy link
Contributor

deads2k commented Nov 28, 2016

@deads2k @smarterclayton - if you're fine with this, can one of you apply label?

@liggitt is more familiar with this area than I am. I'll defer to him.

@wojtek-t
Copy link
Member Author

Does this need backporting?

Good question - maybe? But it's not that crucial, because if you are http sending a request to apiserver, the the "List handler" will always create ListOptions and for that path it's never nil.
So it's only a matter of some calls coming from apiserver internals.

@liggitt
Copy link
Member

liggitt commented Nov 28, 2016

So it's only a matter of some calls coming from apiserver internals.

If one of those internal paths was the IP allocator, that seems important to backport

@wojtek-t
Copy link
Member Author

If one of those internal paths was the IP allocator, that seems important to backport

I wasn't trying to say not to backport it - I'm fine with backporting.

@wojtek-t
Copy link
Member Author

But first we probably need to have in in head :)

@liggitt
Copy link
Member

liggitt commented Nov 28, 2016

LGTM

@liggitt liggitt added lgtm "Looks good to me", indicates that a PR is ready to be merged. cherrypick-candidate labels Nov 28, 2016
@liggitt
Copy link
Member

liggitt commented Nov 28, 2016

I would pick it to 1.5 and 1.4

@wojtek-t
Copy link
Member Author

@liggitt - definitely for 1.5. But I'm fine with cherrypicking it to 1.4 too - I will prepare cherrypicks once this is merged.

@dims
Copy link
Member

dims commented Nov 28, 2016

@k8s-bot test this

@saad-ali saad-ali added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 28, 2016
@wojtek-t
Copy link
Member Author

@dims - why did you trigger test on all PRs? This is why we ended in a situation where a bunch of tests on different PRs are failing with "error in queue" or "waiting for status" or sth like that.

@k8s-bot test this, issue: #IGNORE

@dims
Copy link
Member

dims commented Nov 28, 2016

@wojtek-t : just 1.5 milestone with lgtm(s), trying to get the submit queue to pick it up.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 1760660. 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.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 1760660. Full PR test history.

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

@wojtek-t
Copy link
Member Author

@dims - you do NOT need to trigger tests to merge bot to pick them up. If retest will be needed, merge bot will do it itself. What you did actually pushed a bunch of PRs OUT of queue, because after your trigger a bunch of them stuck in "error in queue" or sth like that state. So please do NOT do it again.

@wojtek-t
Copy link
Member Author

@k8s-bot test this, issue: #IGNORE (build started)

@wojtek-t
Copy link
Member Author

@k8s-bot e2e test this, issue: #IGNORE (error in queue)

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 1760660. Full PR test history.

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

@wojtek-t
Copy link
Member Author

@k8s-bot etcd3 test this, issue: #33380

@dims
Copy link
Member

dims commented Nov 29, 2016

@wojtek-t ack. will not repeat that. apologies.

@eparis eparis added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 29, 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 985079a into kubernetes:master Nov 30, 2016
@saad-ali saad-ali mentioned this pull request Nov 30, 2016
Merged
saad-ali added a commit that referenced this pull request Nov 30, 2016
@saad-ali
Merge pull request #37670 from saad-ali/automated-cherry-pick-of-#37489-
96a69ce 
#37321-#37394-#37345-#37397-#37411-#37379-#37323-#37195-#37139-#37225-#37562-#37487-upstream-release-1.5

Automated cherry pick of #37489 #37321 #37394 #37345 #37397 #37411 #37379 #37323 #37195 #37139 #37225 #37562 #37487 upstream release 1.5
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.5" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@wojtek-t wojtek-t mentioned this pull request Nov 30, 2016
Merged
@jessfraz jessfraz added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 30, 2016
@saad-ali saad-ali added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Dec 1, 2016
jessfraz referenced this pull request Dec 1, 2016
@jessfraz
Merge pull request #37672 from wojtek-t/automated-cherry-pick-of-#374…
e94c133 
…87-upstream-release-1.4

Fix TestServiceAlloc flakes
@wojtek-t wojtek-t deleted the kubernetes_service branch December 20, 2016 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@liggitt liggitt liggitt left review comments

Assignees

@liggitt liggitt

@smarterclayton smarterclayton

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/flake Categorizes issue or PR as related to a flaky test. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.