public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs
@ 2025-01-13  8:55 Daniel Herzig
  2025-01-13  8:55 ` [pve-devel] [PATCH v2 1/12] fix #4225: qemuserver: drive: add optional required parameter Daniel Herzig
                   ` (12 more replies)
  0 siblings, 13 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:55 UTC (permalink / raw)
  To: pve-devel

This patch series addresses bugzilla entry #4225.

Currently VMs refuse to to start if a configured isofile becomes unavailable,
be it a deleted file or an unavailable network storage.

This patch series introduces a new parameter in Drive.pm, called 'required'.
Depending on whether this parameter is set or not, the situation will be handled
differently.

If the parameter is set to 0, the configuration will temporarily changed to use
'none' as file for the cd drive, which allows qemu to start up the machine.
The configuration is not changed in this process to avoid unexpected behaviour.
Instead a log_warn will be issued.

For transition reasons an unset parameter acts like 'required=1'. In this case
the startup process will die earlier than currently, if the file is missing or
the underlying storage not available.

If however a new VM is created from the WebGUI, the corresponding added checkbox
is not checked by default, and the resulting 'required=0' will be written to
the configuration.

To allow for proper testing and building some additions and minor changes
where made to to the testing framework as well.

Not exactly part of #4225, but related to it, this patch series adds an 'Eject'
button to the hardwareview in the WebGUI, which can be used as a convenience
shortcut to manually editing the missing ISO file to 'Do not use any media'.

This series supersedes:
https://lore.proxmox.com/pve-devel/20241025150243.134466-1-d.herzig@proxmox.com/

Changes from initial series:
* rebased onto current master
* fix cifs mocking in run_config2command_tests.pl
* fix expected outcome in ide-required.conf.cmd

qemu-server: Daniel Herzig (9):
  fix #4225: qemuserver: drive: add optional required parameter
  qemuserver: add helper function for mocking files
  fix #4225: qemuserver: add function to eject isofiles
  test: chomp all trailing newlines from errors and warnings
  test: mock cifs-store
  test: add nfs-offline storage
  test: mock existing files
  test: mock log_warn warnings
  test: cfg2cmd: add tests for testing the iso required parameter

 PVE/QemuServer.pm                             | 44 +++++++++++++++++++
 PVE/QemuServer/Drive.pm                       |  9 +++-
 test/cfg2cmd/ide-required-iso-missing.conf    | 12 +++++
 .../cfg2cmd/ide-required-iso-missing.conf.cmd |  0
 .../cfg2cmd/ide-required-iso-offline-nfs.conf | 12 +++++
 .../ide-required-iso-offline-nfs.conf.cmd     |  0
 test/cfg2cmd/ide-required.conf                | 14 ++++++
 test/cfg2cmd/ide-required.conf.cmd            | 39 ++++++++++++++++
 test/cfg2cmd/ide-unrequired-iso-missing.conf  | 12 +++++
 .../ide-unrequired-iso-missing.conf.cmd       | 33 ++++++++++++++
 .../ide-unrequired-iso-offline-nfs.conf       | 12 +++++
 .../ide-unrequired-iso-offline-nfs.conf.cmd   | 33 ++++++++++++++
 test/run_config2command_tests.pl              | 44 ++++++++++++++++++-
 13 files changed, 261 insertions(+), 3 deletions(-)
 create mode 100644 test/cfg2cmd/ide-required-iso-missing.conf
 create mode 100644 test/cfg2cmd/ide-required-iso-missing.conf.cmd
 create mode 100644 test/cfg2cmd/ide-required-iso-offline-nfs.conf
 create mode 100644 test/cfg2cmd/ide-required-iso-offline-nfs.conf.cmd
 create mode 100644 test/cfg2cmd/ide-required.conf
 create mode 100644 test/cfg2cmd/ide-required.conf.cmd
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-missing.conf
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-missing.conf.cmd
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf.cmd

manager: Daniel Herzig (3):
  fix #4225: ui: form: isoselector: add optional required checkbox
  fix #4225: ui: qemu: cdedit: enable required checkbox for isos
  ui: qemu: hardware: add eject button for cdroms

 www/manager6/form/IsoSelector.js  | 22 ++++++++++++++++
 www/manager6/qemu/CDEdit.js       |  6 +++++
 www/manager6/qemu/HardwareView.js | 43 +++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+)


-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 1/12] fix #4225: qemuserver: drive: add optional required parameter
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
@ 2025-01-13  8:55 ` Daniel Herzig
  2025-01-13  8:55 ` [pve-devel] [PATCH v2 2/12] qemuserver: add helper function for mocking files Daniel Herzig
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:55 UTC (permalink / raw)
  To: pve-devel

Add parameter to allow for marking a drive as required.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 PVE/QemuServer/Drive.pm | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
index 7b298454..b816fbec 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -193,7 +193,14 @@ my %drivedesc_base = (
 	verbose_description => "Mark this locally-managed volume as available on all nodes.\n\nWARNING: This option does not share the volume automatically, it assumes it is shared already!",
 	optional => 1,
 	default => 0,
-    }
+    },
+    required => {
+	type => 'boolean',
+	description => 'Mark this iso volume as required for booting the VM.',
+	verbose_description => 'If unset or set to 1, and the iso file is unavailable, the VM will not start.\nThis parameter is considered for cdrom iso drives only.',
+	optional => 1,
+	default => 1,
+    },
 );
 
 my %iothread_fmt = ( iothread => {
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 2/12] qemuserver: add helper function for mocking files
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
  2025-01-13  8:55 ` [pve-devel] [PATCH v2 1/12] fix #4225: qemuserver: drive: add optional required parameter Daniel Herzig
@ 2025-01-13  8:55 ` Daniel Herzig
  2025-01-16 15:18   ` Daniel Kral
  2025-01-13  8:55 ` [pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles Daniel Herzig
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:55 UTC (permalink / raw)
  To: pve-devel

This stub function can be used for mocking a file's existance in testruns.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 PVE/QemuServer.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 5cde94a1..d07c170e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8999,4 +8999,9 @@ sub delete_ifaces_ipams_ips {
     }
 }
 
+sub file_exists {
+    my $file_path = shift;
+    return -e $file_path
+}
+
 1;
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
  2025-01-13  8:55 ` [pve-devel] [PATCH v2 1/12] fix #4225: qemuserver: drive: add optional required parameter Daniel Herzig
  2025-01-13  8:55 ` [pve-devel] [PATCH v2 2/12] qemuserver: add helper function for mocking files Daniel Herzig
@ 2025-01-13  8:55 ` Daniel Herzig
  2025-01-16 15:19   ` Daniel Kral
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 4/12] test: chomp all trailing newlines from errors and warnings Daniel Herzig
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:55 UTC (permalink / raw)
  To: pve-devel

Current behaviour prevents a VM from starting, if an ISO file defined
in the configuration becomes unavailable.

The function eject_nonrequired_isos checks on whether a cdrom drive is
marked as 'required' or not. If the parameter 'required' is not
defined, it will assume 'required' to be true and keep the current
behaviour.

If 'required' is set to 0, the function 'ejects' the ISO file by
setting the drive's file value to 'none', if the underlying storage is
unavailable or if the defined file is unavailable for another reason.

