CreateVolume() clone flow might create volume from wrong snapshot #55
Conversation
WalkthroughWalkthroughThe changes involve modifying the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (1)
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
|
||
| // Appending current time-unit and randome number to the snapshot name to make it unique. | ||
| // e.g snapshot-<volume-name>-<current-time> <31-bit non-negative-int> snapshot-volName-YYYYMMDDHHMMSS-1279846997 | ||
| formattedTime := time.Now().Format("20060102150405") | ||
| snapName := "snapshot-" + req.Name + "-" + formattedTime + "-" + strconv.Itoa(int(rand.Int31())) |
There was a problem hiding this comment.
Enhancement of Snapshot Name Uniqueness
The addition of a timestamp and a random integer to the snapshot name is a robust solution to ensure uniqueness. This change effectively addresses the issue of potential collisions in snapshot names which could lead to creating volumes from incorrect snapshots.
Suggestion for Improvement:
Consider extracting the snapshot name construction into a separate function for better readability and maintainability. This would also make unit testing easier.
+ func generateSnapshotName(baseName string) string {
+ formattedTime := time.Now().Format("20060102150405")
+ return "snapshot-" + baseName + "-" + formattedTime + "-" + strconv.Itoa(int(rand.Int31()))
+ }
+
snapName := generateSnapshotName(req.Name)Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
see comments in ticket, lets see if we can find/address the skup on the delete
| // Appending current time-unit and randome number to the snapshot name to make it unique. | ||
| // e.g snapshot-<volume-name>-<current-time> <31-bit non-negative-int> snapshot-volName-YYYYMMDDHHMMSS-1279846997 | ||
| formattedTime := time.Now().Format("20060102150405") | ||
| snapName := "snapshot-" + req.Name + "-" + formattedTime + "-" + strconv.Itoa(int(rand.Int31())) |
There was a problem hiding this comment.
UUIDv7 is exactly doing this: https://uuid7.com/ and the uuid library you are using is already capable of generating them: https://github.com/google/uuid/blob/master/version7.go
In my POV this would make the code easier to understand and also follow a well known and documented standard.
There was a problem hiding this comment.
I see your point.
Appending formatted current time and 31-bit non-negative number to the intermediary snapshot name to make it unique. This change considerably increases the avoidance of this possible bug occurrence.
Issue : LBM1-19398
Summary by CodeRabbit
New Features
Bug Fixes