Fix resource.UniqueId to be properly ordered over multiple runs #15280
Conversation
glasser
force-pushed
the
glasser/unique-id-timestamp-again
branch
from
ba7ffc2
to
59155bb
Compare
glasser
force-pushed
the
glasser/unique-id-timestamp-again
branch
from
59155bb
to
d73f604
Compare
|
Thanks @glasser, This looks good. The hesitation before has been that time.Now() is not monotonic, but we are close to go1.9 which introduces monotonic time, so this will be fine going forward. |
helper/resource/id.go
Outdated
| // IDs every 10th of a millisecond, which ought to be good enough. | ||
| if timestamp != lastTimestamp { | ||
| lastTimestamp = timestamp | ||
| idCounter = 0 |
There was a problem hiding this comment.
I think we can just skip resetting this altogether, and just let it roll over if it ever gets that far. You could seed it with a pseudo random number of you want to keep it visually interesting, but that's not required ;)
While it doesn't hurt in most cases, until go1.9 there's actually a good chance that a group of resources made in quick succession could have the timestamp bounce back again. Not to mention Windows only has a 15ms resolution.
|
To make sure I follow - your suggestion is that we simplify the code by not
resetting the counter because 4 billion isn't going to overflow anyway?
BTW I don't think 1.9 monotonic time is relevant since I think it is only
used for duration calculation, not formatting.
|
|
Yes. I'm essentially saying it technically doesn't matter if it overflows (not that you could even run a plan that large) because if it does, we would be on a new timestamp by then, or else this wouldn't work in the first place. ;) In go1.9 |
|
Not that it's relevant here, but unless I'm completely misreading the
design doc, the new monotonic time is only used for calculating durations
(t.Sub and friends), not formatting.
|
|
Also I don't think your logic is quite right. Yes, we probably are never
going to generate 4 billion IDs and roll over anyway, but if we do, there's
no reason to believe that UINT32_MAX and 0 won't happen within the same
timestamp.
That said I don't actually know what the unit test failures that motivated
your last change were so I'm not sure why strict increasing (as opposed to
the general time indication I care about) matters.
|
|
Oh, I think you might be right - I haven't looked at that doc since it was published, but it excludes Format as changing. I'm not sure if that's because it doesn't take the skew into consideration, or there's just no measurable impact on the output since it's not directly in comparison with something. I'll check it out later. Regardless, not resetting the counter should cover the situation. If it rolls over in the same timestamp, the current code wouldn't work either, because it's only reset when the timestamp changes. The change was implemented because I was getting identical IDs on windows and out-of order IDs in tests. |
|
Oh, I see what you mean about the rollover happening in the same timestamp now, it wouldn't collide, but it would be out of order. While hitting that is only hypothetical, if we're going to reset, maybe once/sec? |
|
That's wacky that you would get identical IDs with the random bit!
I could change it to reset every second instead of every time the timestamp
changes though arguably that would make the code even more complex?
Hopefully our unit tests don't actually allocate 4 billion IDs anyway
though, right?
|
|
Nope, I don't think we will ever allocate that many IDs in the tests ;) Which is why I think it's OK to never reset it. If it were possible to have it roll over during execution, and it happened within a single timestamp it's not going to cause a duplicate ID, so it's only a minor ordering issue at that point. |
glasser
force-pushed
the
glasser/unique-id-timestamp-again
branch
from
d73f604
to
251cfd3
Compare
|
Updated as requested. |
glasser
force-pushed
the
glasser/unique-id-timestamp-again
branch
from
251cfd3
to
f62dd7f
Compare
helper/resource/id.go
Outdated
| // string. The max possible hex value here with 12 random bytes is | ||
| // "01000000000000000000000000", so there's no chance of rollover during | ||
| // operation. | ||
| // idCounter is a monotonic counter for generating ordered unique ids. We reset |
There was a problem hiding this comment.
just need to remove the comment about resetting here
radeksimko
added
bug
core
waiting-response
The timestamp prefix added in hashicorp#8249 was removed in hashicorp#10152 to ensure that returned IDs really are properly ordered. However, this meant that IDs were no longer ordered over multiple invocations of terraform, which was the main motivation for adding the timestamp in the first place. This commit does a hybrid: timestamp-plus-incrementing-counter instead of just incrementing counter or timestamp-plus-random.
|
Oops, done. |
glasser
force-pushed
the
glasser/unique-id-timestamp-again
branch
from
f62dd7f
to
0a1f915
Compare
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
The timestamp prefix added in #8249 was removed in #10152 to ensure that
returned IDs really are properly ordered. However, this meant that IDs were no
longer ordered over multiple invocations of terraform, which was the main
motivation for adding the timestamp in the first place. This commit does a
hybrid: timestamp-plus-incrementing-counter instead of just incrementing counter
or timestamp-plus-random.