fix: busy system snapshot detection in physical mode (#605)

Author: agneum

engine/internal/provision/thinclones/zfs/zfs.go

@@ -508,7 +508,7 @@ func (m *Manager) CleanupSnapshots(retentionLimit int) ([]string, error) {
 	busySnapshots := m.getBusySnapshotList(clonesOutput)
 
 	cleanupCmd := fmt.Sprintf(
-		"zfs list -t snapshot -H -o name -s %s -s creation -r %s | grep -v clone | head -n -%d %s"+
+		"zfs list -t snapshot -H -o name -s %s -s creation -r %s | grep -v clone | grep _pre$ | head -n -%d %s"+
 			"| xargs -n1 --no-run-if-empty zfs destroy -R ",
 		dataStateAtLabel, m.config.Pool.Name, retentionLimit, excludeBusySnapshots(busySnapshots))
 
@@ -527,10 +527,10 @@ func (m *Manager) CleanupSnapshots(retentionLimit int) ([]string, error) {
 }
 
 func (m *Manager) getBusySnapshotList(clonesOutput string) []string {
-	systemClones, userClones := make(map[string]string), make(map[string]struct{})
-	branchingSnapshots := []string{}
+	systemClones := make(map[string]string)
+	branchingSnapshotDatasets := []string{}
 
-	userClonePrefix := m.config.Pool.Name + "/"
+	systemDatasetPrefix := fmt.Sprintf("%s/%s/%s/clone_pre_", m.config.Pool.Name, branching.BranchDir, branching.DefaultBranch)
 
 	for _, line := range strings.Split(clonesOutput, "\n") {
 		cloneLine := strings.FieldsFunc(line, unicode.IsSpace)
@@ -539,39 +539,32 @@ func (m *Manager) getBusySnapshotList(clonesOutput string) []string {
 			continue
 		}
 
-		// Keep the user-defined snapshot and the snapshot it is based on.
-		if strings.HasPrefix(cloneLine[0], userClonePrefix+"branch") {
-			branchingSnapshots = append(branchingSnapshots, cloneLine[0], cloneLine[1])
-
+		// Make dataset-snapshot map for system snapshots.
+		if strings.HasPrefix(cloneLine[0], systemDatasetPrefix) {
+			systemClones[cloneLine[0]] = cloneLine[1]
 			continue
 		}
 
-		//nolint:lll
-		if cloneName, _ := strings.CutPrefix(cloneLine[0], userClonePrefix); strings.HasPrefix(cloneLine[0], userClonePrefix) && !strings.Contains(cloneName, m.config.PreSnapshotSuffix) {
-			origin := cloneLine[1]
-
-			if idx := strings.Index(origin, "@"); idx != -1 {
-				origin = origin[:idx]
+		// Keep snapshots related to the user-defined datasets.
+		if strings.HasPrefix(cloneLine[1], systemDatasetPrefix) {
+			systemDataset, _, found := strings.Cut(cloneLine[1], "@")
+			if found {
+				branchingSnapshotDatasets = append(branchingSnapshotDatasets, systemDataset)
 			}
 
-			userClones[origin] = struct{}{}
-
 			continue
 		}
-
-		systemClones[cloneLine[0]] = cloneLine[1]
 	}
 
-	busySnapshots := make([]string, 0, len(userClones))
+	busySnapshots := make([]string, 0, len(branchingSnapshotDatasets))
 
-	for userClone := range userClones {
-		if systemClones[userClone] != "" {
-			busySnapshots = append(busySnapshots, systemClones[userClone])
+	for _, busyDataset := range branchingSnapshotDatasets {
+		busySnapshot, ok := systemClones[busyDataset]
+		if ok {
+			busySnapshots = append(busySnapshots, busySnapshot)
 		}
 	}
 
-	busySnapshots = append(busySnapshots, branchingSnapshots...)
-
 	return busySnapshots
 }
 

engine/internal/provision/thinclones/zfs/zfs_test.go

@@ -116,29 +116,34 @@ func TestFailedListClones(t *testing.T) {
 
 func TestBusySnapshotList(t *testing.T) {
 	const preSnapshotSuffix = "_pre"
-	m := Manager{config: Config{Pool: &resources.Pool{Name: "dblab_pool"}, PreSnapshotSuffix: preSnapshotSuffix}}
-
-	out := `dblab_pool	-
-	dblab_pool/clone_pre_20210127105215	dblab_pool@snapshot_20210127105215_pre
-	dblab_pool/clone_pre_20210127113000	dblab_pool@snapshot_20210127113000_pre
-	dblab_pool/clone_pre_20210127120000	dblab_pool@snapshot_20210127120000_pre
-	dblab_pool/clone_pre_20210127123000	dblab_pool@snapshot_20210127123000_pre
-	dblab_pool/clone_pre_20210127130000	dblab_pool@snapshot_20210127130000_pre
-	dblab_pool/clone_pre_20210127133000	dblab_pool@snapshot_20210127133000_pre
-	dblab_pool/clone_pre_20210127140000	dblab_pool@snapshot_20210127140000_pre
-	dblab_pool/cls19p20l4rc73bc2v9g	dblab_pool/clone_pre_20210127133000@snapshot_20210127133008
-	dblab_pool/cls19p20l4rc73bc2v9g	dblab_pool/clone_pre_20210127123000@snapshot_20210127133008
-	dblab_pool/branch/main/20241121094823 dblab_pool@snapshot_20241121094523
-	`
-	expected := []string{"dblab_pool@snapshot_20210127133000_pre", "dblab_pool@snapshot_20210127123000_pre", "dblab_pool/branch/main/20241121094823",
-		"dblab_pool@snapshot_20241121094523"}
+	m := Manager{config: Config{Pool: &resources.Pool{Name: "test_dblab_pool"}, PreSnapshotSuffix: preSnapshotSuffix}}
+
+	out := `test_dblab_pool	-
+test_dblab_pool/branch	-
+test_dblab_pool/branch/main	-
+test_dblab_pool/branch/main/clone_pre_20250403061908	-
+test_dblab_pool/branch/main/clone_pre_20250403061908/r0	test_dblab_pool@snapshot_20250403061908_pre
+test_dblab_pool/branch/main/clone_pre_20250403085500	-
+test_dblab_pool/branch/main/clone_pre_20250403085500/r0	test_dblab_pool@snapshot_20250403085500_pre
+test_dblab_pool/branch/main/clone_pre_20250403090000	-
+test_dblab_pool/branch/main/clone_pre_20250403090000/r0	test_dblab_pool@snapshot_20250403090000_pre
+test_dblab_pool/branch/main/clone_pre_20250403090500	-
+test_dblab_pool/branch/main/clone_pre_20250403090500/r0	test_dblab_pool@snapshot_20250403090500_pre
+test_dblab_pool/branch/main/cvn2j50n9i6s73as3k9g	-
+test_dblab_pool/branch/main/cvn2j50n9i6s73as3k9g/r0	test_dblab_pool/branch/main/clone_pre_20250403061908/r0@snapshot_20250403061908
+test_dblab_pool/branch/main/cvn2kdon9i6s73as3ka0	-
+test_dblab_pool/branch/main/cvn2kdon9i6s73as3ka0/r0	test_dblab_pool/branch/new001@20250403062641
+test_dblab_pool/branch/new001	test_dblab_pool/branch/main/cvn2j50n9i6s73as3k9g/r0@20250403062503
+test_dblab_pool/branch/new001/cvn4n38n9i6s73as3kag	-
+test_dblab_pool/branch/new001/cvn4n38n9i6s73as3kag/r0	test_dblab_pool/branch/new001@20250403062641
+`
+	expected := []string{
+		"test_dblab_pool@snapshot_20250403061908_pre",
+	}
 
 	list := m.getBusySnapshotList(out)
-	require.Equal(t, 4, len(list))
-	assert.Contains(t, list, expected[0])
-	assert.Contains(t, list, expected[1])
-	assert.Contains(t, list, expected[2])
-	assert.Contains(t, list, expected[3])
+	require.Len(t, list, len(expected))
+	assert.ElementsMatch(t, list, expected)
 }
 
 func TestExcludingBusySnapshots(t *testing.T) {