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

Simplify rand seeding #69670

Merged
merged 3 commits into from Oct 15, 2018

Conversation

@ash2k
Member

ash2k commented Oct 11, 2018

What this PR does / why we need it:
Simplify/cleanup random number generator seeding and usage. Please see descriptions of individual commits.

Release note:

NONE

/kind cleanup
/sig api-machinery

ash2k added some commits Oct 11, 2018

Simplify random seed initialization
There is no need to set the time zone as the result does not
depend on it
Remove unnecessary random re-seeding
Package k8s.io/apimachinery/pkg/util/rand seeds the random based on time
during the package initialization, so no need to re-seed it.
@wgliang

This comment has been minimized.

Show comment
Hide comment
@wgliang

wgliang Oct 11, 2018

Member

/retest
/lgtm

Member

wgliang commented Oct 11, 2018

/retest
/lgtm

@jennybuckley

This comment has been minimized.

Show comment
Hide comment
@jennybuckley
Contributor

jennybuckley commented Oct 11, 2018

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Oct 11, 2018

Contributor

@jennybuckley: GitHub didn't allow me to request PR reviews from the following users: logicalhan.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @logicalhan

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.

Contributor

k8s-ci-robot commented Oct 11, 2018

@jennybuckley: GitHub didn't allow me to request PR reviews from the following users: logicalhan.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @logicalhan

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.

@ash2k

This comment has been minimized.

Show comment
Hide comment
@ash2k

ash2k Oct 12, 2018

Member

/retest

Member

ash2k commented Oct 12, 2018

/retest

@ash2k

This comment has been minimized.

Show comment
Hide comment
@ash2k

ash2k Oct 12, 2018

Member

/assign mikedanese wojtek-t liggitt saad-ali

Member

ash2k commented Oct 12, 2018

/assign mikedanese wojtek-t liggitt saad-ali

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt

liggitt Oct 15, 2018

Member

/approve

Member

liggitt commented Oct 15, 2018

/approve

@dims

dims approved these changes Oct 15, 2018

/lgtm
/approve

@mikedanese

It seems like we could do a little better initializing rand consistently in e2e tests but this all looks good.

/lgtm
/approve

@@ -68,7 +67,6 @@ func (util *FlockerUtil) CreateVolume(c *flockerVolumeProvisioner) (datasetUUID
}
// select random node
rand.Seed(time.Now().UTC().UnixNano())

This comment has been minimized.

@mikedanese

mikedanese Oct 15, 2018

Member

...wat...

@mikedanese

mikedanese Oct 15, 2018

Member

...wat...

This comment has been minimized.

@logicalhan

logicalhan Oct 15, 2018

Contributor

i believe that file uses the api-machinery rand, so this line should be able to be safely removed.

@logicalhan

logicalhan Oct 15, 2018

Contributor

i believe that file uses the api-machinery rand, so this line should be able to be safely removed.

This comment has been minimized.

@mikedanese

mikedanese Oct 15, 2018

Member

I agree it is safe to remove. I'm more confused how it existed in the first place. We are reseeding a global rand every call to this function. I'm surprised this wasn't more problematic.

@mikedanese

mikedanese Oct 15, 2018

Member

I agree it is safe to remove. I'm more confused how it existed in the first place. We are reseeding a global rand every call to this function. I'm surprised this wasn't more problematic.

This comment has been minimized.

@ash2k

ash2k Oct 15, 2018

Member

I think I saw two more weird places like this, but they were in vendor. One in aws-go-sdk and another one in some azure thing IIRC.

@ash2k

ash2k Oct 15, 2018

Member

I think I saw two more weird places like this, but they were in vendor. One in aws-go-sdk and another one in some azure thing IIRC.

@logicalhan

This comment has been minimized.

Show comment
Hide comment
@logicalhan

logicalhan Oct 15, 2018

Contributor

/lgtm

Contributor

logicalhan commented Oct 15, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Oct 15, 2018

Contributor

@logicalhan: changing LGTM is restricted to assignees, and only kubernetes/kubernetes repo collaborators may be assigned issues.

In response to this:

/lgtm

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.

Contributor

k8s-ci-robot commented Oct 15, 2018

@logicalhan: changing LGTM is restricted to assignees, and only kubernetes/kubernetes repo collaborators may be assigned issues.

In response to this:

/lgtm

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.

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t Oct 15, 2018

Member

/lgtm

Member

wojtek-t commented Oct 15, 2018

/lgtm

@wojtek-t

This comment has been minimized.

Show comment
Hide comment
@wojtek-t

wojtek-t Oct 15, 2018

Member

/approve

Member

wojtek-t commented Oct 15, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Oct 15, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, dims, liggitt, mikedanese, wojtek-t

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

Contributor

k8s-ci-robot commented Oct 15, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, dims, liggitt, mikedanese, wojtek-t

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

@k8s-ci-robot k8s-ci-robot merged commit d54e0fc into kubernetes:master Oct 15, 2018

18 checks passed

cla/linuxfoundation ash2k authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Job succeeded.
Details
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@ash2k ash2k deleted the atlassian:simplify-rand-seed branch Oct 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment