From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 147981FF2C7
for <inbox@lore.proxmox.com>; Wed, 24 Jul 2024 15:05:56 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 45CC51C3E3;
Wed, 24 Jul 2024 15:06:31 +0200 (CEST)
Message-ID: <c276a8c8-9e28-44f1-8515-7d96c4252106@proxmox.com>
Date: Wed, 24 Jul 2024 15:05:57 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Markus Frank <m.frank@proxmox.com>
References: <20240529122348.1267369-1-m.frank@proxmox.com>
<20240529122348.1267369-4-m.frank@proxmox.com>
Content-Language: en-US
From: Fiona Ebner <f.ebner@proxmox.com>
In-Reply-To: <20240529122348.1267369-4-m.frank@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.060 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH qemu-server v11 3/5] migration: add
check_non_migratable_resources function
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
Am 29.05.24 um 14:23 schrieb Markus Frank:
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 2a1d4d7..8f36cf8 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4504,7 +4504,7 @@ __PACKAGE__->register_method({
> $res->{running} = PVE::QemuServer::check_running($vmid) ? 1:0;
>
> my ($local_resources, $mapped_resources, $missing_mappings_by_node) =
> - PVE::QemuServer::check_local_resources($vmconf, 1);
> + PVE::QemuServer::check_local_resources($vmconf, $res->{running}, 1);
> delete $missing_mappings_by_node->{$localnode};
>
> my $vga = PVE::QemuServer::parse_vga($vmconf->{vga});
> @@ -5192,6 +5192,9 @@ __PACKAGE__->register_method({
> die "unable to use snapshot name 'pending' (reserved name)\n"
> if lc($snapname) eq 'pending';
>
> + my $vmconf = PVE::QemuConfig->load_config($vmid);
> + PVE::QemuServer::check_non_migratable_resources($vmconf, $param->{vmstate}, 0);
This can be fine as an early check. But there should be another check
after the 'snapshot' lock was set in the config (or right before while
still holding the config file lock). Otherwise, there is no guarantee
that the config is the same at the time of the check and during the
actual snapshot operation.
My suggestion is to introduce a new abstract method (named something
like __snapshot_assert_no_blockers()) in AbstractConfig that is called
during __snapshot_prepare() which QemuConfig can override. Like that we
can do the check before writing the modified config for the snapshot.
> +
> my $realcmd = sub {
> PVE::Cluster::log_msg('info', $authuser, "snapshot VM $vmid: $snapname");
> PVE::QemuConfig->snapshot_create($vmid, $snapname, $param->{vmstate},
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 31a7ee9..9f342bf 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2563,13 +2563,32 @@ sub config_list {
> return $res;
> }
>
> +sub check_non_migratable_resources {
> + my ($conf, $state, $noerr) = @_;
> +
> + my @non_migr_res = ();
Style nit: I'd prefer writing out the name or use something like
"blockers" (inside the function where it's clear what is meant) if it
really is too long
> + if ($state && $conf->{amd_sev}) {
> + push @non_migr_res, "amd_sev";
> + }
> +
> + if (scalar @non_migr_res && !$noerr) {
Style nit: I'd prefer parentheses for scalar(@stuff)
> + die "Cannot live-migrate, snapshot (with RAM), or hibernate a VM with:"
> + ." @non_migr_res\n";
> + }
> +
> + return @non_migr_res;
> +}
> +
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel