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 [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id AB34B1FF16B
for <inbox@lore.proxmox.com>; Thu, 6 Mar 2025 13:07:26 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 26E254D5A;
Thu, 6 Mar 2025 13:07:20 +0100 (CET)
Message-ID: <57047be4-9880-42c1-98df-ac406256edc7@proxmox.com>
Date: Thu, 6 Mar 2025 13:07:15 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird Beta
To: Fiona Ebner <f.ebner@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250306104459.1272297-1-d.csapak@proxmox.com>
<20250306104459.1272297-2-d.csapak@proxmox.com>
<ef6b621c-1821-4966-aa9a-31150af32e52@proxmox.com>
Content-Language: en-US
From: Dominik Csapak <d.csapak@proxmox.com>
In-Reply-To: <ef6b621c-1821-4966-aa9a-31150af32e52@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.021 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] [RFC PATCH qemu-server 1/8] tests: cfg2cmd: pin
QEMU version
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-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
On 3/6/25 13:00, Fiona Ebner wrote:
> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>> but warn when we're out of date compared to the installed one, and die
>> when we're one major (+1 minor) release behind.
>> (the warning is not very visible when running tests or when building)
>>
>> We don't want to depend on the installed QEMU version for such tests,
>> otherwise a developer might need to adapt tests because the installed
>> QEMU version is different to what is e.g. in the build environment for
>> our packaging.
>>
>> Also, this way, we have to show intent for bumping this and it will be
>> obvious that we need to adapt tests because of a changed QEMU version.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> test/run_config2command_tests.pl | 23 ++++++++++++++++++++++-
>> 1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
>> index 7e3d10e6..5fa6f2de 100755
>> --- a/test/run_config2command_tests.pl
>> +++ b/test/run_config2command_tests.pl
>> @@ -20,6 +20,10 @@ use PVE::QemuServer::Monitor;
>> use PVE::QemuServer::QMPHelpers;
>> use PVE::QemuServer::CPUConfig;
>>
>> +# bump when QEMU version changes
>> +my $tested_version_major = 9;
>> +my $tested_version_minor = 2;
>> +
>> my $base_env = {
>> storage_config => {
>> ids => {
>> @@ -80,6 +84,7 @@ my $base_env = {
>> }
>> },
>> vmid => 8006,
>> + tested_qemu_version => "$tested_version_major.$tested_version_minor",
>> real_qemu_version => PVE::QemuServer::Helpers::kvm_user_version(), # not yet mocked
>
> I'd not keep the real_qemu_version in base_env anymore, but have it as a
> stand-alone variable for the version comparison check.
makes sense
>
>> };
>>
>> @@ -184,7 +189,7 @@ sub parse_test($) {
>> }
>>
>> sub get_test_qemu_version {
>> - $current_test->{qemu_version} // $base_env->{real_qemu_version} // '2.12';
>> + $current_test->{qemu_version} // $base_env->{tested_qemu_version} // '2.12';
>> }
>>
>> my $qemu_server_module;
>> @@ -528,3 +533,19 @@ if (my $file = shift) {
>> }
>>
>> done_testing();
>
> Nit: Since the check below can die, I'd put it at the very beginning
> rather than at the end.
mhmm ok, the rationale was that when letting the tests run manually,
the warning can be seen when it's at the end, but if it's printed at the
start, it'll most likely be overlooked...
i played around with requiring input from the user to continue,
but that didn't work at all when building (and is probably not
a good idea in general when running tests..)
Not sure how we can make the warning more visible while not failing the
tests at the same time...
>
>> +
>> +# reset warn
>
> IMHO "Why is the reset needed?" is the interesting part worth commenting
> here ;)
true ;), we override the warn handler above to check the warning for the expected
ones, we could of course add some code there to handle this case here,
but it seemed shorter/more elegant to just reset the handler completely
when we're done with testing...
>
>> +$SIG{__WARN__} = undef;
>> +if ($base_env->{real_qemu_version} =~ m/^(\d+.\d+)/) {
>> + if (PVE::QemuServer::Helpers::version_cmp($1, $tested_version_major, $2, $tested_version_minor) < 0) {
>> + warn "\nWARNING: installed QEMU version bigger than tested one, please bump!\n";
>> + }
>> +
>> + # if we did not bump since the last major QEMU (+1 minor) release fail the test
>> + if (PVE::QemuServer::Helpers::version_cmp($1, $tested_version_major + 1, $2, 1) >= 0) {
>
> I'd rather have a fixed difference between versions trigger the die.
> Your check behaves the same when the tested version is 10.0 and when the
> tested version is 10.2. In both cases, having the real version 11.1 will
> trigger the die.
>
not exactly sure what you mean here:
IIUC, in the scenario where we pinned X.Y, you want the test to fail when the real version is:
X+1.Y ?
or when
X+1.Y+1 ?
or when X+1.Y-1 ?
my thought here was that we want to update at least once per major release (not sure
if that means anything for qemu though), but leave a wiggle room until the first point release
>> + die "\nERROR: installed QEMU version one major release bigger than tested one, please bump!\n";
>> + }
>> +
>> +} else {
>> + die "\nERROR: invalid QEMU version\n";
>> +}
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel