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

Cleanup math/rand package usage #71170

Merged
merged 2 commits into from Jul 2, 2019

Conversation

@ash2k
Copy link
Member

commented Nov 17, 2018

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Removes math/rand seeding from unexpected places and adds it to proper places.

Special notes for your reviewer:
This is a simplified and non-controversial version of #69930.

Does this PR introduce a user-facing change?:

NONE
@@ -536,7 +536,7 @@ func (f *FakeDockerClient) normalSleep(mean, stdDev, cutOffMillis int) {
return
}
cutoff := (time.Duration)(cutOffMillis) * time.Millisecond
delay := (time.Duration)(rand.NormFloat64()*float64(stdDev)+float64(mean)) * time.Millisecond
delay := (time.Duration)(f.RandGenerator.NormFloat64()*float64(stdDev)+float64(mean)) * time.Millisecond

This comment has been minimized.

Copy link
@ash2k

ash2k Nov 17, 2018

Author Member

Other places in this file use field, not global rng.

@@ -242,16 +242,15 @@ func TestHammerController(t *testing.T) {
currentNames := sets.String{}
rs := rand.NewSource(rand.Int63())
f := fuzz.New().NilChance(.5).NumElements(0, 2).RandSource(rs)
r := rand.New(rs) // Mustn't use r and f concurrently!

This comment has been minimized.

Copy link
@ash2k

ash2k Nov 17, 2018

Author Member

Global rng is used to seed rs so there is no point in creating a new rand?

@@ -115,17 +115,14 @@ type eventBroadcasterImpl struct {
// The return value can be ignored or used to stop recording, if desired.
// TODO: make me an object with parameterizable queue length and retry interval
func (eventBroadcaster *eventBroadcasterImpl) StartRecordingToSink(sink EventSink) watch.Interface {
// The default math/rand package functions aren't thread safe, so create a

This comment has been minimized.

Copy link
@ash2k

ash2k Nov 17, 2018

Author Member

I don't think this is true.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Jul 1, 2019

Member

It's not true now and probably wasn't true when I wrote this comment, either, but it was true in very early versions of go.

@@ -73,6 +75,11 @@ func init() {
})
}

func TestMain(m *testing.M) {

This comment has been minimized.

Copy link
@ash2k

ash2k Nov 17, 2018

Author Member

All e2e tests now have rand seeded IIUC.

@jennybuckley

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

/cc @jpbetz

@k8s-ci-robot k8s-ci-robot requested a review from jpbetz Nov 26, 2018

@ash2k

This comment has been minimized.

Copy link
Member Author

commented Dec 8, 2018

This is ready to be merged.

@ash2k

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2018

/assign dchen1107 karataliu tallclair sttts

@ash2k

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

@@ -34,6 +36,7 @@ import (
)

func main() {
rand.Seed(time.Now().UnixNano())

This comment has been minimized.

Copy link
@tallclair

tallclair Jan 3, 2019

Member

Why are we using a variable seed to generate docs? I don't know how it is used, but I would expect that we want deterministic output for gen_kube_docs?

This comment has been minimized.

Copy link
@ash2k

ash2k Jan 3, 2019

Author Member

Random numbers probably shouldn't be used to generate docs? Or if they are, then why not really random? I think properly seeding RNG is a good way to catch any issues.

This comment has been minimized.

Copy link
@tallclair

tallclair Jan 22, 2019

Member

Agree that they probably shouldn't be used, but if they are you want consistent results. Otherwise, every time I need to rebuild the docs, I'll end up with a bunch of unrelated changes (breaking caching, among other things).

This comment has been minimized.

Copy link
@ash2k

ash2k Jan 22, 2019

Author Member

I think the right way would be to use an RNG seeded to a fixed number explicitly, rather than relying on the global one being not initialized. The only way to find out if this is actually an issue is to keep it here, but I'll remove it if you want it that way, np.

This comment has been minimized.

Copy link
@tallclair

tallclair Jan 22, 2019

Member

@ash2k agreed, fixed seed SGTM

This comment has been minimized.

Copy link
@ash2k

ash2k Jan 22, 2019

Author Member

Sorry, I was not clear enough. I meant that the code, that uses RNG and wants a fixed number sequence (if such code exists in this docs generator at all), should be using a non-global RNG and should seed it with some constant. It should not be relying on the global RNG to be seeded to a fixed number (because some other part of the program may have the opposite need).

In other words:

Agree that they probably shouldn't be used, but if they are you want consistent results. Otherwise, every time I need to rebuild the docs, I'll end up with a bunch of unrelated changes (breaking caching, among other things).

My answer is that it'd be bug and it should be found and fixed instead of seeding global RNG with a fixed number to work it around.

I'll just remove this line from here because it does not matter in practice most likely and we cannot seem to agree on this one :)

This comment has been minimized.

Copy link
@lavalamp

lavalamp Jul 1, 2019

Member

FWIW, doc generation which uses random numbers need to be found and fixed, so I support making it actually random.

@@ -39,6 +41,7 @@ import (
)

func main() {
rand.Seed(time.Now().UnixNano())

This comment has been minimized.

Copy link
@tallclair

tallclair Jan 3, 2019

Member

Ditto here.

This comment has been minimized.

Copy link
@tallclair

tallclair Jan 23, 2019

Member

I think this is in the same situation as the docs generator (this generates the man pages)

This comment has been minimized.

Copy link
@ash2k

ash2k Jan 24, 2019

Author Member

Removed.

Show resolved Hide resolved test/e2e/e2e_test.go
Show resolved Hide resolved staging/src/k8s.io/client-go/util/workqueue/main_test.go
@ash2k

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

/test pull-kubernetes-godeps

@ash2k

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

@ash2k ash2k force-pushed the atlassian:rand-cleanup2 branch from 922a0ce to f188f0c Jan 22, 2019

@ash2k

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

/retest

1 similar comment
@ash2k

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

/retest

@ash2k ash2k force-pushed the atlassian:rand-cleanup2 branch from f188f0c to faf04ab Jan 24, 2019

@dims

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@ash2k wanna rebase? i'll throw in labels to try to get this into 1.15 if you are able to.

@dims

This comment has been minimized.

Copy link
Member

commented May 23, 2019

/area dependency

ash2k added some commits Nov 17, 2018

@ash2k ash2k force-pushed the atlassian:rand-cleanup2 branch from eef72c1 to ca38fba May 23, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@ash2k: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws f2dc9b1 link /test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@dims

This comment has been minimized.

Copy link
Member

commented May 24, 2019

/lgtm
/approve

(my approval and re-applying @tallclair 's lgtm)

@dims

This comment has been minimized.

Copy link
Member

commented May 24, 2019

/milestone v1.15

@k8s-ci-robot k8s-ci-robot added this to the v1.15 milestone May 24, 2019

@ash2k

This comment has been minimized.

Copy link
Member Author

commented May 26, 2019

/retest

@soggiest

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Hello! Code Freeze is just about on us. We'll be entering into Freeze tomorrow, May 31st. Is this still planned to land in 1.15?

@liggitt liggitt modified the milestones: v1.15, v1.16 Jun 3, 2019

@timothysc
Copy link
Member

left a comment

@@ -216,15 +216,13 @@ func TestCopyShifting(t *testing.T) {
}

func BenchmarkDelayingQueue_AddAfter(b *testing.B) {
r := rand.New(rand.NewSource(time.Now().Unix()))

This comment has been minimized.

Copy link
@lavalamp

lavalamp Jul 1, 2019

Member

Why remove this?

@@ -702,8 +701,6 @@ func run(s *options.KubeletServer, kubeDeps *kubelet.Dependencies, stopCh <-chan

utilruntime.ReallyCrash = s.ReallyCrashForTesting

rand.Seed(time.Now().UnixNano())

This comment has been minimized.

Copy link
@lavalamp

lavalamp Jul 1, 2019

Member

Where is this seeded now?

@lavalamp

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

lgtm, only question is where is kubelet rand seeded now?

@lavalamp

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, dims, lavalamp, tallclair, timothysc

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 2, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit cdddcf9 into kubernetes:master Jul 2, 2019

23 checks passed

cla/linuxfoundation ash2k authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@ash2k ash2k deleted the atlassian:rand-cleanup2 branch Jul 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.