Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CreateVolume() clone flow might create volume from wrong snapshot #55

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

CreateVolume() clone flow might create volume from wrong snapshot #55

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c96666e
This change will enhance snapshot's name uniqueness to avoid creating…
rahman-lb Sep 4, 2024
d49eb27
Merge branch 'master' into feature/fix-LBM1-19398
rahman-lb Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion pkg/driver/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ package driver
import (
"context"
"fmt"
"math/rand"
"strconv"
"strings"
"time"

"github.com/container-storage-interface/spec/lib/go/csi"
guuid "github.com/google/uuid"
Expand Down Expand Up @@ -452,7 +455,11 @@ func (d *Driver) doCreateVolume( //revive:disable-line:unused-receiver
// snapshot deletion strategy this will also prevent creation of
// identically named clone volumes in the future due to snapshot name
// collisions (see note below on cleanup).
snapName := "snapshot-" + req.Name

// 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()))
Comment on lines +458 to +462
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments in ticket, lets see if we can find/address the skup on the delete

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point.

log.Infof("auto-creating intermediate snapshot '%s' to clone from a volume", snapName)
snap, err := doCreateSnapshot(ctx, log, clnt, snapName, *srcVid,
"auto-snap for clone, by: LB CSI")
Expand Down