CP-49135: speed up UUID generation #6018
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We generate uuids frequently, e.g. when creating sessions. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
edwintorok
force-pushed
the
private/edvint/prng
branch
from
e13e26b to
ffc4698
Compare
|
Back to draft, this seems to reveal a bug somewhere else in the codebase (someone closed the wrong file descriptor and ended up closing the urandom one...) |
edwintorok
force-pushed
the
private/edvint/prng
branch
from
ffc4698 to
2eeccd1
Compare
psafont
reviewed
Sep 26, 2024
Ah crowbar runs And forkexecd isn't happy about the extra FD either (fixed). TODO: split up the last commit that adds type safety for session into its own commit. |
edwintorok
force-pushed
the
private/edvint/prng
branch
3 times, most recently
from
472bc3e to
8e605e3
Compare
|
Thanks, It's interesting to clearly see the types weren't used properly before, and now they are being tracked as they should |
Reduces the amount of file descriptors kept open,
and avoids 2 syscalls when creating sessions.
Creating session uuids/refs is ~3.8x faster now:
Before:
```
uuidx creation/Uuidx.make_uuid_urnd (ns):
{ monotonic-clock per run = 5224.741331 (confidence: 5263.321869 to 5187.101530);
r² = Some 0.997606 }
```
After:
```
uuidx creation/Uuidx.make_uuid_urnd (ns):
{ monotonic-clock per run = 1369.962560 (confidence: 1378.331674 to 1362.496608);
r² = Some 0.998731 }
```
Adjust forkexecd test, where the called program is an OCaml program linked with Uuidx,
and thus has 1 extra FD open (that it didn't receive/inherit from the parent).
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
This will allow restricting which refs can be created from strings, without breaking RPC (the RPC code inside ref.ml will have access to the string conversion functions, but external users can get a more restricted interface that e.g. wouldn't allow constructing secret Refs from arbitrary strings) No functional change. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Introduce an `all` type that contains all possible type parameters. This'll ensure that spelling is consistent, e.g. we've got both VM and Vm, and both Session and session currently. It also prepares for using separate types for secret and non-secret refs/uuids. Fix type signatures: * some signatures just specified 'a Ref.t, these must now be explicit * some signatures specified `API.ref_task Uuidx.t`, instead of `task Uuidx.t`, this is fixed * spelling of VM, Vm, Session, session, Console, console, Crashdump, crashdump, User, user, PBD, pbd is made consistent * test code must chose a concrete type, cannot leave the parameter abstract, use Generic No functional change. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Split Uuidx.all into without_secret and secret parts. This ensures that all sessions are created only with Uuidx.make_uuid_rnd which is guaranteed to use a CSPRNG. Currently Uuidx.make uses the same generator, but that will change in a future commit. Introduce Ref.make_secret, Ref.of_secret_string, and change the other function's signatures so that none of them return a secret (except Ref.null). It is important that uuids/refs that are used as a secret/auth token are generated in a cryptographically safe way, if a user can guess them then they can impersonate an authenticated user. Ref.of_secret_string bypasses these checks, but it is only used in a limited set of places, where we receive session ids from an external source (HTTP requests, or CLI parameters), or test code. No functional change. Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Not yet enabled by default, behind xapi.conf feature flag.
Will be enabled by default after more testing/auditing of the code.
Pool secret and Session are the only secret uuids/refs,
for everything else avoid the overhead of reading from /dev/urandom every time,
and use an internal PRNG.
Both the opaqueref and UUID of a Session is a secret, because you can retrieve one if you can guess the other,
so both must be created using CSPRNG.
This is about 15x faster than using /dev/urandom:
```
╭───────────────────────────────────────┬───────────────────────────┬───────────────────────────┬───────────────────────────╮
│name │ major-allocated │ minor-allocated │ monotonic-clock │
├───────────────────────────────────────┼───────────────────────────┼───────────────────────────┼───────────────────────────┤
│ uuidx creation/Uuidx.make │ 0.0001 mjw/run│ 8.0001 mnw/run│ 88.1485 ns/run│
│ uuidx creation/Uuidx.make_uuid_urnd │ 0.0000 mjw/run│ 10.0014 mnw/run│ 1369.9626 ns/run│
╰───────────────────────────────────────┴───────────────────────────┴───────────────────────────┴───────────────────────────╯
```
Uses the type system to ensure that we cannot create Refs or Uuids of type 'session' with the wrong function.
Example type error from a testcase that has ben fixed:
```
File "ocaml/tests/test_client.ml", line 18, characters 47-57:
18 | Test_common.make_session ~__context ~ref:session_id
^^^^^^^^^^
Error: This expression has type [< Uuidx.not_session ] Ref.t
but an expression was expected of type [ `session ] Ref.t
Type [< Uuidx.not_session ] = [< `session of [ `use_make_uuid_urnd ] ]
is not compatible with type [ `session ]
Types for tag `session are incompatible
```
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
edwintorok
force-pushed
the
private/edvint/prng
branch
from
8e605e3 to
6635a00
Compare
|
I've split the type safety changes into their own commits, and also introduced a 'Ref.of_secret_string' to more clearly separate the two. Of course the type system can't tell us if we use secret UUIDs anywhere other than Session, it is up to us to tell it the list of classes that have secret uuids. |
psafont
approved these changes
Sep 27, 2024
robhoes
reviewed
Sep 30, 2024
| x);" | ||
| ; "end" | ||
| ] | ||
| ; ["module Ref = Ref"] |
There was a problem hiding this comment.
Is there still any point to this line, as it is not renaming anything?
There was a problem hiding this comment.
I think I got a build failure without it, we could probably find what generates Foo.Ref and make it generate just Ref instead.
robhoes
approved these changes
Sep 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
2 optimizations here:
The latter is not yet enabled by default, needs more testing/auditing on whether we have any other secret opaquerefs/UUIDs in the codebase, but the 1st one is enabled by default (we still use
/dev/urandomfor generating the UUIDs, we just don't keep closing and reopening it).