Re: [pve-devel] [PATCH qemu-server v2] vm_start: add syslog info with which PID a VM was started
This works flawlessly on my testsetup. I find this information very
useful.
Reviewed-by: Daniel Herzig
Daniel Kral writes:
> Adds a syslog entry to log the process id that has been given to the
> QEMU VM process at start. This is helpful debugging information if the
> pid shows up at other places, like a kernel stack trace, while the VM
> has been running, but cannot be retrieved anymore (e.g. the pidfile has
> been deleted or only the syslog is available).
>
> The syslog has been put in the `PVE::QemuServer::vm_start_nolock`
> subroutine to make sure that the PID is logged not only when the VM has
> been started by the API endpoint `vm_start`, but also when the VM is
> started by a remote migration.
>
> Suggested-by: Hannes Dürr
> Suggested-by: Friedrich Weber
> Signed-off-by: Daniel Kral
> ---
> Differences to v1:
> - Fix commit message trailer order
> - Uppercase PID in syslog message
>
> PVE/QemuServer.pm | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index b26da505..ec370473 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -43,6 +43,7 @@ use PVE::ProcFSTools;
> use PVE::PBSClient;
> use PVE::RESTEnvironment qw(log_warn);
> use PVE::RPCEnvironment;
> +use PVE::SafeSyslog;
> use PVE::Storage;
> use PVE::SysFSTools;
> use PVE::Systemd;
> @@ -5971,6 +5972,8 @@ sub vm_start_nolock {
> eval { PVE::QemuServer::PCI::reserve_pci_usage($pci_reserve_list, $vmid,
> undef, $pid) };
> warn $@ if $@;
>
> +syslog("info", "VM $vmid started with PID $pid.");
> +
> if (defined(my $migrate = $res->{migrate})) {
> if ($migrate->{proto} eq 'tcp') {
> my $nodename = nodename();
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 12/12] ui: qemu: hardware: add eject button for cdroms
Eject by setting file to none.
Signed-off-by: Daniel Herzig
---
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
[pve-devel] [PATCH container] fix #5742: lxc: setup: centos template keep static ipv6 assignment
As reported here [0], statically assigned IPv6 addresses get lost
after a short while for Rocky and Almalinux 9 containers.
As pointed out by Andres, the additional option `IPV6_AUTOCONF=no`
for the corresponding network interface seems to prevent the
configured IPv6 address to be dropped.
[0] https://bugzilla.proxmox.com/show_bug.cgi?id=5745
Reported-by: Andres Moya
Signed-off-by: Daniel Herzig
---
src/PVE/LXC/Setup/CentOS.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/PVE/LXC/Setup/CentOS.pm b/src/PVE/LXC/Setup/CentOS.pm
index 2ad1f0c..fddcffa 100644
--- a/src/PVE/LXC/Setup/CentOS.pm
+++ b/src/PVE/LXC/Setup/CentOS.pm
@@ -228,7 +228,7 @@ sub setup_network {
} elsif ($d->{ip6} eq 'dhcp') {
$data .= "DHCPV6C=yes\n";
} else {
- $data .= "IPV6ADDR=$d->{ip6}\n";
+ $data .= "IPV6ADDR=$d->{ip6}\nIPV6_AUTOCONF=no\n";
if (defined($d->{gw6})) {
if (!PVE::Network::is_ip_in_cidr($d->{gw6}, $d->{ip6}, 6) &&
!PVE::Network::is_ip_in_cidr($d->{gw6}, 'fe80::/10', 6)
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH widget-toolkit v2] node/ServiceView: fix #5611: hiding the non-installed services
This patch adds a filter to identify services, which are reported as
'not-found' by the api. By default they will not be shown in the UI
anymore, but visibility can still be toggled using a new checkbox.
Signed-off-by: Daniel Herzig
---
changes since v1:
* applied Thomas' suggestions:
** use camelCase for filtername.
** invert default preselection logic and corresponding UI labelling.
src/node/ServiceView.js | 22 ++
1 file changed, 22 insertions(+)
diff --git a/src/node/ServiceView.js b/src/node/ServiceView.js
index 19cfc18..5c6333b 100644
--- a/src/node/ServiceView.js
+++ b/src/node/ServiceView.js
@@ -29,6 +29,8 @@ Ext.define('Proxmox.node.ServiceView', {
},
});
+ let filterInstalledOnly = record => record.get('unit-state') !==
'not-found';
+
let store = Ext.create('Proxmox.data.DiffStore', {
rstore: rstore,
sortAfterUpdate: true,
@@ -38,6 +40,24 @@ Ext.define('Proxmox.node.ServiceView', {
direction: 'ASC',
},
],
+ filters: [
+ filterInstalledOnly,
+ ],
+ });
+
+ let unHideCB = Ext.create('Ext.form.field.Checkbox', {
+ boxLabel: gettext('Show only installed services'),
+ value: true,
+ boxLabelAlign: 'before',
+ listeners: {
+ change: function(_cb, value) {
+ if (value) {
+ store.addFilter([filterInstalledOnly]);
+ } else {
+ store.clearFilter();
+ }
+ },
+ },
});
let view_service_log = function() {
@@ -166,6 +186,8 @@ Ext.define('Proxmox.node.ServiceView', {
restart_btn,
'-',
syslog_btn,
+ '->',
+ unHideCB,
],
columns: [
{
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH qemu-server, manager 0/12] bugzilla #4225 -- improve handling of unavailable ISOs
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'. *** BLURB HERE *** 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 | 38 +++- 13 files changed, 255 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
[pve-devel] [PATCH qemu-server 3/12] fix #4225: qemuserver: add function to eject isofiles
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
---
PVE/QemuServer.pm | 39 +++
1 file changed, 39 insertions(+)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index a45bdab9..8fddff92 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3994,6 +3994,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};
@@ -8942,6 +8944,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
[pve-devel] [PATCH qemu-server 2/12] qemuserver: add helper function for mocking files
This stub function can be used for mocking a file's existance in testruns.
Signed-off-by: Daniel Herzig
---
This is just a method to allow for mocking a files (not) existance for testing,
as I did not find a way to mock '-e' itself -- ideas very welcome!
PVE/QemuServer.pm | 5 +
1 file changed, 5 insertions(+)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 0df3bda0..a45bdab9 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8942,4 +8942,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
[pve-devel] [PATCH qemu-server 4/12] test: chomp all trailing newlines from errors and warnings
Ease EXPECT_ERROR and EXPECT_WARN string matching with errors/warnings
that have more than one trailing newline.
Signed-off-by: Daniel Herzig
---
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 8c525f09..2911483e 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
[pve-devel] [PATCH qemu-server 1/12] fix #4225: qemuserver: drive: add optional required parameter
Add parameter to allow for marking a drive as required.
Signed-off-by: Daniel Herzig
---
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 6e98c095..c80c0f07 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -154,7 +154,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
[pve-devel] [PATCH qemu-server 9/12] test: cfg2cmd: add tests for testing the iso required parameter
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 --- 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 ..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 ..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 ..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 ..e69de29b diff --git a/test/cfg2cmd/ide-required.conf b/test/cfg2cmd/ide-required.conf new file mode 100644 index ..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 ..7fd4888c --- /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 \
[pve-devel] [PATCH qemu-server 5/12] test: mock cifs-store
Let cifs-store appear as online to a call from
PVE::Storage::activate_storage.
Signed-off-by: Daniel Herzig
---
test/run_config2command_tests.pl | 13 +
1 file changed, 13 insertions(+)
diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index 2911483e..1a96c278 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -18,6 +18,7 @@ use PVE::QemuServer;
use PVE::QemuServer::Monitor;
use PVE::QemuServer::QMPHelpers;
use PVE::QemuServer::CPUConfig;
+use PVE::Storage::CIFSPlugin;
my $base_env = {
storage_config => {
@@ -392,6 +393,18 @@ $pci_module->mock(
}
);
+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
[pve-devel] [PATCH qemu-server 8/12] test: mock log_warn warnings
Strip log_warn wrapper for catching warnings on testruns.
Signed-off-by: Daniel Herzig
---
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 3414eea7..dd6717e2 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -214,6 +214,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
[pve-devel] [PATCH manager 10/12] fix #4225: ui: form: isoselector: add optional required checkbox
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
---
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
[pve-devel] [PATCH qemu-server 7/12] test: mock existing files
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
---
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 eda89dcb..3414eea7 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -210,6 +210,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
[pve-devel] [PATCH qemu-server 6/12] test: add nfs-offline storage
Add an nfs-offline storage to allow for comparatative testing
against potentially mocked online storages.
Signed-off-by: Daniel Herzig
---
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 1a96c278..eda89dcb 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -50,6 +50,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
[pve-devel] [PATCH manager 11/12] fix #4225: ui: qemu: cdedit: enable required checkbox for isos
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
---
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
[pve-devel] [PATCH storage] dirplugin: fix #3986: check for trailing slashes
This patch removes trailing slashes from manually entered
paths by adding an additional if clause in the sub
check_config.
Signed-off-by: Daniel Herzig
---
src/PVE/Storage/DirPlugin.pm | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
index 2efa8d5..9dcdf4a 100644
--- a/src/PVE/Storage/DirPlugin.pm
+++ b/src/PVE/Storage/DirPlugin.pm
@@ -244,6 +244,9 @@ sub check_config {
if ($opts->{path} !~ m|^/[-/a-zA-Z0-9_.@]+$|) {
die "illegal path for directory storage: $opts->{path}\n";
}
+if ($opts->{path} =~ /.*\/$/) {
+ $opts->{path} = substr($opts->{path}, 0 , -1);
+}
return $opts;
}
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH docs] increase verbosity of sdn dhcp docs.
These minor additions to and rearrangements within the documentation target easier accessibility for the SDN DHCP feature. Signed-off-by: Daniel Herzig --- pvesdn.adoc | 37 + 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/pvesdn.adoc b/pvesdn.adoc index 39de80f..d4f63fc 100644 --- a/pvesdn.adoc +++ b/pvesdn.adoc @@ -110,6 +110,12 @@ apt install dnsmasq systemctl disable --now dnsmasq +This disables the default dnsmasq service and allows for a separate dnsmasq service for +each configured Zone (`dnsmasq@$ZONE.service`). + +See the corresponding section in xref:pvesdn_config_dhcp[DHCP feature] for +configuration details. + [[pvesdn_install_frrouting]] FRRouting ~ @@ -627,14 +633,17 @@ available when using the xref:pvesdn_ipam_plugin_pveipam[PVE IPAM plugin]. Configuration ~ -You can enable automatic DHCP for a zone in the Web UI via the Zones panel and -enabling DHCP in the advanced options of a zone. +NOTE: Currently only Simple Zones have support for automatic DHCP. +Do not forget to follow the installation steps for the +xref:pvesdn_install_dhcp_ipam[dnsmasq DHCP plugin]! -NOTE: Currently only Simple Zones have support for automatic DHCP +You can enable automatic DHCP for a zone in the Web UI via the Zones panel and +enabling DHCP in the advanced options of a Zone. After automatic DHCP has been enabled for a Zone, DHCP Ranges need to be -configured for the subnets in a Zone. In order to that, go to the Vnets panel and -select the Subnet for which you want to configure DHCP ranges. In the edit +configured for the Subnets in a Zone. In order to that, go to the VNets panel and +create a VNet, which is attached to your Zone. Click your VNet and create +the Subnet for which you want to configure DHCP ranges. In the edit dialogue you can configure DHCP ranges in the respective Tab. Alternatively you can set DHCP ranges for a Subnet via the following CLI command: @@ -644,14 +653,26 @@ pvesh set /cluster/sdn/vnets//subnets/ -dhcp-range start-address=10.0.2.100,end-address=10.0.2.200 -You also need to have a gateway configured for the subnet - otherwise +You also need to have a gateway configured for the Subnet - otherwise automatic DHCP will not work. +Make sure the gateway's IP address is within the range of your Subnet +(eg 192.0.2.1 for a Subnet 192.0.2.0/24). This will be the address under +which you can reach your PVE host from the guest. If you want your guests +to have internet access, check the SNAT box as well. + +NOTE: The node will configure the gateway IP with the configured netmask +on the virtual bridge. Keep in mind that this range should not be in use +elsewhere in your network to avoid unexpected routing issues. + +After finishing your configuration, apply it from the SDN panel. + +The configuration results in a Linux bridge (named like your VNet) being configured +with the gateway's IP as its address. + The DHCP plugin will then allocate IPs in the IPAM only in the configured ranges. -Do not forget to follow the installation steps for the -xref:pvesdn_install_dhcp_ipam[dnsmasq DHCP plugin] as well. Plugins ~~~ -- 2.39.5 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH widget-toolkit] node/ServiceView: fix #5611: hiding the non-installed services
This patch adds a filter to identify services, which are reported as
'not-found' by the api. By default they will not be shown in the UI
anymore, but visibility can still be toggled using a new checkbox.
Signed-off-by: Daniel Herzig
---
src/node/ServiceView.js | 24
1 file changed, 24 insertions(+)
diff --git a/src/node/ServiceView.js b/src/node/ServiceView.js
index 19cfc18..f1c2e5f 100644
--- a/src/node/ServiceView.js
+++ b/src/node/ServiceView.js
@@ -29,6 +29,10 @@ Ext.define('Proxmox.node.ServiceView', {
},
});
+ let hide_not_installed = function(record) {
+ return record.get('unit-state') !== 'not-found';
+ };
+
let store = Ext.create('Proxmox.data.DiffStore', {
rstore: rstore,
sortAfterUpdate: true,
@@ -38,6 +42,24 @@ Ext.define('Proxmox.node.ServiceView', {
direction: 'ASC',
},
],
+ filters: [
+ hide_not_installed,
+ ],
+ });
+
+ let unHideCB = Ext.create('Ext.form.field.Checkbox', {
+ boxLabel: gettext('Show non-installed services'),
+ value: false,
+ boxLabelAlign: 'before',
+ listeners: {
+ change: function(_cb, value) {
+ if (value) {
+ store.clearFilter();
+ } else {
+ store.addFilter([hide_not_installed]);
+ }
+ },
+ },
});
let view_service_log = function() {
@@ -166,6 +188,8 @@ Ext.define('Proxmox.node.ServiceView', {
restart_btn,
'-',
syslog_btn,
+ '->',
+ unHideCB,
],
columns: [
{
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH docs/widget-toolkit/manager v3] implement tagview
Dominik Csapak writes: > On 11/7/24 14:52, Aaron Lauterer wrote: >> gave this a quick test. >> two things I noticed: >> * root element in tree per tag: wouldn't it be better to override >> the display style to 'full'? Otherwise I might have a lot of >> colorful dots, but don't know what the tags are called. > > that should be the case, but you need the widget toolkit patch installed for > that > >> * I am not 100% sure, but would it be possible to distinguish guests >> that have no tag assigned a bit better? maybe have a default >> "no-tags" tree for them? > > mhmm. possibly, but i used the same style as we have in the 'pool view' where > guests/storages > outside of pools are also just displayed one level higher > This looks neat! Earlier I actually misused the pools-view to achieve this kind of visual guest separation. Not directly applying to this thread, but Aaron's suggestion made me think if it wouldn't be nice to have something like 'not assigned to any pool' in the pool-view as well. >> On 2024-11-07 12:25, Dominik Csapak wrote: >>> this adds a 'tagview' to the web ui, organizing guests by their tags >>> (for details see the pve-manager patch) >>> >>> I'm not super happy all in all with how much special casing must be >>> done, but i could not (yet?) figure out a better way. >>> >>> changes from v2: >>> * rebased on master (tooltip generation changed so adapted to that) >>> * implemented fionas feedback, so selection should stay even when tags >>> are removed or the selection is changed from the tag view >>> >>> changes from v1: >>> * rebase on master >>> * adapt to recent tooltip changes >>> * add a comment to TagConfig class to better explain what it does >>> >>> pve-docs: >>> >>> Dominik Csapak (1): >>> gui: add anchor for tags chapter >>> >>> pve-gui.adoc | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> proxmox-widget-toolkit: >>> >>> Dominik Csapak (1): >>> css: add some conditions to the tag classes for the tag view >>> >>> src/css/ext6-pmx.css | 22 +++--- >>> 1 file changed, 15 insertions(+), 7 deletions(-) >>> >>> pve-manager: >>> >>> Dominik Csapak (1): >>> ui: implement 'Tag View' for the resource tree >>> >>> www/manager6/Makefile | 1 + >>> www/manager6/Workspace.js | 4 +- >>> www/manager6/form/ViewSelector.js | 28 +++ >>> www/manager6/grid/ResourceGrid.js | 2 +- >>> www/manager6/panel/TagConfig.js | 6 +++ >>> www/manager6/tree/ResourceTree.js | 81 --- >>> 6 files changed, 113 insertions(+), 9 deletions(-) >>> create mode 100644 www/manager6/panel/TagConfig.js >>> >> > > > > ___ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH manager] proxy: ui: vm console: autodetect novnc or xtermjs
I tested this patch a try with two guests: + debian bookworm (added a serial port, left display as default, mods from cyberciti.biz as below): + /etc/default/grub + ~GRUB_CMDLINE_LINUX='console=tty0 console=ttyS0,19200n8'~ + ~GRUB_TERMINAL=serial~ + ~GRUB_SERIAL_COMMAND="serial --speed=19200 --unit=0 --word=8 --parity=no --stop=1"~ + /etc/inittab + ~T0:23:respawn:/sbin/getty -L ttyS0 19200 vt100~ + /etc/securetty + ~ttyS0~ + debian bookworm (fresh installation, using ~Serial Terminal 0~ as graphiccard, using non-graphical installer, startup mods from infosecworrier.dk as below): + replace ~quiet~ with ~console=ttyS0,115200n81~ A rough edge I could think of would be, that if one merely adds a serial port to the hardware configuration on a OS that does not autosetup serial consoles, one would lose graphical connectivity. On the setups above this works like a charm! Tested-by: Daniel Herzig ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage/qemu-server/manager v6] implement ova/ovf import for file based storages
I've just tested this series with the following images: + GNS3 with VMware ESXi image from https://www.gns3.com/software/download-vm, unzipped and uploaded to local dir storage. + Ubuntu Noble from https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.ova, downloaded straight through the 'Download from URL' button. GNS3 imports nicely and runs. Cannot really tell about noble -- it imports nicely but will get stuck during startup with default settings (~btrfs loaded, zoned=yes, fsverity=yes~). Fiddling with hardware settings tends to let it boot, but I haven't yet managed to provide it with a username and password that I'd know. Best, Daniel Things that I've encountered: + Download from URL does not support zipped images. + If I use a root-mounted nfs share as extraction storage, I yield a (not having a clue where the chown comes from): extracting local:import/GNS3_VM.ova/GNS3_VM-disk1.vmdk tar: GNS3_VM-disk1.vmdk: Cannot change ownership to uid 64, gid 64: Operation not permitted tar: Exiting with failure status due to previous errors TASK ERROR: unable to create VM 112 - error during extraction: command 'tar -x --force-local \ -C /mnt/pve/nfs/images/112/tmp_3017_112 -f /var/lib/vz/import/GNS3_VM.ova GNS3_VM-disk1.vmdk' failed: exit code 2 + Although I understand the purpose, it did not feel very 'natural' having to add the 'disk image' storage type to the local directory with the .ova-file for a successful import. ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage/qemu-server/manager v6] implement ova/ovf import for file based storages
Daniel Herzig writes: > I've just tested this series with the following images: > > + GNS3 with VMware ESXi image from https://www.gns3.com/software/download-vm, > unzipped and uploaded to local dir storage. > + Ubuntu Noble from > https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.ova, > downloaded straight through the 'Download from URL' button. > > GNS3 imports nicely and runs. > > Cannot really tell about noble -- it imports nicely > but will get stuck during startup with default settings > (~btrfs loaded, zoned=yes, fsverity=yes~). Fiddling with > hardware settings tends to let it boot, but I haven't yet managed to > provide it with a username and password that I'd know. > Finally got it: + VirtIO SCSI single as SCSI Controller (from LSI53C895A). + VirtIO paravirtualized (from VMware vmxnet3). + Cloudinit drive with username, password and ip=dhcp. Works nicely for Noble in my SDN simple zone setup! > Best, > Daniel > > Things that I've encountered: > + Download from URL does not support zipped images. > + If I use a root-mounted nfs share as extraction storage, I yield a > (not having a clue where the chown comes from): > > extracting local:import/GNS3_VM.ova/GNS3_VM-disk1.vmdk > tar: GNS3_VM-disk1.vmdk: Cannot change ownership to uid 64, gid 64: Operation > not permitted > tar: Exiting with failure status due to previous errors > TASK ERROR: unable to create VM 112 - error during extraction: command > 'tar -x --force-local \ > -C /mnt/pve/nfs/images/112/tmp_3017_112 -f /var/lib/vz/import/GNS3_VM.ova > GNS3_VM-disk1.vmdk' failed: exit code 2 > > + Although I understand the purpose, it did not feel very 'natural' > having to add the 'disk image' storage type to the local directory with > the .ova-file for a successful import. > > > > ___ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage] dirplugin: fix #3986: check for trailing slashes
Thanks for your feedback, I started working on a v2 including your suggestions.
Stoiko Ivanov writes:
> Thanks for tackling this long-open issue!
>
> On Wed, Nov 13, 2024 at 05:32:49PM +0100, Daniel Herzig wrote:
>> This patch removes trailing slashes from manually entered
>> paths by adding an additional if clause in the sub
>> check_config.
> For me the context of the change, and why something in the commit is done
> the way it is, is more helpful than explaining what the code does (this
> should be clear from the code itself).
>
> I wanted to drop a small suggestion for a shortening of the perl-code (see
> comment inline below) - but then wondered why an adaptation of the path
> happens in check_config of a storage plugin - as it's been a while since I
> dealt with the storage-config.
>
> The change might be ok - as PVE::SectionConfig calls check_config of the
> plugin when parsing the configuration file - at least to me that's not
> directly obvious.
>
> Also mentioning how you tested a change can also be helpful to reviewers.
>
I'm currently testing the change by either adding a new dir storage
setting a path with trailing slashes via the GUI and `pvesm add dir
$STORAGENAME --path $PATH_TO_DIR`.
What's interesting here, is that if there's a path with trailing slash
set before applying the patch, it only seems to get updated once a new
dir storage gets added after applying it. I haven't yet finally figured out,
when exactly the config_check is getting triggered.
I.e. further tackling needed.
>>
>> Signed-off-by: Daniel Herzig
>> ---
>> src/PVE/Storage/DirPlugin.pm | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
>> index 2efa8d5..9dcdf4a 100644
>> --- a/src/PVE/Storage/DirPlugin.pm
>> +++ b/src/PVE/Storage/DirPlugin.pm
>> @@ -244,6 +244,9 @@ sub check_config {
>> if ($opts->{path} !~ m|^/[-/a-zA-Z0-9_.@]+$|) {
>> die "illegal path for directory storage: $opts->{path}\n";
>> }
>> +if ($opts->{path} =~ /.*\/$/) {
>> +$opts->{path} = substr($opts->{path}, 0 , -1);
>> +}
> without explicit testing - this could be put as:
> $opts->{path} =~ s/(.*)\/$/$1/;
> (you could also add + to strip multiple trailing slashes)
>
This is cool and works, thanks!
>> return $opts;
>> }
>>
>> --
>> 2.39.5
>>
>>
>>
>> ___
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>
>
> ___
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH guest-common 1/4] fix #5900: add helper function
This patch adds a small helper function to retrieve the bridge name
from the netN parameter string of a container or VM configuration.
Signed-off-by: Daniel Herzig
---
src/PVE/GuestHelpers.pm | 11 +++
1 file changed, 11 insertions(+)
diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
index 592b4a8..c6006ba 100644
--- a/src/PVE/GuestHelpers.pm
+++ b/src/PVE/GuestHelpers.pm
@@ -450,4 +450,15 @@ sub abort_guest_tasks {
return $aborted_tasks;
}
+sub get_bridge {
+my $net_params = shift;
+my $param_array = [ split(/,/, $net_params) ];
+my $bridge;
+for my $net_param (@$param_array) {
+ $bridge = $net_param if ($net_param =~ /bridge\=/);
+ $bridge =~ s|bridge\=|| if (defined($bridge));
+}
+return $bridge;
+}
+
1;
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH network 2/4] fix #5900: add helper functions
This patch adds helper functions to evaluate if a vnet (bridge) associated
with a zone under SDN's auto-dhcp control via dnsmasq can retrieve a
dhcp lease.
Signed-off-by: Daniel Herzig
---
src/PVE/Network/SDN/Dhcp.pm | 83 +
1 file changed, 83 insertions(+)
diff --git a/src/PVE/Network/SDN/Dhcp.pm b/src/PVE/Network/SDN/Dhcp.pm
index d48de34..4ddd128 100644
--- a/src/PVE/Network/SDN/Dhcp.pm
+++ b/src/PVE/Network/SDN/Dhcp.pm
@@ -128,4 +128,87 @@ sub regenerate_config {
}
}
+sub defined_dhcp_ip_count_in_zone {
+my $zone_id = shift;
+my $vnets_in_zone = PVE::Network::SDN::Zones::get_vnets($zone_id);
+my $range_count_array;
+my $res;
+for my $vnet_id (keys %$vnets_in_zone) {
+ my $subnets_in_vnet = PVE::Network::SDN::Vnets::get_subnets($vnet_id);
+ for my $subnet (keys %$subnets_in_vnet) {
+ my $dhcp_ranges =
PVE::Network::SDN::Subnets::get_dhcp_ranges(${subnets_in_vnet}->{$subnet});
+ if (scalar @$dhcp_ranges) {
+ for my $dhcp_range (@$dhcp_ranges) {
+ my $start_ip = ${dhcp_range}->{'start-address'};
+ my $end_ip = ${dhcp_range}->{'end-address'};
+ my $subnet_ip_count = new Net::IP("$start_ip -
$end_ip")->size();
+ push (@$range_count_array, $subnet_ip_count);
+ }
+ }
+ }
+}
+if ($range_count_array) {
+ $res = eval join '+', @$range_count_array;
+}
+return $res;
+}
+
+sub used_dhcp_ips_in_zone {
+my $zone_id = shift;
+my $pve_ipam_db = PVE::Network::SDN::Ipams::PVEPlugin::read_db();
+my $subnets_in_zone = $pve_ipam_db->{'zones'}->{$zone_id}->{'subnets'};
+my $res;
+for my $subnet_in_zone (keys %$subnets_in_zone) {
+ my $ips_in_subnet = ${subnets_in_zone}->{$subnet_in_zone}->{'ips'};
+ if (scalar (keys %$ips_in_subnet)) {
+ for my $leased_ip (keys %$ips_in_subnet) {
+ $res++ if (!exists ${ips_in_subnet}->{$leased_ip}->{'gateway'});
+ }
+ }
+}
+return $res;
+}
+
+sub available_dhcp_ips_in_zone {
+my $zone_id = shift;
+my $available_ip_count = defined_dhcp_ip_count_in_zone($zone_id);
+my $used_ip_count = used_dhcp_ips_in_zone($zone_id);
+if (!defined($available_ip_count)) {
+ $available_ip_count = 0;
+}
+if (!defined($used_ip_count)) {
+ $used_ip_count = 0;
+}
+my $res = $available_ip_count - $used_ip_count;
+return $res;
+}
+
+sub test_bridge_for_sdn_dnsmasq {
+my $bridge = shift;
+my $vnets_cfg = PVE::Network::SDN::Vnets::config();
+my $vnet_ids = [ PVE::Network::SDN::Vnets::sdn_vnets_ids($vnets_cfg) ];
+my $zones_cfg = PVE::Network::SDN::Zones::config();
+my $zone_ids = [ PVE::Network::SDN::Zones::sdn_zones_ids($zones_cfg) ];
+my $dhcp_dnsmasq_zones;
+my $vnets_in_dhcp_dnsmasq_zones;
+for my $zone (@$zone_ids) {
+ push(@$dhcp_dnsmasq_zones, $zone)
+ if (defined(${zones_cfg}->{'ids'}->{$zone}->{'dhcp'}) &&
+ (${zones_cfg}->{'ids'}->{$zone}->{'dhcp'} eq 'dnsmasq'))
+}
+for my $vnet (@$vnet_ids) {
+ my $vnet_zone = ${vnets_cfg}->{'ids'}->{$vnet}->{'zone'};
+ push(@$vnets_in_dhcp_dnsmasq_zones, $vnet)
+ if ("@$dhcp_dnsmasq_zones" =~ /$vnet_zone/)
+}
+if (("@$vnets_in_dhcp_dnsmasq_zones" =~ /$bridge/)) {
+ my $zone_id = ${vnets_cfg}->{'ids'}->{$bridge}->{'zone'};
+ if ((PVE::Network::SDN::Dhcp::available_dhcp_ips_in_zone($zone_id)) lt
1) {
+ die "No DHCP leases left in zone '$zone_id' for bridge '$bridge',
please check your SDN config.\n";
+ }
+}
+}
+
+
+
1;
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH container 3/4] fix #5900: do not create container if dhcp range is exhausted
This patch prevents containers to be created if their bridge is
associated with a SDN zone with dnsmasq automatic dhcp enabled, and if
the dhcp-range is exhausted or unset.
Signed-off-by: Daniel Herzig
---
src/PVE/API2/LXC.pm | 12
1 file changed, 12 insertions(+)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 213e518..35fb0a6 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -244,6 +244,18 @@ __PACKAGE__->register_method({
PVE::LXC::Config->check_protection($conf, "unable to restore CT
$vmid");
}
+ for my $opt (keys %$param) {
+ if ($opt =~ /net/) {
+ my $bridge_err;
+ my $bridge = PVE::GuestHelpers::get_bridge(${param}->{$opt});
+ if (defined($bridge)) {
+ eval {
PVE::Network::SDN::Dhcp::test_bridge_for_sdn_dnsmasq($bridge) };
+ ${bridge_err}->{$opt} = $@ if $@;
+ raise_param_exc($bridge_err) if $bridge_err;
+ }
+ }
+ }
+
my $password = extract_param($param, 'password');
my $ssh_keys = extract_param($param, 'ssh-public-keys');
PVE::Tools::validate_ssh_public_keys($ssh_keys) if defined($ssh_keys);
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH guest-common, network, container, qemu-server 0/4] fix #5900: enhance SDN auto-dhcp behaviour
This patch series prevents containers and vms to be created when their bridges are within an SDN zone, that is set to deploy IPs automatically via dnsmasq, while the dhcp range is exhausted. Currently adding a bridge under these circumstances is already impossible, however on VM or container creation, this is overridden, which can lead to confusing behaviour as described in detail on bugzilla. *** BLURB HERE *** guest-common: Daniel Herzig (1): fix #5900: add helper function src/PVE/GuestHelpers.pm | 11 +++ 1 file changed, 11 insertions(+) network: Daniel Herzig (1): fix #5900: add helper functions src/PVE/Network/SDN/Dhcp.pm | 83 + 1 file changed, 83 insertions(+) container: Daniel Herzig (1): fix #5900: do not create container if dhcp range is exhausted src/PVE/API2/LXC.pm | 12 1 file changed, 12 insertions(+) qemu-server: Daniel Herzig (1): fix #5900: do not create vm if dhcp range is exhausted PVE/API2/Qemu.pm | 12 1 file changed, 12 insertions(+) -- 2.39.5 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH qemu-server 4/4] fix #5900: do not create vm if dhcp range is exhausted
This patch prevents VMs to be created if their bridge is
associated with a SDN zone with dnsmasq automatic dhcp enabled, and if
the dhcp-range is exhausted or unset.
Signed-off-by: Daniel Herzig
---
PVE/API2/Qemu.pm | 12
1 file changed, 12 insertions(+)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index ec45d5ff..73580c64 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1065,6 +1065,18 @@ __PACKAGE__->register_method({
raise_perm_exc();
}
+ for my $opt (keys %$param) {
+ if ($opt =~ /net/) {
+ my $bridge_err;
+ my $bridge = PVE::GuestHelpers::get_bridge(${param}->{$opt});
+ if (defined($bridge)) {
+ eval {
PVE::Network::SDN::Dhcp::test_bridge_for_sdn_dnsmasq($bridge) };
+ ${bridge_err}->{$opt} = $@ if $@;
+ raise_param_exc($bridge_err) if $bridge_err;
+ }
+ }
+ }
+
if ($archive) {
for my $opt (sort keys $param->%*) {
if (PVE::QemuServer::Drive::is_valid_drivename($opt)) {
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v3 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos
Fiona Ebner writes:
> Am 31.01.25 um 14:58 schrieb Daniel Herzig:
>> Fiona Ebner writes:
>>
>>> Am 31.01.25 um 10:36 schrieb Fiona Ebner:
>>>> Am 30.01.25 um 12:31 schrieb Daniel Herzig:
>>>>> +
>>>>> +$drive->{essential} = 1 if !defined($drive->{essential});
>>>>
>>>> This should rather be done when parsing the drive.
>>>
>>> Or I guess to avoid (potentially) writing it out for every drive, it
>>> should only be a local variable here and not set in the drive hash.
>>
>> Thanks, I'll double check on this for v4. But I'd assume the hash to be
>> scoped to
>> `config_to_command` here, or am I missing something?
>>
>
> But you don't need the modified version in config_to_command() later,
> or? And if yes, that can just check the same way again, i.e. using
> default value if not set. If we want to explicitly set the value, that
> should happen during parsing the drive string. Most of the time it's
> surprising to implicitly modify a passed-in value. Better to either
> avoid that or if really necessary, explicitly mention it in the function
> description.
No, I do not need the modified version of the VM-config later.
What I'm trying to achieve is a more 'forgiving' behaviour in the case
of accidently server-side-deleted iso file/unavailable server (for whatever
reason)
attached to a VM. So this is actually aiming at `qm start`, which
implicitly calls `config_to_command` -- without modifying the existing
VM config at all.
If the parameter 'essential' is set to '0', `config_to_command` would --
in case of file unavailability of the iso file -- generate a kvm startup
command that contains "-drive 'if=none,media=cdrom,[...]" instead of
"-drive 'file=$SOME_PATH_TO_ISO,[..]' when we at this point already know
that $SOME_PATH_TO_ISO is unavailable/non-existent.
The VM-config itself is not changed, as an eg nfs-server might come back
at a point in time later and the user might want to do something with the iso
stored there.
If the parameter 'essential' is unset, or set to '1', the die would
happen before `qm start` leads to an invocation of kvm, as it cannot be
expected to lead to a successful action, if $SOME_PATH_TO_ISO is not
reachable. This would be the exact behaviour as we have it now, just not
letting kvm run into this situation, but detect and exit earlier, while
`config_to_commands` iterates over all volumes via `foreach_volume`
anyway before producing the 'final' kvm command.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v3 0/6] bugzilla #4225 -- improve handling of unavailable ISOs
Fiona Ebner writes: > Am 30.01.25 um 12:31 schrieb Daniel Herzig: >> 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 'essential'. >> 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. > > Since you use the word "transition", is there a plan to change the > default behavior at some point? IMHO that requires more rationale > (especially in the backend). > I just used this phrasing to underline that the option 'essential', if pro-actively set to 0, will change current behaviour. Without intervention, people should not notice any difference to current behaviour with the changes from these patches. Except the startup error additionaly listing the file, that's triggering the contact to a non-available network share. I might have a slight preference to not see ISOs as essential files after a successful installation indeed, but there certainly are setups, where it does not make sense to attempt starting up a machine (or let the attempt fail) without an available ISO attached to the VM. I'm eg thinking about ephemeral VMs running from live-ISOs or the like. >> >> If a new VM is created from the WebGUI, a corresponding added checkbox >> is checked by default, but allows for convenient unchecking during setup >> time, >> eg for media that is only needed for installation time. >> >> This patch series adds an 'Eject' button to the hardwareview in the WebGUI, >> which can be used as a convenience shortcut to setting file to 'none' for the >> cdrom drive. >> >> This series supersedes: >> https://lore.proxmox.com/pve-devel/20250113085608.99498-1-d.her...@proxmox.com/ ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v3 1/6] fix #4225: qemuserver: drive: add parameter to mark drive required
Fiona Ebner writes:
> The 'qemuserver' prefix in the commit title doesn't add any information
> and should not be there. Commit title prefixes are not for file names.
> This also doesn't fix the issue yet, so I'd also drop that prefix too.
>
Right, missed that redundancy. Will be fixed in some v4.
> Am 30.01.25 um 12:31 schrieb Daniel Herzig:
>> This commit add the parameter `essential` to mark a drive as required
>> for booting the VM.
>>
>> Signed-off-by: Daniel Herzig
>> ---
>> 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 1041c1dd..38136787 100644
>> --- a/PVE/QemuServer/Drive.pm
>> +++ b/PVE/QemuServer/Drive.pm
>> @@ -266,7 +266,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,
>> -}
>> +},
>> +essential => {
>> +type => 'boolean',
>> +description => 'Mark this iso volume as required for booting the VM.',
>
> Nit: Since the 'media=cdrom' option is used to decide this, I'd state
> "CD-ROM" here to be more precise. I'd also say "for starting the VM"
> rather than "for booting the VM". The device isn't necessarily involved
> into booting, but can still be considered essential to allow starting
> the VM.
Thanks for pointing this out, this part needs some re-working indeed.
>
>> +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 => {
>
> There should be some checking/handling in the API endpoints and/or
> parse_drive() for making sure this can only be set in combination with
> 'media=cdrom'. There are already such checks in parse_drive() which will
> return undef in those cases, but please also issue a warning for a new
> such check so that it will be clear what went wrong ;)
>
I'll look this up and check against various ~qm set~ scenarios.
> Nit: Usually, it's nicer to have booleans be 0 if not present. So can we
> invert this e.g. "ignore-if-missing" (or "detach-if-missing" or
> "eject-if-missing")? Otherwise, maybe "essential-for-start" to be more
> descriptive?
Good idea, getting ready to invert my brains :)
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v3 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos
Fiona Ebner writes:
> Am 31.01.25 um 10:36 schrieb Fiona Ebner:
>> Am 30.01.25 um 12:31 schrieb Daniel Herzig:
>>> +
>>> +$drive->{essential} = 1 if !defined($drive->{essential});
>>
>> This should rather be done when parsing the drive.
>
> Or I guess to avoid (potentially) writing it out for every drive, it
> should only be a local variable here and not set in the drive hash.
Thanks, I'll double check on this for v4. But I'd assume the hash to be scoped
to
`config_to_command` here, or am I missing something?
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH 2/8 container] cloudinit: basic implementation
From: Leo Nunner
The code to generate the actual configuration works pretty much the same
as with the VM system. We generate an instance ID by hashing the user
configuration, causing cloud-init to run every time said configuration
changes.
Instead of creating a config drive, we write files directly into the
volume of the container. We create a folder at
'/var/lib/cloud/seed/nocloud-net' and write the files 'user-data',
'vendor-data' and 'meta-data'. Cloud-init looks at the instance ID
inside 'meta-data' to decide whether it should run (again) or not.
Custom scripts need to be located inside the snippets directory, and
overwrite the default generated configuration file.
Signed-off-by: Leo Nunner
---
src/PVE/LXC.pm| 1 +
src/PVE/LXC/Cloudinit.pm | 114 ++
src/PVE/LXC/Makefile | 1 +
src/lxc-pve-prestart-hook | 5 ++
4 files changed, 121 insertions(+)
create mode 100644 src/PVE/LXC/Cloudinit.pm
diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 4d20645..35bb6b5 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -40,6 +40,7 @@ use PVE::Tools qw(
use PVE::Syscall qw(:fsmount);
use PVE::LXC::CGroup;
+use PVE::LXC::Cloudinit;
use PVE::LXC::Config;
use PVE::LXC::Monitor;
use PVE::LXC::Tools;
diff --git a/src/PVE/LXC/Cloudinit.pm b/src/PVE/LXC/Cloudinit.pm
new file mode 100644
index 000..3e8617b
--- /dev/null
+++ b/src/PVE/LXC/Cloudinit.pm
@@ -0,0 +1,114 @@
+package PVE::LXC::Cloudinit;
+
+use strict;
+use warnings;
+
+use Digest::SHA;
+use File::Path;
+
+use PVE::LXC;
+
+sub gen_cloudinit_metadata {
+my ($user) = @_;
+
+my $uuid_str = Digest::SHA::sha1_hex($user);
+return cloudinit_metadata($uuid_str);
+}
+
+sub cloudinit_metadata {
+my ($uuid) = @_;
+my $raw = "";
+
+$raw .= "instance-id: $uuid\n";
+
+return $raw;
+}
+
+sub cloudinit_userdata {
+my ($conf) = @_;
+
+my $content = "#cloud-config\n";
+
+my $username = $conf->{ciuser};
+my $password = $conf->{cipassword};
+
+$content .= "user: $username\n" if defined($username);
+$content .= "password: $password\n" if defined($password);
+
+if (defined(my $keys = $conf->{sshkeys})) {
+ $keys = URI::Escape::uri_unescape($keys);
+ $keys = [map { my $key = $_; chomp $key; $key } split(/\n/, $keys)];
+ $keys = [grep { /\S/ } @$keys];
+ $content .= "ssh_authorized_keys:\n";
+ foreach my $k (@$keys) {
+ $content .= " - $k\n";
+ }
+}
+$content .= "chpasswd:\n";
+$content .= " expire: False\n";
+
+if (!defined($username) || $username ne 'root') {
+ $content .= "users:\n";
+ $content .= " - default\n";
+}
+
+$content .= "package_upgrade: true\n" if $conf->{ciupgrade};
+
+return $content;
+}
+
+sub read_cloudinit_snippets_file {
+my ($storage_conf, $volid) = @_;
+
+my ($full_path, undef, $type) = PVE::Storage::path($storage_conf, $volid);
+die "$volid is not in the snippets directory\n" if $type ne 'snippets';
+return PVE::Tools::file_get_contents($full_path, 1 * 1024 * 1024);
+}
+
+sub read_custom_cloudinit_files {
+my ($conf) = @_;
+
+my $cloudinit_conf = $conf->{cicustom};
+my $files = $cloudinit_conf ?
PVE::JSONSchema::parse_property_string('pve-pct-cicustom', $cloudinit_conf) :
{};
+
+my $user_volid = $files->{user};
+my $vendor_volid = $files->{vendor};
+
+my $storage_conf = PVE::Storage::config();
+
+my $user_data;
+if ($user_volid) {
+ $user_data = read_cloudinit_snippets_file($storage_conf, $user_volid);
+}
+
+my $vendor_data;
+if ($vendor_volid) {
+ $user_data = read_cloudinit_snippets_file($storage_conf, $vendor_volid);
+}
+
+return ($user_data, $vendor_data);
+}
+
+sub create_cloudinit_files {
+my ($conf, $setup) = @_;
+
+my $cloudinit_dir = "/var/lib/cloud/seed/nocloud-net";
+
+my ($user_data, $vendor_data) = read_custom_cloudinit_files($conf);
+$user_data = cloudinit_userdata($conf) if !defined($user_data);
+$vendor_data = '' if !defined($vendor_data);
+
+my $meta_data = gen_cloudinit_metadata($user_data);
+
+$setup->protected_call(sub {
+ my $plugin = $setup->{plugin};
+
+ $plugin->ct_make_path($cloudinit_dir);
+
+ $plugin->ct_file_set_contents("$cloudinit_dir/user-data", $user_data);
+ $plugin->ct_file_set_contents("$cloudinit_dir/vendor-data",
$vendor_data);
+ $plugin->ct_file_set_contents("$cloudinit_dir/meta-data", $meta_data);
+});
+}
+
+1;
diff --git a/src/PVE/LXC/Makefile b/src/PVE/LXC/Makefile
index a190260..5d595ba 100644
--- a/src/PVE/LXC/Makefile
+++ b/src/PVE/LXC/Makefile
@@ -1,5 +1,6 @@
SOURCES= \
CGroup.pm \
+ Cloudinit.pm \
Command.pm \
Config.pm \
Create.pm \
diff --git a/src/lxc-pve-prestart-hook b/src/lxc-pve-prestart-hook
index fdaead2..c9f8ff0 100755
--- a/src/lxc-pve-prestart-hook
+++ b/src/lxc-pve-prestart-hook
@@ -13,6 +13,7 @@ use POSIX
[pve-devel] [PATCH 6/8 manager] cloudinit: introduce panel for LXCs
From: Leo Nunner
based on the already existing panel for VMs. Some things have been
changed, there is no network configuration, and a separate "enable"
options toggles cloud-init (simillar to adding/removing a cloud-init
drive for VMs).
Signed-off-by: Leo Nunner
---
www/manager6/Makefile | 1 +
www/manager6/lxc/CloudInit.js | 237 ++
www/manager6/lxc/Config.js| 6 +
3 files changed, 244 insertions(+)
create mode 100644 www/manager6/lxc/CloudInit.js
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index c94a5cdf..a0e7a2e6 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -189,6 +189,7 @@ JSSRC=
\
dc/PCIMapView.js\
dc/USBMapView.js\
lxc/CmdMenu.js \
+ lxc/CloudInit.js\
lxc/Config.js \
lxc/CreateWizard.js \
lxc/DeviceEdit.js \
diff --git a/www/manager6/lxc/CloudInit.js b/www/manager6/lxc/CloudInit.js
new file mode 100644
index ..11d5448d
--- /dev/null
+++ b/www/manager6/lxc/CloudInit.js
@@ -0,0 +1,237 @@
+Ext.define('PVE.lxc.CloudInit', {
+extend: 'Proxmox.grid.PendingObjectGrid',
+xtype: 'pveLxcCiPanel',
+
+tbar: [
+ {
+ xtype: 'proxmoxButton',
+ disabled: true,
+ dangerous: true,
+ confirmMsg: function(rec) {
+ let view = this.up('grid');
+ var warn = gettext('Are you sure you want to remove entry {0}');
+
+ var entry = rec.data.key;
+ var msg = Ext.String.format(warn, "'"
+ + view.renderKey(entry, {}, rec) + "'");
+
+ return msg;
+ },
+ enableFn: function(record) {
+ let view = this.up('grid');
+ var caps = Ext.state.Manager.get('GuiCap');
+ if (view.rows[record.data.key].never_delete ||
+ !caps.vms['VM.Config.Network']) {
+ return false;
+ }
+
+ if (record.data.key === 'cipassword' && !record.data.value) {
+ return false;
+ }
+ return true;
+ },
+ handler: function() {
+ let view = this.up('grid');
+ let records = view.getSelection();
+ if (!records || !records.length) {
+ return;
+ }
+
+ var id = records[0].data.key;
+
+ var params = {};
+ params.delete = id;
+ Proxmox.Utils.API2Request({
+ url: view.baseurl + '/config',
+ waitMsgTarget: view,
+ method: 'PUT',
+ params: params,
+ failure: function(response, opts) {
+ Ext.Msg.alert('Error', response.htmlStatus);
+ },
+ callback: function() {
+ view.reload();
+ },
+ });
+ },
+ text: gettext('Remove'),
+ },
+ {
+ xtype: 'proxmoxButton',
+ disabled: true,
+ enableFn: function(rec) {
+ let view = this.up('pveLxcCiPanel');
+ return !!view.rows[rec.data.key].editor;
+ },
+ handler: function() {
+ let view = this.up('grid');
+ view.run_editor();
+ },
+ text: gettext('Edit'),
+ },
+],
+
+border: false,
+
+renderKey: function(key, metaData, rec, rowIndex, colIndex, store) {
+ var me = this;
+ var rows = me.rows;
+ var rowdef = rows[key] || {};
+
+ var icon = "";
+ if (rowdef.iconCls) {
+ icon = ' ';
+ }
+ return icon + (rowdef.header || key);
+},
+
+listeners: {
+ activate: function() {
+ var me = this;
+ me.rstore.startUpdate();
+ },
+ itemdblclick: function() {
+ var me = this;
+ me.run_editor();
+ },
+},
+
+initComponent: function() {
+ var me = this;
+
+ var nodename = me.pveSelNode.data.node;
+ if (!nodename) {
+ throw "no node name specified";
+ }
+
+ var vmid = me.pveSelNode.data.vmid;
+ if (!vmid) {
+ throw "no VM ID specified";
+ }
+ var caps = Ext.state.Manager.get('GuiCap');
+ me.baseurl = '/api2/extjs/nodes/' + nodename + '/lxc/' + vmid;
+ me.url = me.baseurl + '/pending';
+ me.editorConfig.url = me.baseurl + '/config';
+ me.editorConfig.pveSelNode = me.pveSelNode;
+
+ let caps_ci = caps.vms['VM.Config.Cloudinit'] ||
caps.vms['VM.Config.Network'];
+ /* editor is string and object
[pve-devel] [PATCH 3/8 container] cloudinit: add dump command to pct
From: Leo Nunner
Introduce a 'pct cloudinit dump ' command to dump the
generated cloudinit configuration for a section.
Signed-off-by: Leo Nunner
---
src/PVE/API2/LXC.pm | 33 +
src/PVE/CLI/pct.pm | 4
src/PVE/LXC/Cloudinit.pm | 11 +++
3 files changed, 48 insertions(+)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index d6e647d..e883c43 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -3041,4 +3041,37 @@ __PACKAGE__->register_method({
return { socket => $socket };
}});
+
+__PACKAGE__->register_method({
+name => 'cloudinit_generated_config_dump',
+path => '{vmid}/cloudinit/dump',
+method => 'GET',
+proxyto => 'node',
+description => "Get automatically generated cloudinit config.",
+permissions => {
+ check => ['perm', '/vms/{vmid}', [ 'VM.Audit' ]],
+},
+parameters => {
+ additionalProperties => 0,
+ properties => {
+ node => get_standard_option('pve-node'),
+ vmid => get_standard_option('pve-vmid', { completion =>
\&PVE::LXC::complete_ctid }),
+ type => {
+ description => 'Config type.',
+ type => 'string',
+ enum => ['user', 'meta'],
+ },
+ },
+},
+returns => {
+ type => 'string',
+},
+code => sub {
+ my ($param) = @_;
+
+ my $conf = PVE::LXC::Config->load_config($param->{vmid});
+
+ return PVE::LXC::Cloudinit::dump_cloudinit_config($conf,
$param->{type});
+}});
+
1;
diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
index b0a1ec2..60c2ccd 100755
--- a/src/PVE/CLI/pct.pm
+++ b/src/PVE/CLI/pct.pm
@@ -1046,6 +1046,10 @@ our $cmddef = {
rescan => [ __PACKAGE__, 'rescan', []],
cpusets => [ __PACKAGE__, 'cpusets', []],
fstrim => [ __PACKAGE__, 'fstrim', ['vmid']],
+
+cloudinit => {
+ dump => [ "PVE::API2::LXC", 'cloudinit_generated_config_dump', ['vmid',
'type'], { node => $nodename }, sub { print "$_[0]\n"; }],
+},
};
1;
diff --git a/src/PVE/LXC/Cloudinit.pm b/src/PVE/LXC/Cloudinit.pm
index 3e8617b..b6fec2c 100644
--- a/src/PVE/LXC/Cloudinit.pm
+++ b/src/PVE/LXC/Cloudinit.pm
@@ -111,4 +111,15 @@ sub create_cloudinit_files {
});
}
+sub dump_cloudinit_config {
+my ($conf, $type) = @_;
+
+if ($type eq 'user') {
+ return cloudinit_userdata($conf);
+} else { # metadata config
+ my $user = cloudinit_userdata($conf);
+ return gen_cloudinit_metadata($user);
+}
+}
+
1;
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH 8/8 docs] pct: document cloudinit for LXC
From: Leo Nunner
adds documentation for Cloud-Init for containers. Most of it has been
taken from the VM documentation, since the configuration mostly works
the same. Added a script to extract the cloudinit parameters the same
way it's already done for VMs.
Signed-off-by: Leo Nunner
---
Makefile| 1 +
pct-cloud-init.adoc | 114
pct.adoc| 4 ++
3 files changed, 119 insertions(+)
create mode 100644 pct-cloud-init.adoc
diff --git a/Makefile b/Makefile
index f30d77a..24836b8 100644
--- a/Makefile
+++ b/Makefile
@@ -53,6 +53,7 @@ GEN_SCRIPTS= \
gen-pct.conf.5-opts.pl \
gen-pct-network-opts.pl \
gen-pct-mountpoint-opts.pl \
+ gen-pct-cloud-init-opts.pl \
gen-qm.conf.5-opts.pl \
gen-cpu-models.conf.5-opts.pl \
gen-qm-cloud-init-opts.pl \
diff --git a/pct-cloud-init.adoc b/pct-cloud-init.adoc
new file mode 100644
index 000..1398e7b
--- /dev/null
+++ b/pct-cloud-init.adoc
@@ -0,0 +1,114 @@
+[[pct_cloud_init]]
+Cloud-Init Support
+--
+ifdef::wiki[]
+:pve-toplevel:
+endif::wiki[]
+
+{pve} supports the Cloud-init 'nocloud' format for LXC.
+
+{pve} writes the Cloud-init configuration directly into the container.
+When the 'cienable' option is set to true, the configuration is updated
+directly before before every boot. Each configuration is identified by
+an 'instance id', which Cloud-Init uses to decide whether it should run
+again or not.
+
+Preparing Cloud-Init Templates
+~~
+
+The first step is to prepare your container. Any template will suffice.
+Simply install the Cloud-Init packages inside the CT that you want to
+prepare. On Debian/Ubuntu based systems this is as simple as:
+
+
+apt install cloud-init
+
+
+WARNING: This command is *not* intended to be executed on the {pve} host, but
+only inside the container.
+
+A simple preparation for a cloud-init capable container could look like this:
+
+
+# download an image
+pveam download local ubuntu-22.10-standard_22.10-1_amd64.tar.zst
+
+# create a new container
+pct create 9000 local:vztmpl/ubuntu-22.10-standard_22.10-1_amd64.tar.zst \
+--storage local-lvm --memory 512 \
+--net0 name=eth0,bridge=vmbr0,ip=dhcp,type=veth
+
+
+Now, the package can be installed inside the container:
+
+
+# install the needed packages
+pct start 9000
+pct exec 9000 apt update
+pct exec 9000 apt install cloud-init
+pct stop 9000
+
+
+Finally, it can be helpful to turn the container into a template, to be able
+to quickly create clones whenever needed.
+
+
+pct template 9000
+
+
+Deploying Cloud-Init Templates
+~~
+
+A template can easily be deployed by cloning:
+
+
+pct clone 9000 3000 --hostname ubuntu
+
+
+Cloud-Init can now be enabled, and will run automatically on the next boot.
+
+
+pct set 3000 --cienable=1
+
+
+Custom Cloud-Init Configuration
+~~~
+
+The Cloud-Init integration also allows custom config files to be used instead
+of the automatically generated configs. This is done via the `cicustom`
+option on the command line:
+
+
+pct set 9000 --cicustom "user=,meta="
+
+
+The custom config files have to be on a storage that supports snippets and have
+to be available on all nodes the container is going to be migrated to.
Otherwise the
+container won't be able to start.
+For example:
+
+
+qm set 9000 --cicustom "user=local:snippets/userconfig.yaml"
+
+
+In contrast to the options for VMs, containers only support custom 'user'
+and 'vendor' scripts, but not 'network'. Network configuration is done through
+the already existing facilities integrated into {pve}. They can all be
specified
+together or mixed and matched however needed.
+The automatically generated config will be used for any section that doesn't
have a
+custom config file specified.
+
+The generated config can be dumped to serve as a base for custom configs:
+
+
+pct cloudinit dump 9000 user
+
+
+The same command exists for `meta`.
+
+
+Cloud-Init specific Options
+~~~
+
+include::pct-cloud-init-opts.adoc[]
+
diff --git a/pct.adoc b/pct.adoc
index 529b72f..de80347 100644
--- a/pct.adoc
+++ b/pct.adoc
@@ -622,6 +622,10 @@ It will be called during various phases of the guests
lifetime. For an example
and documentation see the example script under
`/usr/share/pve-docs/examples/guest-example-hookscript.pl`.
+ifndef::wiki[]
+include::pct-cloud-init.adoc[]
+endif::wiki[]
+
Security Considerations
---
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH 1/8 container] cloudinit: introduce config parameters
From: Leo Nunner
Introduce configuration parameters for cloud-init. Like with VMs, it's
possible to specify:
- user
- password
- ssh keys
- enable/disable updates on first boot
It's also possible to pass through custom config files for the user and
vendor settings. We don't allow configuring the network through
cloud-init, since it will clash with whatever configuration we already
did for the container.
Signed-off-by: Leo Nunner
---
src/PVE/API2/LXC.pm| 3 ++
src/PVE/API2/LXC/Config.pm | 7 -
src/PVE/LXC/Config.pm | 61 ++
3 files changed, 70 insertions(+), 1 deletion(-)
diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 7cb5122..d6e647d 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -2515,6 +2515,9 @@ __PACKAGE__->register_method({
my $pending_delete_hash =
PVE::LXC::Config->parse_pending_delete($conf->{pending}->{delete});
+ $conf->{cipassword} = '**' if defined($conf->{cipassword});
+ $conf->{pending}->{cipassword} = '** ' if
defined($conf->{pending}->{cipassword});
+
return PVE::GuestHelpers::config_with_pending_array($conf,
$pending_delete_hash);
}});
diff --git a/src/PVE/API2/LXC/Config.pm b/src/PVE/API2/LXC/Config.pm
index 5cbc014..55e9730 100644
--- a/src/PVE/API2/LXC/Config.pm
+++ b/src/PVE/API2/LXC/Config.pm
@@ -79,7 +79,7 @@ __PACKAGE__->register_method({
} else {
$conf = PVE::LXC::Config->load_current_config($param->{vmid},
$param->{current});
}
-
+ $conf->{cipassword} = '**' if $conf->{cipassword};
return $conf;
}});
@@ -148,6 +148,11 @@ __PACKAGE__->register_method({
$param->{cpuunits} = PVE::CGroup::clamp_cpu_shares($param->{cpuunits})
if defined($param->{cpuunits}); # clamp value depending on cgroup
version
+ if (defined(my $cipassword = $param->{cipassword})) {
+ $param->{cipassword} = PVE::Tools::encrypt_pw($cipassword)
+ if $cipassword !~ /^\$(?:[156]|2[ay])(\$.+){2}/;
+ }
+
my $code = sub {
my $conf = PVE::LXC::Config->load_config($vmid);
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 5cc37f7..e3ed93b 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -450,6 +450,63 @@ my $features_desc = {
},
};
+my $cicustom_fmt = {
+user => {
+ type => 'string',
+ optional => 1,
+ description => 'To pass a custom file containing all user data to the
container via cloud-init.',
+ format => 'pve-volume-id',
+ format_description => 'volume',
+},
+vendor => {
+ type => 'string',
+ optional => 1,
+ description => 'To pass a custom file containing all vendor data to the
container via cloud-init.',
+ format => 'pve-volume-id',
+ format_description => 'volume',
+},
+};
+PVE::JSONSchema::register_format('pve-pct-cicustom', $cicustom_fmt);
+
+my $confdesc_cloudinit = {
+cienable => {
+ optional => 1,
+ type => 'boolean',
+ description => "cloud-init: provide cloud-init configuration to
container.",
+},
+ciuser => {
+ optional => 1,
+ type => 'string',
+ description => "cloud-init: User name to change ssh keys and password
for instead of the"
+ ." image's configured default user.",
+},
+cipassword => {
+ optional => 1,
+ type => 'string',
+ description => 'cloud-init: Password to assign the user. Using this is
generally not'
+ .' recommended. Use ssh keys instead. Also note that older
cloud-init versions do not'
+ .' support hashed passwords.',
+},
+ciupgrade => {
+ optional => 1,
+ type => 'boolean',
+ description => 'cloud-init: do an automatic package update on boot.'
+},
+cicustom => {
+ optional => 1,
+ type => 'string',
+ description => 'cloud-init: Specify custom files to replace the
automatically generated'
+ .' ones at start.',
+ format => 'pve-pct-cicustom',
+},
+sshkeys => {
+ optional => 1,
+ type => 'string',
+ format => 'urlencoded',
+ description => "cloud-init: Setup public SSH keys (one key per line,
OpenSSH format).",
+},
+};
+
my $confdesc = {
lock => {
optional => 1,
@@ -622,6 +679,10 @@ my $confdesc = {
},
};
+foreach my $key (keys %$confdesc_cloudinit) {
+$confdesc->{$key} = $confdesc_cloudinit->{$key};
+}
+
my $valid_lxc_conf_keys = {
'lxc.apparmor.profile' => 1,
'lxc.apparmor.allow_incomplete' => 1,
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH 0/8 container/manager/docs] fix #4686: Cloudinit support for LXC
This series introduces basic cloudinit support for containers. All in all, it works quite similar to VMs, with the caveat that we only allow network configuration through the alrady existing systems, and not via cloud-init. Changes since original patch series from Leo Nunner [0]: * rebased onto current masters: ** pve-container: 5f9527e ** pve-manager: 20e637a2 ** pve-docs: 7da9c0c [0] https://lore.proxmox.com/pve-devel/20230602115731.121151-1-l.nun...@proxmox.com/ pve-container: Leo Nunner (4): cloudinit: introduce config parameters cloudinit: basic implementation cloudinit: add dump command to pct cloudinit: add function dumping options for docs src/PVE/API2/LXC.pm| 36 +++ src/PVE/API2/LXC/Config.pm | 7 ++- src/PVE/CLI/pct.pm | 4 ++ src/PVE/LXC.pm | 1 + src/PVE/LXC/Cloudinit.pm | 125 + src/PVE/LXC/Config.pm | 64 +++ src/PVE/LXC/Makefile | 1 + src/lxc-pve-prestart-hook | 5 ++ 8 files changed, 242 insertions(+), 1 deletion(-) create mode 100644 src/PVE/LXC/Cloudinit.pm pve-manager: Leo Nunner (2): cloudinit: rename qemu cloudinit panel cloudinit: introduce panel for LXCs www/manager6/Makefile | 1 + www/manager6/lxc/CloudInit.js | 237 + www/manager6/lxc/Config.js | 6 + www/manager6/qemu/CloudInit.js | 4 +- www/manager6/qemu/Config.js| 2 +- 5 files changed, 247 insertions(+), 3 deletions(-) create mode 100644 www/manager6/lxc/CloudInit.js pve-docs: Leo Nunner (2): pct: add script to generate cloudinit options pct: document cloudinit for LXC Makefile | 1 + gen-pct-cloud-init-opts.pl | 16 ++ pct-cloud-init.adoc| 114 + pct.adoc | 4 ++ 4 files changed, 135 insertions(+) create mode 100755 gen-pct-cloud-init-opts.pl create mode 100644 pct-cloud-init.adoc -- 2.39.5 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH 7/8 docs] pct: add script to generate cloudinit options
From: Leo Nunner …the same way as it's already being done for VMs. Signed-off-by: Leo Nunner --- gen-pct-cloud-init-opts.pl | 16 1 file changed, 16 insertions(+) create mode 100755 gen-pct-cloud-init-opts.pl diff --git a/gen-pct-cloud-init-opts.pl b/gen-pct-cloud-init-opts.pl new file mode 100755 index 000..c22c4d1 --- /dev/null +++ b/gen-pct-cloud-init-opts.pl @@ -0,0 +1,16 @@ +#!/usr/bin/perl + +use lib '.'; +use strict; +use warnings; +use PVE::JSONSchema; +use PVE::RESTHandler; +use PVE::LXC::Config; + +my $prop = PVE::LXC::Config::cloudinit_config_properties(); + +my $data = PVE::RESTHandler::dump_properties($prop, 'asciidoc', 'config'); + +$data =~ s/cloud-init: //g; + +print $data; -- 2.39.5 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH 5/8 manager] cloudinit: rename qemu cloudinit panel
From: Leo Nunner
Signed-off-by: Leo Nunner
---
www/manager6/qemu/CloudInit.js | 4 ++--
www/manager6/qemu/Config.js| 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/www/manager6/qemu/CloudInit.js b/www/manager6/qemu/CloudInit.js
index 0449d810..5c8c3859 100644
--- a/www/manager6/qemu/CloudInit.js
+++ b/www/manager6/qemu/CloudInit.js
@@ -1,6 +1,6 @@
Ext.define('PVE.qemu.CloudInit', {
extend: 'Proxmox.grid.PendingObjectGrid',
-xtype: 'pveCiPanel',
+xtype: 'pveQemuCiPanel',
onlineHelp: 'qm_cloud_init',
@@ -64,7 +64,7 @@ Ext.define('PVE.qemu.CloudInit', {
xtype: 'proxmoxButton',
disabled: true,
enableFn: function(rec) {
- let view = this.up('pveCiPanel');
+ let view = this.up('pveQemuCiPanel');
return !!view.rows[rec.data.key].editor;
},
handler: function() {
diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
index 48eb753e..41dd0415 100644
--- a/www/manager6/qemu/Config.js
+++ b/www/manager6/qemu/Config.js
@@ -286,7 +286,7 @@ Ext.define('PVE.qemu.Config', {
title: 'Cloud-Init',
itemId: 'cloudinit',
iconCls: 'fa fa-cloud',
- xtype: 'pveCiPanel',
+ xtype: 'pveQemuCiPanel',
},
{
title: gettext('Options'),
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH 4/8 container] cloudinit: add function dumping options for docs
From: Leo Nunner
Adds a cloudinit_config_properties function which dumps the config
parameters from the cloudinit config description. This is called
automatically when generating the docs.
Signed-off-by: Leo Nunner
---
src/PVE/LXC/Config.pm | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index e3ed93b..7e184ee 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1322,6 +1322,9 @@ sub check_type {
}
}
+sub cloudinit_config_properties {
+return Storable::dclone($confdesc_cloudinit);
+}
# add JSON properties for create and set function
sub json_config_properties {
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH pve-manager v3 4/6] fix #4225: ui: form: isoselector: add checkbox for 'essential' param
Add a checkbox for marking an iso file as required for booting via the
'essential' drive parameter.
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
---
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..425b1792 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',
+ fieldLabel: gettext('Essential'),
+ name: 'essential',
+ reference: 'requiredForBoot',
+ value: 1,
+ 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
[pve-devel] [PATCH qemu-server v3 0/6] bugzilla #4225 -- improve handling of unavailable ISOs
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 'essential'. 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 a new VM is created from the WebGUI, a corresponding added checkbox is checked by default, but allows for convenient unchecking during setup time, eg for media that is only needed for installation time. This patch series adds an 'Eject' button to the hardwareview in the WebGUI, which can be used as a convenience shortcut to setting file to 'none' for the cdrom drive. This series supersedes: https://lore.proxmox.com/pve-devel/20250113085608.99498-1-d.her...@proxmox.com/ Changes since v2: * rebased onto current masters (qemu-server: 0fc03fc1, pve-manager: bbba1c53) * refactored patch series structure (THX @Dano) * shortened code and streamlined code logic (qemu-server, THX @Dano) * reduced changes on testing framework (qemu-server) * reuse existing code for parsing values (pve-manager, THX @Friedrich) * properly html-encode warning messages (pve-manager, THX @Friedrich) qemu-server: Daniel Herzig (3): fix #4225: qemuserver: drive: add parameter to mark drive required fix #4225: qemuserver: introduce sub eject_nonrequired_isos fix #4225: qemuserver, test: put eject_nonrequired_isos in place PVE/QemuServer.pm | 37 ++ 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 | 36 + 13 files changed, 248 insertions(+), 1 deletion(-) 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 pve-manager: Daniel Herzig (3): fix #4225: ui: form: isoselector: add checkbox for 'essential' param fix #4225: ui: qemu: cdedit: enable 'Essential' checkbox for isos fix #4225: ui: qemu: hardware: add eject button for cdroms www/manager6/form/IsoSelector.js | 22 www/manager6/qemu/CDEdit.js | 8 ++ www/manager6/qemu/HardwareView.js | 42 +++ 3 files changed, 72 insertions(+) -- 2.39.5 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH pve-manager v3 5/6] fix #4225: ui: qemu: cdedit: enable 'Essential' checkbox for isos
Enables the 'Essential' checkbox for the IsoSelector.
If the parameter `essential` is not set in the configuration,
it will be assumed to be true, to avoid breaking changes.
The box is checked by default for the same reason.
If unchecked, `essential=0` is written to the configuration and the VM
will attempt to start, even if the file or underlying storage is not
available (by temporarily setting the value to 'none,media=cdrom').
Signed-off-by: Daniel Herzig
---
www/manager6/qemu/CDEdit.js | 8
1 file changed, 8 insertions(+)
diff --git a/www/manager6/qemu/CDEdit.js b/www/manager6/qemu/CDEdit.js
index 3cc16205..b115debc 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.essential = values.essential ? undefined : '0';
} else if (values.mediaType === 'cdrom') {
me.drive.file = 'cdrom';
} else {
@@ -44,6 +45,11 @@ Ext.define('PVE.qemu.CDInputPanel', {
} else {
values.mediaType = 'iso';
values.cdimage = drive.file;
+ if (drive.essential === '1' || drive.essential === undefined) {
+ values.essential = '1';
+ } else {
+ values.essential = '0';
+ }
}
me.drive = drive;
@@ -88,6 +94,7 @@ Ext.define('PVE.qemu.CDInputPanel', {
cdImageField.validate();
} else {
cdImageField.reset();
+ delete me.drive.essential;
}
},
},
@@ -98,6 +105,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
[pve-devel] [PATCH pve-manager v3 6/6] fix #4225: ui: qemu: hardware: add eject button for cdroms
This commit adds a button in the hardware view for quickly ejecting
unnecessary cdroms or ISO files by setting file to 'none'.
Signed-off-by: Daniel Herzig
---
www/manager6/qemu/HardwareView.js | 42 +++
1 file changed, 42 insertions(+)
diff --git a/www/manager6/qemu/HardwareView.js
b/www/manager6/qemu/HardwareView.js
index c6d193fc..d8a9e187 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -541,6 +541,44 @@ 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 confirmMessage = gettext("Are you sure you want to eject
'{0}' ?");
+ let isoFile = PVE.Parser.parsePropertyString(rec.data.value,
'volid').volid;
+ return Ext.htmlEncode(Ext.String.format(confirmMessage,
isoFile));
+ },
+ 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',
@@ -611,6 +649,7 @@ Ext.define('PVE.qemu.HardwareView', {
edit_btn.disable();
diskaction_btn.disable();
revert_btn.disable();
+ eject_btn.disable();
return;
}
const { key, value } = rec.data;
@@ -622,6 +661,7 @@ Ext.define('PVE.qemu.HardwareView', {
const isCloudInit = isCloudInitKey(value);
const isCDRom = value && !!value.toString().match(/media=cdrom/);
+ const isEjectedCDRom = value &&
!!value.toString().match(/none,media=cdrom/);
const isUnusedDisk = key.match(/^unused\d+/);
const isUsedDisk = !isUnusedDisk && row.isOnStorageBus && !isCDRom;
@@ -651,6 +691,7 @@ Ext.define('PVE.qemu.HardwareView', {
resize_menuitem.setDisabled(pending || !isUsedDisk);
revert_btn.setDisabled(!pending);
+ eject_btn.setDisabled(!cdromCap || !isCDRom || isCloudInit ||
isEjectedCDRom);
};
let editorFactory = (classPath, extraOptions) => {
@@ -754,6 +795,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
[pve-devel] [PATCH qemu-server v3 2/6] fix #4225: qemuserver: introduce sub eject_nonrequired_isos
The function `eject_nonrequired_isos` checks on whether a cdrom drive is
marked as required for booting the VM or not. If the parameter `essential` is
not
defined, it will assume `essential` 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 meant to be called from `config_to_command` while it
iterates over all volumes to allow for early storage activation and
early exit in the case of missing required files or unavailable
network storages.
This commit also adds a small helper sub `file_exists` to facilitate
mocking of file existence, which is needed for testing
`eject_nonrequired_isos` once in place.
---
PVE/QemuServer.pm | 35 +++
1 file changed, 35 insertions(+)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 808c0e1c..d151c322 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -8797,4 +8797,39 @@ sub delete_ifaces_ipams_ips {
}
}
+sub eject_nonrequired_isos {
+my ($ds, $drive, $vmid, $storecfg, $conf) = @_;
+
+# exclude cloudinit drives from processing (parameter '1')
+return if ( !drive_is_cdrom($drive, 1) ||
+ $drive->{file} eq 'none' ||
+ $drive->{file} eq 'cdrom' );
+
+$drive->{essential} = 1 if !defined($drive->{essential});
+my $iso_volid = $drive->{file};
+my $iso_path = PVE::QemuServer::Drive::get_iso_path($storecfg, $vmid,
$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 = $@;
+}
+
+return if ( !$store_err &&
+ file_exists($iso_path) );
+
+if ($drive->{essential}) {
+ die "'${ds}: ${iso_volid}': storage unavailable or file does not
exist\n";
+} else {
+ 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
+}
+
1;
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH qemu-server v3 1/6] fix #4225: qemuserver: drive: add parameter to mark drive required
This commit add the parameter `essential` to mark a drive as required
for booting the VM.
Signed-off-by: Daniel Herzig
---
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 1041c1dd..38136787 100644
--- a/PVE/QemuServer/Drive.pm
+++ b/PVE/QemuServer/Drive.pm
@@ -266,7 +266,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,
-}
+},
+essential => {
+ 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
[pve-devel] [PATCH qemu-server v3 3/6] fix #4225: qemuserver, test: put eject_nonrequired_isos in place
This commit puts `eject_nonrequired_isos` into the foreach_volume
section of `config_to_command`.
To ensure successful package-building it also does a few adaptions to
the test framework. Namely these are:
* Mock cifs-store to appear as online against a call from
PVE::Storage::activate_storage.
* Add an nfs-offline storage to allow for comparitative testing.
* Mock all files tested by `file_exists` as existing, except the path
contains the 'magic string' 'I_DO_NOT_EXIST'.
* Put `log_warn` output into actual warn context to allow catching the
warnings during testing.
* Add testcases for new behaviour triggered by `eject_nonrequired_isos`.
Signed-off-by: Daniel Herzig
---
PVE/QemuServer.pm | 2 +
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 | 36 +
12 files changed, 205 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/PVE/QemuServer.pm b/PVE/QemuServer.pm
index d151c322..b8d81a21 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3826,6 +3826,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};
diff --git a/test/cfg2cmd/ide-required-iso-missing.conf
b/test/cfg2cmd/ide-required-iso-missing.conf
new file mode 100644
index ..7f9abf87
--- /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: 'ide0: cifs-store:iso/I_DO_NOT_EXIST.iso': storage unavailable
or file does not exist
+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 ..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 ..a13831bf
--- /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: 'ide0: nfs-offline:iso/any.iso': storage unavailable or file
does not exist
+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 ..e69de29b
diff --git a/test/cfg2cmd/ide-required.conf b/test/cfg2cmd/ide-required.conf
new file mode 100644
index ..54b3f8ee
--- /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,essential=1
+ide1: cifs-store:iso/one.iso,media=cdrom,size=112M,essential=1
+ide2: cifs
Re: [pve-devel] [PATCH network 2/4] fix #5900: add helper functions
Hey Stefan,
thanks for the feedback!
Stefan Hanreich writes:
>
> If we do it this way (see top-level discussion), I think we should
> abstract this into the IpamPlugins itself, since this implementation is
> specific to the PVE Plugin, but that's just one type of IPAM plugin.
> Something like:
>
> Add a abstract method in the base Ipam plugin
> (Network/SDN/Ipams/Plugin.pm), i.e.
>
> PVE::Network::SDN::Ipams::Plugin::vnet_has_free_ip($range, $ipversions)
>
> Then implement it for every IPAM Plugin separately.
>
> Add a helper method to the VNet that selects the correct plugin based on
> the zone setting and then iterates over all its subnets to check for
> free IPs - something like:
>
> PVE::Network::SDN::Vnets::has_free_ip($range, $ipversions)
>
I like this thought a lot, sounds like a much cleaner and modular
solution.
>
>
> The current implementation only works for the PVE plugin and would
> actually break on zones using Netbox / Phpipam (if my brain compiler is
> correct).
>
>
Thanks for the hint -- I need to do some research on that! The code
assumes the ~key: value~ of ~dhcp: dnsmasq~ to be exclusive for IPAM
PVE.
```
for my $zone (@$zone_ids) {
push(@$dhcp_dnsmasq_zones, $zone)
if (defined(${zones_cfg}->{'ids'}->{$zone}->{'dhcp'}) &&
(${zones_cfg}->{'ids'}->{$zone}->{'dhcp'} eq 'dnsmasq'))
}
```
If this is not the case, it will affect (and not ignore as intended)
zones with Netbox/Phpipam indeed. That would be bad.
A check for ~ipam: pve~ could however be easily implemented in the same section.
>>
>> +sub defined_dhcp_ip_count_in_zone {
>> +my $zone_id = shift;
>
> even with 1 argument I think we prefer `my ($arg) = @_;`, but I haven't
> actually found a definitive answer in our style guide.
Thanks, not having any feelings here.
>
>> +my $vnets_in_zone = PVE::Network::SDN::Zones::get_vnets($zone_id);
>> +my $range_count_array;
>> +my $res;
>> +for my $vnet_id (keys %$vnets_in_zone) {
>> +my $subnets_in_vnet = PVE::Network::SDN::Vnets::get_subnets($vnet_id);
>> +for my $subnet (keys %$subnets_in_vnet) {
>> +my $dhcp_ranges =
>> PVE::Network::SDN::Subnets::get_dhcp_ranges(${subnets_in_vnet}->{$subnet});
>> +if (scalar @$dhcp_ranges) {
>> +for my $dhcp_range (@$dhcp_ranges) {
>
> You can just iterate over @$dhcp_ranges, get_dhcp_ranges() always
> returns an array reference. If it is empty, then there are just 0
> iterations of the loop, no need to check for existence.
>
Right, this is too timid indeed :)
>> +
>> +sub available_dhcp_ips_in_zone {
>> +my $zone_id = shift;
>> +my $available_ip_count = defined_dhcp_ip_count_in_zone($zone_id);
>> +my $used_ip_count = used_dhcp_ips_in_zone($zone_id);
>> +if (!defined($available_ip_count)) {
>> +$available_ip_count = 0;
>> +}
>> +if (!defined($used_ip_count)) {
>> +$used_ip_count = 0;
>> +}
>
> If you define $res to be 0 as suggested above, then those checks become
> unnecessary, since 0 becomes the default value.
>
Very cool, thank you, I did not see that.
>> +my $vnet_ids = [ PVE::Network::SDN::Vnets::sdn_vnets_ids($vnets_cfg) ];
>
> no need for selecting the ids, you can directly iterate over the VNets:
>
Thank you for your insights and the hints very much. I cannot claim that I'm
exactly happy about
the current implementation of this patch from an aesthetical point of
view by now! I will act on it further, if we consider the
'ask-for-permission' approach in general.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common 1/4] fix #5900: add helper function
Hi Stefan,
thanks for the feedback.
Thanks for hinting at ~PVE::LXC::Config::parse_lxc_network~. I oversaw
that and did not want to pull ~PVE::QemuServer::parse_net~ into LXC's
deps.
Stefan Hanreich writes:
> On 12/5/24 17:33, Daniel Herzig wrote:
>> This patch adds a small helper function to retrieve the bridge name
>> from the netN parameter string of a container or VM configuration.
>>
>> Signed-off-by: Daniel Herzig
>> ---
>> src/PVE/GuestHelpers.pm | 11 +++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
>> index 592b4a8..c6006ba 100644
>> --- a/src/PVE/GuestHelpers.pm
>> +++ b/src/PVE/GuestHelpers.pm
>> @@ -450,4 +450,15 @@ sub abort_guest_tasks {
>> return $aborted_tasks;
>> }
>>
>> +sub get_bridge {
>> +my $net_params = shift;
>> +my $param_array = [ split(/,/, $net_params) ];
>> +my $bridge;
>> +for my $net_param (@$param_array) {
>> +$bridge = $net_param if ($net_param =~ /bridge\=/);
>> +$bridge =~ s|bridge\=|| if (defined($bridge));
>> +}
>> +return $bridge;
>> +}
>> +
>> 1;
>
> net is a property string, if you want to parse it there are helpers for
> that in PVE::JSONSchema.
>
> For VMs as well as CTs we already have helpers for parsing the network
> property string defined:
>
> PVE::QemuServer::parse_net
> PVE::LXC::Config::parse_lxc_network
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH storage v2] dirplugin: fix #3986: check for trailing slashes
Currently, setting up a directory storage with trailing slashes in
the path results in log messages with double slashes, if this path
gets expanded by an action like vzdump.
This patch removes those trailing slashes once the directory storage
class config gets updated.
Signed-off-by: Daniel Herzig
---
changes since v1:
* rebased onto master
* added a comment in the code
* merged Stoikos on- and Miras offlist input into a new regex
v1:
https://lore.proxmox.com/pve-devel/20241113163249.76434-1-d.her...@proxmox.com/
Notes:
Functionality can be tested by adding new directory storage with trailing
slashes either via the GUI or `pvesm add dir $STORAGENAME --path /some/path/`.
`/new/path/` will show up as `/new/path` in `/etc/pve/storage.cfg`, and trailing
slashes from earlier defined directory storage paths will be removed.
src/PVE/Storage/DirPlugin.pm | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
index fb23e0a..c72c321 100644
--- a/src/PVE/Storage/DirPlugin.pm
+++ b/src/PVE/Storage/DirPlugin.pm
@@ -245,6 +245,8 @@ sub check_config {
if ($opts->{path} !~ m|^/[-/a-zA-Z0-9_.@]+$|) {
die "illegal path for directory storage: $opts->{path}\n";
}
+# remove trailing slashes from path
+$opts->{path} =~ s|(.*[^/])/+$|$1|;
return $opts;
}
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage v2] dirplugin: fix #3986: check for trailing slashes
Thomas Lamprecht writes:
> Am 22.11.24 um 14:13 schrieb Daniel Herzig:
>> diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
>> index fb23e0a..c72c321 100644
>> --- a/src/PVE/Storage/DirPlugin.pm
>> +++ b/src/PVE/Storage/DirPlugin.pm
>> @@ -245,6 +245,8 @@ sub check_config {
>> if ($opts->{path} !~ m|^/[-/a-zA-Z0-9_.@]+$|) {
>> die "illegal path for directory storage: $opts->{path}\n";
>> }
>> +# remove trailing slashes from path
>> +$opts->{path} =~ s|(.*[^/])/+$|$1|;
>
> If we want to use the always available File::Spec module, like Max wanted to
> in a recent series, this could be changed to
>
> $opts->{path} = File::Spec->canonpath($opts->{path});
>
Thanks! Well, this would look the cleanest to me, I did not have this module on
my
radar yet.
Will use it in v3!
>> return $opts;
>> }
>>
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH storage v3] dirplugin: fix #3986: check for trailing slashes
Currently, setting up a directory storage with trailing slashes in
the path results in log messages with double slashes, if this path
gets expanded by an action like vzdump.
This patch removes those trailing slashes once the directory storage
class config gets updated.
Signed-off-by: Daniel Herzig
---
changes since v2:
* make use of File::Spec->canonpath() for better readability
v2:
https://lore.proxmox.com/pve-devel/20241122131305.59062-1-d.her...@proxmox.com/
Thanks for the hint, Thomas!
Notes:
Functionality can be tested by adding new directory storage with trailing
slashes either via the GUI or `pvesm add dir $STORAGENAME --path /some/path/`.
`/new/path/` will show up as `/new/path` in `/etc/pve/storage.cfg`, and trailing
slashes from earlier defined directory storage paths will be removed.
src/PVE/Storage/DirPlugin.pm | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
index fb23e0a..15b67ee 100644
--- a/src/PVE/Storage/DirPlugin.pm
+++ b/src/PVE/Storage/DirPlugin.pm
@@ -6,6 +6,7 @@ use warnings;
use Cwd;
use Encode qw(decode encode);
use File::Path;
+use File::Spec;
use IO::File;
use POSIX;
@@ -245,6 +246,8 @@ sub check_config {
if ($opts->{path} !~ m|^/[-/a-zA-Z0-9_.@]+$|) {
die "illegal path for directory storage: $opts->{path}\n";
}
+# remove trailing slashes from path
+$opts->{path} = File::Spec->canonpath($opts->{path});
return $opts;
}
--
2.39.5
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH container/manager v4 0/3] Show container ip in summary and network tab
I just tested this series against an alpine container with sdn simple zone dhcp
IP assignment and it seems to do what it promises.
This makes a very nice streamlined experience with VM behaviour.
Tested-by: Daniel Herzig
Gabriel Goller writes:
> Show the ip/hwaddress of the network interfaces of containers in the summary
> tab of the container and in the network tab on a per-interface basis.
>
> This series was originally by Leo Nunner:
> https://lore.proxmox.com/pve-devel/20230615094333.66179-1-l.nun...@proxmox.com/
>
> v4, thanks @Daniel:
> - remove duplicate code (copy paste error)
> - rebase on latest master
>
> v3, thanks @Thomas, @Maximiliano:
> - fixed wording in schema description
> - stay backwards-compatible by keeping old attributes
> - use array reference instead of array
>
> v2, thanks @Dominik:
> - show if ip is static or dynamic (dhcp)
> - show multiple ips per interface
> - refactor/reuse AgentIPView instead of adding ContainerIPView component
> - various other small improvements
>
> manager:
>
> Gabriel Goller (2):
> lxc: show dynamically assigned IPs in network tab
> guest: refactor and reuse AgentIPView for containers
>
> www/manager6/Makefile | 2 +-
> www/manager6/lxc/Network.js | 85 +++
> www/manager6/panel/GuestStatusView.js | 140 +-
> www/manager6/panel/GuestSummary.js| 2 +-
> .../{qemu/AgentIPView.js => panel/IPView.js} | 79 ++
> 5 files changed, 211 insertions(+), 97 deletions(-)
> rename www/manager6/{qemu/AgentIPView.js => panel/IPView.js} (58%)
>
>
> container:
>
> Gabriel Goller (1):
> api: return all addresses of an interface
>
> src/PVE/API2/LXC.pm | 37 +++--
> src/PVE/LXC.pm | 15 +++
> 2 files changed, 50 insertions(+), 2 deletions(-)
>
>
> Summary over all repositories:
> 7 files changed, 261 insertions(+), 99 deletions(-)
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms
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 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
>> ---
>> 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
[pve-devel] [PATCH v2 9/12] test: cfg2cmd: add tests for testing the iso required parameter
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 --- 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 ..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 ..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 ..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 ..e69de29b diff --git a/test/cfg2cmd/ide-required.conf b/test/cfg2cmd/ide-required.conf new file mode 100644 index ..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 ..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 \
[pve-devel] [PATCH v2 4/12] test: chomp all trailing newlines from errors and warnings
Ease EXPECT_ERROR and EXPECT_WARN string matching with errors/warnings
that have more than one trailing newline.
Signed-off-by: Daniel Herzig
---
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
[pve-devel] [PATCH v2 8/12] test: mock log_warn warnings
Strip log_warn wrapper for catching warnings on testruns.
Signed-off-by: Daniel Herzig
---
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
[pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms
Eject by setting file to none.
Signed-off-by: Daniel Herzig
---
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
[pve-devel] [PATCH v2 1/12] fix #4225: qemuserver: drive: add optional required parameter
Add parameter to allow for marking a drive as required.
Signed-off-by: Daniel Herzig
---
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
[pve-devel] [PATCH v2 2/12] qemuserver: add helper function for mocking files
This stub function can be used for mocking a file's existance in testruns.
Signed-off-by: Daniel Herzig
---
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
[pve-devel] [PATCH v2 11/12] fix #4225: ui: qemu: cdedit: enable required checkbox for isos
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
---
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
[pve-devel] [PATCH v2 7/12] test: mock existing files
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
---
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
[pve-devel] [PATCH v2 5/12] test: mock cifs-store
Let cifs-store appear as online to a call from
PVE::Storage::activate_storage.
Signed-off-by: Daniel Herzig
---
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
[pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs
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.her...@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
[pve-devel] [PATCH v2 6/12] test: add nfs-offline storage
Add an nfs-offline storage to allow for comparatative testing
against potentially mocked online storages.
Signed-off-by: Daniel Herzig
---
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
[pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles
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
---
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
[pve-devel] [PATCH v2 10/12] fix #4225: ui: form: isoselector: add optional required checkbox
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
---
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
Re: [pve-devel] [PATCH qemu-server] api: vmlist: add plugged cpu count to vmlist table
I just tested this patch and think it gives very valuable information.
One thing I noticed, more by chance than intentional -- if the host has
a lower cpu-count than the count configured for the VM, the stats will
not be updated. E.g. my pve-host only has 2 cores available and if I
configure 4 cores for the VM, `qm list` will still show 2 cores,
although the VM is actually configured in an unbootable state.
Do you think it's possible to render something like '$NUMBER:
too_many_for_this_host' (or something similar) in this edge case?
Daniel Kral writes:
> Includes the maximum amount of cpu cores in the vmlist as it
> semantically fits in with the other properties. This allows for a more
> comfortable view of a node's VM configured CPU core counts in general
> and by users of "pvereport" without calculating each core count manually
> with the respective VM's configuration files.
>
> Suggested-by: Hannes Dürr
> Signed-off-by: Daniel Kral
> ---
> PVE/CLI/qm.pm | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 4214a7ca..389a97d6 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -1124,12 +1124,13 @@ our $cmddef = {
> my $vmlist = shift;
> exit 0 if (!scalar(@$vmlist));
>
> - printf "%10s %-20s %-10s %-10s %12s %-10s\n",
> - qw(VMID NAME STATUS MEM(MB) BOOTDISK(GB) PID);
> + printf "%10s %-20s %-10s %-5s %10s %12s %-10s\n",
> + qw(VMID NAME STATUS CORES MEM(MB) BOOTDISK(GB) PID);
>
> foreach my $rec (sort { $a->{vmid} <=> $b->{vmid} } @$vmlist) {
> - printf "%10s %-20s %-10s %-10s %12.2f %-10s\n", $rec->{vmid},
> $rec->{name},
> + printf "%10s %-20s %-10s %-5s %10s %12.2f %-10s\n", $rec->{vmid},
> $rec->{name},
> $rec->{qmpstatus} || $rec->{status},
> + $rec->{cpus} || 0,
> ($rec->{maxmem} || 0)/(1024*1024),
> ($rec->{maxdisk} || 0)/(1024*1024*1024),
> $rec->{pid} || 0;
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles
Daniel Kral 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
>> ---
>> 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 unavail
Re: [pve-devel] [PATCH v2 11/12] fix #4225: ui: qemu: cdedit: enable required checkbox for isos
Daniel Kral 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
Re: [pve-devel] [PATCH v2 12/12] ui: qemu: hardware: add eject button for cdroms
Daniel Kral 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
Re: [pve-devel] [PATCH v2 qemu-server, manager 00/12] bugzilla #4225 -- improve handling of unavailable ISOs
Thanks for this review, this helps a lot on setting up an optimized v3.
Daniel Kral 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.her...@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
Re: [pve-devel] [PATCH v2 2/12] qemuserver: add helper function for mocking files
Daniel Kral 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
>> ---
>> 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
Re: [pve-devel] [PATCH 1/8 container] cloudinit: introduce config parameters
Fiona Ebner writes: > Am 10.02.25 um 13:07 schrieb Daniel Herzig: >> From: Leo Nunner >> >> Introduce configuration parameters for cloud-init. Like with VMs, it's >> possible to specify: >> - user >> - password >> - ssh keys >> - enable/disable updates on first boot >> >> It's also possible to pass through custom config files for the user and >> vendor settings. We don't allow configuring the network through >> cloud-init, since it will clash with whatever configuration we already >> did for the container. > > Since I didn't mention it yet, when you do changes, you should keep a > brief changelog of what changed between Leo's version and your version > in the git trailers, e.g.: > > Signed-off-by: Leo Nunner > [DH: add foo > refactor bar > fix regex for baz] > Signed-off-by: Daniel Herzig Thanks for the review and the input Fiona, I'll add this once I actually start changing stuff apart from the initial rebase! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH 2/8 container] cloudinit: basic implementation
Mira Limbeck writes: > On 2/13/25 12:01, Fiona Ebner wrote: >> Am 10.02.25 um 13:07 schrieb Daniel Herzig: >>> From: Leo Nunner >>> >> >> @Mira do you know more by chance? > I don't think vendor-data should be part of the instance-id. It's used > to create a first configuration that a user can override via the user > config. > The vendor-data won't be used again once it's already configured. > I'm not a 100% sure, but changing the instance-id leads to rerunning > lots of modules (e.g. User, Network and others), but the vendor-data > parts do not. > > Only a complete `cloud-init clean` should trigger the modules using > vendor-data to run again. > > > https://cloudinit.readthedocs.io/en/latest/explanation/vendordata.html#vendor-data > Thanks for the input Mira! ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
