feat: support multiple MirageOS block devices

pkg/unikontainers/block.go

@@ -170,25 +170,62 @@ func copyMountfiles(targetPath string, mounts []specs.Mount) error {
 	return nil
 }
 
-func handleExplicitBlockImage(blockImg string, mountPoint string) (types.BlockDevParams, error) {
-	if blockImg == "" {
-		return types.BlockDevParams{}, nil
+// splitAnnotList splits an ordered comma separated annotation value into
+// trimmed elements. An empty string yields a nil slice.
+func splitAnnotList(s string) []string {
+	if s == "" {
+		return nil
+	}
+	parts := strings.Split(s, ",")
+	for i := range parts {
+		parts[i] = strings.TrimSpace(parts[i])
+	}
+	return parts
+}
+
+// handleExplicitBlockImages parses the block image and mountpoint annotations,
+// each of which may hold an ordered comma separated list, into one
+// BlockDevParams per declared block device. Both lists must have the same
+// number of entries. A block mounted at "/" is the guest's rootfs and gets
+// the "rootfs" ID.
+func handleExplicitBlockImages(blockImgs string, mountPoints string) ([]types.BlockDevParams, error) {
+	if blockImgs == "" {
+		return nil, nil
 	}
 
-	if mountPoint == "" {
-		return types.BlockDevParams{}, fmt.Errorf("annotation for block device was set without a mountpoint")
+	paths := splitAnnotList(blockImgs)
+	mounts := splitAnnotList(mountPoints)
+	if len(paths) != len(mounts) {
+		return nil, fmt.Errorf("block images (%d) and mount points (%d) must have the same count",
+			len(paths), len(mounts))
 	}
 
-	id := ""
-	if mountPoint == "/" {
-		id = "rootfs"
+	// A block device mounted at "/" is the guest rootfs, resolved elsewhere
+	// through a path that does not parse lists, so mixing it with others
+	// would silently drop it. Empty entries shift the rest onto wrong IDs.
+	for i, path := range paths {
+		if path == "" || mounts[i] == "" {
+			return nil, fmt.Errorf("block image and mount point entries can not be empty")
+		}
+		if mounts[i] == "/" && len(paths) > 1 {
+			return nil, fmt.Errorf("a block device mounted at / can not be combined with other block devices")
+		}
+	}
+
+	blocks := make([]types.BlockDevParams, 0, len(paths))
+	for i, path := range paths {
+		id := ""
+		if mounts[i] == "/" {
+			id = "rootfs"
+		}
+		blocks = append(blocks, types.BlockDevParams{
+			Source:     path,
+			MountPoint: mounts[i],
+			ID:         id,
+		})
 	}
 
-	return types.BlockDevParams{
-		Source:     blockImg,
-		MountPoint: mountPoint,
-		ID:         id,
-	}, nil
+	return blocks, nil
 }
 
 // Search all the mount entries in the container's config and

pkg/unikontainers/block_test.go

@@ -37,3 +37,59 @@ func TestGetBlockDevice(t *testing.T) {
 	assert.Equal(t, tmpMnt.FsType, rootFs.FsType, "Expected filesystem type to be proc")
 	assert.Equal(t, tmpMnt.ID, rootFs.ID, "Expected ID to be empty")
 }
+
+func TestHandleExplicitBlockImages(t *testing.T) {
+	t.Run("parses an ordered list of block images and mount points", func(t *testing.T) {
+		blocks, err := handleExplicitBlockImages("/.boot/disk1, /.boot/disk2", "/data1, /data2")
+		assert.NoError(t, err)
+		assert.Len(t, blocks, 2)
+		assert.Equal(t, "/.boot/disk1", blocks[0].Source)
+		assert.Equal(t, "/data1", blocks[0].MountPoint)
+		assert.Equal(t, "/.boot/disk2", blocks[1].Source)
+		assert.Equal(t, "/data2", blocks[1].MountPoint)
+	})
+
+	t.Run("single image stays backward compatible", func(t *testing.T) {
+		blocks, err := handleExplicitBlockImages("/.boot/rootfs", "/")
+		assert.NoError(t, err)
+		assert.Len(t, blocks, 1)
+		assert.Equal(t, "rootfs", blocks[0].ID)
+	})
+
+	t.Run("empty block image yields no devices", func(t *testing.T) {
+		blocks, err := handleExplicitBlockImages("", "")
+		assert.NoError(t, err)
+		assert.Empty(t, blocks)
+	})
+
+	t.Run("mismatched counts return an error", func(t *testing.T) {
+		_, err := handleExplicitBlockImages("/a,/b", "/data")
+		assert.Error(t, err)
+	})
+
+	t.Run("rootfs combined with other block devices returns an error", func(t *testing.T) {
+		_, err := handleExplicitBlockImages("/.boot/rootfs,/.boot/data", "/,/data")
+		assert.Error(t, err)
+	})
+
+	t.Run("empty entries return an error", func(t *testing.T) {
+		_, err := handleExplicitBlockImages("/a,,/c", "/1,/2,/3")
+		assert.Error(t, err)
+
+		_, err = handleExplicitBlockImages("/a,/b", "/1,")
+		assert.Error(t, err)
+	})
+}
+
+func TestNoRootfsMultipleBlockDevs(t *testing.T) {
+	n := noRootfs{
+		annotBlockPath:       "/.boot/disk1,/.boot/disk2",
+		annotBlockMountPoint: "/data1,/data2",
+	}
+	blocks, err := n.getBlockDevs()
+	assert.NoError(t, err)
+	assert.Len(t, blocks, 2)
+	assert.Equal(t, "/.boot/disk1", blocks[0].Source)
+	assert.Equal(t, "/.boot/disk2", blocks[1].Source)
+	assert.NotEqual(t, blocks[0].ID, blocks[1].ID, "multiple block devices must have unique IDs")
+}

pkg/unikontainers/rootfs.go

@@ -140,18 +140,26 @@ func (n noRootfs) getMounts() ([]specs.Mount, error) {
 }
 
 func (n noRootfs) getBlockDevs() ([]types.BlockDevParams, error) {
-	blkImgs := []types.BlockDevParams{}
-	blockFromAnnot, err := handleExplicitBlockImage(n.annotBlockPath,
+	blocks, err := handleExplicitBlockImages(n.annotBlockPath,
 		n.annotBlockMountPoint)
 	if err != nil {
 		return nil, err
 	}
 
-	if blockFromAnnot.Source != "" && blockFromAnnot.MountPoint != "/" {
-		// TODO: Add proper support for multiple block Images from the container's
-		// image. This requires adding more annotations too.
-		blockFromAnnot.ID = "annot_vol"
-		blkImgs = append(blkImgs, blockFromAnnot)
+	blkImgs := []types.BlockDevParams{}
+	for i, b := range blocks {
+		if b.Source == "" || b.MountPoint == "/" {
+			continue
+		}
+		// This ID is just a placeholder. MirageOS/Solo5 overrides it with
+		// the solo5BlkDev annotation. We keep it unique so other monitors
+		// don't end up with clashing block device IDs.
+		if len(blocks) == 1 {
+			b.ID = "annot_vol"
+		} else {
+			b.ID = fmt.Sprintf("annot_vol%d", i)
+		}
+		blkImgs = append(blkImgs, b)
 	}
 
 	return blkImgs, nil

pkg/unikontainers/types/types.go

@@ -90,7 +90,7 @@ type UnikernelParams struct {
 	Version    string   // The version of the unikernel
 	InitrdPath string   // The path to the initrd of the unikernel
 	NetDevName string   // The name of the guest network device declared at build time
-	BlkDevName string   // The name of the guest block device declared at build time
+	BlkDevName string   // Ordered comma separated block device names declared at build time
 	Net        NetDevParams
 	Block      []BlockDevParams
 	Rootfs     RootfsParams  // Information about rootfs

pkg/unikontainers/unikernels/mirage.go

@@ -24,12 +24,12 @@ import (
 const MirageUnikernel string = "mirage"
 
 type Mirage struct {
-	Command    string
-	Monitor    string
-	Net        MirageNet
-	Block      []MirageBlock
-	netDevName string
-	blkDevName string
+	Command     string
+	Monitor     string
+	Net         MirageNet
+	Block       []MirageBlock
+	netDevName  string
+	blkDevNames []string
 }
 
 type MirageNet struct {
@@ -73,22 +73,24 @@ func (m *Mirage) MonitorBlockCli() []types.MonitorBlockArgs {
 	}
 	switch m.Monitor {
 	case "hvt", "spt":
-		// TODO: Explore options for multiple block devices in MirageOS
-		// over Solo5-spt and Solo5-hvt. Solo5 expects to use as an ID
-		// a specific name which the guest is also aware of in order to
-		// attach the respective block. As a result, urunc needs to know
-		// the correct ID to set, which is not straightforward. Therefore,
-		// there are two options. Either we read the Solo5 manifest or,
-		// we require specific IDs. Till we decide about that, we will
-		// use a single block device. We also need to find some use cases
-		// where multiple block devices are configured in MirageOS and check
-		// how MirageOS handles/configures them.
-		return []types.MonitorBlockArgs{
-			{
-				ID:   m.blkDevName,
-				Path: m.Block[0].HostPath,
-			},
+		// Solo5 attaches each block device using an ID that the guest
+		// also knows from its manifest. Image builders declare those IDs
+		// at build time through the solo5BlkDev annotation as an ordered
+		// comma separated list, which we map positionally onto the block
+		// devices here. When no ID is given for a device, we fall back to
+		// the historical default of "storage".
+		args := make([]types.MonitorBlockArgs, 0, len(m.Block))
+		for i, blk := range m.Block {
+			id := "storage"
+			if i < len(m.blkDevNames) {
+				id = m.blkDevNames[i]
+			}
+			args = append(args, types.MonitorBlockArgs{
+				ID:   id,
+				Path: blk.HostPath,
+			})
 		}
+		return args
 	default:
 		return nil
 	}
@@ -136,9 +138,11 @@ func (m *Mirage) Init(data types.UnikernelParams) error {
 		m.netDevName = "service"
 	}
 	if data.BlkDevName != "" {
-		m.blkDevName = data.BlkDevName
-	} else {
-		m.blkDevName = "storage"
+		names := strings.Split(data.BlkDevName, ",")
+		for i := range names {
+			names[i] = strings.TrimSpace(names[i])
+		}
+		m.blkDevNames = names
 	}
 
 	return nil

pkg/unikontainers/unikernels/mirage_test.go

@@ -125,3 +125,60 @@ func TestMirageBlkDevName(t *testing.T) {
 		assert.Equal(t, "storage", args[0].ID)
 	})
 }
+
+func TestMirageMultipleBlkDevs(t *testing.T) {
+	t.Run("maps an ordered id list positionally to block devices", func(t *testing.T) {
+		t.Parallel()
+		m := &Mirage{}
+		err := m.Init(types.UnikernelParams{
+			Monitor:    "hvt",
+			BlkDevName: "storage,data",
+			Block: []types.BlockDevParams{
+				{Source: "/path/to/rootfs"},
+				{Source: "/path/to/data"},
+			},
+		})
+		assert.NoError(t, err)
+		args := m.MonitorBlockCli()
+		assert.Len(t, args, 2)
+		assert.Equal(t, "storage", args[0].ID)
+		assert.Equal(t, "/path/to/rootfs", args[0].Path)
+		assert.Equal(t, "data", args[1].ID)
+		assert.Equal(t, "/path/to/data", args[1].Path)
+	})
+
+	t.Run("trims whitespace around each id", func(t *testing.T) {
+		t.Parallel()
+		m := &Mirage{}
+		err := m.Init(types.UnikernelParams{
+			Monitor:    "hvt",
+			BlkDevName: "storage, data",
+			Block: []types.BlockDevParams{
+				{Source: "/a"},
+				{Source: "/b"},
+			},
+		})
+		assert.NoError(t, err)
+		args := m.MonitorBlockCli()
+		assert.Len(t, args, 2)
+		assert.Equal(t, "data", args[1].ID)
+	})
+
+	t.Run("falls back to storage for devices without an id", func(t *testing.T) {
+		t.Parallel()
+		m := &Mirage{}
+		err := m.Init(types.UnikernelParams{
+			Monitor:    "hvt",
+			BlkDevName: "storage",
+			Block: []types.BlockDevParams{
+				{Source: "/a"},
+				{Source: "/b"},
+			},
+		})
+		assert.NoError(t, err)
+		args := m.MonitorBlockCli()
+		assert.Len(t, args, 2)
+		assert.Equal(t, "storage", args[0].ID)
+		assert.Equal(t, "storage", args[1].ID)
+	})
+}