From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 731A71FF0E0 for ; Thu, 09 Jul 2026 16:21:01 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id C78F42148A; Thu, 09 Jul 2026 16:21:00 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 09 Jul 2026 16:20:56 +0200 Message-Id: Subject: Re: [PATCH qemu-server] fix #7786: migrate: deactivate shared storage volumes of offline VMs From: "Daniel Kral" To: "Daniel Herzig" X-Mailer: aerc 0.21.0-147-g43ac7b685014-dirty References: <20260708073135.55426-1-d.herzig@proxmox.com> <87tsq8z3wp.fsf@proxmox.com> In-Reply-To: <87tsq8z3wp.fsf@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783606846595 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.010 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: QZGNQ5GRQNHHVT3RL6KDW7O3TKWFALFU X-Message-ID-Hash: QZGNQ5GRQNHHVT3RL6KDW7O3TKWFALFU X-MailFrom: d.kral@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu Jul 9, 2026 at 3:30 PM CEST, Daniel Herzig wrote: > Thanks for looking into this! > > "Daniel Kral" writes: > >>> @@ -846,6 +866,13 @@ sub phase1 { >>> $self->handle_replication($vmid); >>> =20 >>> $self->sync_offline_local_volumes(); >>> + >>> + # make sure that volumes of non-running VMs on shared storage >>> + # are deactivated. >>> + if (! $self->{running}) { >>> + $self->deactivate_volumes_on_shared_storage(); >>> + } >> >> As discussed off-list, I wonder whether this can be a >> >> if (!$self->{running}) { >> my $vollist =3D PVE::QemuServer::get_vm_volumes($conf); >> PVE::Storage::deactivate_volumes($self->{storecfg}, $vollist); >> } >> >> Just to be sure that there are no active volumes in the case of a >> offline migration. >> > I guess a call to `deactivate_volumes` without restricting to shared > storage (as in `deactivate_volumes_on_shared_storage()`) would also > do. But in such a case we won't have complications anyway, as > `sync_offline_local_volumes` is taking care of these with its own > call to `deactivate_volumes` in the end. > > I'm not against shortening down the code (will see what I can > do) -- but I'd like to keep the `eval->error->log` sequence and not > bloat `phase1` at the same time, as it's such a lean sub. > No hard feelings from my side, just thought that it might not hurt here as with an offline migration it seems natural that the _local_ volumes on the source node are deactivated with or without shared storage. But you're right, sync_offline_local_volumes() already does handle that for some volumes. >> I wonder whether there are any assumptions in later migration steps >> where this might break something that I'm not aware of? > > I'll double-check, but for now I think the issue is, that we > currently don't do anything with the activation state in case of an > offline migration on shared storage. Clearly the `if (!self->{running})` > check is important, as it would cause issues with online-migrations, > where volume deactivation is handled in a dedicated step. > Yeah, it might be worth to investigate if we can reduce the amount of time volumes are active when these are needed, e.g. after the disk resize, clone, and disk move without deleting the original volume. >> >>> + >>> $self->phase1_remote($vmid) if $self->{opts}->{remote}; >>> } >>> =20