Skip to content

Commit cbe1b51

Browse files
tomponlinetomponline
authored
Volume disk device source-type (#14886)
I'm torn between the `volumeType/volumeName` and `sourcetype` syntaxes for specifying other types of storage volumes in disk devices. A few factors here, some I knew when I wrote my spec, but a few I didn't: - By using `volumeType/volumeName`, we introduced novel syntax that the server doesn't use anywhere else, and doesn't fit nicely with the principle of "one value per map key" (see #14491). - This syntax is already overloaded in the client to mean both `volumeType/volumeName` and `poolName/volumeName` (see [`lxc storage volume copy`](https://canonical-microcloud.readthedocs-hosted.com/en/latest/lxd/reference/manpages/lxc/storage/volume/copy/#lxc-storage-volume-copy-md)) - The `storage.backups_volume` and `storage.image_volume` [server configuration keys](https://canonical-microcloud.readthedocs-hosted.com/en/latest/lxd/server/#miscellaneous-options) also use `poolName/volumeName` The current syntax for specifying volumes as disk devices is: ``` devices: cosmic-dust-analyzer-data: pool: default source: custom/cosmic-dust-analyzer-data type: disk ``` This PR uses this style instead: ``` devices: cosmic-dust-analyzer-data: pool: default source: cosmic-dust-analyzer-data sourcetype: custom type: disk ``` `sourcetype` is `custom` by default so it can be omitted for custom volumes.
2 parents 7e35f87 + 63dc1c3 commit cbe1b51

File tree

7 files changed

+100
-113
lines changed

7 files changed

+100
-113
lines changed

‎doc/metadata.txt‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,15 @@ See {ref}`devices-disk-types` for details.
186186

187187
```
188188

189+
```{config:option} source-type device-disk-device-conf
190+
:defaultdesc: "`custom`"
191+
:required: "no"
192+
:shortdesc: "Type of the backing storage volume"
193+
:type: "string"
194+
Possible values are `custom` (the default) or `virtual-machine`. This
195+
key is only valid when `source` is the name of a storage volume.
196+
```
197+
189198
<!-- config group device-disk-device-conf end -->
190199
<!-- config group device-gpu-mdev-device-conf start -->
191200
```{config:option} id device-gpu-mdev-device-conf

‎lxc/storage_volume.go‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -259,10 +259,15 @@ func (c *cmdStorageVolumeAttach) run(cmd *cobra.Command, args []string) error {
259259
device := map[string]string{
260260
"type": "disk",
261261
"pool": resource.name,
262-
"source": args[1],
262+
"source": volName,
263263
"path": devPath,
264264
}
265265

266+
// Only specify sourcetype when not the default
267+
if volType != "custom" {
268+
device["source-type"] = volType
269+
}
270+
266271
// Add the device to the instance
267272
err = instanceDeviceAdd(resource.server, args[2], devName, device)
268273
if err != nil {
@@ -358,14 +363,19 @@ func (c *cmdStorageVolumeAttachProfile) run(cmd *cobra.Command, args []string) e
358363
device := map[string]string{
359364
"type": "disk",
360365
"pool": resource.name,
361-
"source": args[1],
366+
"source": volName,
362367
}
363368

364369
// Ignore path for block volumes
365370
if vol.ContentType != "block" {
366371
device["path"] = devPath
367372
}
368373

374+
// Only specify sourcetype when not the default
375+
if volType != "custom" {
376+
device["source-type"] = volType
377+
}
378+
369379
// Add the device to the instance
370380
err = profileDeviceAdd(resource.server, args[2], devName, device)
371381
if err != nil {
@@ -869,9 +879,12 @@ func (c *cmdStorageVolumeDetach) run(cmd *cobra.Command, args []string) error {
869879
// Find the device
870880
if devName == "" {
871881
for n, d := range inst.Devices {
872-
sourceName, sourceType := parseVolume("custom", d["source"])
882+
sourceType := "custom"
883+
if d["source-type"] != "" {
884+
sourceType = d["source-type"]
885+
}
873886

874-
if d["type"] == "disk" && d["pool"] == resource.name && volType == sourceType && volName == sourceName {
887+
if d["type"] == "disk" && d["pool"] == resource.name && volType == sourceType && volName == d["source"] {
875888
if devName != "" {
876889
return errors.New(i18n.G("More than one device matches, specify the device name"))
877890
}
@@ -970,9 +983,12 @@ func (c *cmdStorageVolumeDetachProfile) run(cmd *cobra.Command, args []string) e
970983
// Find the device
971984
if devName == "" {
972985
for n, d := range profile.Devices {
973-
sourceName, sourceType := parseVolume("custom", d["source"])
986+
sourceType := "custom"
987+
if d["source-type"] != "" {
988+
sourceType = d["source-type"]
989+
}
974990

975-
if d["type"] == "disk" && d["pool"] == resource.name && volType == sourceType && volName == sourceName {
991+
if d["type"] == "disk" && d["pool"] == resource.name && volType == sourceType && volName == d["source"] {
976992
if devName != "" {
977993
return errors.New(i18n.G("More than one device matches, specify the device name"))
978994
}

‎lxd/device/disk.go‎

Lines changed: 53 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,27 @@ func (d *disk) sourceIsLocalPath(source string) bool {
147147
return true
148148
}
149149

150+
func (d *disk) sourceVolumeFields() (volumeName string, volumeType storageDrivers.VolumeType, dbVolumeType int, volumeTypeName string, err error) {
151+
volumeName = d.config["source"]
152+
153+
volumeTypeName = cluster.StoragePoolVolumeTypeNameCustom
154+
if d.config["source-type"] != "" {
155+
volumeTypeName = d.config["source-type"]
156+
}
157+
158+
dbVolumeType, err = storagePools.VolumeTypeNameToDBType(volumeTypeName)
159+
if err != nil {
160+
return volumeName, volumeType, dbVolumeType, volumeTypeName, err
161+
}
162+
163+
volumeType, err = storagePools.VolumeDBTypeToType(dbVolumeType)
164+
if err != nil {
165+
return volumeName, volumeType, dbVolumeType, volumeTypeName, err
166+
}
167+
168+
return volumeName, volumeType, dbVolumeType, volumeTypeName, nil
169+
}
170+
150171
// Check that unshared custom storage block volumes are not added to profiles or
151172
// multiple instances unless they will not be accessed concurrently.
152173
func (d *disk) checkBlockVolSharing(instanceType instancetype.Type, projectName string, volume *api.StorageVolume) error {
@@ -250,6 +271,15 @@ func (d *disk) validateConfig(instConf instance.ConfigReader) error {
250271
// required: yes
251272
// shortdesc: Source of a file system or block device
252273
"source": validate.IsAny,
274+
// lxdmeta:generate(entities=device-disk; group=device-conf; key=source-type)
275+
// Possible values are `custom` (the default) or `virtual-machine`. This
276+
// key is only valid when `source` is the name of a storage volume.
277+
// ---
278+
// type: string
279+
// defaultdesc: `custom`
280+
// required: no
281+
// shortdesc: Type of the backing storage volume
282+
"source-type": validate.Optional(validate.IsOneOf(cluster.StoragePoolVolumeTypeNameCustom, cluster.StoragePoolVolumeTypeNameVM)),
253283
// lxdmeta:generate(entities=device-disk; group=device-conf; key=limits.read)
254284
// You can specify a value in byte/s (various suffixes supported, see {ref}`instances-limit-units`) or in IOPS (must be suffixed with `iops`).
255285
// See also {ref}`storage-configure-io`.
@@ -389,6 +419,10 @@ func (d *disk) validateConfig(instConf instance.ConfigReader) error {
389419
return fmt.Errorf(`Cannot use both "required" and deprecated "optional" properties at the same time`)
390420
}
391421

422+
if d.config["source-type"] != "" && d.config["pool"] == "" {
423+
return fmt.Errorf(`"source-type" can only be used on storage volume disk devices`)
424+
}
425+
392426
if d.config["source"] == "" && d.config["path"] != "/" {
393427
return fmt.Errorf(`Non root disk devices require the "source" property`)
394428
}
@@ -475,7 +509,7 @@ func (d *disk) validateConfig(instConf instance.ConfigReader) error {
475509

476510
// Non-root volume validation.
477511
if !instancetype.IsRootDiskDevice(d.config) {
478-
volumeType, dbVolumeType, _, volumeName, err := storagePools.DiskVolumeSourceParse(d.config["source"])
512+
volumeName, volumeType, dbVolumeType, _, err := d.sourceVolumeFields()
479513
if err != nil {
480514
return err
481515
}
@@ -492,10 +526,8 @@ func (d *disk) validateConfig(instConf instance.ConfigReader) error {
492526
}
493527

494528
// Derive the effective storage project name from the instance config's project.
495-
storageProjectName, err = project.StorageVolumeProject(d.state.DB.Cluster, instConf.Project().Name, dbVolumeType)
496-
if err != nil {
497-
return err
498-
}
529+
instProj := instConf.Project()
530+
storageProjectName = project.StorageVolumeProjectFromRecord(&instProj, dbVolumeType)
499531

500532
// GetStoragePoolVolume returns a volume with an empty Location field for remote drivers.
501533
err = d.state.DB.Cluster.Transaction(context.TODO(), func(ctx context.Context, tx *db.ClusterTx) error {
@@ -706,15 +738,13 @@ func (d *disk) Register() error {
706738
return err
707739
}
708740
} else if d.config["path"] != "/" && d.config["source"] != "" && d.config["pool"] != "" {
709-
volumeType, dbVolumeType, volumeTypeName, volumeName, err := storagePools.DiskVolumeSourceParse(d.config["source"])
741+
volumeName, volumeType, dbVolumeType, volumeTypeName, err := d.sourceVolumeFields()
710742
if err != nil {
711743
return err
712744
}
713745

714-
storageProjectName, err := project.StorageVolumeProject(d.state.DB.Cluster, d.inst.Project().Name, dbVolumeType)
715-
if err != nil {
716-
return err
717-
}
746+
instProj := d.inst.Project()
747+
storageProjectName := project.StorageVolumeProjectFromRecord(&instProj, dbVolumeType)
718748

719749
// Try to mount the volume that should already be mounted to reinitialise the ref counter.
720750
_, err = d.pool.MountVolume(storageProjectName, volumeName, volumeType, nil)
@@ -836,16 +866,14 @@ func (d *disk) startContainer() (*deviceConfig.RunConfig, error) {
836866
// If ownerShift is none and pool is specified then check whether the pool itself
837867
// has owner shifting enabled, and if so enable shifting on this device too.
838868
if ownerShift == deviceConfig.MountOwnerShiftNone && d.config["pool"] != "" {
839-
_, dbVolumeType, _, volumeName, err := storagePools.DiskVolumeSourceParse(d.config["source"])
869+
volumeName, _, dbVolumeType, _, err := d.sourceVolumeFields()
840870
if err != nil {
841871
return nil, err
842872
}
843873

844874
// Only custom volumes can be attached currently.
845-
storageProjectName, err := project.StorageVolumeProject(d.state.DB.Cluster, d.inst.Project().Name, dbVolumeType)
846-
if err != nil {
847-
return nil, err
848-
}
875+
instProj := d.inst.Project()
876+
storageProjectName := project.StorageVolumeProjectFromRecord(&instProj, dbVolumeType)
849877

850878
var dbVolume *db.StorageVolume
851879
err = d.state.DB.Cluster.Transaction(context.TODO(), func(ctx context.Context, tx *db.ClusterTx) error {
@@ -1088,16 +1116,14 @@ func (d *disk) startVM() (*deviceConfig.RunConfig, error) {
10881116
if d.config["pool"] != "" {
10891117
var revertFunc func()
10901118

1091-
volumeType, dbVolumeType, _, volumeName, err := storagePools.DiskVolumeSourceParse(d.config["source"])
1119+
volumeName, volumeType, dbVolumeType, _, err := d.sourceVolumeFields()
10921120
if err != nil {
10931121
return nil, err
10941122
}
10951123

10961124
// Derive the effective storage project name from the instance config's project.
1097-
storageProjectName, err := project.StorageVolumeProject(d.state.DB.Cluster, d.inst.Project().Name, dbVolumeType)
1098-
if err != nil {
1099-
return nil, err
1100-
}
1125+
instProj := d.inst.Project()
1126+
storageProjectName := project.StorageVolumeProjectFromRecord(&instProj, dbVolumeType)
11011127

11021128
// GetStoragePoolVolume returns a volume with an empty Location field for remote drivers.
11031129
var dbVolume *db.StorageVolume
@@ -1601,16 +1627,14 @@ func (d *disk) mountPoolVolume() (func(), string, *storagePools.MountInfo, error
16011627
return nil, "", nil, fmt.Errorf(`When the "pool" property is set "source" must specify the name of a volume, not a path`)
16021628
}
16031629

1604-
volumeType, dbVolumeType, volumeTypeName, volumeName, err := storagePools.DiskVolumeSourceParse(d.config["source"])
1630+
volumeName, volumeType, dbVolumeType, volumeTypeName, err := d.sourceVolumeFields()
16051631
if err != nil {
16061632
return nil, "", nil, err
16071633
}
16081634

16091635
// Only custom volumes can be attached currently.
1610-
storageProjectName, err := project.StorageVolumeProject(d.state.DB.Cluster, d.inst.Project().Name, dbVolumeType)
1611-
if err != nil {
1612-
return nil, "", nil, err
1613-
}
1636+
instProj := d.inst.Project()
1637+
storageProjectName := project.StorageVolumeProjectFromRecord(&instProj, dbVolumeType)
16141638

16151639
mountInfo, err = d.pool.MountVolume(storageProjectName, volumeName, volumeType, nil)
16161640
if err != nil {
@@ -1646,7 +1670,7 @@ func (d *disk) mountPoolVolume() (func(), string, *storagePools.MountInfo, error
16461670

16471671
err = d.storagePoolVolumeAttachShift(storageProjectName, d.pool.Name(), volumeName, dbVolumeType, srcPath)
16481672
if err != nil {
1649-
return nil, "", nil, fmt.Errorf("Failed shifting storage volume %q of type %q on storage pool %q: %w", volumeName, volumeTypeName, d.pool.Name(), err)
1673+
return nil, "", nil, fmt.Errorf(`Failed shifting storage volume "%s/%s" on storage pool %q: %w`, volumeTypeName, volumeName, d.pool.Name(), err)
16501674
}
16511675
}
16521676

@@ -2085,16 +2109,14 @@ func (d *disk) postStop() error {
20852109

20862110
// Check if pool-specific action should be taken to unmount custom volume disks.
20872111
if d.config["pool"] != "" && d.config["path"] != "/" {
2088-
volumeType, dbVolumeType, _, volumeName, err := storagePools.DiskVolumeSourceParse(d.config["source"])
2112+
volumeName, volumeType, dbVolumeType, _, err := d.sourceVolumeFields()
20892113
if err != nil {
20902114
return err
20912115
}
20922116

20932117
// Only custom volumes can be attached currently.
2094-
storageProjectName, err := project.StorageVolumeProject(d.state.DB.Cluster, d.inst.Project().Name, dbVolumeType)
2095-
if err != nil {
2096-
return err
2097-
}
2118+
instProj := d.inst.Project()
2119+
storageProjectName := project.StorageVolumeProjectFromRecord(&instProj, dbVolumeType)
20982120

20992121
_, err = d.pool.UnmountVolume(storageProjectName, volumeName, volumeType, nil)
21002122
if err != nil && !errors.Is(err, storageDrivers.ErrInUse) {

‎lxd/metadata/configuration.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,15 @@
199199
"shortdesc": "Source of a file system or block device",
200200
"type": "string"
201201
}
202+
},
203+
{
204+
"source-type": {
205+
"defaultdesc": "`custom`",
206+
"longdesc": "Possible values are `custom` (the default) or `virtual-machine`. This\nkey is only valid when `source` is the name of a storage volume.",
207+
"required": "no",
208+
"shortdesc": "Type of the backing storage volume",
209+
"type": "string"
210+
}
202211
}
203212
]
204213
}

‎lxd/storage/utils.go‎

Lines changed: 4 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package storage
33
import (
44
"context"
55
"encoding/json"
6-
"errors"
76
"fmt"
87
"os"
98
"path/filepath"
@@ -186,49 +185,6 @@ func VolumeContentTypeNameToContentType(contentTypeName string) (int, error) {
186185
return -1, fmt.Errorf("Invalid volume content type name")
187186
}
188187

189-
// DiskVolumeSourceParse parses a disk device's `source` property when it refers to a
190-
// storage volume.
191-
func DiskVolumeSourceParse(source string) (volType drivers.VolumeType, dbVolType int, volTypeName string, volName string, err error) {
192-
source = filepath.Clean(source)
193-
194-
slash := strings.Index(source, "/")
195-
if (slash > 0) && (len(source) > slash) {
196-
// Extract volume name.
197-
volName = source[(slash + 1):]
198-
// Extract volume type.
199-
volTypeName = source[:slash]
200-
} else {
201-
volName = source
202-
}
203-
204-
// Check volume type can be attached as a disk device.
205-
switch volTypeName {
206-
case cluster.StoragePoolVolumeTypeNameContainer:
207-
err = errors.New("Using container storage volumes is not supported")
208-
case cluster.StoragePoolVolumeTypeNameVM, cluster.StoragePoolVolumeTypeNameCustom:
209-
case "":
210-
// We simply received the name of a custom storage volume.
211-
volTypeName = cluster.StoragePoolVolumeTypeNameCustom
212-
case cluster.StoragePoolVolumeTypeNameImage:
213-
err = errors.New("Using image storage volumes is not supported")
214-
default:
215-
err = fmt.Errorf("Unknown storage type prefix %q found", volTypeName)
216-
}
217-
218-
if err != nil {
219-
return volType, dbVolType, "", "", err
220-
}
221-
222-
dbVolType, err = VolumeTypeNameToDBType(volTypeName)
223-
if err != nil {
224-
return volType, dbVolType, "", "", err
225-
}
226-
227-
volType, err = VolumeDBTypeToType(dbVolType)
228-
229-
return volType, dbVolType, volTypeName, volName, err
230-
}
231-
232188
// VolumeDBGet loads a volume from the database.
233189
func VolumeDBGet(pool Pool, projectName string, volumeName string, volumeType drivers.VolumeType) (*db.StorageVolume, error) {
234190
p, ok := pool.(*lxdBackend)
@@ -1001,12 +957,12 @@ func volumeIsUsedByDevice(vol api.StorageVolume, inst *db.InstanceArgs, dev map[
1001957
}
1002958
}
1003959

1004-
_, _, volumeTypeName, volumeName, err := DiskVolumeSourceParse(dev["source"])
1005-
if err != nil {
1006-
return false, err
960+
volumeTypeName := cluster.StoragePoolVolumeTypeNameCustom
961+
if dev["source-type"] != "" {
962+
volumeTypeName = dev["source-type"]
1007963
}
1008964

1009-
if volumeTypeName == vol.Type && volumeName == vol.Name {
965+
if volumeTypeName == vol.Type && dev["source"] == vol.Name {
1010966
return true, nil
1011967
}
1012968

‎lxd/storage_volumes_utils.go‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,7 @@ func storagePoolVolumeUpdateUsers(s *state.State, projectName string, oldPoolNam
3838
_, exists := newDevices[devName]
3939
if exists {
4040
newDevices[devName]["pool"] = newPoolName
41-
42-
if strings.Contains(localDevices[devName]["source"], "/") {
43-
newDevices[devName]["source"] = newVol.Type + "/" + newVol.Name
44-
} else {
45-
newDevices[devName]["source"] = newVol.Name
46-
}
41+
newDevices[devName]["source"] = newVol.Name
4742
}
4843
}
4944

@@ -93,12 +88,7 @@ func storagePoolVolumeUpdateUsers(s *state.State, projectName string, oldPoolNam
9388

9489
if shared.ValueInSlice(devName, usedByDevices) {
9590
newDevices[devName]["pool"] = newPoolName
96-
97-
if strings.Contains(dev["source"], "/") {
98-
newDevices[devName]["source"] = newVol.Type + "/" + newVol.Name
99-
} else {
100-
newDevices[devName]["source"] = newVol.Name
101-
}
91+
newDevices[devName]["source"] = newVol.Name
10292
}
10393
}
10494

0 commit comments

Comments
 (0)