The function is called while config_to_command iterates over all
volumes to allow for early storage activation and early exit in the
case of missing required files.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 PVE/QemuServer.pm | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index d07c170e..f72878d3 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4041,6 +4041,8 @@ sub config_to_command {
     PVE::QemuConfig->foreach_volume($conf, sub {
 	my ($ds, $drive) = @_;
 
+	eject_nonrequired_isos($ds, $drive, $vmid, $storecfg, $conf);
+
 	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
 	    check_volume_storage_type($storecfg, $drive->{file});
 	    push @$vollist, $drive->{file};
@@ -8999,6 +9001,43 @@ sub delete_ifaces_ipams_ips {
     }
 }
 
+sub eject_nonrequired_isos {
+    my ($ds, $drive, $vmid, $storecfg, $conf) = @_;
+    # set 1 to exclude cloudinit. cloudinit isos are always required.
+    if (drive_is_cdrom($drive, 1)
+	&& $drive->{file} ne 'none'
+	&& $drive->{file} ne 'cdrom') {
+	$drive->{required} = 1 if !defined($drive->{required});
+	my $iso_volid = $drive->{file};
+	my $iso_path = get_iso_path($storecfg, $vmid, $drive->{file});
+	my $store_err;
+	if ($iso_volid !~ m|^/|) {
+	    my $iso_storage  = PVE::Storage::parse_volume_id($iso_volid, 1);
+	    eval { PVE::Storage::activate_storage($storecfg, $iso_storage); };
+	    $store_err = $@;
+	}
+	if ($store_err) {
+	    if ($drive->{required}) {
+		die "cannot access required file: '${ds}: ${iso_volid}': ${store_err}\n";
+	    } else {
+		log_warn("eject '${ds}: ${iso_volid}': ${store_err}");
+		$drive->{file} = 'none';
+		$conf->{$ds} = print_drive($drive);
+	    }
+	} else {
+	    if (!file_exists($iso_path)) {
+		if ($drive->{required}) {
+		    die "required file does not exist: '${ds}: ${iso_volid}'\n";
+		} else {
+		    log_warn("eject '${ds}: ${iso_volid}': file does not exist");
+		    $drive->{file} = 'none';
+		    $conf->{$ds} = print_drive($drive);
+		}
+	    }
+	}
+    }
+}
+
 sub file_exists {
     my $file_path = shift;
     return -e $file_path
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 4/12] test: chomp all trailing newlines from errors and warnings
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
                   ` (2 preceding siblings ...)
  2025-01-13  8:55 ` [pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles Daniel Herzig
@ 2025-01-13  8:56 ` Daniel Herzig
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 5/12] test: mock cifs-store Daniel Herzig
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:56 UTC (permalink / raw)
  To: pve-devel

Ease EXPECT_ERROR and EXPECT_WARN string matching with errors/warnings
that have more than one trailing newline.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 test/run_config2command_tests.pl | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index 5308b1fc..3f37e881 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -426,7 +426,9 @@ sub diff($$) {
 
 $SIG{__WARN__} = sub {
     my $warning = shift;
-    chomp $warning;
+    while ((chomp($warning))) {
+	chomp($warning);
+    }
     if (my $warn_expect = $current_test->{expect_warning}) {
 	if ($warn_expect ne $warning) {
 	    fail($current_test->{testname});
@@ -461,7 +463,9 @@ sub do_test($) {
 	    note("did NOT get any error, but expected error: $err_expect");
 	    return;
 	}
-	chomp $err;
+	while ((chomp($err))) {
+	    chomp($err);
+	}
 	if ($err ne $err_expect) {
 	    fail("$testname");
 	    note("error does not match expected error: '$err' !~ '$err_expect'");
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 5/12] test: mock cifs-store
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
                   ` (3 preceding siblings ...)
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 4/12] test: chomp all trailing newlines from errors and warnings Daniel Herzig
@ 2025-01-13  8:56 ` Daniel Herzig
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 6/12] test: add nfs-offline storage Daniel Herzig
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:56 UTC (permalink / raw)
  To: pve-devel

Let cifs-store appear as online to a call from
PVE::Storage::activate_storage.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 test/run_config2command_tests.pl | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index 3f37e881..a922086f 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -392,6 +392,25 @@ $pci_module->mock(
     }
 );
 
+my $pve_storage_plugin_module = Test::MockModule->new("PVE::Storage::Plugin");
+$pve_storage_plugin_module->mock(
+    activate_storage => sub {
+       return 1;
+    },
+);
+
+my $pve_storage_cifsplugin_module = Test::MockModule->new("PVE::Storage::CIFSPlugin");
+$pve_storage_cifsplugin_module->mock(
+    check_connection => sub {
+	return 1;
+    },
+    cifs_is_mounted => sub {
+	my ($scfg, $mountdata) = @_;
+	my ($mountpoint, $server, $share) = $scfg->@{'path', 'server', 'share'};
+	return $mountpoint;
+    },
+);
+
 sub diff($$) {
     my ($a, $b) = @_;
     return if $a eq $b;
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 6/12] test: add nfs-offline storage
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
                   ` (4 preceding siblings ...)
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 5/12] test: mock cifs-store Daniel Herzig
@ 2025-01-13  8:56 ` Daniel Herzig
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 7/12] test: mock existing files Daniel Herzig
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:56 UTC (permalink / raw)
  To: pve-devel

Add an nfs-offline storage to allow for comparatative testing
against potentially mocked online storages.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 test/run_config2command_tests.pl | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index a922086f..cfd7309b 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -49,6 +49,15 @@ my $base_env = {
 		    iso => 1,
 		},
 	    },
+	    'nfs-offline' => {
+		type => 'nfs',
+		export => '/srv/nfs/isos',
+		path => '/mnt/pve/nfs-offline',
+		server => '127.0.0.42',
+		content => {
+		    iso => 1,
+		},
+	    },
 	    'rbd-store' => {
 		monhost => '127.0.0.42,127.0.0.21,::1',
 		fsid => 'fc4181a6-56eb-4f68-b452-8ba1f381ca2a',
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 7/12] test: mock existing files
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
                   ` (5 preceding siblings ...)
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 6/12] test: add nfs-offline storage Daniel Herzig
@ 2025-01-13  8:56 ` Daniel Herzig
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 8/12] test: mock log_warn warnings Daniel Herzig
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:56 UTC (permalink / raw)
  To: pve-devel

Let all files checked by file_exists appear as existing if the path
does not contain the string 'I_DO_NOT_EXIST'.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 test/run_config2command_tests.pl | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index cfd7309b..71b00e9a 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -209,6 +209,10 @@ $qemu_server_module->mock(
     cleanup_pci_devices => {
 	# do nothing
     },
+    file_exists => sub {
+	my $file_path = shift;
+	return 1 if !($file_path =~ m|I_DO_NOT_EXIST|);
+    },
 );
 
 my $qemu_server_config;
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 8/12] test: mock log_warn warnings
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
                   ` (6 preceding siblings ...)
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 7/12] test: mock existing files Daniel Herzig
@ 2025-01-13  8:56 ` Daniel Herzig
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 9/12] test: cfg2cmd: add tests for testing the iso required parameter Daniel Herzig
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:56 UTC (permalink / raw)
  To: pve-devel

Strip log_warn wrapper for catching warnings on testruns.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 test/run_config2command_tests.pl | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index 71b00e9a..8df4f4e8 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -213,6 +213,10 @@ $qemu_server_module->mock(
 	my $file_path = shift;
 	return 1 if !($file_path =~ m|I_DO_NOT_EXIST|);
     },
+    log_warn => sub {
+	my $logwarn = shift;
+	return warn("${logwarn}\n");
+    },
 );
 
 my $qemu_server_config;
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 9/12] test: cfg2cmd: add tests for testing the iso required parameter
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
                   ` (7 preceding siblings ...)
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 8/12] test: mock log_warn warnings Daniel Herzig
@ 2025-01-13  8:56 ` Daniel Herzig
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 10/12] fix #4225: ui: form: isoselector: add optional required checkbox Daniel Herzig
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:56 UTC (permalink / raw)
  To: pve-devel

This adds tests for the errors and warnings issued by
PVE::QemuServer::eject_unrequired_isos.

Empty cmd files were added for the test configurations that are not
expected to have any output (as they die before a command is prepared):
* ide-required-iso-missing.conf
* ide-required-iso-offline-nfs.conf

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 test/cfg2cmd/ide-required-iso-missing.conf    | 12 ++++++
 .../cfg2cmd/ide-required-iso-missing.conf.cmd |  0
 .../cfg2cmd/ide-required-iso-offline-nfs.conf | 12 ++++++
 .../ide-required-iso-offline-nfs.conf.cmd     |  0
 test/cfg2cmd/ide-required.conf                | 14 +++++++
 test/cfg2cmd/ide-required.conf.cmd            | 39 +++++++++++++++++++
 test/cfg2cmd/ide-unrequired-iso-missing.conf  | 12 ++++++
 .../ide-unrequired-iso-missing.conf.cmd       | 33 ++++++++++++++++
 .../ide-unrequired-iso-offline-nfs.conf       | 12 ++++++
 .../ide-unrequired-iso-offline-nfs.conf.cmd   | 33 ++++++++++++++++
 10 files changed, 167 insertions(+)
 create mode 100644 test/cfg2cmd/ide-required-iso-missing.conf
 create mode 100644 test/cfg2cmd/ide-required-iso-missing.conf.cmd
 create mode 100644 test/cfg2cmd/ide-required-iso-offline-nfs.conf
 create mode 100644 test/cfg2cmd/ide-required-iso-offline-nfs.conf.cmd
 create mode 100644 test/cfg2cmd/ide-required.conf
 create mode 100644 test/cfg2cmd/ide-required.conf.cmd
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-missing.conf
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-missing.conf.cmd
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf
 create mode 100644 test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf.cmd

diff --git a/test/cfg2cmd/ide-required-iso-missing.conf b/test/cfg2cmd/ide-required-iso-missing.conf
new file mode 100644
index 00000000..16ce79ab
--- /dev/null
+++ b/test/cfg2cmd/ide-required-iso-missing.conf
@@ -0,0 +1,12 @@
+# TEST: Config with default machine type, Linux & one IDE CD-ROM with online storage and missing required ISO file.
+# EXPECT_ERROR: required file does not exist: 'ide0: cifs-store:iso/I_DO_NOT_EXIST.iso'
+bootdisk: scsi0
+cores: 2
+ide0: cifs-store:iso/I_DO_NOT_EXIST.iso,media=cdrom,size=112M
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsi0: local:100/vm-100-disk-2.qcow2,size=10G
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/ide-required-iso-missing.conf.cmd b/test/cfg2cmd/ide-required-iso-missing.conf.cmd
new file mode 100644
index 00000000..e69de29b
diff --git a/test/cfg2cmd/ide-required-iso-offline-nfs.conf b/test/cfg2cmd/ide-required-iso-offline-nfs.conf
new file mode 100644
index 00000000..47db264d
--- /dev/null
+++ b/test/cfg2cmd/ide-required-iso-offline-nfs.conf
@@ -0,0 +1,12 @@
+# TEST: Config with default machine type, Linux & one IDE CD-ROM with required ISO file on offline storage.
+# EXPECT_ERROR: cannot access required file: 'ide0: nfs-offline:iso/any.iso': storage 'nfs-offline' is not online
+bootdisk: scsi0
+cores: 2
+ide0: nfs-offline:iso/any.iso,media=cdrom,size=112M
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsi0: local:100/vm-100-disk-2.qcow2,size=10G
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/ide-required-iso-offline-nfs.conf.cmd b/test/cfg2cmd/ide-required-iso-offline-nfs.conf.cmd
new file mode 100644
index 00000000..e69de29b
diff --git a/test/cfg2cmd/ide-required.conf b/test/cfg2cmd/ide-required.conf
new file mode 100644
index 00000000..cf56b724
--- /dev/null
+++ b/test/cfg2cmd/ide-required.conf
@@ -0,0 +1,14 @@
+# TEST: Config with default machine type, Linux & four IDE CD-ROMs marked as required
+bootdisk: scsi0
+cores: 2
+ide0: cifs-store:iso/zero.iso,media=cdrom,size=112M,required=1
+ide1: cifs-store:iso/one.iso,media=cdrom,size=112M,required=1
+ide2: cifs-store:iso/two.iso,media=cdrom,size=112M,required=1
+ide3: cifs-store:iso/three.iso,media=cdrom,size=112M,required=1
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsi0: local:100/vm-100-disk-2.qcow2,size=10G
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/ide-required.conf.cmd b/test/cfg2cmd/ide-required.conf.cmd
new file mode 100644
index 00000000..33c6aadc
--- /dev/null
+++ b/test/cfg2cmd/ide-required.conf.cmd
@@ -0,0 +1,39 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
+  -smp '2,sockets=1,cores=2,maxcpus=2' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -drive 'file=/mnt/pve/cifs-store/template/iso/zero.iso,if=none,id=drive-ide0,media=cdrom,format=raw,aio=threads' \
+  -device 'ide-cd,bus=ide.0,unit=0,drive=drive-ide0,id=ide0,bootindex=200' \
+  -drive 'file=/mnt/pve/cifs-store/template/iso/one.iso,if=none,id=drive-ide1,media=cdrom,format=raw,aio=threads' \
+  -device 'ide-cd,bus=ide.0,unit=1,drive=drive-ide1,id=ide1,bootindex=201' \
+  -drive 'file=/mnt/pve/cifs-store/template/iso/two.iso,if=none,id=drive-ide2,media=cdrom,format=raw,aio=threads' \
+  -device 'ide-cd,bus=ide.1,unit=0,drive=drive-ide2,id=ide2,bootindex=202' \
+  -drive 'file=/mnt/pve/cifs-store/template/iso/three.iso,if=none,id=drive-ide3,media=cdrom,format=raw,aio=threads' \
+  -device 'ide-cd,bus=ide.1,unit=1,drive=drive-ide3,id=ide3,bootindex=203' \
+  -device 'virtio-scsi-pci,id=scsihw0,bus=pci.0,addr=0x5' \
+  -drive 'file=/var/lib/vz/images/100/vm-100-disk-2.qcow2,if=none,id=drive-scsi0,format=qcow2,cache=none,aio=io_uring,detect-zeroes=on' \
+  -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,rx_queue_size=1024,tx_queue_size=256,bootindex=300' \
+  -machine 'type=pc+pve0'
diff --git a/test/cfg2cmd/ide-unrequired-iso-missing.conf b/test/cfg2cmd/ide-unrequired-iso-missing.conf
new file mode 100644
index 00000000..d4393772
--- /dev/null
+++ b/test/cfg2cmd/ide-unrequired-iso-missing.conf
@@ -0,0 +1,12 @@
+# TEST: Config with default machine type, Linux & and one IDE CD-ROM with online storage and missing unrequired ISO file.
+# EXPECT_WARN: eject 'ide0: cifs-store:iso/I_DO_NOT_EXIST.iso': file does not exist
+bootdisk: scsi0
+cores: 2
+ide0: cifs-store:iso/I_DO_NOT_EXIST.iso,media=cdrom,size=112M,required=0
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsi0: local:100/vm-100-disk-2.qcow2,size=10G
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/ide-unrequired-iso-missing.conf.cmd b/test/cfg2cmd/ide-unrequired-iso-missing.conf.cmd
new file mode 100644
index 00000000..e6efd8a9
--- /dev/null
+++ b/test/cfg2cmd/ide-unrequired-iso-missing.conf.cmd
@@ -0,0 +1,33 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
+  -smp '2,sockets=1,cores=2,maxcpus=2' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -drive 'if=none,id=drive-ide0,media=cdrom,aio=io_uring' \
+  -device 'ide-cd,bus=ide.0,unit=0,drive=drive-ide0,id=ide0,bootindex=200' \
+  -device 'virtio-scsi-pci,id=scsihw0,bus=pci.0,addr=0x5' \
+  -drive 'file=/var/lib/vz/images/100/vm-100-disk-2.qcow2,if=none,id=drive-scsi0,format=qcow2,cache=none,aio=io_uring,detect-zeroes=on' \
+  -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,rx_queue_size=1024,tx_queue_size=256,bootindex=300' \
+  -machine 'type=pc+pve0'
diff --git a/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf b/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf
new file mode 100644
index 00000000..c7a59d0d
--- /dev/null
+++ b/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf
@@ -0,0 +1,12 @@
+# TEST: Config with default machine type, Linux & and one IDE CD-ROM with unrequired ISO file on offline storage.
+# EXPECT_WARN: eject 'ide0: nfs-offline:iso/any.iso': storage 'nfs-offline' is not online
+bootdisk: scsi0
+cores: 2
+ide0: nfs-offline:iso/any.iso,media=cdrom,size=112M,required=0
+memory: 512
+net0: virtio=2E:01:68:F9:9C:87,bridge=vmbr0
+ostype: l26
+scsi0: local:100/vm-100-disk-2.qcow2,size=10G
+scsihw: virtio-scsi-pci
+smbios1: uuid=3dd750ce-d910-44d0-9493-525c0be4e687
+vmgenid: 54d1c06c-8f5b-440f-b5b2-6eab1380e13d
diff --git a/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf.cmd b/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf.cmd
new file mode 100644
index 00000000..e6efd8a9
--- /dev/null
+++ b/test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf.cmd
@@ -0,0 +1,33 @@
+/usr/bin/kvm \
+  -id 8006 \
+  -name 'vm8006,debug-threads=on' \
+  -no-shutdown \
+  -chardev 'socket,id=qmp,path=/var/run/qemu-server/8006.qmp,server=on,wait=off' \
+  -mon 'chardev=qmp,mode=control' \
+  -chardev 'socket,id=qmp-event,path=/var/run/qmeventd.sock,reconnect=5' \
+  -mon 'chardev=qmp-event,mode=control' \
+  -pidfile /var/run/qemu-server/8006.pid \
+  -daemonize \
+  -smbios 'type=1,uuid=3dd750ce-d910-44d0-9493-525c0be4e687' \
+  -smp '2,sockets=1,cores=2,maxcpus=2' \
+  -nodefaults \
+  -boot 'menu=on,strict=on,reboot-timeout=1000,splash=/usr/share/qemu-server/bootsplash.jpg' \
+  -vnc 'unix:/var/run/qemu-server/8006.vnc,password=on' \
+  -cpu kvm64,enforce,+kvm_pv_eoi,+kvm_pv_unhalt,+lahf_lm,+sep \
+  -m 512 \
+  -device 'pci-bridge,id=pci.1,chassis_nr=1,bus=pci.0,addr=0x1e' \
+  -device 'pci-bridge,id=pci.2,chassis_nr=2,bus=pci.0,addr=0x1f' \
+  -device 'vmgenid,guid=54d1c06c-8f5b-440f-b5b2-6eab1380e13d' \
+  -device 'piix3-usb-uhci,id=uhci,bus=pci.0,addr=0x1.0x2' \
+  -device 'usb-tablet,id=tablet,bus=uhci.0,port=1' \
+  -device 'VGA,id=vga,bus=pci.0,addr=0x2' \
+  -device 'virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3,free-page-reporting=on' \
+  -iscsi 'initiator-name=iqn.1993-08.org.debian:01:aabbccddeeff' \
+  -drive 'if=none,id=drive-ide0,media=cdrom,aio=io_uring' \
+  -device 'ide-cd,bus=ide.0,unit=0,drive=drive-ide0,id=ide0,bootindex=200' \
+  -device 'virtio-scsi-pci,id=scsihw0,bus=pci.0,addr=0x5' \
+  -drive 'file=/var/lib/vz/images/100/vm-100-disk-2.qcow2,if=none,id=drive-scsi0,format=qcow2,cache=none,aio=io_uring,detect-zeroes=on' \
+  -device 'scsi-hd,bus=scsihw0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0,id=scsi0,bootindex=100' \
+  -netdev 'type=tap,id=net0,ifname=tap8006i0,script=/var/lib/qemu-server/pve-bridge,downscript=/var/lib/qemu-server/pve-bridgedown,vhost=on' \
+  -device 'virtio-net-pci,mac=2E:01:68:F9:9C:87,netdev=net0,bus=pci.0,addr=0x12,id=net0,rx_queue_size=1024,tx_queue_size=256,bootindex=300' \
+  -machine 'type=pc+pve0'
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 10/12] fix #4225: ui: form: isoselector: add optional required checkbox
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
                   ` (8 preceding siblings ...)
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 9/12] test: cfg2cmd: add tests for testing the iso required parameter Daniel Herzig
@ 2025-01-13  8:56 ` Daniel Herzig
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 11/12] fix #4225: ui: qemu: cdedit: enable required checkbox for isos Daniel Herzig
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:56 UTC (permalink / raw)
  To: pve-devel

Add a checkbox for marking an iso file as required.

This option is used in the backend to determine if the VM should start
up in case the configured ISO file is not available.

By default this box is not visible and disabled.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 www/manager6/form/IsoSelector.js | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/www/manager6/form/IsoSelector.js b/www/manager6/form/IsoSelector.js
index 66229e88..1803cd9d 100644
--- a/www/manager6/form/IsoSelector.js
+++ b/www/manager6/form/IsoSelector.js
@@ -15,12 +15,14 @@ Ext.define('PVE.form.IsoSelector', {
     insideWizard: false,
     labelWidth: undefined,
     labelAlign: 'right',
+    showRequired: false,
 
     cbindData: function() {
 	let me = this;
 	return {
 	    nodename: me.nodename,
 	    insideWizard: me.insideWizard,
+	    showRequired: me.showRequired,
 	};
     },
 
@@ -113,5 +115,25 @@ Ext.define('PVE.form.IsoSelector', {
 		},
 	    },
 	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    name: 'required',
+	    reference: 'requiredForBoot',
+	    inputValue: true,
+	    fieldLabel: gettext('Required'),
+	    cbind: {
+		nodename: '{nodename}',
+		disabled: '{!showRequired}',
+		hidden: '{!showRequired}',
+		labelWidth: '{labelWidth}',
+		labelAlign: '{labelAlign}',
+	    },
+	    allowBlank: false,
+	    listeners: {
+		change: function() {
+		    this.up('pveIsoSelector').checkChange();
+		},
+	    },
+	},
     ],
 });
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 11/12] fix #4225: ui: qemu: cdedit: enable required checkbox for isos
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
                   ` (9 preceding siblings ...)
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 10/12] fix #4225: ui: form: isoselector: add optional required checkbox Daniel Herzig
@ 2025-01-13  8:56 ` Daniel Herzig
  2025-01-16 15:19   ` Daniel Kral
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms Daniel Herzig
  2025-01-16 15:18 ` [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Kral
  12 siblings, 1 reply; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:56 UTC (permalink / raw)
  To: pve-devel

Enables the 'required' checkbox for the IsoSelector.
If the parameter is not set, the backend will use the default (set to
1).

Behaviour:
* Only send parameter if not default (required=0)
* Checked if parameter is missing (default)
* Unchecked when adding a new CD-ROM

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 www/manager6/qemu/CDEdit.js | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/www/manager6/qemu/CDEdit.js b/www/manager6/qemu/CDEdit.js
index 3cc16205..9f518f68 100644
--- a/www/manager6/qemu/CDEdit.js
+++ b/www/manager6/qemu/CDEdit.js
@@ -12,6 +12,7 @@ Ext.define('PVE.qemu.CDInputPanel', {
 	me.drive.media = 'cdrom';
 	if (values.mediaType === 'iso') {
 	    me.drive.file = values.cdimage;
+	    me.drive.required = values.required ? undefined : '0';
 	} else if (values.mediaType === 'cdrom') {
 	    me.drive.file = 'cdrom';
 	} else {
@@ -44,6 +45,9 @@ Ext.define('PVE.qemu.CDInputPanel', {
 	} else {
 	    values.mediaType = 'iso';
 	    values.cdimage = drive.file;
+	    if (drive.required === '1' || drive.required === undefined) {
+		values.required = '1';
+	    }
 	}
 
 	me.drive = drive;
@@ -88,6 +92,7 @@ Ext.define('PVE.qemu.CDInputPanel', {
 			cdImageField.validate();
 		    } else {
 			cdImageField.reset();
+			delete me.drive.required;
 		    }
 		},
 	    },
@@ -98,6 +103,7 @@ Ext.define('PVE.qemu.CDInputPanel', {
 	    nodename: me.nodename,
 	    insideWizard: me.insideWizard,
 	    name: 'cdimage',
+	    showRequired: true,
 	});
 
 	items.push(me.isosel);
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
                   ` (10 preceding siblings ...)
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 11/12] fix #4225: ui: qemu: cdedit: enable required checkbox for isos Daniel Herzig
@ 2025-01-13  8:56 ` Daniel Herzig
  2025-01-16 15:19   ` Daniel Kral
  2025-01-23 16:39   ` Friedrich Weber
  2025-01-16 15:18 ` [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Kral
  12 siblings, 2 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-13  8:56 UTC (permalink / raw)
  To: pve-devel

Eject by setting file to none.

Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
---
 www/manager6/qemu/HardwareView.js | 43 +++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 59e670db..5d1c18a5 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -538,6 +538,45 @@ Ext.define('PVE.qemu.HardwareView', {
 	    apiurl: '/api2/extjs/' + baseurl,
 	});
 
+	let eject_btn = new Proxmox.button.Button({
+	    text: gettext('Eject'),
+	    disabled: true,
+	    selModel: sm,
+	    RESTMethod: 'POST',
+	    confirmMsg: function(rec) {
+		let warn = gettext("Are you sure you want to eject '{0}' ?");
+		let isofile = rec.data.value.split(",")[0];
+		let msg = Ext.String.format(warn, isofile);
+		return msg;
+	    },
+	    handler: function(btn, e, rec) {
+		let params = {};
+		params[rec.data.key] = 'none,media=cdrom';
+		if (btn.RESTMethod === 'POST') {
+		    params.background_delay = 5;
+		}
+		Proxmox.Utils.API2Request({
+		    url: '/api2/extjs/' + baseurl,
+		    waitMsgTarget: me,
+		    method: btn.RESTMethod,
+		    params: params,
+		    callback: () => me.reload(),
+		    failure: response => Ext.Msg.alert('Error', response.htmlStatus),
+		    success: function(response, options) {
+			if (btn.RESTMethod === 'POST' && response.result.data !== null) {
+			    Ext.create('Proxmox.window.TaskProgress', {
+				autoShow: true,
+				upid: response.result.data,
+				listeners: {
+				    destroy: () => me.reload(),
+				},
+			    });
+			}
+		    },
+		});
+	    },
+	});
+
 	let efidisk_menuitem = Ext.create('Ext.menu.Item', {
 	    text: gettext('EFI Disk'),
 	    iconCls: 'fa fa-fw fa-hdd-o black',
@@ -608,6 +647,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		edit_btn.disable();
 		diskaction_btn.disable();
 		revert_btn.disable();
+		eject_btn.disable();
 		return;
 	    }
 	    const { key, value } = rec.data;
@@ -619,6 +659,7 @@ Ext.define('PVE.qemu.HardwareView', {
 
 	    const isCloudInit = isCloudInitKey(value);
 	    const isCDRom = value && !!value.toString().match(/media=cdrom/);
+	    const isISO = value && !!value.toString().match(/.iso/);
 
 	    const isUnusedDisk = key.match(/^unused\d+/);
 	    const isUsedDisk = !isUnusedDisk && row.isOnStorageBus && !isCDRom;
@@ -648,6 +689,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	    resize_menuitem.setDisabled(pending || !isUsedDisk);
 
 	    revert_btn.setDisabled(!pending);
+	    eject_btn.setDisabled((isCDRom && !cdromCap) || !isISO);
 	};
 
 	let editorFactory = (classPath, extraOptions) => {
@@ -751,6 +793,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		remove_btn,
 		edit_btn,
 		diskaction_btn,
+		eject_btn,
 		revert_btn,
 	    ],
 	    rows: rows,
-- 
2.39.5


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs
  2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
                   ` (11 preceding siblings ...)
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms Daniel Herzig
@ 2025-01-16 15:18 ` Daniel Kral
  2025-01-17 11:34   ` Daniel Herzig
  12 siblings, 1 reply; 26+ messages in thread
From: Daniel Kral @ 2025-01-16 15:18 UTC (permalink / raw)
  To: Proxmox VE development discussion, Daniel Herzig

On 1/13/25 09:55, Daniel Herzig wrote:
> This patch series addresses bugzilla entry #4225.>
> Currently VMs refuse to to start if a configured isofile becomes unavailable,
> be it a deleted file or an unavailable network storage.
> 
> This patch series introduces a new parameter in Drive.pm, called 'required'.
> Depending on whether this parameter is set or not, the situation will be handled
> differently.

Thanks for tackling this, this is a great addition to the CDROM drives!

I'm looking forward to this being merged as it happens quite frequently 
when setting up a lot of VMs with different ISOs!

> 
> If the parameter is set to 0, the configuration will temporarily changed to use
> 'none' as file for the cd drive, which allows qemu to start up the machine.
> The configuration is not changed in this process to avoid unexpected behaviour.
> Instead a log_warn will be issued.
> 
> For transition reasons an unset parameter acts like 'required=1'. In this case
> the startup process will die earlier than currently, if the file is missing or
> the underlying storage not available.

Hm, I have discussed with Friedrich about this off-list, because I'm 
thinking about "optional" being another name for this flag, since it 
should be required by default for VMs that are not explicitly setting 
this option, i.e. `optional=0`, and if someone sets it explicitly to 
`optional=1` the CDROM can be ignored if it is non-existent.

I think this could also simplify the logic overall, but it depends on 
how we want to present this to users (i.e. the WebGUI).

Are there reasons against this? What do you think?

> 
> If however a new VM is created from the WebGUI, the corresponding added checkbox
> is not checked by default, and the resulting 'required=0' will be written to
> the configuration.

IMO, I also think that new VMs should be set to `required=0` by default, 
but this change should probably be postponed to 9.0 as it would break 
the current WebGUI "user-API".

> 
> To allow for proper testing and building some additions and minor changes
> where made to to the testing framework as well.
> 
> Not exactly part of #4225, but related to it, this patch series adds an 'Eject'
> button to the hardwareview in the WebGUI, which can be used as a convenience
> shortcut to manually editing the missing ISO file to 'Do not use any media'.

In this case it is better to move unrelated changes into a separate 
patch series, so they can be reviewed on their own :).

> 
> This series supersedes:
> https://lore.proxmox.com/pve-devel/20241025150243.134466-1-d.herzig@proxmox.com/
> 
> Changes from initial series:
> * rebased onto current master
> * fix cifs mocking in run_config2command_tests.pl
> * fix expected outcome in ide-required.conf.cmd
> 
> qemu-server: Daniel Herzig (9):
>    fix #4225: qemuserver: drive: add optional required parameter
>    qemuserver: add helper function for mocking files
>    fix #4225: qemuserver: add function to eject isofiles
>    test: chomp all trailing newlines from errors and warnings
>    test: mock cifs-store
>    test: add nfs-offline storage
>    test: mock existing files
>    test: mock log_warn warnings
>    test: cfg2cmd: add tests for testing the iso required parameter
> 
>   PVE/QemuServer.pm                             | 44 +++++++++++++++++++
>   PVE/QemuServer/Drive.pm                       |  9 +++-
>   test/cfg2cmd/ide-required-iso-missing.conf    | 12 +++++
>   .../cfg2cmd/ide-required-iso-missing.conf.cmd |  0
>   .../cfg2cmd/ide-required-iso-offline-nfs.conf | 12 +++++
>   .../ide-required-iso-offline-nfs.conf.cmd     |  0
>   test/cfg2cmd/ide-required.conf                | 14 ++++++
>   test/cfg2cmd/ide-required.conf.cmd            | 39 ++++++++++++++++
>   test/cfg2cmd/ide-unrequired-iso-missing.conf  | 12 +++++
>   .../ide-unrequired-iso-missing.conf.cmd       | 33 ++++++++++++++
>   .../ide-unrequired-iso-offline-nfs.conf       | 12 +++++
>   .../ide-unrequired-iso-offline-nfs.conf.cmd   | 33 ++++++++++++++
>   test/run_config2command_tests.pl              | 44 ++++++++++++++++++-
>   13 files changed, 261 insertions(+), 3 deletions(-)
>   create mode 100644 test/cfg2cmd/ide-required-iso-missing.conf
>   create mode 100644 test/cfg2cmd/ide-required-iso-missing.conf.cmd
>   create mode 100644 test/cfg2cmd/ide-required-iso-offline-nfs.conf
>   create mode 100644 test/cfg2cmd/ide-required-iso-offline-nfs.conf.cmd
>   create mode 100644 test/cfg2cmd/ide-required.conf
>   create mode 100644 test/cfg2cmd/ide-required.conf.cmd
>   create mode 100644 test/cfg2cmd/ide-unrequired-iso-missing.conf
>   create mode 100644 test/cfg2cmd/ide-unrequired-iso-missing.conf.cmd
>   create mode 100644 test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf
>   create mode 100644 test/cfg2cmd/ide-unrequired-iso-offline-nfs.conf.cmd
> 
> manager: Daniel Herzig (3):
>    fix #4225: ui: form: isoselector: add optional required checkbox
>    fix #4225: ui: qemu: cdedit: enable required checkbox for isos
>    ui: qemu: hardware: add eject button for cdroms
> 
>   www/manager6/form/IsoSelector.js  | 22 ++++++++++++++++
>   www/manager6/qemu/CDEdit.js       |  6 +++++
>   www/manager6/qemu/HardwareView.js | 43 +++++++++++++++++++++++++++++++
>   3 files changed, 71 insertions(+)
> 
> 

I also just noticed that the repository names are gone from the patches 
- seems like they were accidentally removed when formatting the second 
version of these patches because they were there in v1.

All things considered, this is a great feature addition I'd like to see 
merged, so consider the whole patch series as:

Reviewed-by: Daniel Kral <d.kral@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>



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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 2/12] qemuserver: add helper function for mocking files
  2025-01-13  8:55 ` [pve-devel] [PATCH v2 2/12] qemuserver: add helper function for mocking files Daniel Herzig
@ 2025-01-16 15:18   ` Daniel Kral
  2025-01-17 11:36     ` Daniel Herzig
  0 siblings, 1 reply; 26+ messages in thread
From: Daniel Kral @ 2025-01-16 15:18 UTC (permalink / raw)
  To: Proxmox VE development discussion, Daniel Herzig

On 1/13/25 09:55, Daniel Herzig wrote:
> This stub function can be used for mocking a file's existance in testruns.
> 
> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
> ---
>   PVE/QemuServer.pm | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 5cde94a1..d07c170e 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -8999,4 +8999,9 @@ sub delete_ifaces_ipams_ips {
>       }
>   }
>   
> +sub file_exists {
> +    my $file_path = shift;
> +    return -e $file_path
> +}
> +
>   1;

nit: I think this could be squashed into patch #3.


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles
  2025-01-13  8:55 ` [pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles Daniel Herzig
@ 2025-01-16 15:19   ` Daniel Kral
  2025-01-17 12:32     ` Daniel Herzig
  0 siblings, 1 reply; 26+ messages in thread
From: Daniel Kral @ 2025-01-16 15:19 UTC (permalink / raw)
  To: Proxmox VE development discussion, Daniel Herzig

On 1/13/25 09:55, Daniel Herzig wrote:
> Current behaviour prevents a VM from starting, if an ISO file defined
> in the configuration becomes unavailable.
> 
> The function eject_nonrequired_isos checks on whether a cdrom drive is
> marked as 'required' or not. If the parameter 'required' is not
> defined, it will assume 'required' to be true and keep the current
> behaviour.
> 
> If 'required' is set to 0, the function 'ejects' the ISO file by
> setting the drive's file value to 'none', if the underlying storage is
> unavailable or if the defined file is unavailable for another reason.
> 
> The function is called while config_to_command iterates over all
> volumes to allow for early storage activation and early exit in the
> case of missing required files.
> 
> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
> ---
>   PVE/QemuServer.pm | 39 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index d07c170e..f72878d3 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4041,6 +4041,8 @@ sub config_to_command {
>       PVE::QemuConfig->foreach_volume($conf, sub {
>   	my ($ds, $drive) = @_;
>   
> +	eject_nonrequired_isos($ds, $drive, $vmid, $storecfg, $conf);
> +

This change will unfortunately make two config2cmd test cases fail and 
therefore the build process will also fail. It is important that the 
package can be built at each individual commit so to make the package 
bisectable.

IMO this patch could be split into "introduce eject_norequired_isos" and 
patches #4-#9 could be squashed and put together with adding 
"eject_nonrequired_isos" to config_to_command in the same patch. 
Therefore someone reviewing (now or in the future) and know what tests 
needed to be added/changed when adding this function call to 
config_to_command.

>   	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
>   	    check_volume_storage_type($storecfg, $drive->{file});
>   	    push @$vollist, $drive->{file};
> @@ -8999,6 +9001,43 @@ sub delete_ifaces_ipams_ips {
>       }
>   }
>   
> +sub eject_nonrequired_isos {
> +    my ($ds, $drive, $vmid, $storecfg, $conf) = @_;
> +    # set 1 to exclude cloudinit. cloudinit isos are always required.
> +    if (drive_is_cdrom($drive, 1)
> +	&& $drive->{file} ne 'none'
> +	&& $drive->{file} ne 'cdrom') {

nit: IMO, this could be an early return:

return if !drive_is_cdrom($drive, 1);
return if $drive->{file} eq 'none' || $drive->{file} eq 'cdrom';

So that we can reduce the following to only 2 indentation levels.

> +	$drive->{required} = 1 if !defined($drive->{required});
> +	my $iso_volid = $drive->{file};
> +	my $iso_path = get_iso_path($storecfg, $vmid, $drive->{file});

nit: third argument could be $iso_volid

> +	my $store_err;
> +	if ($iso_volid !~ m|^/|) {
> +	    my $iso_storage  = PVE::Storage::parse_volume_id($iso_volid, 1);
> +	    eval { PVE::Storage::activate_storage($storecfg, $iso_storage); };
> +	    $store_err = $@;
> +	}
> +	if ($store_err) {
> +	    if ($drive->{required}) {
> +		die "cannot access required file: '${ds}: ${iso_volid}': ${store_err}\n";
> +	    } else {
> +		log_warn("eject '${ds}: ${iso_volid}': ${store_err}");
> +		$drive->{file} = 'none';
> +		$conf->{$ds} = print_drive($drive);
> +	    }
> +	} else {
> +	    if (!file_exists($iso_path)) {
> +		if ($drive->{required}) {
> +		    die "required file does not exist: '${ds}: ${iso_volid}'\n";
> +		} else {
> +		    log_warn("eject '${ds}: ${iso_volid}': file does not exist");
> +		    $drive->{file} = 'none';
> +		    $conf->{$ds} = print_drive($drive);
> +		}
> +	    }
> +	}

nit: the logic between an unavailable storage and an unavailable ISO 
image are very similar (both `$drive->{required} && $store_err` as well 
as `$drive->{required} && !file_exists($iso_path)` have the same exit 
control path), so we could simplify this e.g. to this (changes the 
warning message to a generic message for unavailable storages too):

if ($drive->{required}) {
     die "cannot access required file: '${ds}: ${iso_volid}': 
${store_err}\n" if $store_err;
     die "required file does not exist: '${ds}: ${iso_volid}'\n" if 
!file_exists($iso_path);
}

return if !$store_err && file_exists($iso_path);

log_warn("eject '${ds}: ${iso_volid}': storage unavailable or file does 
not exist");

$drive->{file} = 'none';
$conf->{$ds} = print_drive($drive);


> +    }
> +}
> +
>   sub file_exists {
>       my $file_path = shift;
>       return -e $file_path

Else consider this:

Reviewed-by: Daniel Kral <d.kral@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 11/12] fix #4225: ui: qemu: cdedit: enable required checkbox for isos
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 11/12] fix #4225: ui: qemu: cdedit: enable required checkbox for isos Daniel Herzig
@ 2025-01-16 15:19   ` Daniel Kral
  2025-01-17 12:38     ` Daniel Herzig
  0 siblings, 1 reply; 26+ messages in thread
From: Daniel Kral @ 2025-01-16 15:19 UTC (permalink / raw)
  To: Proxmox VE development discussion, Daniel Herzig

On 1/13/25 09:56, Daniel Herzig wrote:
> Enables the 'required' checkbox for the IsoSelector.
> If the parameter is not set, the backend will use the default (set to
> 1).
> 
> Behaviour:
> * Only send parameter if not default (required=0)
> * Checked if parameter is missing (default)
> * Unchecked when adding a new CD-ROM

IMO the part of this patch where new VMs get created with `required=0` 
CDROMs should be split into its own patch and marked as "for-9.0" or 
else add a TODO comment for the 9.0 release.

Else this looks good, so consider this:

Reviewed-by: Daniel Kral <d.kral@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms Daniel Herzig
@ 2025-01-16 15:19   ` Daniel Kral
  2025-01-17 12:47     ` Daniel Herzig
  2025-01-23 16:39   ` Friedrich Weber
  1 sibling, 1 reply; 26+ messages in thread
From: Daniel Kral @ 2025-01-16 15:19 UTC (permalink / raw)
  To: Proxmox VE development discussion, Daniel Herzig

On 1/13/25 09:56, Daniel Herzig wrote:
> Eject by setting file to none.

That's a great addition to the WebGUI!

One small thing: What about also allow ejecting physical CDROMs? :)

Else consider this:

Reviewed-by: Daniel Kral <d.kral@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs
  2025-01-16 15:18 ` [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Kral
@ 2025-01-17 11:34   ` Daniel Herzig
  2025-01-17 13:04     ` Daniel Kral
  0 siblings, 1 reply; 26+ messages in thread
From: Daniel Herzig @ 2025-01-17 11:34 UTC (permalink / raw)
  To: Daniel Kral; +Cc: Proxmox VE development discussion

Thanks for this review, this helps a lot on setting up an optimized v3.

Daniel Kral <d.kral@proxmox.com> writes:

>
>> If the parameter is set to 0, the configuration will temporarily
>> changed to use
>> 'none' as file for the cd drive, which allows qemu to start up the machine.
>> The configuration is not changed in this process to avoid unexpected behaviour.
>> Instead a log_warn will be issued.
>> For transition reasons an unset parameter acts like 'required=1'. In
>> this case
>> the startup process will die earlier than currently, if the file is missing or
>> the underlying storage not available.
>
> Hm, I have discussed with Friedrich about this off-list, because I'm
> thinking about "optional" being another name for this flag, since it
> should be required by default for VMs that are not explicitly setting
> this option, i.e. `optional=0`, and if someone sets it explicitly to
> `optional=1` the CDROM can be ignored if it is non-existent.
>
> I think this could also simplify the logic overall, but it depends on
> how we want to present this to users (i.e. the WebGUI).
>
> Are there reasons against this? What do you think?

I have no hard feelings about the naming of this parameter. Indeed,
earlier it already had other names as well already.

I think the only reason why this obviously best-matching label did not
come into closer consideration, is that on parameter definition this
would lead to the possibly confusing construction:

# optional => {
#     [..]
#     optional => 1,
#     [...]

I'm not entirely sure, if this could lead to unexpected side-effects
despite looking funny.

I'm open for different parameter-naming though!

>
>> If however a new VM is created from the WebGUI, the corresponding
>> added checkbox
>> is not checked by default, and the resulting 'required=0' will be written to
>> the configuration.
>
> IMO, I also think that new VMs should be set to `required=0` by
> default, but this change should probably be postponed to 9.0 as it
> would break the current WebGUI "user-API".
>

With the patches applied, this will be handled that way when a new VM gets
created through the GUI (the box is unchecked by default in this case).
So currently it's kind of soft-defaulting to 'required=0' with visual feedback.

But I'm not against rather propagating 'required=1' for 8.x.

To avoid conflicts with automated setup via 'qm create' that possibly
depend on attached ISOs after intial installation nothing will be
set at all on 'headless' actions.

>> To allow for proper testing and building some additions and minor
>> changes
>> where made to to the testing framework as well.
>> Not exactly part of #4225, but related to it, this patch series adds
>> an 'Eject'
>> button to the hardwareview in the WebGUI, which can be used as a convenience
>> shortcut to manually editing the missing ISO file to 'Do not use any media'.
>
> In this case it is better to move unrelated changes into a separate
> patch series, so they can be reviewed on their own :).
>

True :).

>> This series supersedes:
>> https://lore.proxmox.com/pve-devel/20241025150243.134466-1-d.herzig@proxmox.com/
>
> I also just noticed that the repository names are gone from the
> patches - seems like they were accidentally removed when formatting
> the second version of these patches because they were there in v1.

Thanks, good catch, they'll be back in v3!


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 2/12] qemuserver: add helper function for mocking files
  2025-01-16 15:18   ` Daniel Kral
@ 2025-01-17 11:36     ` Daniel Herzig
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-17 11:36 UTC (permalink / raw)
  To: Daniel Kral; +Cc: Proxmox VE development discussion

Daniel Kral <d.kral@proxmox.com> writes:

> On 1/13/25 09:55, Daniel Herzig wrote:
>> This stub function can be used for mocking a file's existance in testruns.
>> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
>> ---
>>   PVE/QemuServer.pm | 5 +++++
>>   1 file changed, 5 insertions(+)
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 5cde94a1..d07c170e 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -8999,4 +8999,9 @@ sub delete_ifaces_ipams_ips {
>>       }
>>   }
>>   +sub file_exists {
>> +    my $file_path = shift;
>> +    return -e $file_path
>> +}
>> +
>>   1;
>
> nit: I think this could be squashed into patch #3.

Right, good point, this would make things more compact.


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles
  2025-01-16 15:19   ` Daniel Kral
@ 2025-01-17 12:32     ` Daniel Herzig
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-17 12:32 UTC (permalink / raw)
  To: Daniel Kral; +Cc: Proxmox VE development discussion

Daniel Kral <d.kral@proxmox.com> writes:

> On 1/13/25 09:55, Daniel Herzig wrote:
>> Current behaviour prevents a VM from starting, if an ISO file defined
>> in the configuration becomes unavailable.
>> The function eject_nonrequired_isos checks on whether a cdrom drive
>> is
>> marked as 'required' or not. If the parameter 'required' is not
>> defined, it will assume 'required' to be true and keep the current
>> behaviour.
>> If 'required' is set to 0, the function 'ejects' the ISO file by
>> setting the drive's file value to 'none', if the underlying storage is
>> unavailable or if the defined file is unavailable for another reason.
>> The function is called while config_to_command iterates over all
>> volumes to allow for early storage activation and early exit in the
>> case of missing required files.
>> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
>> ---
>>   PVE/QemuServer.pm | 39 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index d07c170e..f72878d3 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -4041,6 +4041,8 @@ sub config_to_command {
>>       PVE::QemuConfig->foreach_volume($conf, sub {
>>   	my ($ds, $drive) = @_;
>>   +	eject_nonrequired_isos($ds, $drive, $vmid, $storecfg, $conf);
>> +
>
> This change will unfortunately make two config2cmd test cases fail and
> therefore the build process will also fail. It is important that the
> package can be built at each individual commit so to make the package
> bisectable.
>
> IMO this patch could be split into "introduce eject_norequired_isos"
> and patches #4-#9 could be squashed and put together with adding
> "eject_nonrequired_isos" to config_to_command in the same
> patch. Therefore someone reviewing (now or in the future) and know
> what tests needed to be added/changed when adding this function call
> to config_to_command.
>

Thanks for pointing that out. Batch-applying the patches on my
test-system I completely missed this.

I'll work out something along your lines for v3.


>>   	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
>>   	    check_volume_storage_type($storecfg, $drive->{file});
>>   	    push @$vollist, $drive->{file};
>> @@ -8999,6 +9001,43 @@ sub delete_ifaces_ipams_ips {
>>       }
>>   }
>>   +sub eject_nonrequired_isos {
>> +    my ($ds, $drive, $vmid, $storecfg, $conf) = @_;
>> +    # set 1 to exclude cloudinit. cloudinit isos are always required.
>> +    if (drive_is_cdrom($drive, 1)
>> +	&& $drive->{file} ne 'none'
>> +	&& $drive->{file} ne 'cdrom') {
>
> nit: IMO, this could be an early return:
>
> return if !drive_is_cdrom($drive, 1);
> return if $drive->{file} eq 'none' || $drive->{file} eq 'cdrom';
>
> So that we can reduce the following to only 2 indentation levels.
>

I like this idea.

>> +	$drive->{required} = 1 if !defined($drive->{required});
>> +	my $iso_volid = $drive->{file};
>> +	my $iso_path = get_iso_path($storecfg, $vmid, $drive->{file});
>
> nit: third argument could be $iso_volid
>

Scratching my head how I missed this :).

>> +	my $store_err;
>> +	if ($iso_volid !~ m|^/|) {
>> +	    my $iso_storage  = PVE::Storage::parse_volume_id($iso_volid, 1);
>> +	    eval { PVE::Storage::activate_storage($storecfg, $iso_storage); };
>> +	    $store_err = $@;
>> +	}
>> +	if ($store_err) {
>> +	    if ($drive->{required}) {
>> +		die "cannot access required file: '${ds}: ${iso_volid}': ${store_err}\n";
>> +	    } else {
>> +		log_warn("eject '${ds}: ${iso_volid}': ${store_err}");
>> +		$drive->{file} = 'none';
>> +		$conf->{$ds} = print_drive($drive);
>> +	    }
>> +	} else {
>> +	    if (!file_exists($iso_path)) {
>> +		if ($drive->{required}) {
>> +		    die "required file does not exist: '${ds}: ${iso_volid}'\n";
>> +		} else {
>> +		    log_warn("eject '${ds}: ${iso_volid}': file does not exist");
>> +		    $drive->{file} = 'none';
>> +		    $conf->{$ds} = print_drive($drive);
>> +		}
>> +	    }
>> +	}
>
> nit: the logic between an unavailable storage and an unavailable ISO
> image are very similar (both `$drive->{required} && $store_err` as
> well as `$drive->{required} && !file_exists($iso_path)` have the same
> exit control path), so we could simplify this e.g. to this (changes
> the warning message to a generic message for unavailable storages
> too):
>
> if ($drive->{required}) {
>     die "cannot access required file: '${ds}: ${iso_volid}':
>     ${store_err}\n" if $store_err;
>     die "required file does not exist: '${ds}: ${iso_volid}'\n" if
>     !file_exists($iso_path);
> }
>
> return if !$store_err && file_exists($iso_path);
>
> log_warn("eject '${ds}: ${iso_volid}': storage unavailable or file
> does not exist");
>
> $drive->{file} = 'none';
> $conf->{$ds} = print_drive($drive);
>

Good point, thanks.



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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 11/12] fix #4225: ui: qemu: cdedit: enable required checkbox for isos
  2025-01-16 15:19   ` Daniel Kral
@ 2025-01-17 12:38     ` Daniel Herzig
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-17 12:38 UTC (permalink / raw)
  To: Daniel Kral; +Cc: Proxmox VE development discussion

Daniel Kral <d.kral@proxmox.com> writes:

> On 1/13/25 09:56, Daniel Herzig wrote:
>> Enables the 'required' checkbox for the IsoSelector.
>> If the parameter is not set, the backend will use the default (set to
>> 1).
>> Behaviour:
>> * Only send parameter if not default (required=0)
>> * Checked if parameter is missing (default)
>> * Unchecked when adding a new CD-ROM
>
> IMO the part of this patch where new VMs get created with `required=0`
> CDROMs should be split into its own patch and marked as "for-9.0" or
> else add a TODO comment for the 9.0 release.
>
Good point to stay more 'conservative' for now. Thanks.


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms
  2025-01-16 15:19   ` Daniel Kral
@ 2025-01-17 12:47     ` Daniel Herzig
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-17 12:47 UTC (permalink / raw)
  To: Daniel Kral; +Cc: Proxmox VE development discussion

Daniel Kral <d.kral@proxmox.com> writes:

> On 1/13/25 09:56, Daniel Herzig wrote:
>> Eject by setting file to none.
>
> That's a great addition to the WebGUI!
>
> One small thing: What about also allow ejecting physical CDROMs? :)
>
Let me think about this :)


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs
  2025-01-17 11:34   ` Daniel Herzig
@ 2025-01-17 13:04     ` Daniel Kral
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Kral @ 2025-01-17 13:04 UTC (permalink / raw)
  To: Daniel Herzig; +Cc: Proxmox VE development discussion

On 1/17/25 12:34, Daniel Herzig wrote:
> Thanks for this review, this helps a lot on setting up an optimized v3.
> 
> Daniel Kral <d.kral@proxmox.com> writes:
> 
>>
>>> If the parameter is set to 0, the configuration will temporarily
>>> changed to use
>>> 'none' as file for the cd drive, which allows qemu to start up the machine.
>>> The configuration is not changed in this process to avoid unexpected behaviour.
>>> Instead a log_warn will be issued.
>>> For transition reasons an unset parameter acts like 'required=1'. In
>>> this case
>>> the startup process will die earlier than currently, if the file is missing or
>>> the underlying storage not available.
>>
>> Hm, I have discussed with Friedrich about this off-list, because I'm
>> thinking about "optional" being another name for this flag, since it
>> should be required by default for VMs that are not explicitly setting
>> this option, i.e. `optional=0`, and if someone sets it explicitly to
>> `optional=1` the CDROM can be ignored if it is non-existent.
>>
>> I think this could also simplify the logic overall, but it depends on
>> how we want to present this to users (i.e. the WebGUI).
>>
>> Are there reasons against this? What do you think?
> 
> I have no hard feelings about the naming of this parameter. Indeed,
> earlier it already had other names as well already.
> 
> I think the only reason why this obviously best-matching label did not
> come into closer consideration, is that on parameter definition this
> would lead to the possibly confusing construction:
> 
> # optional => {
> #     [..]
> #     optional => 1,
> #     [...]
> 
> I'm not entirely sure, if this could lead to unexpected side-effects
> despite looking funny.
> 
> I'm open for different parameter-naming though!

Good question, I'm not entirely sure about that either, but AFAIK the 
boolean `optional` property for parameters should be parsed differently 
than the hash `optional` parameter definition by JSONSchema... We do 
something similar with the parameter `type` that is both used to define 
the parameter's type (e.g. string, array) or as a parameter itself (e.g. 
storage type, VM machine type), but there could be edge cases for this 
as we don't use a parameter named `optional` anywhere else.

On second thought, I think there sure is a better name for this that 
describes what it's doing more discretely, but I'm not entirely sure 
what. Something I just came up with is 
"eject-when-missing"/"ignore-when-missing", but it is a little bit too 
long IMO and also the first one fixates itself to be only used for CDROM 
ISOs even when there would be a place for a similar parameter for other 
media types in the future.

But that's just my two cents and I might just overthink this, maybe 
someone else has a better opinion on this?

> 
>>
>>> If however a new VM is created from the WebGUI, the corresponding
>>> added checkbox
>>> is not checked by default, and the resulting 'required=0' will be written to
>>> the configuration.
>>
>> IMO, I also think that new VMs should be set to `required=0` by
>> default, but this change should probably be postponed to 9.0 as it
>> would break the current WebGUI "user-API".
>>
> 
> With the patches applied, this will be handled that way when a new VM gets
> created through the GUI (the box is unchecked by default in this case).
> So currently it's kind of soft-defaulting to 'required=0' with visual feedback.
> 
> But I'm not against rather propagating 'required=1' for 8.x.
> 
> To avoid conflicts with automated setup via 'qm create' that possibly
> depend on attached ISOs after intial installation nothing will be
> set at all on 'headless' actions.

Good thinking!

I'm also not sure how we handle "API stability" for the WebGUI as we're 
more often concerned about the actual API. I'm just thinking about users 
that have clicked through the "Create VM" dialog thousands of times, 
which might not catch that CDROM images are optional by default now, but 
they are the ones who would've catched this at the first VM start.

> 
>>> To allow for proper testing and building some additions and minor
>>> changes
>>> where made to to the testing framework as well.
>>> Not exactly part of #4225, but related to it, this patch series adds
>>> an 'Eject'
>>> button to the hardwareview in the WebGUI, which can be used as a convenience
>>> shortcut to manually editing the missing ISO file to 'Do not use any media'.
>>
>> In this case it is better to move unrelated changes into a separate
>> patch series, so they can be reviewed on their own :).
>>
> 
> True :).
> 
>>> This series supersedes:
>>> https://lore.proxmox.com/pve-devel/20241025150243.134466-1-d.herzig@proxmox.com/
>>
>> I also just noticed that the repository names are gone from the
>> patches - seems like they were accidentally removed when formatting
>> the second version of these patches because they were there in v1.
> 
> Thanks, good catch, they'll be back in v3!

Looking forward to it! :)


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms
  2025-01-13  8:56 ` [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms Daniel Herzig
  2025-01-16 15:19   ` Daniel Kral
@ 2025-01-23 16:39   ` Friedrich Weber
  2025-01-24  7:35     ` Daniel Herzig
  1 sibling, 1 reply; 26+ messages in thread
From: Friedrich Weber @ 2025-01-23 16:39 UTC (permalink / raw)
  To: Proxmox VE development discussion, Daniel Herzig

Hi, I have two small things that I noticed skimming the series (inline).

On 13/01/2025 09:56, Daniel Herzig wrote:
> Eject by setting file to none.
> 
> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
> ---
>  www/manager6/qemu/HardwareView.js | 43 +++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 59e670db..5d1c18a5 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -538,6 +538,45 @@ Ext.define('PVE.qemu.HardwareView', {
>  	    apiurl: '/api2/extjs/' + baseurl,
>  	});
>  
> +	let eject_btn = new Proxmox.button.Button({
> +	    text: gettext('Eject'),
> +	    disabled: true,
> +	    selModel: sm,
> +	    RESTMethod: 'POST',
> +	    confirmMsg: function(rec) {
> +		let warn = gettext("Are you sure you want to eject '{0}' ?");
> +		let isofile = rec.data.value.split(",")[0];

Not a frontend expert, but it might be nicer to use something like
PVE.Parser.parsePropertyString to retrieve the ISO name here, instead of
manually splitting the string.

> +		let msg = Ext.String.format(warn, isofile);
> +		return msg;

This should be html-encoded before displaying, e.g. using Ext.htmlEncode.


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms
  2025-01-23 16:39   ` Friedrich Weber
@ 2025-01-24  7:35     ` Daniel Herzig
  0 siblings, 0 replies; 26+ messages in thread
From: Daniel Herzig @ 2025-01-24  7:35 UTC (permalink / raw)
  To: Friedrich Weber; +Cc: Proxmox VE development discussion

Hi Friedrich,

thanks for looking into this, this comes right in time, as I've so far
finished v3 of the backend-part.

Friedrich Weber <f.weber@proxmox.com> writes:

> Hi, I have two small things that I noticed skimming the series (inline).
>
> On 13/01/2025 09:56, Daniel Herzig wrote:
>> Eject by setting file to none.
>> 
>> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
>> ---
>>  www/manager6/qemu/HardwareView.js | 43 +++++++++++++++++++++++++++++++
>>  1 file changed, 43 insertions(+)
>> 
>> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
>> index 59e670db..5d1c18a5 100644
>> --- a/www/manager6/qemu/HardwareView.js
>> +++ b/www/manager6/qemu/HardwareView.js
>> @@ -538,6 +538,45 @@ Ext.define('PVE.qemu.HardwareView', {
>>  	    apiurl: '/api2/extjs/' + baseurl,
>>  	});
>>  
>> +	let eject_btn = new Proxmox.button.Button({
>> +	    text: gettext('Eject'),
>> +	    disabled: true,
>> +	    selModel: sm,
>> +	    RESTMethod: 'POST',
>> +	    confirmMsg: function(rec) {
>> +		let warn = gettext("Are you sure you want to eject '{0}' ?");
>> +		let isofile = rec.data.value.split(",")[0];
>
> Not a frontend expert, but it might be nicer to use something like
> PVE.Parser.parsePropertyString to retrieve the ISO name here, instead of
> manually splitting the string.
>
Perfect, will switch to using this.

>> +		let msg = Ext.String.format(warn, isofile);
>> +		return msg;
>
> This should be html-encoded before displaying, e.g. using Ext.htmlEncode.

Thanks for this one as well!


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


^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2025-01-24  7:35 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-01-13  8:55 [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Herzig
2025-01-13  8:55 ` [pve-devel] [PATCH v2 1/12] fix #4225: qemuserver: drive: add optional required parameter Daniel Herzig
2025-01-13  8:55 ` [pve-devel] [PATCH v2 2/12] qemuserver: add helper function for mocking files Daniel Herzig
2025-01-16 15:18   ` Daniel Kral
2025-01-17 11:36     ` Daniel Herzig
2025-01-13  8:55 ` [pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles Daniel Herzig
2025-01-16 15:19   ` Daniel Kral
2025-01-17 12:32     ` Daniel Herzig
2025-01-13  8:56 ` [pve-devel] [PATCH v2 4/12] test: chomp all trailing newlines from errors and warnings Daniel Herzig
2025-01-13  8:56 ` [pve-devel] [PATCH v2 5/12] test: mock cifs-store Daniel Herzig
2025-01-13  8:56 ` [pve-devel] [PATCH v2 6/12] test: add nfs-offline storage Daniel Herzig
2025-01-13  8:56 ` [pve-devel] [PATCH v2 7/12] test: mock existing files Daniel Herzig
2025-01-13  8:56 ` [pve-devel] [PATCH v2 8/12] test: mock log_warn warnings Daniel Herzig
2025-01-13  8:56 ` [pve-devel] [PATCH v2 9/12] test: cfg2cmd: add tests for testing the iso required parameter Daniel Herzig
2025-01-13  8:56 ` [pve-devel] [PATCH v2 10/12] fix #4225: ui: form: isoselector: add optional required checkbox Daniel Herzig
2025-01-13  8:56 ` [pve-devel] [PATCH v2 11/12] fix #4225: ui: qemu: cdedit: enable required checkbox for isos Daniel Herzig
2025-01-16 15:19   ` Daniel Kral
2025-01-17 12:38     ` Daniel Herzig
2025-01-13  8:56 ` [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms Daniel Herzig
2025-01-16 15:19   ` Daniel Kral
2025-01-17 12:47     ` Daniel Herzig
2025-01-23 16:39   ` Friedrich Weber
2025-01-24  7:35     ` Daniel Herzig
2025-01-16 15:18 ` [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs Daniel Kral
2025-01-17 11:34   ` Daniel Herzig
2025-01-17 13:04     ` Daniel Kral

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