Add Universally Unique IDentifier (UUID) support by eduard-bagdasaryan · Pull Request #1015 · squid-cache/squid

eduard-bagdasaryan · 2022-04-03T19:38:48Z

These 128-bit UUIDs have a very high chance of being unique across
SMP Squid kids and even across independent Squid instances.

Our UUID creation algorithm uses random number generation as described
in RFC 4122 Section 4.4 (UUID version 4 variant 1).

Since the standard (RFC4122) guarantees uniqueness for these IDs, they can be safely used as various identifiers across all Squid instances and kids within one Squid deployment. The implemented UUID creation algorithm is based on random numbers generation, conforming to RFC 4122 Section 4.4.

eduard-bagdasaryan · 2022-04-03T19:42:52Z

FYI: we plan to utilize these new IDs as Vary identifiers in an upcoming PR.

rousskov

Please also review my PR description edits and adjust further as needed.

src/base/RandomUuid.h

src/base/RandomUuid.cc

by always converting them to network byte order. Also check during deserialization whether the passed bytes represent a valid version 4 variant 1 UUID.

eduard-bagdasaryan

Addressed the review requests (5068a70).

src/base/RandomUuid.cc

src/base/RandomUuid.h

src/base/RandomUuid.cc

We cannot get rid of a local random_device variable because it is passed by reference to mt19937_64 constructor.

There are some concerns that this object may consume some system resources - so it looks better to construct it each time instead of keeping it in a static variable.

rousskov

Thank you for advancing this PR. I believe I found one bug to fix.

I would also like to request the addition of CppUnit tests that check uniqueness (to a very limited extent possible) and serialization/deserialization. Here are (very) rough sketches:

RandomUuid() != RandomUuid(); There is a tiny risk a correct implementation fails this test, but I think we can and should take that risk in "make check" in this exceptional case. Moreover, I would not be against testing that the first 1000 or so generated UUIDs are unique.
const RanomUuid uuid; RanomUuid(uuid.serialized()) == uuid;
ToSBuf(RanomUuid(knownSerializedValue)) == textRepresentationOfTheKnownSerializedValue where knownSerializedValue is a valid hard-coded value you have observed in your tests and textRepresentationOfTheKnownSerializedValue is its hard-coded printed representation (stored as an SBuf string). You will probably need to hard-code knownSerializedValue by giving Serialized constructor 16 integers (one per byte).
try { RanomUuid(allZeros); ... } catch { ... } where allZeros is a hard-coded Serialized array with all bits set to zero; look for the "should be rejected" string in src/tests/testCacheManager.cc for an example of how this "negative" test can be done
try { RanomUuid(allOnes); ... } catch { ... } where allOnes is a hard-coded Serialized array with all bits (not bytes) set to one; another "negative" test

src/base/RandomUuid.cc

src/base/RandomUuid.h

* set correct bits in TimeHiAndVersion * do not call serialize() recursively

following a PR request

This reverts commit 12882c1.

* Serialize all multi-byte fields in network byte order to make identical string representation on different plantforms * Fixed a bug in timeHiAndVersion field * Added more unit tests

eduard-bagdasaryan · 2022-04-11T08:01:13Z

addition of CppUnit tests

Done at 778b1aa.

... to be high enough for bit manipulations in RandomUuid constructor to result in no significant effects on UUID uniqueness: 1) We concatenate two 64-bit random values to produce one 128-bit value, effectively halving std::mt19937_64 generation period (2^219937-1). If my math is right, it would still take ~2^9913 _years_ to generate P/2 UUIDs while generating one UUID per nanosecond! 2) We drop/ignore 6 of those 128 random bits at specific positions (occupied by UUID version/variant metadata) There is currently no Squid Project consensus whether these code manipulations decrease, increase, or leave unchanged any important qualities of the generated UUIDs. There is only consensus that the generated UUIDs will be of high-enough quality.

rousskov

Thank you for addressing my concerns. I hope we can fix the build without significant changes. I plan to clear this for merging after the build is fixed.

@yadij, there have been a few polishing changes since your last comment on this PR, so you may want to take a look before this goes in.

These 128-bit UUIDs have a very high chance of being unique across SMP Squid kids and even across independent Squid instances. Our UUID creation algorithm uses random number generation as described in RFC 4122 Section 4.4 (UUID version 4 variant 1).

rousskov self-requested a review April 4, 2022 13:59

rousskov requested changes Apr 4, 2022

View reviewed changes

src/base/RandomUuid.h Outdated Show resolved Hide resolved

src/base/RandomUuid.cc Show resolved Hide resolved

rousskov added the S-waiting-for-author author action is expected (and usually required) label Apr 4, 2022

Make RandomUuid serialized bytes endianness-agnostic

5068a70

by always converting them to network byte order. Also check during deserialization whether the passed bytes represent a valid version 4 variant 1 UUID.

eduard-bagdasaryan commented Apr 5, 2022

View reviewed changes

src/base/RandomUuid.cc Show resolved Hide resolved

src/base/RandomUuid.h Outdated Show resolved Hide resolved

rousskov self-requested a review April 5, 2022 16:56

rousskov added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Apr 5, 2022

yadij reviewed Apr 5, 2022

View reviewed changes

src/base/RandomUuid.cc Outdated Show resolved Hide resolved

src/base/RandomUuid.cc Outdated Show resolved Hide resolved

src/base/RandomUuid.cc Show resolved Hide resolved

eduard-bagdasaryan added 3 commits April 6, 2022 17:21

Polished, addressing review comments

14e8827

Partly undid 14e8827

5d15b3a

We cannot get rid of a local random_device variable because it is passed by reference to mt19937_64 constructor.

Release std::random_device as soon as possible

f021669

There are some concerns that this object may consume some system resources - so it looks better to construct it each time instead of keeping it in a static variable.

rousskov requested changes Apr 6, 2022

View reviewed changes

src/base/RandomUuid.cc Outdated Show resolved Hide resolved

src/base/RandomUuid.cc Show resolved Hide resolved

src/base/RandomUuid.h Outdated Show resolved Hide resolved

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Apr 6, 2022

eduard-bagdasaryan added 6 commits April 7, 2022 09:55

A better name for a method

482d79d

Fixed a couple of bugs

716566e

* set correct bits in TimeHiAndVersion * do not call serialize() recursively

Add a unit test for base/RandomUuid.cc

1eef4aa

Converted to 'byte-order-agnostic implementation'

12882c1

following a PR request

Revert "Converted to 'byte-order-agnostic implementation'"

ae2f9b5

This reverts commit 12882c1.

Use the standard dash-separated UUID string representation

778b1aa

* Serialize all multi-byte fields in network byte order to make identical string representation on different plantforms * Fixed a bug in timeHiAndVersion field * Added more unit tests

Simplified RandomUuid::serialize()

c475614

eduard-bagdasaryan requested a review from rousskov April 11, 2022 08:38

rousskov removed the S-waiting-for-author author action is expected (and usually required) label Apr 11, 2022

Refactored UUID tests with std::map

7417338

yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Apr 17, 2022

Prohibit implicit conversion from RandomId::Serialized

2e940cf

rousskov approved these changes May 11, 2022

View reviewed changes

eduard-bagdasaryan and others added 2 commits May 12, 2022 14:05

Merge master into SQUID-735-support-standard-UUIDs-2

04c13b5

Fixed 'undefined reference' problem in testRandomBuild

a4dc12b

squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label May 12, 2022

rousskov added S-waiting-for-committer privileged action is expected (and usually required) and removed S-waiting-for-author author action is expected (and usually required) labels May 12, 2022

rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-committer privileged action is expected (and usually required) labels May 15, 2022

squid-anubis closed this May 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Universally Unique IDentifier (UUID) support#1015

Add Universally Unique IDentifier (UUID) support#1015
eduard-bagdasaryan wants to merge 31 commits intosquid-cache:masterfrom
measurement-factory:SQUID-735-support-standard-UUIDs-2

eduard-bagdasaryan commented Apr 3, 2022 •

edited by rousskov

Loading

Uh oh!

eduard-bagdasaryan commented Apr 3, 2022

Uh oh!

rousskov left a comment

Uh oh!

Uh oh!

Uh oh!

eduard-bagdasaryan left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rousskov left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eduard-bagdasaryan commented Apr 11, 2022

Uh oh!

rousskov left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

eduard-bagdasaryan commented Apr 3, 2022 • edited by rousskov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eduard-bagdasaryan commented Apr 3, 2022

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eduard-bagdasaryan commented Apr 11, 2022

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eduard-bagdasaryan commented Apr 3, 2022 •

edited by rousskov

Loading