Cleanup code and improve error messages.

This commit does the following:
- Reorder struct variables and move up cleanup.
- Fixes OSD cleanup.
- Skips symlinking WAL and DB devices since Ceph does it for us.
- Properly order script arguments.
- Factor out stable disk path management.

Signed-off-by: Luciano Lo Giudice <[email protected]>

microceph/api/disks.go

@@ -49,6 +49,7 @@ func cmdDisksPost(s *state.State, r *http.Request) response.Response {
 	var req types.DisksPost
 	var wal *types.DiskParameter
 	var db *types.DiskParameter
+	var data types.DiskParameter
 
 	logger.Debugf("cmdDisksPost: %v", req)
 	err := json.NewDecoder(r.Body).Decode(&req)
@@ -59,12 +60,12 @@ func cmdDisksPost(s *state.State, r *http.Request) response.Response {
 	mu.Lock()
 	defer mu.Unlock()
 
-	data := types.DiskParameter{req.Path, req.Wipe, req.Encrypt}
+	data = types.DiskParameter{req.Path, req.Encrypt, req.Wipe}
 	if req.WALDev != nil {
-		wal = &types.DiskParameter{*req.WALDev, req.WALWipe, req.WALEncrypt}
+		wal = &types.DiskParameter{*req.WALDev, req.WALEncrypt, req.WALWipe}
 	}
 	if req.DBDev != nil {
-		db = &types.DiskParameter{*req.DBDev, req.DBWipe, req.DBEncrypt}
+		db = &types.DiskParameter{*req.DBDev, req.DBEncrypt, req.DBWipe}
 	}
 
 	err = ceph.AddOSD(s, data, wal, db)

microceph/ceph/osd.go

@@ -17,6 +17,7 @@ import (
 	"syscall"
 	"time"
 
+	"github.com/canonical/lxd/shared/api"
 	"github.com/canonical/lxd/shared/logger"
 	"github.com/canonical/microceph/microceph/common"
 
@@ -98,7 +99,12 @@ func prepareDisk(disk *types.DiskParameter, suffix string, osdPath string, osdID
 		}
 		disk.Path = path
 	}
-	return os.Symlink(disk.Path, filepath.Join(osdPath, "block"+suffix))
+	// Only the data device needs to be symlinked (suffix != "").
+	// Other devices (WAL and DB) are automatically handled by Ceph itself.
+	if suffix != "" {
+		return nil
+	}
+	return os.Symlink(disk.Path, filepath.Join(osdPath, "block"))
 }
 
 // setupEncryptedOSD sets up an encrypted OSD on the given disk.
@@ -286,71 +292,74 @@ func updateFailureDomain(s *state.State) error {
 	return nil
 }
 
-// AddOSD adds an OSD to the cluster, given a device path and a flag for wiping
-func AddOSD(s *state.State, data types.DiskParameter, wal *types.DiskParameter, db *types.DiskParameter) error {
-	logger.Debugf("Adding OSD %s", data.Path)
-
-	revert := revert.New()
-	defer revert.Fail()
-
+func setStablePath(storage *api.ResourcesStorage, param *types.DiskParameter) error {
 	// Validate the path.
-	if !shared.IsBlockdevPath(data.Path) {
-		return fmt.Errorf("Invalid disk path: %s", data.Path)
+	if !shared.IsBlockdevPath(param.Path) {
+		return fmt.Errorf("Invalid disk path: %s", param.Path)
 	}
 
-	_, _, major, minor, _, _, err := shared.GetFileStat(data.Path)
+	_, _, major, minor, _, _, err := shared.GetFileStat(param.Path)
 	if err != nil {
 		return fmt.Errorf("Invalid disk path: %w", err)
 	}
 
 	dev := fmt.Sprintf("%d:%d", major, minor)
 
-	// Lookup a stable path for it.
-	storage, err := resources.GetStorage()
-	if err != nil {
-		return fmt.Errorf("Unable to list system disks: %w", err)
-	}
-
 	for _, disk := range storage.Disks {
 		// Check if full disk.
 		if disk.Device == dev {
 			candidate := fmt.Sprintf("/dev/disk/by-id/%s", disk.DeviceID)
 
 			// check if candidate exists
 			if shared.PathExists(candidate) && !shared.IsDir(candidate) {
-				data.Path = candidate
+				param.Path = candidate
 			} else {
 				candidate = fmt.Sprintf("/dev/disk/by-path/%s", disk.DevicePath)
 				if shared.PathExists(candidate) && !shared.IsDir(candidate) {
-					data.Path = candidate
+					param.Path = candidate
 				}
 			}
 
 			break
 		}
 
 		// Check if partition.
-		found := false
 		for _, part := range disk.Partitions {
 			if part.Device == dev {
 				candidate := fmt.Sprintf("/dev/disk/by-id/%s-part%d", disk.DeviceID, part.Partition)
 				if shared.PathExists(candidate) {
-					data.Path = candidate
+					param.Path = candidate
 				} else {
 					candidate = fmt.Sprintf("/dev/disk/by-path/%s-part%d", disk.DevicePath, part.Partition)
 					if shared.PathExists(candidate) {
-						data.Path = candidate
+						param.Path = candidate
 					}
 				}
 
 				break
 			}
 		}
+	}
 
-		if found {
-			break
-		}
-		// Fallthrough. We didn't find a /dev/disk path for this device, use the original path.
+	return nil
+}
+
+// AddOSD adds an OSD to the cluster, given the data, WAL and DB devices and their respective
+// flags for wiping and encrypting.
+func AddOSD(s *state.State, data types.DiskParameter, wal *types.DiskParameter, db *types.DiskParameter) error {
+	logger.Debugf("Adding OSD %s", data.Path)
+
+	revert := revert.New()
+	defer revert.Fail()
+
+	// Lookup a stable path for it.
+	storage, err := resources.GetStorage()
+	if err != nil {
+		return fmt.Errorf("Unable to list system disks: %w", err)
+	}
+
+	if err := setStablePath(storage, &data); err != nil {
+		return fmt.Errorf("Failed to set stable disk path: %w", err)
 	}
 
 	// Get a OSD number.
@@ -375,16 +384,25 @@ func AddOSD(s *state.State, data types.DiskParameter, wal *types.DiskParameter,
 
 	logger.Debugf("Created disk record for osd.%d", nr)
 
+	// Keep the old path in case it changes after encrypting.
+	oldPath := data.Path
+
 	dataPath := filepath.Join(os.Getenv("SNAP_COMMON"), "data")
 	osdDataPath := filepath.Join(dataPath, "osd", fmt.Sprintf("ceph-%d", nr))
 
-	// Keep the old path in case it changes after encrypting.
-	oldPath := data.Path
+	// if we fail later, make sure we free up the record
+	revert.Add(func() {
+		os.RemoveAll(osdDataPath)
+		s.Database.Transaction(s.Context, func(ctx context.Context, tx *sql.Tx) error {
+			database.DeleteDisk(ctx, tx, s.Name(), oldPath)
+			return nil
+		})
+	})
 
 	// Create directory.
 	err = os.MkdirAll(osdDataPath, 0700)
 	if err != nil {
-		return fmt.Errorf("Failed to bootstrap monitor: %w", err)
+		return fmt.Errorf("Failed to create OSD directory: %w", err)
 	}
 
 	// Wipe and/or encrypt the disk if needed.
@@ -393,15 +411,6 @@ func AddOSD(s *state.State, data types.DiskParameter, wal *types.DiskParameter,
 		return fmt.Errorf("Failed to prepare data device: %w", err)
 	}
 
-	// if we fail later, make sure we free up the record
-	revert.Add(func() {
-		os.RemoveAll(osdDataPath)
-		s.Database.Transaction(s.Context, func(ctx context.Context, tx *sql.Tx) error {
-			database.DeleteDisk(ctx, tx, s.Name(), oldPath)
-			return nil
-		})
-	})
-
 	// Generate keyring.
 	err = genAuth(filepath.Join(osdDataPath, "keyring"), fmt.Sprintf("osd.%d", nr), []string{"mgr", "allow profile osd"}, []string{"mon", "allow profile osd"}, []string{"osd", "allow *"})
 	if err != nil {
@@ -420,13 +429,21 @@ func AddOSD(s *state.State, data types.DiskParameter, wal *types.DiskParameter,
 	// Bootstrap OSD.
 	args := []string{"--mkfs", "--no-mon-config", "-i", fmt.Sprintf("%d", nr)}
 	if wal != nil {
+		if err = setStablePath(storage, wal); err != nil {
+			return fmt.Errorf("Failed to set stable path for WAL: %w", err)
+		}
+
 		err = prepareDisk(wal, ".wal", osdDataPath, nr)
 		if err != nil {
 			return fmt.Errorf("Failed to set up WAL device: %w", err)
 		}
 		args = append(args, []string{"--bluestore-block-wal-path", wal.Path}...)
 	}
 	if db != nil {
+		if err = setStablePath(storage, db); err != nil {
+			return fmt.Errorf("Failed to set stable path for DB: %w", err)
+		}
+
 		err = prepareDisk(db, ".db", osdDataPath, nr)
 		if err != nil {
 			return fmt.Errorf("Failed to set up DB device: %w", err)