public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Fiona Ebner <f.ebner@proxmox.com>
To: Dominik Csapak <d.csapak@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [RFC PATCH qemu-server 1/8] tests: cfg2cmd: pin QEMU version
Date: Thu, 6 Mar 2025 13:43:16 +0100	[thread overview]
Message-ID: <211e286c-2b7b-4d0c-8719-f28bf46098e6@proxmox.com> (raw)
In-Reply-To: <57047be4-9880-42c1-98df-ac406256edc7@proxmox.com>

Am 06.03.25 um 13:07 schrieb Dominik Csapak:
> On 3/6/25 13:00, Fiona Ebner wrote:
>> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>>> @@ -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...
Thinking again about the whole pinning, I'm not sure we really want to.
If there are changes based on the binary version (rare), I do want to
have the actual tests be run immediately. Tests should not change
between binary versions without intent. The exception is here, with the
bumped pve version. And I don't mean the bumping itself, that has intent
and is fine. If we bump the pve version, that QEMU version is already
public (or we could've still made the change based on the QEMU version
rather than pve version ;)). The actual issue is with QEMU 10.0 where we
will need to adapt the tests for un-bumping the pve version again. That
does not have intent and that will temporarily break build.

Maybe we can still use the installed version for running tests by
default. We can add an environment variable or similar for overriding
the test version. Additionally, record the expected version in the test
script and die if it doesn't match the installed binary, suggesting to
use the override.

Then the package can still be built by setting the environment variable
as an escape hatch. While usual builds will use and test the current
version that will be running on people's systems.

> 
>>
>>> +
>>> +# 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 ?

Yes, for example. I don't think we should wait longer.

> 
> or when
> 
> X+1.Y+1 ?

That release might not exist, e.g. 10.3 will never exist. Of course you
can make what you want work by distinguishing based on major version
difference.

> 
> or when X+1.Y-1 ?

Similar here.

> 
> 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

Yes, but I'd prefer being reminded after a fixed difference.


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

  reply	other threads:[~2025-03-06 12:43 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-06 10:44 [pve-devel] [PATCH qemu-server 0/8] disable S3/S4 power states by default Dominik Csapak
2025-03-06 10:44 ` [pve-devel] [RFC PATCH qemu-server 1/8] tests: cfg2cmd: pin QEMU version Dominik Csapak
2025-03-06 12:00   ` Fiona Ebner
2025-03-06 12:07     ` Dominik Csapak
2025-03-06 12:43       ` Fiona Ebner [this message]
2025-03-06 10:44 ` [pve-devel] [PATCH qemu-server 2/8] config to command: add one '-global' option for each flag Dominik Csapak
2025-03-06 12:13   ` Fiona Ebner
2025-03-06 12:15     ` Dominik Csapak
2025-03-06 12:55       ` Fiona Ebner
2025-03-07  9:54         ` Dominik Csapak
2025-03-07 10:00           ` Fiona Ebner
2025-03-07 10:05             ` Dominik Csapak
2025-03-07 10:14               ` Fiona Ebner
2025-03-06 10:44 ` [pve-devel] [PATCH qemu-server 3/8] meta info: also add current pve-machine version Dominik Csapak
2025-03-06 10:44 ` [pve-devel] [PATCH qemu-server 4/8] machine: correctly select pve machine version for non pinned windows guests Dominik Csapak
2025-03-06 13:10   ` Fiona Ebner
2025-03-06 13:36     ` Dominik Csapak
2025-03-06 14:10       ` Fiona Ebner
2025-03-06 14:14         ` Fiona Ebner
2025-03-06 14:15         ` Dominik Csapak
2025-03-06 14:20           ` Fiona Ebner
2025-03-07  9:55             ` Dominik Csapak
2025-03-06 10:44 ` [pve-devel] [PATCH qemu-server 5/8] machine: incorporate pve machine version when pinning " Dominik Csapak
2025-03-06 14:32   ` Fiona Ebner
2025-03-07  9:58     ` Dominik Csapak
2025-03-07 10:06       ` Fiona Ebner
2025-03-06 10:44 ` [pve-devel] [PATCH qemu-server 6/8] machine: add S3/S4 power state properties Dominik Csapak
2025-03-06 14:52   ` Fiona Ebner
2025-03-07 10:02     ` Dominik Csapak
2025-03-07 10:10       ` Fiona Ebner
2025-03-06 10:44 ` [pve-devel] [PATCH qemu-server 7/8] machine: bump pve machine version and reverse the s3/s4 defaults Dominik Csapak
2025-03-06 15:04   ` Fiona Ebner
2025-03-06 10:44 ` [pve-devel] [PATCH qemu-server 8/8] tests: cfg2cmd: add test for windows machine pinning from meta info Dominik Csapak
2025-03-06 15:10   ` Fiona Ebner
2025-03-07 14:46 ` [pve-devel] [PATCH qemu-server 0/8] disable S3/S4 power states by default Dominik Csapak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=211e286c-2b7b-4d0c-8719-f28bf46098e6@proxmox.com \
    --to=f.ebner@proxmox.com \
    --cc=d.csapak@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal