Re: [PATCH qemu-server v3 1/3] cleanup: refactor to make cleanup flow consistent

[Fiona Ebner <f.ebner@proxmox.com>]

Am 26.02.26 um 3:07 PM schrieb Dominik Csapak:
> diff --git a/debian/preinst b/debian/preinst
> new file mode 100755
> index 00000000..1697020f
> --- /dev/null
> +++ b/debian/preinst
> @@ -0,0 +1,16 @@
> +#!/bin/sh
> +
> +set -e
> +

Missing
#DEBHELPER#

> +case "$1" in
> +    upgrade)
> +        if dpkg --compare-versions "$2" 'lt' '9.1.5'; then

As you already anticipated, a bump is needed here.

> +            # set use_old_cleanup flag file so the old cleanup code will be used until reboot
> +            touch /run/qemu-server/use_old_cleanup
> +        fi
> +    ;;
> +    install|abort-install)
> +    ;;
> +esac
> +
> +exit 0

---snip 8<---

> diff --git a/src/PVE/QemuServer/RunState.pm b/src/PVE/QemuServer/RunState.pm
> index 6a5fdbd7..e2bd4d94 100644
> --- a/src/PVE/QemuServer/RunState.pm
> +++ b/src/PVE/QemuServer/RunState.pm
> @@ -6,6 +6,7 @@ use warnings;
>  use POSIX qw(strftime);
>  
>  use PVE::Cluster;
> +use PVE::File;
>  use PVE::RPCEnvironment;
>  use PVE::Storage;
>  
> @@ -183,4 +184,31 @@ sub vm_resume {
>      );
>  }
>  
> +my sub get_cleanup_path {
> +    my ($vmid) = @_;
> +    return "/var/run/qemu-server/$vmid.cleanup";

Nit: our code is already mixing /var/run/qemu-server and
/run/qemu-server. I'd suggest using the latter for new usages (part of
your patch already does). It's auto-created by systemd-tmpfiles now.

> +}
> +
> +sub create_cleanup_flag {
> +    my ($vmid) = @_;
> +    PVE::File::file_set_contents(get_cleanup_path($vmid), time());

Is writing the time a precaution if we need future adaptations or is
that actually used somewhere? Might be worth a comment what it's for.

> +}
> +
> +sub clear_cleanup_flag {
> +    my ($vmid) = @_;
> +    my $path = get_cleanup_path($vmid);
> +    unlink $path or $! == POSIX::ENOENT or die "removing cleanup flag for $vmid failed: $!\n";
> +}
> +
> +sub cleanup_flag_exists {
> +    my ($vmid) = @_;
> +    return -f get_cleanup_path($vmid);
> +}
> +
> +# checks if /run/qemu-server/use_old_cleanup exists that will be created on
> +# package update and cleared on bootup so we can be sure the guests were
> +# started recently enough
> +sub can_use_cleanup_flag {
> +    !-f "/run/qemu-server/use_old_cleanup";
> +}

Style nit: missing blank here

>  1;
> diff --git a/src/test/MigrationTest/QmMock.pm b/src/test/MigrationTest/QmMock.pm
> index 78be47d3..4d5fa3ac 100644
> --- a/src/test/MigrationTest/QmMock.pm
> +++ b/src/test/MigrationTest/QmMock.pm
> @@ -77,6 +77,22 @@ $qemu_server_helpers_module->mock(
>      },
>  );
>  
> +my $qemu_server_runstate_module = Test::MockModule->new("PVE::QemuServer::RunState");
> +$qemu_server_runstate_module->mock(
> +    create_cleanup_flag => sub {
> +        return undef;
> +    },
> +    clear_cleanup_flag => sub {
> +        return undef;
> +    },
> +    cleanup_flag_exists => sub {
> +        return 1;

Wouldn't it be nicer to use an actual path the test may write to (i.e.
the migration test run dir) and only mock get_cleanup_path() instead of
the above three functions (if making get_cleanup_path() public for
mocking, the name should be less generic and include 'flag').

> +    },
> +    can_use_cleanup_flag => sub {
> +        return 1;
> +    },
> +);
> +
>  our $qemu_server_machine_module = Test::MockModule->new("PVE::QemuServer::Machine");
>  $qemu_server_machine_module->mock(
>      get_current_qemu_machine => sub {