-
Notifications
You must be signed in to change notification settings - Fork 931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attach VM root volumes as disk devices #14532
base: main
Are you sure you want to change the base?
Changes from all commits
6b1ba3b
f3578ee
02b5291
0ab5c40
0a167e0
56966ea
2fb9f07
5b18c5e
57449d7
2a8fb8d
5f11337
10abbd2
ea5dd7a
5821d03
c756034
d051bdc
c6a5310
70febb6
a4ebc1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -303,6 +303,7 @@ uptime | |
URI | ||
URIs | ||
userspace | ||
UUIDs | ||
vCPU | ||
vCPUs | ||
VDPA | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,10 +164,12 @@ type cmdStorageVolumeAttach struct { | |
|
||
func (c *cmdStorageVolumeAttach) command() *cobra.Command { | ||
cmd := &cobra.Command{} | ||
cmd.Use = usage("attach", i18n.G("[<remote>:]<pool> <volume> <instance> [<device name>] [<path>]")) | ||
cmd.Use = usage("attach", i18n.G("[<remote>:]<pool> [<type>/]<volume> <instance> [<device name>] [<path>]")) | ||
cmd.Short = i18n.G("Attach new storage volumes to instances") | ||
cmd.Long = cli.FormatSection(i18n.G("Description"), i18n.G( | ||
`Attach new storage volumes to instances`)) | ||
`Attach new storage volumes to instances | ||
|
||
<type> must be one of "custom" or "virtual-machine"`)) | ||
|
||
cmd.RunE = c.run | ||
|
||
|
@@ -210,8 +212,8 @@ func (c *cmdStorageVolumeAttach) run(cmd *cobra.Command, args []string) error { | |
} | ||
|
||
volName, volType := parseVolume("custom", args[1]) | ||
if volType != "custom" { | ||
return errors.New(i18n.G("Only \"custom\" volumes can be attached to instances")) | ||
if volType != "custom" && volType != "virtual-machine" { | ||
return errors.New(i18n.G(`Only "custom" and "virtual-machine" volumes can be attached to instances`)) | ||
} | ||
|
||
// Attach the volume | ||
|
@@ -257,7 +259,7 @@ func (c *cmdStorageVolumeAttach) run(cmd *cobra.Command, args []string) error { | |
device := map[string]string{ | ||
"type": "disk", | ||
"pool": resource.name, | ||
"source": volName, | ||
"source": args[1], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if the user provides just the volume name without the type? |
||
"path": devPath, | ||
} | ||
|
||
|
@@ -279,10 +281,12 @@ type cmdStorageVolumeAttachProfile struct { | |
|
||
func (c *cmdStorageVolumeAttachProfile) command() *cobra.Command { | ||
cmd := &cobra.Command{} | ||
cmd.Use = usage("attach-profile", i18n.G("[<remote:>]<pool> <volume> <profile> [<device name>] [<path>]")) | ||
cmd.Use = usage("attach-profile", i18n.G("[<remote:>]<pool> [<type>/]<volume> <profile> [<device name>] [<path>]")) | ||
cmd.Short = i18n.G("Attach new storage volumes to profiles") | ||
cmd.Long = cli.FormatSection(i18n.G("Description"), i18n.G( | ||
`Attach new storage volumes to profiles`)) | ||
`Attach new storage volumes to profiles | ||
|
||
<type> must be one of "custom" or "virtual-machine"`)) | ||
|
||
cmd.RunE = c.run | ||
|
||
|
@@ -340,8 +344,8 @@ func (c *cmdStorageVolumeAttachProfile) run(cmd *cobra.Command, args []string) e | |
} | ||
|
||
volName, volType := parseVolume("custom", args[1]) | ||
if volType != "custom" { | ||
return errors.New(i18n.G("Only \"custom\" volumes can be attached to instances")) | ||
if volType != "custom" && volType != "virtual-machine" { | ||
return errors.New(i18n.G(`Only "custom" and "virtual-machine" volumes can be attached to profiles`)) | ||
} | ||
|
||
// Check if the requested storage volume actually exists | ||
|
@@ -354,7 +358,7 @@ func (c *cmdStorageVolumeAttachProfile) run(cmd *cobra.Command, args []string) e | |
device := map[string]string{ | ||
"type": "disk", | ||
"pool": resource.name, | ||
"source": vol.Name, | ||
"source": args[1], | ||
} | ||
|
||
// Ignore path for block volumes | ||
|
@@ -803,7 +807,7 @@ type cmdStorageVolumeDetach struct { | |
|
||
func (c *cmdStorageVolumeDetach) command() *cobra.Command { | ||
cmd := &cobra.Command{} | ||
cmd.Use = usage("detach", i18n.G("[<remote>:]<pool> <volume> <instance> [<device name>]")) | ||
cmd.Use = usage("detach", i18n.G("[<remote>:]<pool> [<type>/]<volume> <instance> [<device name>]")) | ||
cmd.Short = i18n.G("Detach storage volumes from instances") | ||
cmd.Long = cli.FormatSection(i18n.G("Description"), i18n.G( | ||
`Detach storage volumes from instances`)) | ||
|
@@ -861,14 +865,13 @@ func (c *cmdStorageVolumeDetach) run(cmd *cobra.Command, args []string) error { | |
} | ||
|
||
volName, volType := parseVolume("custom", args[1]) | ||
if volType != "custom" { | ||
return errors.New(i18n.G(`Only "custom" volumes can be attached to instances`)) | ||
} | ||
|
||
// Find the device | ||
if devName == "" { | ||
for n, d := range inst.Devices { | ||
if d["type"] == "disk" && d["pool"] == resource.name && d["source"] == volName { | ||
sourceType, sourceName := parseVolume("custom", d["source"]) | ||
|
||
if d["type"] == "disk" && d["pool"] == resource.name && volType == sourceType && volName == sourceName { | ||
if devName != "" { | ||
return errors.New(i18n.G("More than one device matches, specify the device name")) | ||
} | ||
|
@@ -906,7 +909,7 @@ type cmdStorageVolumeDetachProfile struct { | |
|
||
func (c *cmdStorageVolumeDetachProfile) command() *cobra.Command { | ||
cmd := &cobra.Command{} | ||
cmd.Use = usage("detach-profile", i18n.G("[<remote:>]<pool> <volume> <profile> [<device name>]")) | ||
cmd.Use = usage("detach-profile", i18n.G("[<remote:>]<pool> [<type>/]<volume> <profile> [<device name>]")) | ||
cmd.Short = i18n.G("Detach storage volumes from profiles") | ||
cmd.Long = cli.FormatSection(i18n.G("Description"), i18n.G( | ||
`Detach storage volumes from profiles`)) | ||
|
@@ -963,14 +966,13 @@ func (c *cmdStorageVolumeDetachProfile) run(cmd *cobra.Command, args []string) e | |
} | ||
|
||
volName, volType := parseVolume("custom", args[1]) | ||
if volType != "custom" { | ||
return errors.New(i18n.G(`Only "custom" volumes can be attached to instances`)) | ||
} | ||
|
||
// Find the device | ||
if devName == "" { | ||
for n, d := range profile.Devices { | ||
if d["type"] == "disk" && d["pool"] == resource.name && d["source"] == volName { | ||
sourceType, sourceName := parseVolume("custom", d["source"]) | ||
MggMuggins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if d["type"] == "disk" && d["pool"] == resource.name && volType == sourceType && volName == sourceName { | ||
if devName != "" { | ||
return errors.New(i18n.G("More than one device matches, specify the device name")) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -155,7 +155,7 @@ func (d *disk) checkBlockVolSharing(instanceType instancetype.Type, projectName | |
} | ||
|
||
if instanceType == instancetype.Any { | ||
return fmt.Errorf("Cannot add custom storage block volume to profiles if security.shared is false or unset") | ||
return fmt.Errorf("Cannot add block volume to profiles if security.shared is false or unset") | ||
} | ||
|
||
err := storagePools.VolumeUsedByInstanceDevices(d.state, d.pool.Name(), projectName, volume, true, func(inst db.InstanceArgs, project api.Project, usedByDevices []string) error { | ||
|
@@ -164,13 +164,23 @@ func (d *disk) checkBlockVolSharing(instanceType instancetype.Type, projectName | |
return nil | ||
} | ||
|
||
return db.ErrListStop | ||
}) | ||
if err != nil { | ||
if err == db.ErrListStop { | ||
return fmt.Errorf("Cannot add custom storage block volume to more than one instance if security.shared is false or unset") | ||
// Don't count a VM volume's instance if security.protection.start is preventing that instance from starting | ||
if volume.Type == cluster.StoragePoolVolumeTypeNameVM && volume.Project == inst.Project && volume.Name == inst.Name { | ||
apiInst, err := inst.ToAPI() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
apiInst.ExpandedConfig = instancetype.ExpandInstanceConfig(d.state.GlobalConfig.Dump(), apiInst.Config, inst.Profiles) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could put a comment here explaining why |
||
if shared.IsTrue(apiInst.ExpandedConfig["security.protection.start"]) { | ||
return nil | ||
} | ||
Comment on lines
+167
to
+178
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea here with If I am wondering now if we should also allow custom block volumes to be attached to more than one instance if . |
||
} | ||
|
||
return fmt.Errorf("Cannot add block volume to more than one instance if security.shared is false or unset") | ||
}) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
|
@@ -468,6 +478,17 @@ func (d *disk) validateConfig(instConf instance.ConfigReader) error { | |
return err | ||
} | ||
|
||
if d.inst != nil { | ||
instVolType, err := storagePools.InstanceTypeToVolumeType(d.inst.Type()) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if instVolType == volumeType && d.inst.Name() == volumeName { | ||
return errors.New("Instance root device cannot be attached to itself") | ||
} | ||
} | ||
|
||
// Derive the effective storage project name from the instance config's project. | ||
storageProjectName, err = project.StorageVolumeProject(d.state.DB.Cluster, instConf.Project().Name, dbVolumeType) | ||
if err != nil { | ||
|
@@ -1579,9 +1600,6 @@ func (d *disk) mountPoolVolume() (func(), string, *storagePools.MountInfo, error | |
return nil, "", nil, err | ||
} | ||
|
||
volStorageName := project.StorageVolume(storageProjectName, volumeName) | ||
srcPath := storageDrivers.GetVolumeMountPath(d.config["pool"], volumeType, volStorageName) | ||
|
||
mountInfo, err = d.pool.MountVolume(storageProjectName, volumeName, volumeType, nil) | ||
if err != nil { | ||
return nil, "", nil, fmt.Errorf(`Failed mounting storage volume "%s/%s" from storage pool %q: %w`, volumeTypeName, volumeName, d.pool.Name(), err) | ||
|
@@ -1600,6 +1618,15 @@ func (d *disk) mountPoolVolume() (func(), string, *storagePools.MountInfo, error | |
return nil, "", nil, fmt.Errorf("Failed to fetch local storage volume record: %w", err) | ||
} | ||
|
||
var volStorageName string | ||
if dbVolume.Type == cluster.StoragePoolVolumeTypeNameCustom { | ||
volStorageName = project.StorageVolume(storageProjectName, volumeName) | ||
} else { | ||
volStorageName = project.Instance(storageProjectName, volumeName) | ||
} | ||
|
||
srcPath := storageDrivers.GetVolumeMountPath(d.config["pool"], volumeType, volStorageName) | ||
|
||
if d.inst.Type() == instancetype.Container { | ||
if dbVolume.ContentType != cluster.StoragePoolVolumeContentTypeNameFS { | ||
return nil, "", nil, fmt.Errorf("Only filesystem volumes are supported for containers") | ||
|
@@ -1612,8 +1639,6 @@ func (d *disk) mountPoolVolume() (func(), string, *storagePools.MountInfo, error | |
} | ||
|
||
if dbVolume.ContentType == cluster.StoragePoolVolumeContentTypeNameBlock || dbVolume.ContentType == cluster.StoragePoolVolumeContentTypeNameISO { | ||
volStorageName := project.StorageVolume(storageProjectName, volumeName) | ||
|
||
volume := d.pool.GetVolume(volumeType, storageDrivers.ContentType(dbVolume.ContentType), volStorageName, dbVolume.Config) | ||
|
||
srcPath, err = d.pool.Driver().GetVolumeDiskPath(volume) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I unserstood the scope of this PR correctly, this should state
virtual machines
instead ofinstances
ascontainer
volumes won't be available for attaching.