From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 94DA31FF16B for ; Thu, 6 Mar 2025 14:36:50 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0520A775A; Thu, 6 Mar 2025 14:36:44 +0100 (CET) Message-ID: <034ab693-bb30-4deb-a7e2-a2e177ad6884@proxmox.com> Date: Thu, 6 Mar 2025 14:36:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Fiona Ebner , Proxmox VE development discussion References: <20250306104459.1272297-1-d.csapak@proxmox.com> <20250306104459.1272297-5-d.csapak@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL 0.022 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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 4/8] machine: correctly select pve machine version for non pinned windows guests X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On 3/6/25 14:10, Fiona Ebner wrote: > Am 06.03.25 um 11:44 schrieb Dominik Csapak: >> when we don't have a specific machine version on a windows guest, we use >> the creation meta info to pin the machine version. Currently we always >> append the pve machine version from the current installed kvm version, >> which is not necessarily the version we pinned the guest to. >> >> Instead, use either the info from the creation meta info if it exists, >> or use 'pve0'. >> >> For non-windows machines, we used the current QEMU machine version so we >> should use the pve machine version from that too. >> >> Signed-off-by: Dominik Csapak >> --- >> PVE/QemuServer/Machine.pm | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm >> index f1acde8f..e3da8e21 100644 >> --- a/PVE/QemuServer/Machine.pm >> +++ b/PVE/QemuServer/Machine.pm >> @@ -237,14 +237,19 @@ sub get_vm_machine { >> if (PVE::QemuServer::Helpers::min_version($meta->{'creation-qemu'}, 9, 1)) { >> # need only major.minor >> ($base_version) = ($meta->{'creation-qemu'} =~ m/^(\d+.\d+)/); >> + # append pve machine version if we have one >> + if (my $pvever = $meta->{'creation-pve-machine'}) { >> + $base_version .= "+pve$pvever" > > Since this is only the fallback handling for the rare edge case where no > explicit machine version is set for a Windows guest, not sure if it's > even worth doing this. I.e. can we just avoid the additional meta > property and always use pve0 here? Did you intend any other use for the > creation-pve-machine? > I though the intention of the code was to pin the guest to that version where it was created. If we omit the machine version, this is not true anymore, since I'd then create a windows vm with e.g. pc-q35-9.2+pve1 then set it to q35, and would get pc-q35-9.2+pve0 and while i don't have anymore use cases for the current case, wouldn't this approach here correct also when we decide to bump again in the future? also recording with which pve version the vm was created could help in debugging issues at some point.. (my patch only adds the version in the meta info if it's > 0 anyway) > Further below we already have: > >> # for version-pinned machines that do not include a pve-version (e.g. >> # pc-q35-4.1), we assume 0 to keep them stable in case we bump >> $machine .= '+pve0'; > > so let's just follow this here too? this here make sense IMHO, since the user wanted a specific version but without pve versions info, so default to pve0 sounds fine to me? > > >> + } >> } >> } >> $machine = windows_get_pinned_machine_version($machine, $base_version, $kvmversion); >> + } else{ >> + $arch //= 'x86_64'; >> + $machine ||= default_machine_for_arch($arch); >> + my $pvever = get_pve_version($kvmversion); >> + $machine .= "+pve$pvever"; >> } >> - $arch //= 'x86_64'; >> - $machine ||= default_machine_for_arch($arch); >> - my $pvever = get_pve_version($kvmversion); >> - $machine .= "+pve$pvever"; >> } >> >> if ($machine !~ m/\+pve\d+?(?:\.pxe)?$/) { > _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel