-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
pause scope: don't use the global math/rand RNG #12593
Conversation
Otherwise, we'll always get the same sequence of random numbers which may lead to conflicts. Also bump the number of maximum attempts to 10 instead of 3. [NO NEW TESTS NEEDED] as I cannot enforce random number collisions. Existing tests should continue be green and flake slightly less. Signed-off-by: Valentin Rothberg <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg 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 |
LGTM |
@containers/podman-maintainers PTAL |
/lgtm |
/hold cancel |
Sorry but that looks dangerous to me, reinitializing the random seed every time the function is called can also cause conflicts. When two callers call the function at the exact same time they will end up with the same random value. |
We recently had updates to use a time-based seed as in this PR, so I used it. What's the benefit of |
You do not have to seed crypt/rand it uses getrandom(2), see https://pkg.go.dev/crypto/rand |
Interesting! Mind opening a PR? |
Sure |
PR #12610 |
Otherwise, we'll always get the same sequence of random numbers which
may lead to conflicts. Also bump the number of maximum attempts to 10
instead of 3.
[NO NEW TESTS NEEDED] as I cannot enforce random number collisions.
Existing tests should continue be green and flake slightly less.
Signed-off-by: Valentin Rothberg [email protected]