fix(devbox): move unmout operation to transaction

[Zllinc]

No description provided.

[dinoallo]

The comments are outdated

[Copilot]

Pull request overview

This PR modifies the findMountPointByDevice function to return a slice of mount points instead of a single mount point string, enabling support for devices mounted to multiple directories. The unmount logic in cleanupDirectories is updated to iterate through and unmount all mount points for each logical volume.

Key changes:

• Modified findMountPointByDevice to return []string instead of string to support multiple mount points per device
• Updated cleanup logic to unmount all mount points for each LV being cleaned up
• Added comprehensive test coverage for multiple mount point scenarios

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
snapshots/devbox/devbox.go	Modified findMountPointByDevice function signature to return slice of mount points; updated cleanup logic to iterate through all mount points when unmounting
snapshots/devbox/devbox_test.go	Updated all test cases to handle slice return type; added two new tests for multiple mount point scenarios (TestFindMountPointByDevice_MultipleMountPoints and TestCleanupUnmountAllMountPoints)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The function comment is outdated. It states "Returns the mount point path if found, empty string if not mounted" but the function now returns a slice of strings (multiple mount points), not a single string. The comment should be updated to reflect that it returns all mount points for the device, or an empty slice if not mounted.

Suggested change

	// findMountPointByDevice finds the mount point for a given device path by reading /proc/mounts
	// Returns the mount point path if found, empty string if not mounted, and error on failure
	// findMountPointByDevice finds the mount point(s) for a given device path by reading /proc/mounts
	// Returns a slice of mount point paths if found, an empty slice if not mounted, and an error on failure

[Copilot]

The comment states that unmount operations are performed "outside of the transaction to avoid blocking", but the unmount logic (lines 513-530) is actually executed inside the transaction that starts at line 501 (WithTransaction call). This contradicts the comment and could potentially block the transaction. If the intention is to perform unmounts outside the transaction, this code should be moved after the transaction completes (after line 535). Alternatively, if unmounts should happen inside the transaction (as the PR title suggests), the comment should be updated to reflect this.

Suggested change

	// Unmount any mounted LVs
	for _, lvName := range removedLvNames {
	devicePath := fmt.Sprintf("/dev/%s/%s", o.lvmVgName, lvName)
	mountPoints, err := findMountPointByDevice(devicePath)
	if err != nil {
	log.G(ctx).WithError(err).WithField("lvName", lvName).WithField("devicePath", devicePath).
	Warn("Cleanup: failed to find mount point for LV, continuing")
	continue
	}
	for _, mountPoint := range mountPoints {
	if err := o.unmountLvm(ctx, mountPoint); err != nil {
	log.G(ctx).WithError(err).WithField("lvName", lvName).WithField("mountPoint", mountPoint).
	Warn("Cleanup: failed to unmount LV, will retry on next cleanup")
	// Continue to try to unmount other mount points
	} else {
	log.G(ctx).Infof("Cleanup: successfully unmounted LV %s from %s", lvName, mountPoint)
	}
	}
	}
	return nil
	}); err != nil {
	return nil, nil, err
	}
	return nil
	}); err != nil {
	return nil, nil, err
	}
	// Unmount any mounted LVs outside of the transaction to avoid blocking it.
	for _, lvName := range removedLvNames {
	devicePath := fmt.Sprintf("/dev/%s/%s", o.lvmVgName, lvName)
	mountPoints, err := findMountPointByDevice(devicePath)
	if err != nil {
	log.G(ctx).WithError(err).WithField("lvName", lvName).WithField("devicePath", devicePath).
	Warn("Cleanup: failed to find mount point for LV, continuing")
	continue
	}
	for _, mountPoint := range mountPoints {
	if err := o.unmountLvm(ctx, mountPoint); err != nil {
	log.G(ctx).WithError(err).WithField("lvName", lvName).WithField("mountPoint", mountPoint).
	Warn("Cleanup: failed to unmount LV, will retry on next cleanup")
	// Continue to try to unmount other mount points
	} else {
	log.G(ctx).Infof("Cleanup: successfully unmounted LV %s from %s", lvName, mountPoint)
	}
	}
	}