Re: [pve-devel] [PATCH manager/storage v4 0/5] fix #4997: lvm, lvm-thin: avoid autoactivating LVs

From: Friedrich Weber <f.weber@proxmox.com>
To: Thomas Lamprecht <t.lamprecht@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH manager/storage v4 0/5] fix #4997: lvm, lvm-thin: avoid autoactivating LVs
Date: Tue, 8 Jul 2025 12:16:52 +0200	[thread overview]
Message-ID: <15453d2c-405a-4a70-b2eb-90ccc410f5b9@proxmox.com> (raw)
In-Reply-To: <cf065c51-4a9a-4f28-837a-db5399349cd3@proxmox.com>

On 08/07/2025 10:38, Thomas Lamprecht wrote:
> Thanks for your polished submission, very easy to digest!
> 
> Am 07.07.25 um 10:03 schrieb Friedrich Weber:
>> # pve8to9 script
>>
>> As discussed in v2, this series implements
>>
>> (a) a pve8to9 check to detect thick and thin LVs with autoactivation enabled
>> (b) a script to disable autoactivation on LVs when needed, intended to be run
>>     manually by the user during 8->9 upgrade
>>
>> The question is where to put the script (b). Patch #4 moves the existing checks
>> from `pve8to9` to `pve8to9 checklist`, to be able to implement (b) as a new
>> subcommand `pve8to9 updatelvm`. I realize this is a huge user-facing change,
>> and we don't have to go with this approach. It is also incomplete, as patch #5
>> doesn't update the manpage yet. However, I like about this approach that
>> pve8to9 bundles "tasks that are related to 8->9 upgrades". If we do decide to
>> go with this, I can send another patch to update the manpage as well as add
>> documentation.
> 
> I'd prefer shipping the update in it's own script and outside of path.
> We should ship that either below /usr/libexec, or–maybe even nicer–in a
> package specific /usr/share path, like e.g. inside:
> /usr/share/pve-manager/migrations/...
> 
> The checker script would then output the respective path to use.

OK, then I'll prepare a v5 that moves the `updatelvm` code to a script
under /usr/share/pve-manager/migrations.

> Keeping such migration scripts, that actively change the host, independent
> from the XtoY scripts avoids the need for multiple copies for multiple XtoY
> scripts, as often the next future version might benefit from still having
> these. Having a strict boundary between idempotent checks and code that
> actually changes the hosts seems a bit safer to me too, especially if the
> latter is not accessible via $PATH [...]

Yeah, fair point.

> [...] and also because we execute commands even
> if they are not specified completely but if the user supplied string uniquely
> matches the start of an existing command, like "u" or "update" in this case
> here would be enough to trigger the command.

Right, that might be a bit dangerous. But, thinking about it, even if we
move the `updatelvm` code to a migration helper script, it could still
ask the user for confirmation before actually making changes (e.g.
"Disable autoactivation for all guest volumes in VG 'foo'? [Y/n]").

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel