Merge pull request canonical#253 from sabaini/fix/disk-id-calc

Fix disk add race
sabaini · Nov 2, 2023 · aae9f94 · aae9f94
2 parents 895c58a + 82099f8
commit aae9f94
Show file tree

Hide file tree

Showing 7 changed files with 145 additions and 170 deletions.
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
@@ -261,16 +261,11 @@ jobs:
     - name: Remove OSD again
       run: |
         set -uex
-        lxc exec node-wrk3 -- sh -c "microceph disk remove 3"
+        lxc exec node-head -- sh -c "microceph disk remove 4"
         lxc exec node-head -- sh -c "microceph.ceph -s" | egrep "osd: 3 osds: 3 up.*3 in"
 
     - name: Test migrate services
-      run: |
-        set -uex
-        lxc exec node-head -- sh -c "microceph cluster migrate node-wrk1 node-wrk3"
-        sleep 2
-        lxc exec node-head -- sh -c "microceph status" | grep -F -A 1 node-wrk1 | grep -E "^  Services: osd$"
-        lxc exec node-head -- sh -c "microceph status" | grep -F -A 1 node-wrk3 | grep -E "^  Services: mds, mgr, mon$"
+      run: ~/actionutils.sh test_migration node-wrk1 node-wrk3
 
     - name: Enable services on wrk1
       run: ~/actionutils.sh headexec enable_services node-wrk1

diff --git a/microceph/ceph/osd.go b/microceph/ceph/osd.go
@@ -12,8 +12,6 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"strconv"
-	"strings"
 	"syscall"
 	"time"
 
@@ -31,56 +29,6 @@ import (
 	"github.com/canonical/microceph/microceph/database"
 )
 
-func nextOSD(s *state.State) (int64, error) {
-	// Get the used OSD ids from Ceph.
-	osds, err := cephRun("osd", "ls")
-	if err != nil {
-		return -1, err
-	}
-
-	cephIds := []int64{}
-	for _, line := range strings.Split(osds, "\n") {
-		if line == "" {
-			continue
-		}
-
-		id, err := strconv.ParseInt(line, 10, 64)
-		if err != nil {
-			continue
-		}
-
-		cephIds = append(cephIds, id)
-	}
-
-	// Get the used OSD ids from the database.
-	dbIds := []int64{}
-	err = s.Database.Transaction(s.Context, func(ctx context.Context, tx *sql.Tx) error {
-		disks, err := database.GetDisks(ctx, tx)
-		if err != nil {
-			return fmt.Errorf("Failed to fetch disks: %w", err)
-		}
-
-		for _, disk := range disks {
-			dbIds = append(dbIds, int64(disk.OSD))
-		}
-
-		return nil
-	})
-	if err != nil {
-		return -1, err
-	}
-
-	// Find next available.
-	nextID := int64(0)
-	for {
-		if !shared.Int64InSlice(nextID, cephIds) && !shared.Int64InSlice(nextID, dbIds) {
-			return nextID, nil
-		}
-
-		nextID++
-	}
-}
-
 func prepareDisk(disk *types.DiskParameter, suffix string, osdPath string, osdID int64) error {
 	if disk.Wipe {
 		err := timeoutWipe(disk.Path)
@@ -362,20 +310,13 @@ func AddOSD(s *state.State, data types.DiskParameter, wal *types.DiskParameter,
 		return fmt.Errorf("Failed to set stable disk path: %w", err)
 	}
 
-	// Get a OSD number.
-	nr, err := nextOSD(s)
-	if err != nil {
-		return fmt.Errorf("Failed to find next OSD number: %w", err)
-	}
-	logger.Debugf("nextOSD number is %d for disk %s", nr, data.Path)
-
 	// Record the disk.
+	var nr int64
 	err = s.Database.Transaction(s.Context, func(ctx context.Context, tx *sql.Tx) error {
-		_, err := database.CreateDisk(ctx, tx, database.Disk{Member: s.Name(), Path: data.Path, OSD: int(nr)})
+		nr, err = database.CreateDisk(ctx, tx, database.Disk{Member: s.Name(), Path: data.Path})
 		if err != nil {
 			return fmt.Errorf("Failed to record disk: %w", err)
 		}
-
 		return nil
 	})
 	if err != nil {
@@ -384,17 +325,14 @@ 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))
 
 	// 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)
+			database.DeleteDisk(ctx, tx, s.Name(), data.Path)
 			return nil
 		})
 	})
@@ -514,7 +452,7 @@ func sanityCheck(s common.StateInterface, osd int64) error {
 		return err
 	}
 	if !exists {
-		return fmt.Errorf("ods.%d not found", osd)
+		return fmt.Errorf("osd.%d not found", osd)
 	}
 	return nil
 }

diff --git a/microceph/database/disk.go b/microceph/database/disk.go
@@ -3,35 +3,33 @@ package database
 //go:generate -command mapper lxd-generate db mapper -t disk.mapper.go
 //go:generate mapper reset
 //
-//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e disk objects table=disks
-//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e disk objects-by-Member table=disks
-//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e disk objects-by-Member-and-Path table=disks
-//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e disk id table=disks
-//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e disk create table=disks
-//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e disk delete-by-Member table=disks
-//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e disk delete-by-Member-and-Path table=disks
-//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e disk update table=disks
+//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e Disk objects table=Disks
+//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e Disk objects-by-Member table=Disks
+//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e Disk objects-by-Member-and-Path table=Disks
+//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e Disk id table=Disks
+//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e Disk create table=Disks
+//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e Disk delete-by-Member table=Disks
+//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e Disk delete-by-Member-and-Path table=Disks
+//go:generate mapper stmt -d github.com/canonical/microcluster/cluster -e Disk update table=Disks
 //
-//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e disk GetMany
-//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e disk GetOne
-//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e disk ID
-//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e disk Exists
-//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e disk Create
-//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e disk DeleteOne-by-Member-and-Path
-//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e disk DeleteMany-by-Member
-//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e disk Update
+//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e Disk GetMany
+//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e Disk GetOne
+//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e Disk ID
+//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e Disk Exists
+//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e Disk Create
+//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e Disk DeleteOne-by-Member-and-Path
+//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e Disk DeleteMany-by-Member
+//go:generate mapper method -i -d github.com/canonical/microcluster/cluster -e Disk Update
 
 // Disk is used to track the Ceph disks on a particular server.
 type Disk struct {
 	ID     int
-	Member string `db:"primary=yes&join=internal_cluster_members.name&joinon=disks.member_id"`
-	OSD    int    `db:"primary=yes"`
-	Path   string
+	Member string `db:"primary=yes&join=internal_cluster_members.name&joinon=Disks.member_id"`
+	Path   string `db:"primary=yes"`
 }
 
 // DiskFilter is a required struct for use with lxd-generate. It is used for filtering fields on database fetches.
 type DiskFilter struct {
 	Member *string
 	Path   *string
-	OSD    *int
 }