From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH v2 qemu-server 1/5] disk reassign: add API endpoint
Date: Thu, 03 Sep 2020 09:46:35 +0200 [thread overview]
Message-ID: <1599118042.6ymt99i7i5.astroid@nora.none> (raw)
In-Reply-To: <20200901124421.25901-2-a.lauterer@proxmox.com>
On September 1, 2020 2:44 pm, Aaron Lauterer wrote:
> The goal of this new API endpoint is to provide an easy way to move a
> disk between VMs as this was only possible with manual intervention
> until now. Either by renaming the VM disk or by manually adding the
> disks volid to the config of the other VM.
>
> The latter can easily cause unexpected behavior such as disks attached
> to VM B would be deleted if it used to be a disk of VM A. This happens
> because PVE assumes that the VMID in the volname always matches the VM
> the disk is attached to and thus, would remove any disk with VMID A
> when VM A was deleted.
>
> The term `reassign` was chosen as it is not yet used
> for disk VMs.
>
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> v1 -> v2: print config key and volid info at the end of the job so it
> shows up on the CLI and task log
>
> rfc -> v1:
> * add support to reassign unused disks
> * add support to provide a config digest for the target vm
> * add additional check if disk key is present in config
> * reorder checks a bit
>
> In order to support unused disk I had to extend
> PVE::QemuServer::Drive::valid_drive_names for the API parameter
> validation.
>
> Checks are ordered so that cheap tests are run at the first chance to
> fail early.
>
> The check if both VMs are present on the node is a bit redundant because
> locking the config files will fail if the VM is not present. But with
> the additional check we can provide a useful error message to the user
> instead of a "Configuration file xyz does not exist" error.
>
> Feedback, especially on the checks is welcome.
>
>
> PVE/API2/Qemu.pm | 108 ++++++++++++++++++++++++++++++++++++++++
> PVE/QemuServer/Drive.pm | 4 ++
> 2 files changed, 112 insertions(+)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 8da616a..45aadbf 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4265,4 +4265,112 @@ __PACKAGE__->register_method({
> return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type});
> }});
>
> +__PACKAGE__->register_method({
> + name => 'reassign_vm_disk',
> + path => '{vmid}/reassign_disk',
> + method => 'POST',
> + protected => 1,
> + proxyto => 'node',
> + description => "Reassign a disk to another VM",
> + permissions => {
> + description => "You need 'VM.Config.Disk' permissions on /vms/{vmid}, and 'Datastore.Allocate' permissions on the storage.",
> + check => [ 'and',
> + ['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]],
> + ['perm', '/storage/{storage}', [ 'Datastore.Allocate' ]],
> + ],
> + },
> + parameters => {
> + additionalProperties => 0,
> + properties => {
> + node => get_standard_option('pve-node'),
> + vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
> + target_vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid }),
> + disk => {
> + type => 'string',
> + description => "The config key of the disk to move (for example, ide0 or scsi1).",
> + enum => [PVE::QemuServer::Drive::valid_drive_names_with_unused()],
> + },
> + digest => {
> + type => 'string',
> + description => 'Prevent changes if current the configuration file of the source VM has a different SHA1 digest. This can be used to prevent concurrent modifications.',
> + maxLength => 40,
> + optional => 1,
> + },
> + target_digest => {
> + type => 'string',
> + description => 'Prevent changes if current the configuration file of the target VM has a different SHA1 digest. This can be used to prevent concurrent modifications.',
> + maxLength => 40,
> + optional => 1,
> + },
> + },
> + },
> + returns => {
> + type => 'string',
> + description => "the task ID.",
> + },
> + code => sub {
> + my ($param) = @_;
> +
> + my $rpcenv = PVE::RPCEnvironment::get();
> + my $authuser = $rpcenv->get_user();
> +
> + my $node = extract_param($param, 'node');
> + my $vmid = extract_param($param, 'vmid');
> + my $target_vmid = extract_param($param, 'target_vmid');
> + my $digest = extract_param($param, 'digest');
> + my $target_digest = extract_param($param, 'target_digest');
> + my $disk = extract_param($param, 'disk');
> +
> + my $storecfg = PVE::Storage::config();
> +
> + die "You cannot reassign a disk to the same VM\n"
> + if $vmid eq $target_vmid;
> +
> + my $vmlist = PVE::QemuServer::vzlist();
> + die "Both VMs need to be on the same node\n"
> + if !$vmlist->{$vmid}->{exists} || !$vmlist->{$target_vmid}->{exists};
we could skip this if the target storage is shared, and let the user run
'rescan' afterwards?
> +
> + return PVE::QemuConfig->lock_config($vmid, sub {
> + my $conf = PVE::QemuConfig->load_config($vmid);
> + PVE::QemuConfig->check_lock($conf);
> +
> + die "Source VM config checksum missmatch (file change by other user?)\n"
> + if $digest && $digest ne $conf->{digest};
PVE::Tools::assert_if_modified
> +
> + die "Disk '${disk}' does not exist\n"
> + if !exists($conf->{$disk});
why not defined?
> +
> + my $drive = PVE::QemuServer::parse_drive($disk, $conf->{$disk});
> + die "disk '$disk' has no associated volume\n" if !$drive->{file};
missing check for whether it's actually volume-based, and not
pass-through.. what about templates/base volumes/linked
clones/snapshots?
> + die "Cannot reassign a cdrom drive\n" if PVE::QemuServer::drive_is_cdrom($drive, 1);
"CD drive contents can't be reassigned.\n"
> +
> + die "Cannot reassign disk while the source VM is running\n"
> + if PVE::QemuServer::check_running($vmid) && $disk !~ m/unused[0-9]/;
> +
> + return PVE::QemuConfig->lock_config($target_vmid, sub {
> + my $target_conf = PVE::QemuConfig->load_config($target_vmid);
> + PVE::QemuConfig->check_lock($target_conf);
> +
> + die "Target VM config checksum missmatch (file change by other user?)\n"
> + if $target_digest && $target_digest ne $target_conf->{digest};
same as above.
> +
> + PVE::Cluster::log_msg('info', $authuser, "reassign disk VM $vmid: reassign --disk $disk --target_vmid $target_vmid");
> +
> + my $realcmd = sub {
> + my $new_volid = PVE::Storage::reassign_volume($storecfg, $drive->{file}, $target_vmid);
> +
> + delete $conf->{$disk};
> + PVE::QemuConfig->write_config($vmid, $conf);
> +
> + my $key = PVE::QemuConfig->add_unused_volume($target_conf, $new_volid);
this can fail, now the volume is unreferenced altogether -> maybe wrap
in an eval and print a more helpful error message including the new
volid?
> + PVE::QemuConfig->write_config($target_vmid, $target_conf);
> +
> + print "reassigned disk to VM ${target_vmid} as '${key}: ${new_volid}'\n";
or split this up, and
print 'renamed volume '$old_volid' to '$new_volid'\n";
after the call to reassign_volume, and print the config changes
individually after the respective actions?
print "remove disk '$disk' from VM '$vmid'\n";
print "added volume '$new_volid' as '$key' to VM '$target_vmid'\n";
> + };
> +
> + return $rpcenv->fork_worker('qmreassign', $vmid, $authuser, $realcmd);
I am not so happy about
lock_config(
load_config;
lock_config(
load_config;
fork_worker(
cfs_lock/write_config
)
)
)
as it tends to accumulate more checks and extra functionality and at
some point might introduce an inconsistency.. note that the current
version seems fine (we always lock first then load, we only look at
config files that belong to a specific node, ...), but maybe we could
switch to
# check for early return/nice error
(conf1, conf2) = load_and_check_sub(id1, id2);
fork_worker(
lock_config(
lock_config(
# recheck in locked worker context
(conf1, conf2) = load_and_check_sub(id1, id2);
...
)
)
)
to make it more fool/future-proof, especially since the checks are rather cheap..
> + });
> + });
> + }});
> +
> 1;
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 91c33f8..d2f59cd 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -383,6 +383,10 @@ sub valid_drive_names {
> 'efidisk0');
> }
>
> +sub valid_drive_names_with_unused {
> + return (valid_drive_names(), map {"unused$_"} (0 .. ($MAX_UNUSED_DISKS -1)));
> +}
> +
> sub is_valid_drivename {
> my $dev = shift;
>
> --
> 2.20.1
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
next prev parent reply other threads:[~2020-09-03 7:46 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-01 12:44 [pve-devel] [PATCH v2 series 0/5] disk reassign: add new feature Aaron Lauterer
2020-09-01 12:44 ` [pve-devel] [PATCH v2 qemu-server 1/5] disk reassign: add API endpoint Aaron Lauterer
2020-09-03 7:46 ` Fabian Grünbichler [this message]
2020-09-03 8:30 ` Aaron Lauterer
2020-09-03 9:07 ` Fabian Grünbichler
2020-09-01 12:44 ` [pve-devel] [PATCH v2 qemu-server 2/5] cli: disk reassign: add reassign_disk to qm command Aaron Lauterer
2020-09-01 12:44 ` [pve-devel] [PATCH v2 storage 3/5] add disk reassign feature Aaron Lauterer
2020-09-03 7:55 ` Fabian Grünbichler
2020-09-01 12:44 ` [pve-devel] [PATCH v2 storage 4/5] disk reassign: add not implemented yet message to storages Aaron Lauterer
2020-09-03 7:58 ` Fabian Grünbichler
2020-09-03 9:01 ` Aaron Lauterer
2020-09-03 9:06 ` Aaron Lauterer
2020-09-03 9:19 ` Fabian Grünbichler
2020-09-01 12:44 ` [pve-devel] [PATCH v2 widget-toolkit 5/5] utils: task_desc_table: add qmreassign Aaron Lauterer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1599118042.6ymt99i7i5.astroid@nora.none \
--to=f.gruenbichler@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox