Remove repeated random string generations in scheduler volume predicate #53989

shyamjvs · 2017-10-16T13:15:57Z

@wojtek-t @liggitt @jsafrane - Does this look ok to you?

shyamjvs · 2017-10-16T13:17:12Z

I'll try running kubemark-5000 with this change and see the outcome.

liggitt · 2017-10-16T13:34:09Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

 	}

 	return c.predicate
 }

+func (c *MaxPDVolumeCountChecker) getNextVolumeIDPrefix() string {
+	c.lock.Lock()
+	defer c.lock.Unlock()


can be lock-free with atomic.AddInt64(&c.volumeIDPrefixCounter, 1)

Smooth! Changed it.

shyamjvs · 2017-10-16T15:23:32Z

@liggitt Fixed stuff. PTAL.

liggitt · 2017-10-16T15:27:47Z

change LGTM, will let sig-storage weigh in

shyamjvs · 2017-10-16T15:34:42Z

cc @kubernetes/sig-storage-pr-reviews

msau42 · 2017-10-16T17:33:35Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

+	// predicate() call so a deleted PVC used by two pods is counted as one volume
+	// and not as two. Also, appending a counter to a const string removes
+	// the need to create a random string each time (which is expensive).
+	randomPrefix := c.getNextVolumeIDPrefix()


I'm not sure if a random prefix is needed for each predicate call. It may be good enough to just generate a random prefix once during initialization. All the structures used to count volumes in this predicate are local and shouldn't conflict if the predicate is running in parallel.

That's precisely what I'm doing in this PR - a random prefix initialized once and then just appending it to a counter. Do you mean sth else?

I mean, I don't think you even need a counter. Just a random prefix initialized in the beginning.

In that case do we even need the initialized string to be random? For e.g why not just set it to some hard-coded value like 'foobar'?

don't you risk conflicting with actual object names?

Yes, we just want a prefix that won't conflict with actual object names. So we could generate some random prefix, or have some hardcoded value that we are sure nobody would ever use. I don't think it needs to be generated every time we run the predicate though.

Changed it - PTAL.

k8s-ci-robot · 2017-10-17T08:17:24Z

@shyamjvs: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

shyamjvs · 2017-10-17T08:24:44Z

@kubernetes/test-infra-maintainers Seems like there's a bug with the do-not-merge/release-note-label-needed label. Is it intended to be added (and the release-note-none label removed) each time I change the PR title? Also, even if I added the release-note-none label during PR creation, it still removed it and added the do-not-merge one. Other e.g.s - #54045, #53977. This only started happening recently.

msau42 · 2017-10-17T16:47:30Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

+		maxVolumes:           maxVolumes,
+		pvInfo:               pvInfo,
+		pvcInfo:              pvcInfo,
+		randomVolumeIDPrefix: rand.String(8),


Can this be 32 like originally? That way we reduce the chance of conflicting with the user's object names.

cjwagner · 2017-10-17T18:03:24Z

@shyamjvs The release note process is WAI.
If you want to specify that there is no release note you should either:

make the release note "none" via the release-note block in the PR body like this: ```release-note none ```
or use the /release-note-none command

/release-note-none

shyamjvs · 2017-10-17T18:05:43Z

@cjwagner Sounds good. But any idea why the label is getting removed on renaming the PR title?

msau42 · 2017-10-17T18:23:06Z

/lgtm

cjwagner · 2017-10-17T18:24:45Z

@shyamjvs Yeah, the PR 'edit' event is used to detect when the PR body text (including the release-note block) is changed, but it is also triggered by title changes. The Prow plugin that handled the event saw the release-note-none label on the PR and assumed that it was previously added to the PR based on the release-note block being "none" since there was no /release-note-none command on the PR. Since the plugin didn't find a release-note block after the edit event, it thinks that you removed the release-note block which is why it applies the release-note-needed label.

shyamjvs · 2017-10-17T19:35:55Z

@davidopp / @bsalamat Could one of you please approve the PR?

bsalamat · 2017-10-17T20:54:33Z

/approve

k8s-github-robot · 2017-10-17T20:54:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, msau42, shyamjvs

Associated issue: 53327

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

Needs approval from an approver in each of these OWNERS Files:

~~plugin/pkg/scheduler/OWNERS~~ [bsalamat]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

shyamjvs · 2017-10-18T00:44:35Z

/test pull-kubernetes-e2e-gce

k8s-github-robot · 2017-10-18T01:43:36Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@sjenning

Automatic merge from submit-queue. UPSTREAM: 53989: Remove repeated random string generations in scheduler volume predicate @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

k8s-github-robot assigned resouer and davidopp Oct 16, 2017

shyamjvs added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 16, 2017

liggitt reviewed Oct 16, 2017

View reviewed changes

shyamjvs force-pushed the use-counter-in-scheduler branch 2 times, most recently from 874f578 to 82b2716 Compare October 16, 2017 14:00

shyamjvs changed the title ~~[WIP] Replace random string generation with counter in scheduler volume predicate~~ Replace random string generation with counter in scheduler volume predicate Oct 16, 2017

shyamjvs added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 16, 2017

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Oct 16, 2017

msau42 reviewed Oct 16, 2017

View reviewed changes

bsalamat mentioned this pull request Oct 16, 2017

Large kubemark performance tests failing with timeout during ns deletion #53327

Closed

shyamjvs force-pushed the use-counter-in-scheduler branch from 82b2716 to 07e3a09 Compare October 17, 2017 08:15

shyamjvs changed the title ~~Replace random string generation with counter in scheduler volume predicate~~ Remove repeated random string generations in scheduler volume predicate Oct 17, 2017

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 17, 2017

shyamjvs added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 17, 2017

liggitt mentioned this pull request Oct 17, 2017

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

Merged

msau42 reviewed Oct 17, 2017

View reviewed changes

Remove repeated random string generations in scheduler volume predicate

5a85f9d

shyamjvs force-pushed the use-counter-in-scheduler branch from 07e3a09 to 5a85f9d Compare October 17, 2017 17:58

k8s-ci-robot assigned msau42 Oct 17, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2017

k8s-github-robot merged commit 4105cca into kubernetes:master Oct 18, 2017

shyamjvs deleted the use-counter-in-scheduler branch November 24, 2017 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove repeated random string generations in scheduler volume predicate #53989

Remove repeated random string generations in scheduler volume predicate #53989

shyamjvs commented Oct 16, 2017

shyamjvs commented Oct 16, 2017

liggitt Oct 16, 2017

shyamjvs Oct 16, 2017

shyamjvs commented Oct 16, 2017

liggitt commented Oct 16, 2017

shyamjvs commented Oct 16, 2017

msau42 Oct 16, 2017

shyamjvs Oct 16, 2017

msau42 Oct 16, 2017

shyamjvs Oct 16, 2017

liggitt Oct 16, 2017

msau42 Oct 16, 2017

shyamjvs Oct 17, 2017

k8s-ci-robot commented Oct 17, 2017

shyamjvs commented Oct 17, 2017 •

edited

msau42 Oct 17, 2017

shyamjvs Oct 17, 2017

cjwagner commented Oct 17, 2017

shyamjvs commented Oct 17, 2017

msau42 commented Oct 17, 2017

cjwagner commented Oct 17, 2017

shyamjvs commented Oct 17, 2017

bsalamat commented Oct 17, 2017

k8s-github-robot commented Oct 17, 2017

shyamjvs commented Oct 18, 2017

k8s-github-robot commented Oct 18, 2017

Remove repeated random string generations in scheduler volume predicate #53989

Remove repeated random string generations in scheduler volume predicate #53989

Conversation

shyamjvs commented Oct 16, 2017

shyamjvs commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shyamjvs commented Oct 16, 2017

liggitt commented Oct 16, 2017

shyamjvs commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 17, 2017

shyamjvs commented Oct 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjwagner commented Oct 17, 2017

shyamjvs commented Oct 17, 2017

msau42 commented Oct 17, 2017

cjwagner commented Oct 17, 2017

shyamjvs commented Oct 17, 2017

bsalamat commented Oct 17, 2017

k8s-github-robot commented Oct 17, 2017

shyamjvs commented Oct 18, 2017

k8s-github-robot commented Oct 18, 2017

shyamjvs commented Oct 17, 2017 •

edited