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.

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

UPSTREAM: 53989: Remove repeated random string generations in scheduler volume predicate #16864

Conversation

aveshagarwal
Copy link
Contributor

@aveshagarwal aveshagarwal commented Oct 13, 2017

@sjenning @smarterclayton

Though the upstream PR 53793 has been backported to kube 1.7 branch (53884). I am not sure if we have a plan for another origin rebase to latest kube 1.7, and if we would want to wait for that.

So this backports following 3 PRs:
kubernetes/kubernetes#53793
kubernetes/kubernetes#53720 (partial)
kubernetes/kubernetes#53989

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 13, 2017
@bparees
Copy link
Contributor

bparees commented Oct 13, 2017

/unassign

@aveshagarwal
Copy link
Contributor Author

/unassign soltysh

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 13, 2017
@sjenning
Copy link
Contributor

/retest

@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Oct 14, 2017
@aveshagarwal
Copy link
Contributor Author

what is "needs-upstream" label?

@aveshagarwal
Copy link
Contributor Author

/test extended_conformance_install_update

@sjenning
Copy link
Contributor

@aveshagarwal yeah, i think that label is recently added in openshift/release#214 to all PRs with UPSTREAM commits so that infra can gate those PRs during a rebase.

@stevekuznetsov can you confirm?

@stevekuznetsov
Copy link
Contributor

@sjenning yes you understand correctly

@stevekuznetsov
Copy link
Contributor

@sjenning @aveshagarwal if the messaging is not clear, could we change the label or name it differently to make it more clear?

@sjenning
Copy link
Contributor

@stevekuznetsov I guess my first reaction was color{red} = PR is blocked and the needs-* prefix makes me think that action is required. When really, it is an annotation and only blocking if there is a rebase gate in place.

Maybe a non-red label color and rather than "needs-upstream" maybe just "upstream" or "vendor-patch"?

@stevekuznetsov
Copy link
Contributor

Makes sense. Opened https://github.com/openshift/release/issues/287 to track it

@aveshagarwal
Copy link
Contributor Author

@sjenning +1, also leaving a message (something like "the label is informational and no action is required") in addition to the label might help avoid confusion too.

@stevekuznetsov is there any rebase going on now? if not, why was this label added? robot error?

@aveshagarwal
Copy link
Contributor Author

@liggitt I was waiting for that (kubernetes/kubernetes#53989) to merge to pick that up so I am aware. But these PRs also helped reduce performance regression to some extent.

@aveshagarwal
Copy link
Contributor Author

aveshagarwal commented Oct 17, 2017

@liggitt specifically kubernetes/kubernetes#53720 helped with some performance issue, and the other one helps with leader election issue.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 18, 2017
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2017
@aveshagarwal
Copy link
Contributor Author

@sjenning @liggitt I have backported kubernetes/kubernetes#53989 too in this PR. PTAL.

@aveshagarwal
Copy link
Contributor Author

/test extended_conformance_install_update

@liggitt
Copy link
Contributor

liggitt commented Oct 18, 2017

two cleanups, then LGTM

  • squash UPSTREAM: 53793: modifications needed for 3.7. into the other UPSTREAM: 53793 commit
  • drop UPSTREAM: 53720: Add benchmark for random string generation utility, we don't need to carry a test-only benchmark commit

Avesh Agarwal added 3 commits October 18, 2017 09:22
@aveshagarwal aveshagarwal changed the title UPSTREAM: 53793: User separate client for leader election in scheduler UPSTREAM: 53989: Remove repeated random string generations in scheduler volume predicate Oct 18, 2017
@aveshagarwal
Copy link
Contributor Author

@liggitt updated, PTAL. thanks.

@aveshagarwal
Copy link
Contributor Author

/test extended_conformance_gce
/test end_to_end
/test cmd
/test extended_conformance_install_update

@aveshagarwal
Copy link
Contributor Author

/test extended_conformance_gce
/test extended_conformance_install_update

@aveshagarwal
Copy link
Contributor Author

@sjenning @liggitt reminder

@liggitt
Copy link
Contributor

liggitt commented Oct 21, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aveshagarwal, liggitt, sjenning

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

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

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2017
@aveshagarwal
Copy link
Contributor Author

wonder why this PR is stuck and not merged?

@sjenning sjenning added the kind/bug Categorizes issue or PR as related to a bug. label Oct 25, 2017
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 145cdaf into openshift:master Oct 25, 2017
@stevekuznetsov
Copy link
Contributor

@aveshagarwal remember to look at the Submit Queue status at the bottom of PRs -- it will say what is missing.

@aveshagarwal aveshagarwal deleted the master-backport-53793-53884 branch January 19, 2018 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. vendor-update Touching vendor dir or related files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet