Make random value generation and use more reliable#1065
Make random value generation and use more reliable#1065rhatdan merged 7 commits intocontainers:mainfrom
Conversation
There are many more instances of this in the tests; this PR doesn’t fix that. |
|
Another place where random values are assumed never to conflict is the So, this PR is now trying to make it consistently unique. That code complexity is bordering on not worth it — feel free to tell me to drop this part. The one possibly redeeming quality of it is that we now record a timestamp and PID of the lock owner into the lock state, which might help with later debugging. |
|
I think we need a PR opened against Podman and maybe buildah to make sure there are no regressions. |
|
@rhatdan tests aren't happy and you might need a rebase. |
|
@TomSweeneyRedHat You meant @mtrmac or commented in the wrong PR. |
|
@mtrmac Any idea why this is blowing up? |
|
Not yet, looking… |
|
LGTM |
|
@mtrmac Is this still worth doing? |
|
I think so; but I’m not comfortable with merging this without someone much more familiar with c/storage reviewing it and making that call. |
|
LGTM |
|
Yes, this all looks fine. |
True. The previous version used crypto/rand for all of the 64 bytes, so this is at least a reduction. I can make the random part shorter (padding with zeroes), but then we get to discuss how short exactly :) Probably at least 64 bits, to be commensurate with the PID namespace, and to account for the birthday paradox. 128 bits? 256? |
The global math/rand RNG is a shared resource, and various packages like to seed it, in particular usually just using time.Now().UnixNano(), making our attempts to use a better seed unreliable. So, use a private RNG instead. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Include the image ID (name?) being searched, not the ID of the container that would use the image. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
There isn't any easy-to-see reason to use the same ID for the two objects, and not needing to known the container ID at that point will help generating it correctly without breaking the ContainerStore abstraction. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
If callers to Store don't provide container/image/layer IDs, Store generates random values without checking for collisions. Meanwhile, the underlying per-object stores do check for collisions, which is more reliable, and nothing in Store needs to know the values in advance (any more), so just delete the code that generates the random values, exposing the better per-object ID generation code. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to have only one place that generates the value, and make it easier to change how it is constructed. For now, this is just a simple function call forwarding, but we'll change that shortly. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... simplifying the code. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... by including the time, PID, and a per-process counter. Pad the rest with rando values for continuity. This feels a bit like overkill, OTOH adding the PID might be useful for debugging, so there's that. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
LGTM |
|
OTOH moby/moby removed the non-crypto functions from the package: moby/moby#39336 |
|
Want to open a PR to make the same change here? |
Sure: #1337 |
Backport the commits in containers#1065 to address https://bugzilla.redhat.com/show_bug.cgi?id=2166429 Make sure the values returned by newLastWriterID() are unique ... by including the time, PID, and a per-process counter. Pad the rest with rando values for continuity. This feels a bit like overkill, OTOH adding the PID might be useful for debugging, so there's that. Signed-off-by: Miloslav Trmač <mitr@redhat.com> Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
5 participants
pkg/stringidso that it is unaffected by anyone else thinking thatmath/rand.Seed()guarantees anything in a large codebase.Compare the plight at containers/podman#12155 .
getMaxSizeFromImagepart carefully; it’s quite possible that the use of the container ID there is intentional, I wouldn’t know.It might make sense to merge this only after various other users of
math/randare modified/fixed, in case they unknowingly depend on this seeding the global RNG with unpredictable values (vs. the Go standard library ofSeed(1)).