[pve-devel] [PATCH container 2/6] clear reboot request in vm_start
Signed-off-by: Oguz Bektas --- src/PVE/LXC.pm | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm index c77ee01..091d34a 100644 --- a/src/PVE/LXC.pm +++ b/src/PVE/LXC.pm @@ -1932,6 +1932,9 @@ sub userns_command { sub vm_start { my ($vmid, $conf, $skiplock) = @_; +eval { clear_reboot_request($vmid) }; +warn $@ if $@; + # apply pending changes while starting if (scalar(keys %{$conf->{pending}})) { my $storecfg = PVE::Storage::config(); -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH widget-toolkit 5/6] add reboot for containers into task description table
Signed-off-by: Oguz Bektas --- Utils.js | 1 + 1 file changed, 1 insertion(+) diff --git a/Utils.js b/Utils.js index e3dcfcd..3a8fa9a 100644 --- a/Utils.js +++ b/Utils.js @@ -494,6 +494,7 @@ Ext.define('Proxmox.Utils', { utilities: { vzmount: ['CT', gettext('Mount') ], vzumount: ['CT', gettext('Unmount') ], vzshutdown: ['CT', gettext('Shutdown') ], + vzreboot: ['CT', gettext('Reboot') ], vzsuspend: [ 'CT', gettext('Suspend') ], vzresume: [ 'CT', gettext('Resume') ], hamigrate: [ 'HA', gettext('Migrate') ], -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH container 4/6] pct: add 'pct reboot'
Signed-off-by: Oguz Bektas --- src/PVE/CLI/pct.pm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm index 3a32de4..98e2c6e 100755 --- a/src/PVE/CLI/pct.pm +++ b/src/PVE/CLI/pct.pm @@ -836,7 +836,8 @@ our $cmddef = { resume => [ 'PVE::API2::LXC::Status', 'vm_resume', ['vmid'], { node => $nodename }, $upid_exit], shutdown => [ 'PVE::API2::LXC::Status', 'vm_shutdown', ['vmid'], { node => $nodename }, $upid_exit], stop => [ 'PVE::API2::LXC::Status', 'vm_stop', ['vmid'], { node => $nodename }, $upid_exit], - +reboot => [ 'PVE::API2::LXC::Status', 'vm_reboot', ['vmid'], { node => $nodename }, $upid_exit], + clone => [ "PVE::API2::LXC", 'clone_vm', ['vmid', 'newid'], { node => $nodename }, $upid_exit ], migrate => [ "PVE::API2::LXC", 'migrate_vm', ['vmid', 'target'], { node => $nodename }, $upid_exit], move_volume => [ "PVE::API2::LXC", 'move_volume', ['vmid', 'volume', 'storage'], { node => $nodename }, $upid_exit ], -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH 0/6] implement CT reboot
this patch series implements the ability to reboot containers (main reason being to apply pending changes). most of the code is same/very similar with the qemu counterpart. i made the choice of using separate reboot triggers instead of the ones already used in prestart etc. hooks. the separate triggers in /run/lxc/$vmid.reboot make it possible to differentiate between an API reboot request and a reboot request from within the container. (also wanted to make it as similar to qemu as possible). since the helper code around reboot triggers is pretty much the same, we could move these to guest-common in the future. pve-container: Oguz Bektas (4): add reboot helpers to be used by containers clear reboot request in vm_start api: add reboot api call pct: add 'pct reboot' src/PVE/API2/LXC/Status.pm | 52 ++ src/PVE/CLI/pct.pm | 3 ++- src/PVE/LXC.pm | 44 3 files changed, 98 insertions(+), 1 deletion(-) proxmox-widget-toolkit: Oguz Bektas (1): add reboot for containers into task description table Utils.js | 1 + 1 file changed, 1 insertion(+) pve-manager: Oguz Bektas (1): add reboot button for containers www/manager6/lxc/Config.js | 35 +-- 1 file changed, 21 insertions(+), 14 deletions(-) -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 6/6] add reboot button for containers
also use the opportunity to refactor the shutdown button code into the menu. Signed-off-by: Oguz Bektas --- www/manager6/lxc/Config.js | 35 +-- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js index 0f81c1da..5c760f13 100644 --- a/www/manager6/lxc/Config.js +++ b/www/manager6/lxc/Config.js @@ -52,18 +52,6 @@ Ext.define('PVE.lxc.Config', { iconCls: 'fa fa-play' }); - var stopBtn = Ext.create('Ext.menu.Item',{ - text: gettext('Stop'), - disabled: !caps.vms['VM.PowerMgmt'], - confirmMsg: Proxmox.Utils.format_task_description('vzstop', vmid), - tooltip: Ext.String.format(gettext('Stop {0} immediately'), 'CT'), - dangerous: true, - handler: function() { - vm_command("stop"); - }, - iconCls: 'fa fa-stop' - }); - var shutdownBtn = Ext.create('PVE.button.Split', { text: gettext('Shutdown'), disabled: !caps.vms['VM.PowerMgmt'] || !running, @@ -73,7 +61,27 @@ Ext.define('PVE.lxc.Config', { vm_command('shutdown'); }, menu: { - items:[stopBtn] + items: [{ + text: gettext('Stop'), + disabled: !caps.vms['VM.PowerMgmt'], + confirmMsg: Proxmox.Utils.format_task_description('vzstop', vmid), + tooltip: Ext.String.format(gettext('Stop {0} immediately'), 'CT'), + dangerous: true, + handler: function() { + vm_command("stop"); + }, + iconCls: 'fa fa-stop' + + },{ + text: gettext('Reboot'), + disabled: !caps.vms['VM.PowerMgmt'], + confirmMsg: Proxmox.Utils.format_task_description('vzreboot', vmid), + tooltip: Ext.String.format(gettext('Reboot {0}'), 'CT'), + handler: function() { + vm_command("reboot"); + }, + iconCls: 'fa fa-refresh' + }] }, iconCls: 'fa fa-power-off' }); @@ -344,7 +352,6 @@ Ext.define('PVE.lxc.Config', { startBtn.setDisabled(!caps.vms['VM.PowerMgmt'] || status === 'running' || template); shutdownBtn.setDisabled(!caps.vms['VM.PowerMgmt'] || status !== 'running'); - stopBtn.setDisabled(!caps.vms['VM.PowerMgmt'] || status === 'stopped'); me.down('#removeBtn').setDisabled(!caps.vms['VM.Allocate'] || status !== 'stopped'); consoleBtn.setDisabled(template); }); -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH container 3/6] api: add reboot api call
pretty much the same code with the qemu counterpart, minus the qmp stuff. Signed-off-by: Oguz Bektas --- src/PVE/API2/LXC/Status.pm | 52 ++ 1 file changed, 52 insertions(+) diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm index 1b7a71d..406f1b0 100644 --- a/src/PVE/API2/LXC/Status.pm +++ b/src/PVE/API2/LXC/Status.pm @@ -65,6 +65,7 @@ __PACKAGE__->register_method({ { subdir => 'start' }, { subdir => 'stop' }, { subdir => 'shutdown' }, + { subdir => 'reboot' }, { subdir => 'migrate' }, ]; @@ -445,4 +446,55 @@ __PACKAGE__->register_method({ return $upid; }}); +__PACKAGE__->register_method({ +name => 'vm_reboot', +path => 'reboot', +method => 'POST', +protected => 1, +proxyto => 'node', +description => "Reboot the container by shutting it down, and starting it again. Applies pending changes.", +permissions => { + check => ['perm', '/vms/{vmid}', [ 'VM.PowerMgmt' ]], +}, +parameters => { + additionalProperties => 0, + properties => { + node => get_standard_option('pve-node'), + vmid => get_standard_option('pve-vmid', + { completion => \&PVE::LXC::complete_ctid_running }), + timeout => { + description => "Wait maximal timeout seconds for the shutdown.", + type => 'integer', + minimum => 0, + optional => 1, + }, + }, +}, +returns => { + type => 'string', +}, +code => sub { + my ($param) = @_; + + my $rpcenv = PVE::RPCEnvironment::get(); + my $authuser = $rpcenv->get_user(); + + my $node = extract_param($param, 'node'); + my $vmid = extract_param($param, 'vmid'); + + die "VM $vmid not running\n" if !PVE::LXC::check_running($vmid); + + my $realcmd = sub { + my $upid = shift; + + syslog('info', "requesting reboot of CT $vmid: $upid\n"); + PVE::LXC::vm_reboot($vmid, $param->{timeout}); + return; + }; + + return $rpcenv->fork_worker('vzreboot', $vmid, $authuser, $realcmd); +}}); + + + 1; -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH container 1/6] add reboot helpers to be used by containers
code for create_reboot_request and clear_reboot_request is from qemu, the only difference is that we use /run/lxc/$vmid.reboot path instead of /run/qemu-server. there _is_ actually reboot triggers for lxc which are used by the prestart hook and similar, however i think it's better if we can differentiate between reboot requests from inside the container and from API. (inadvertently this also allows to reboot container from inside without applying pending changes) since we don't have to clean up like in qemu, we can simply run vm_stop and vm_start for a reboot operation. Signed-off-by: Oguz Bektas --- src/PVE/LXC.pm | 41 + 1 file changed, 41 insertions(+) diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm index cdf6d64..c77ee01 100644 --- a/src/PVE/LXC.pm +++ b/src/PVE/LXC.pm @@ -2013,6 +2013,47 @@ sub vm_stop { die "container did not stop\n"; } +sub create_reboot_request { +my ($vmid) = @_; +open(my $fh, '>', "/run/lxc/$vmid.reboot") + or die "failed to create reboot trigger file: $!\n"; +close($fh); +} + +sub clear_reboot_request { +my ($vmid) = @_; +my $path = "/run/lxc/$vmid.reboot"; +my $res = 0; + +if (-e $path) { + $res = unlink($path); + die "could not remove reboot request for $vmid: $!\n" if !$res; +} + +return $res; +} + +sub vm_reboot { +my ($vmid, $timeout, $skiplock) = @_; + +PVE::LXC::Config->lock_config($vmid, sub { + eval { + return if !check_running($vmid); + + create_reboot_request($vmid); + vm_stop($vmid, 0, $timeout, 1); # kill if timeout exceeds + + my $conf = PVE::LXC::Config->load_config($vmid); + vm_start($vmid, $conf); + }; + if (my $err = $@) { + # avoid that the next normal shutdown will be confused for a reboot + clear_reboot_request($vmid); + die $err; + } +}); +} + sub run_unshared { my ($code) = @_; -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH kernel] disable alsa snd pcspkr module
The PC speaker (beeper) can only be managed by one module, and there are two which could do so. The very basic INPUT_PCSPKR, and the more advanced SND_PCSP which allows it to be used as primitive ALSA soundcard, which for Proxmox Server projects, and all modern workstations is not much of use. As they both were aliased to the "pcspkr" module name, and used the same internal driver name (being a replacment of the other), one would get the following error message when both are loaded: "Error: Driver 'pcspkr' is already registered, aborting..." in the kernel log. This happens as by default both are tried to get loaded. We do not want the more complex ALSA one, so disable that. Signed-off-by: Thomas Lamprecht --- debian/rules | 1 + 1 file changed, 1 insertion(+) diff --git a/debian/rules b/debian/rules index 24f027fe1f50..d344d4c1cc42 100755 --- a/debian/rules +++ b/debian/rules @@ -27,6 +27,7 @@ PVE_CONFIG_OPTS= \ -m CONFIG_CEPH_FS \ -m CONFIG_BLK_DEV_NBD \ -m CONFIG_BLK_DEV_RBD \ +-d CONFIG_SND_PCSP \ -m CONFIG_BCACHE \ -m CONFIG_JFS_FS \ -m CONFIG_HFS_FS \ -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH ct/common] mount point hotplugging & new mount api
On Tue, Nov 12, 2019 at 03:09:27PM +0100, Oguz Bektas wrote: > hi, > > built the latest git version of pve-common and pve-container with > wolfgang's patches. > > with running kernel: 5.0.21-4-pve > and the latest pve-kernel-5.3 forgot to mention that it worked as expected with the newer kernel > > found a small issue while testing. > > when one has an older kernel and tries to hotplug a mountpoint > > --- > Parameter verification failed. (400) > > mp1: unable to hotplug mp1: Can't use an undefined value as a symbol > reference at /usr/share/perl5/PVE/LXC.pm line 1670. > --- > > and around this line is: > > --- > > PVE::Tools::move_mount( > fileno($mount_fd), > "", > &AT_FDCWD, > $mp->{mp}, > &MOVE_MOUNT_F_EMPTY_PATH, # line 1670 > ); > }); > } > > --- > > so i'm guessing since that syscall doesn't exist in older kernels, > we get an undefined value. > > it would be better to handle this error with something more > user-friendly and verbose. > > i'm still testing and will write here if i notice something else. > > ___ > pve-devel mailing list > pve-devel@pve.proxmox.com > https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH ct/common] mount point hotplugging & new mount api
hi, built the latest git version of pve-common and pve-container with wolfgang's patches. with running kernel: 5.0.21-4-pve and the latest pve-kernel-5.3 found a small issue while testing. when one has an older kernel and tries to hotplug a mountpoint --- Parameter verification failed. (400) mp1: unable to hotplug mp1: Can't use an undefined value as a symbol reference at /usr/share/perl5/PVE/LXC.pm line 1670. --- and around this line is: --- PVE::Tools::move_mount( fileno($mount_fd), "", &AT_FDCWD, $mp->{mp}, &MOVE_MOUNT_F_EMPTY_PATH, # line 1670 ); }); } --- so i'm guessing since that syscall doesn't exist in older kernels, we get an undefined value. it would be better to handle this error with something more user-friendly and verbose. i'm still testing and will write here if i notice something else. ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH common 1/1] SysFSTools: do not assume pci domain 0000
On 11/12/19 2:55 PM, Thomas Lamprecht wrote: On 11/12/19 2:23 PM, Dominik Csapak wrote: but prepend '' to ids where no domain is given, to keep the ability to use the shorthand syntax (e.g. 00:01.0 instead of :00:01.0) Signed-off-by: Dominik Csapak --- src/PVE/SysFSTools.pm | 20 +--- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm index 2da3a38..a8d9a7f 100644 --- a/src/PVE/SysFSTools.pm +++ b/src/PVE/SysFSTools.pm @@ -73,9 +73,9 @@ sub lspci { dir_glob_foreach("$pcisysfs/devices", $pciregex, sub { my ($fullid, $domain, $bus, $slot, $function) = @_; - my $id = "$bus:$slot.$function"; + my $id = "$domain:$bus:$slot.$function"; - if (defined($filter) && !ref($filter) && $id !~ m/^\Q$filter\E/) { + if (defined($filter) && !ref($filter) && $id !~ m/^(:)?\Q$filter\E/) { return; # filter ids early } @@ -279,13 +279,15 @@ sub pci_dev_group_bind_to_vfio { } die "Cannot find vfio-pci module!\n" if !-d $vfio_basedir; +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/; + # get IOMMU group devices -opendir(my $D, "$pcisysfs/devices/:$pciid/iommu_group/devices/") || die "Cannot open iommu_group: $!\n"; - my @devs = grep /^:/, readdir($D); +opendir(my $D, "$pcisysfs/devices/$pciid/iommu_group/devices/") || die "Cannot open iommu_group: $!\n"; + my @devs = grep /^[0-9a-f]{4}:/, readdir($D); closedir($D); foreach my $pciid (@devs) { - $pciid =~ m/^([:\.\da-f]+)$/ or die "PCI ID $pciid not valid!\n"; + $pciid =~ m/^([:\.0-9a-f]+)$/ or die "PCI ID $pciid not valid!\n"; # pci bridges, switches or root ports are not supported # they have a pci_bus subdirectory so skip them @@ -301,7 +303,9 @@ sub pci_dev_group_bind_to_vfio { sub pci_create_mdev_device { my ($pciid, $uuid, $type) = @_; -my $basedir = "$pcisysfs/devices/:$pciid"; +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/; + +my $basedir = "$pcisysfs/devices/$pciid"; my $mdev_dir = "$basedir/mdev_supported_types"; die "pci device '$pciid' does not support mediated devices \n" @@ -336,7 +340,9 @@ sub pci_create_mdev_device { sub pci_cleanup_mdev_device { my ($pciid, $uuid) = @_; -my $basedir = "$pcisysfs/devices/:$pciid/$uuid"; +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/; maybe above could be moved in a helper? could either return the "full" ID or a tuple like: my ($full, $domain, $bus, $slot) = ... not to sure though, and can also be OK without this for the few call places.. i thought about this too, but we only use it 3 times, and it did not seem worth it for a single line of code and the only place where we use the domain/bus/slot split is in the 'pci_device_info' sub... + +my $basedir = "$pcisysfs/devices/$pciid/$uuid"; if (! -e $basedir) { return 1; # no cleanup necessary if it does not exist ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH common 1/1] SysFSTools: do not assume pci domain 0000
On 11/12/19 2:23 PM, Dominik Csapak wrote: > but prepend '' to ids where no domain is given, to keep the ability > to use the shorthand syntax (e.g. 00:01.0 instead of :00:01.0) > > Signed-off-by: Dominik Csapak > --- > src/PVE/SysFSTools.pm | 20 +--- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm > index 2da3a38..a8d9a7f 100644 > --- a/src/PVE/SysFSTools.pm > +++ b/src/PVE/SysFSTools.pm > @@ -73,9 +73,9 @@ sub lspci { > > dir_glob_foreach("$pcisysfs/devices", $pciregex, sub { > my ($fullid, $domain, $bus, $slot, $function) = @_; > - my $id = "$bus:$slot.$function"; > + my $id = "$domain:$bus:$slot.$function"; > > - if (defined($filter) && !ref($filter) && $id !~ m/^\Q$filter\E/) { > + if (defined($filter) && !ref($filter) && $id !~ > m/^(:)?\Q$filter\E/) { > return; # filter ids early > } > > @@ -279,13 +279,15 @@ sub pci_dev_group_bind_to_vfio { > } > die "Cannot find vfio-pci module!\n" if !-d $vfio_basedir; > > +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/; > + > # get IOMMU group devices > -opendir(my $D, "$pcisysfs/devices/:$pciid/iommu_group/devices/") || > die "Cannot open iommu_group: $!\n"; > - my @devs = grep /^:/, readdir($D); > +opendir(my $D, "$pcisysfs/devices/$pciid/iommu_group/devices/") || die > "Cannot open iommu_group: $!\n"; > + my @devs = grep /^[0-9a-f]{4}:/, readdir($D); > closedir($D); > > foreach my $pciid (@devs) { > - $pciid =~ m/^([:\.\da-f]+)$/ or die "PCI ID $pciid not valid!\n"; > + $pciid =~ m/^([:\.0-9a-f]+)$/ or die "PCI ID $pciid not valid!\n"; > > # pci bridges, switches or root ports are not supported > # they have a pci_bus subdirectory so skip them > @@ -301,7 +303,9 @@ sub pci_dev_group_bind_to_vfio { > sub pci_create_mdev_device { > my ($pciid, $uuid, $type) = @_; > > -my $basedir = "$pcisysfs/devices/:$pciid"; > +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/; > + > +my $basedir = "$pcisysfs/devices/$pciid"; > my $mdev_dir = "$basedir/mdev_supported_types"; > > die "pci device '$pciid' does not support mediated devices \n" > @@ -336,7 +340,9 @@ sub pci_create_mdev_device { > sub pci_cleanup_mdev_device { > my ($pciid, $uuid) = @_; > > -my $basedir = "$pcisysfs/devices/:$pciid/$uuid"; > +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/; maybe above could be moved in a helper? could either return the "full" ID or a tuple like: my ($full, $domain, $bus, $slot) = ... not to sure though, and can also be OK without this for the few call places.. > + > +my $basedir = "$pcisysfs/devices/$pciid/$uuid"; > > if (! -e $basedir) { > return 1; # no cleanup necessary if it does not exist > ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH common/qemu-server/manager] fix #2436: do not hardcode pci domain
this series improves the pci passthrouh/selection code such that we do not assume a pci domain of '' anymore. old configs still work, and the 'default domain', , will not be written out to the config (but shown in the gui) we can ignore the changes in qemus commandline, since such vms can neither be migrated nor snapshotted dependencies: new qemu-server needs pve-common new pve-manager needs pve-common new pve-common breaks old manager and qemu-server the only issue occurs if one wants to edit a vm on a not upgraded cluster member (the selected id does then not exist in the store...) can we ignore this? if not, i would send a v2 where i try to detect the format we get from the backend and set the value in the selector accordingly (but i am not sure if its worth the effort, since partially upgrading a cluster is not recommended) pve-common: Dominik Csapak (1): SysFSTools: do not assume pci domain src/PVE/SysFSTools.pm | 20 +--- 1 file changed, 13 insertions(+), 7 deletions(-) qemu-server: Dominik Csapak (1): fix #2436: pci: do not hardcode pci domain to PVE/QemuServer.pm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) pve-manager: Dominik Csapak (1): gui: pci passthrough: consider domain in PCISelector www/manager6/form/PCISelector.js | 2 +- www/manager6/qemu/PCIEdit.js | 11 +-- 2 files changed, 10 insertions(+), 3 deletions(-) -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH common 1/1] SysFSTools: do not assume pci domain 0000
but prepend '' to ids where no domain is given, to keep the ability to use the shorthand syntax (e.g. 00:01.0 instead of :00:01.0) Signed-off-by: Dominik Csapak --- src/PVE/SysFSTools.pm | 20 +--- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm index 2da3a38..a8d9a7f 100644 --- a/src/PVE/SysFSTools.pm +++ b/src/PVE/SysFSTools.pm @@ -73,9 +73,9 @@ sub lspci { dir_glob_foreach("$pcisysfs/devices", $pciregex, sub { my ($fullid, $domain, $bus, $slot, $function) = @_; - my $id = "$bus:$slot.$function"; + my $id = "$domain:$bus:$slot.$function"; - if (defined($filter) && !ref($filter) && $id !~ m/^\Q$filter\E/) { + if (defined($filter) && !ref($filter) && $id !~ m/^(:)?\Q$filter\E/) { return; # filter ids early } @@ -279,13 +279,15 @@ sub pci_dev_group_bind_to_vfio { } die "Cannot find vfio-pci module!\n" if !-d $vfio_basedir; +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/; + # get IOMMU group devices -opendir(my $D, "$pcisysfs/devices/:$pciid/iommu_group/devices/") || die "Cannot open iommu_group: $!\n"; - my @devs = grep /^:/, readdir($D); +opendir(my $D, "$pcisysfs/devices/$pciid/iommu_group/devices/") || die "Cannot open iommu_group: $!\n"; + my @devs = grep /^[0-9a-f]{4}:/, readdir($D); closedir($D); foreach my $pciid (@devs) { - $pciid =~ m/^([:\.\da-f]+)$/ or die "PCI ID $pciid not valid!\n"; + $pciid =~ m/^([:\.0-9a-f]+)$/ or die "PCI ID $pciid not valid!\n"; # pci bridges, switches or root ports are not supported # they have a pci_bus subdirectory so skip them @@ -301,7 +303,9 @@ sub pci_dev_group_bind_to_vfio { sub pci_create_mdev_device { my ($pciid, $uuid, $type) = @_; -my $basedir = "$pcisysfs/devices/:$pciid"; +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/; + +my $basedir = "$pcisysfs/devices/$pciid"; my $mdev_dir = "$basedir/mdev_supported_types"; die "pci device '$pciid' does not support mediated devices \n" @@ -336,7 +340,9 @@ sub pci_create_mdev_device { sub pci_cleanup_mdev_device { my ($pciid, $uuid) = @_; -my $basedir = "$pcisysfs/devices/:$pciid/$uuid"; +$pciid = ":$pciid" if $pciid !~ m/^[0-9a-f]{4}:/; + +my $basedir = "$pcisysfs/devices/$pciid/$uuid"; if (! -e $basedir) { return 1; # no cleanup necessary if it does not exist -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH qemu-server 1/1] fix #2436: pci: do not hardcode pci domain to 0000
relax the regex for hostpci to allow different pci domains than Signed-off-by: Dominik Csapak --- PVE/QemuServer.pm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm index 1890448..709dcdb 100644 --- a/PVE/QemuServer.pm +++ b/PVE/QemuServer.pm @@ -1337,7 +1337,7 @@ my $usbdesc = { }; PVE::JSONSchema::register_standard_option("pve-qm-usb", $usbdesc); -my $PCIRE = qr/[a-f0-9]{2}:[a-f0-9]{2}(?:\.[a-f0-9])?/; +my $PCIRE = qr/([a-f0-9]{4}:)?[a-f0-9]{2}:[a-f0-9]{2}(?:\.[a-f0-9])?/; my $hostpci_fmt = { host => { default_key => 1, @@ -3709,7 +3709,7 @@ sub config_to_command { if ($d->{mdev} && scalar(@$pcidevices) == 1) { my $pci_id = $pcidevices->[0]->{id}; my $uuid = PVE::SysFSTools::generate_mdev_uuid($vmid, $i); - $sysfspath = "/sys/bus/pci/devices/:$pci_id/$uuid"; + $sysfspath = "/sys/bus/pci/devices/$pci_id/$uuid"; } elsif ($d->{mdev}) { warn "ignoring mediated device '$id' with multifunction device\n"; } @@ -5394,7 +5394,7 @@ sub vm_start { foreach my $pcidevice (@$pcidevices) { my $pciid = $pcidevice->{id}; - my $info = PVE::SysFSTools::pci_device_info(":$pciid"); + my $info = PVE::SysFSTools::pci_device_info("$pciid"); die "IOMMU not present\n" if !PVE::SysFSTools::check_iommu_support(); die "no pci device info for device '$pciid'\n" if !$info; -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH manager 1/1] gui: pci passthrough: consider domain in PCISelector
but remove the default domain '' before sending to the backend, and add it if no domain is given in the config Signed-off-by: Dominik Csapak --- www/manager6/form/PCISelector.js | 2 +- www/manager6/qemu/PCIEdit.js | 11 +-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/www/manager6/form/PCISelector.js b/www/manager6/form/PCISelector.js index 1061ef4d..c6847b67 100644 --- a/www/manager6/form/PCISelector.js +++ b/www/manager6/form/PCISelector.js @@ -27,7 +27,7 @@ Ext.define('PVE.form.PCISelector', { { header: 'ID', dataIndex: 'id', - width: 80 + width: 100 }, { header: gettext('IOMMU Group'), diff --git a/www/manager6/qemu/PCIEdit.js b/www/manager6/qemu/PCIEdit.js index 6ff13808..1853d241 100644 --- a/www/manager6/qemu/PCIEdit.js +++ b/www/manager6/qemu/PCIEdit.js @@ -10,7 +10,10 @@ Ext.define('PVE.qemu.PCIInputPanel', { var hostpci = me.vmconfig[me.confid] || ''; var values = PVE.Parser.parsePropertyString(hostpci, 'host'); - if (values.host && values.host.length < 6) { // 00:00 format not 00:00.0 + if (!values.host.match(/^[0-9a-f]{4}:/i)) { // add optional domain + values.host = ":" + values.host; + } + if (values.host && values.host.length < 11) { // :00:00 format not :00:00.0 values.host += ".0"; values.multifunction = true; } @@ -43,9 +46,13 @@ Ext.define('PVE.qemu.PCIInputPanel', { } } } + // remove optional '' domain + if (values.host.substring(0,5) === ':') { + values.host = values.host.substring(5); + } if (values.multifunction) { // modify host to skip the '.X' - values.host = values.host.substring(0,5); + values.host = values.host.substring(0, values.host.indexOf('.')); delete values.multifunction; } -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH common] fix PVE::Tools::df for big mounts
On 11/12/19 1:56 PM, Dominik Csapak wrote: > if the size/avail of a mount is bigger than a certain amount, > json_encode writes the number in scientific format, which did not > fit inside our \d+ regex > > this resulted in 'undef' values for the result hash and subsequently > led to errors and warnings > > extend it to also catch scientific formatted numbers > > Signed-off-by: Dominik Csapak > --- > src/PVE/Tools.pm | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > applied, changed the commit message a bit, e.g., linked the forum thread and noted that perl itself can use such format as is. Thanks! > diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm > index 02c2886..89de4ec 100644 > --- a/src/PVE/Tools.pm > +++ b/src/PVE/Tools.pm > @@ -1008,7 +1008,7 @@ sub df { > warn $@ if $@; > > # untaint the values > -my ($blocks, $used, $bavail) = map { defined($_) ? (/^(\d+)$/) : 0 } > +my ($blocks, $used, $bavail) = map { defined($_) ? (/^([\d\.e\-+]+)$/) : > 0 } # can be in scientific notation followed up with moving above comment into the untaint one, avoiding very long lines, if not really necessary. > $res->@{qw(blocks used bavail)}; > > return { > ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH pve-firewall 2/2] add synflood protection
Currently, a virtio-net + vhost-net can handle between 200-300 kpps for each vm (with 1core/queue=1). That mean than a vm can easily overloaded with a simple synflood (hping3 --flood -p 80 -S targetip). Also the conntrack of the host can be saturated easily. This patch introduce a new option, enable rate limiting of syn/s by src ip (protection_synflood:1). rate limit can be set with : protection_synflood_rate (default 200 syn/s) with an extra burst: protection_synflood_rate (default 1000). It's also possible to reduce conntrack syn timeout: nf_conntrack_tcp_timeout_syn_recv (default 60). with default values, a src ip can take around (60 * 200 = 12000 conntrack entries). The iptables rules are done in raw table, before reaching the conntrack. This protection works fine for non-spoofed src ip. For spoofed src ip, the only way could be to implement SYNPROXY, but this only works for routed/nat setup. (The host need to be able to reply with the src ip the vm) Some good information about synflood protections https://2014.rmll.info/slides/356/day_1-1400-Jesper_Brouer-DDoS_protection_using_Netfilter_iptables.pdf --- src/PVE/Firewall.pm | 58 +++-- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm index 8f4ff1a..db16e0f 100644 --- a/src/PVE/Firewall.pm +++ b/src/PVE/Firewall.pm @@ -1273,6 +1273,14 @@ our $host_option_properties = { default => 432000, minimum => 7875, }, +nf_conntrack_tcp_timeout_syn_recv => { + description => "Conntrack syn recv timeout.", + type => 'integer', + optional => 1, + default => 60, + minimum => 30, + maximum => 60, +}, ndp => { description => "Enable NDP (Neighbor Discovery Protocol).", type => 'boolean', @@ -1285,6 +1293,24 @@ our $host_option_properties = { default => 0, optional => 1, }, +protection_synflood => { + description => "Enable synflood protection", + type => 'boolean', + default => 0, + optional => 1, +}, +protection_synflood_rate => { + description => "Synflood protection rate syn/sec by ip src.", + type => 'integer', + optional => 1, + default => 200, +}, +protection_synflood_burst => { + description => "Synflood protection rate burst by ip src.", + type => 'integer', + optional => 1, + default => 1000, +}, log_nf_conntrack => { description => "Enable logging of conntrack information.", type => 'boolean', @@ -2752,13 +2778,13 @@ sub parse_hostfw_option { my $loglevels = "emerg|alert|crit|err|warning|notice|info|debug|nolog"; -if ($line =~ m/^(enable|nosmurfs|tcpflags|ndp|log_nf_conntrack|nf_conntrack_allow_invalid):\s*(0|1)\s*$/i) { +if ($line =~ m/^(enable|nosmurfs|tcpflags|ndp|log_nf_conntrack|nf_conntrack_allow_invalid|protection_synflood):\s*(0|1)\s*$/i) { $opt = lc($1); $value = int($2); } elsif ($line =~ m/^(log_level_in|log_level_out|tcp_flags_log_level|smurf_log_level):\s*(($loglevels)\s*)?$/i) { $opt = lc($1); $value = $2 ? lc($3) : ''; -} elsif ($line =~ m/^(nf_conntrack_max|nf_conntrack_tcp_timeout_established):\s*(\d+)\s*$/i) { +} elsif ($line =~ m/^(nf_conntrack_max|nf_conntrack_tcp_timeout_established|nf_conntrack_tcp_timeout_syn_recv|protection_synflood_rate|protection_synflood_burst|protection_limit):\s*(\d+)\s*$/i) { $opt = lc($1); $value = int($2); } else { @@ -3572,6 +3598,22 @@ sub compile_iptables_raw { my $ruleset = {}; +my $hostfw_options = $hostfw_conf->{options} || {}; +my $protection_synflood = $hostfw_options->{protection_synflood} || 0; + +if($protection_synflood) { + + my $protection_synflood_rate = $hostfw_options->{protection_synflood_rate} ? $hostfw_options->{protection_synflood_rate} : 200; + my $protection_synflood_burst = $hostfw_options->{protection_synflood_burst} ? $hostfw_options->{protection_synflood_burst} : 1000; + my $protection_synflood_limit = $hostfw_options->{protection_synflood_limit} ? $hostfw_options->{protection_synflood_limit} : 3000; + my $protection_synflood_expire = $hostfw_options->{nf_conntrack_tcp_timeout_syn_recv} ? $hostfw_options->{nf_conntrack_tcp_timeout_syn_recv} : 60; + $protection_synflood_expire = $protection_synflood_expire * 1000; + my $protection_synflood_mask = $ipversion == 4 ? 32 : 64; + + ruleset_create_chain($ruleset, "PVEFW-PREROUTING"); + ruleset_addrule($ruleset, "PVEFW-PREROUTING", "-p tcp -m tcp --tcp-flags FIN,SYN,RST,ACK SYN -m hashlimit --hashlimit-above $protection_synflood_rate/sec --hashlimit-burst $protection_synflood_burst --hashlimit-mode srcip --hashlimit-name syn --hashlimit-htable-size 2097152 --hashlimit-srcmask $protection_synflood_mask --hashlimit-htable-expire $protection_synfloo
[pve-devel] [PATCH pve-firewall 1/2] iptables : add raw table support
--- src/PVE/Firewall.pm | 122 +--- src/PVE/Service/pve_firewall.pm | 27 --- test/fwtester.pl| 10 +-- 3 files changed, 119 insertions(+), 40 deletions(-) diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm index 97e5384..8f4ff1a 100644 --- a/src/PVE/Firewall.pm +++ b/src/PVE/Firewall.pm @@ -1757,15 +1757,17 @@ sub enable_bridge_firewall { } sub iptables_restore_cmdlist { -my ($cmdlist) = @_; +my ($cmdlist, $table) = @_; -run_command(['iptables-restore', '-n'], input => $cmdlist, errmsg => "iptables_restore_cmdlist"); +$table = 'filter' if !$table; +run_command(['iptables-restore', '-T', $table, '-n'], input => $cmdlist, errmsg => "iptables_restore_cmdlist"); } sub ip6tables_restore_cmdlist { -my ($cmdlist) = @_; +my ($cmdlist, $table) = @_; -run_command(['ip6tables-restore', '-n'], input => $cmdlist, errmsg => "iptables_restore_cmdlist"); +$table = 'filter' if !$table; +run_command(['ip6tables-restore', '-T', $table, '-n'], input => $cmdlist, errmsg => "iptables_restore_cmdlist"); } sub ipset_restore_cmdlist { @@ -1781,9 +1783,10 @@ sub ebtables_restore_cmdlist { } sub iptables_get_chains { -my ($iptablescmd) = @_; +my ($iptablescmd, $t) = @_; $iptablescmd = "iptables" if !$iptablescmd; +$t = 'filter' if !$t; my $res = {}; @@ -1818,7 +1821,7 @@ sub iptables_get_chains { return; } - return if $table ne 'filter'; + return if $table ne $t; if ($line =~ m/^:(\S+)\s/) { my $chain = $1; @@ -1828,7 +1831,7 @@ sub iptables_get_chains { my ($chain, $sig) = ($1, $2); return if !&$is_pvefw_chain($chain); $res->{$chain} = $sig; - } elsif ($line =~ m/^-A\s+(INPUT|OUTPUT|FORWARD)\s+-j\s+PVEFW-\1$/) { + } elsif ($line =~ m/^-A\s+(INPUT|OUTPUT|FORWARD|PREROUTING)\s+-j\s+PVEFW-\1$/) { $hooks->{$1} = 1; } else { # simply ignore the rest @@ -3552,14 +3555,26 @@ sub compile { push @{$cluster_conf->{ipset}->{management}}, { cidr => $localnet }; -my $ruleset = compile_iptables_filter($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, 4); -my $rulesetv6 = compile_iptables_filter($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, 6); +my $ruleset = {}; +my $rulesetv6 = {}; +$ruleset->{filter} = compile_iptables_filter($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, 4); +$ruleset->{raw} = compile_iptables_raw($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, 4); +$rulesetv6->{filter} = compile_iptables_filter($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, 6); +$rulesetv6->{raw} = compile_iptables_raw($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, 6); my $ebtables_ruleset = compile_ebtables_filter($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata); my $ipset_ruleset = compile_ipsets($cluster_conf, $vmfw_configs, $vmdata); return ($ruleset, $ipset_ruleset, $rulesetv6, $ebtables_ruleset); } +sub compile_iptables_raw { +my ($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, $ipversion) = @_; + +my $ruleset = {}; + +return $ruleset; +} + sub compile_iptables_filter { my ($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata, $corosync_conf, $ipversion) = @_; @@ -3958,11 +3973,13 @@ sub print_sig_rule { } sub get_ruleset_cmdlist { -my ($ruleset, $iptablescmd) = @_; +my ($ruleset, $iptablescmd, $table) = @_; -my $cmdlist = "*filter\n"; # we pass this to iptables-restore; +$table = 'filter' if !$table; -my ($active_chains, $hooks) = iptables_get_chains($iptablescmd); +my $cmdlist = "*$table\n"; # we pass this to iptables-restore; + +my ($active_chains, $hooks) = iptables_get_chains($iptablescmd, $table); my $statushash = get_ruleset_status($ruleset, $active_chains, \&iptables_chain_digest); # create missing chains first @@ -3974,7 +3991,7 @@ sub get_ruleset_cmdlist { $cmdlist .= ":$chain - [0:0]\n"; } -foreach my $h (qw(INPUT OUTPUT FORWARD)) { +foreach my $h (qw(INPUT OUTPUT FORWARD PREROUTING)) { my $chain = "PVEFW-$h"; if ($ruleset->{$chain} && !$hooks->{$h}) { $cmdlist .= "-A $h -j $chain\n"; @@ -4009,10 +4026,11 @@ sub get_ruleset_cmdlist { next if $chain eq 'PVEFW-INPUT'; next if $chain eq 'PVEFW-OUTPUT'; next if $chain eq 'PVEFW-FORWARD'; + next if $chain eq 'PVEFW-PREROUTING'; $cmdlist .= "-X $chain\n"; } -my $changes = $cmdlist ne "*filter\n" ? 1 : 0; +my $changes = $cmdlist ne "*$table\n" ? 1 : 0; $cmdlist .= "COMMIT\n"; @@ -4125,9 +4143,11 @@ sub apply_ruleset { my ($ipset_create_cmdlist, $ipset_delete_cmdlist, $ipset_changes) = get_ipset_c
[pve-devel] [PATCH pve-firewall 0/2] Fix #2450: synflood protection
Currently, a virtio-net + vhost-net can handle between 200-300 kpps for each vm (with 1core/queue=1). That mean than a vm can easily overloaded with a simple synflood (hping3 --flood -p 80 -S targetip). Also the conntrack of the host can be saturated easily. This patch introduce a new option, enable rate limiting of syn/s by src ip (protection_synflood:1). rate limit can be set with : protection_synflood_rate (default 200 syn/s) with an extra burst: protection_synflood_rate (default 1000). It's also possible to reduce conntrack syn timeout: nf_conntrack_tcp_timeout_syn_recv (default 60). with default values, a src ip can take around (60 * 200 = 12000 conntrack entries). The iptables rules are done in raw table, before reaching the conntrack. This protection works fine for non-spoofed src ip. For spoofed src ip, the only way could be to implement SYNPROXY, but this only works for routed/nat setup. (The host need to be able to reply with the src ip the vm) and need https://bugzilla.proxmox.com/show_bug.cgi?id=2451 Some good information about synflood protections https://2014.rmll.info/slides/356/day_1-1400-Jesper_Brouer-DDoS_protection_using_Netfilter_iptables.pdf Alexandre Derumier (2): iptables : add raw table support add synflood protection src/PVE/Firewall.pm | 180 +++- src/PVE/Service/pve_firewall.pm | 27 +++-- test/fwtester.pl| 10 +- 3 files changed, 175 insertions(+), 42 deletions(-) -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH common] fix PVE::Tools::df for big mounts
if the size/avail of a mount is bigger than a certain amount, json_encode writes the number in scientific format, which did not fit inside our \d+ regex this resulted in 'undef' values for the result hash and subsequently led to errors and warnings extend it to also catch scientific formatted numbers Signed-off-by: Dominik Csapak --- src/PVE/Tools.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm index 02c2886..89de4ec 100644 --- a/src/PVE/Tools.pm +++ b/src/PVE/Tools.pm @@ -1008,7 +1008,7 @@ sub df { warn $@ if $@; # untaint the values -my ($blocks, $used, $bavail) = map { defined($_) ? (/^(\d+)$/) : 0 } +my ($blocks, $used, $bavail) = map { defined($_) ? (/^([\d\.e\-+]+)$/) : 0 } # can be in scientific notation $res->@{qw(blocks used bavail)}; return { -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage] fix #2085: Handle non-default mount point in path() by introducing new mountpoint property
On 11/12/19 12:18 PM, Fabian Ebner wrote: > > I don't see a clean way to do the automatic adding of the mount point > property (doing it in path() is possible, but we need to assume that our > caller doesn't hold the lock on storage.cfg). Maybe it's better to just do > the warning in activate_storage and leave it to the user to configure the > mount point properly? It feels a bit weird for the plugin code to modify the > config; is there an instance where it does? You could just to this once at bootup, or initial mount. ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH docu] Fix #2459: qm: Make info about core limit clear
On 11/12/19 10:08 AM, Dominic Jäger wrote: > 'assigning' could also mean that creating a VM with more cores than physically > available is impossible. However, this is not the case. Using 'starting' > instead is more precise and still easy to understand. > > Signed-off-by: Dominic Jäger > --- > qm.adoc | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Applied, thanks! ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH storage] fix #2085: Handle non-default mount point in path() by introducing new mountpoint property
On November 12, 2019 12:18 pm, Fabian Ebner wrote: > On 11/7/19 12:59 PM, Fabian Grünbichler wrote: >> On November 7, 2019 12:52 pm, Fabian Ebner wrote: >>> On 11/7/19 9:34 AM, Fabian Grünbichler wrote: On November 6, 2019 1:46 pm, Fabian Ebner wrote: > A new mountpoint property is added to the schema for ZFSPool storages. > When needed for the first time, the current mount point is determined and > written to the storage config. > > Signed-off-by: Fabian Ebner > --- >PVE/Storage/ZFSPoolPlugin.pm | 25 +++-- >1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm > index 16fb0d6..44c8123 100644 > --- a/PVE/Storage/ZFSPoolPlugin.pm > +++ b/PVE/Storage/ZFSPoolPlugin.pm > @@ -32,6 +32,10 @@ sub properties { > description => "use sparse volumes", > type => 'boolean', > }, > + mountpoint => { > + description => "mount point", > + type => 'string', format => 'pve-storage-path', > + }, >}; >} > > @@ -44,6 +48,7 @@ sub options { > disable => { optional => 1 }, > content => { optional => 1 }, > bwlimit => { optional => 1 }, > + mountpoint => { optional => 1 }, >}; >} > > @@ -148,11 +153,27 @@ sub path { >my ($vtype, $name, $vmid) = $class->parse_volname($volname); > >my $path = ''; > +my $mountpoint = $scfg->{mountpoint}; > + > +# automatically write mountpoint to storage config if it is not > present > +if (!$mountpoint) { > + $mountpoint = $class->zfs_request($scfg, undef, 'get', '-o', 'value', > + 'mountpoint', '-H', $scfg->{pool}); > + chomp($mountpoint); > + > + eval { > + PVE::Storage::lock_storage_config(sub { > + my $cfg = PVE::Storage::config(); > + $cfg->{ids}->{$storeid}->{mountpoint} = $mountpoint; > + PVE::Storage::write_config($cfg); > + }, "update storage config failed"); > + }; > + warn $@ if $@; > +} > >if ($vtype eq "images") { > if ($name =~ m/^subvol-/ || $name =~ m/^basevol-/) { > - # fixme: we currently assume standard mount point?! > - $path = "/$scfg->{pool}/$name"; > + $path = "$mountpoint/$name"; > } else { > $path = "/dev/zvol/$scfg->{pool}/$name"; > } it would be great if this "get mountpoint property of dataset" (or better: even "get arbitrary properties of dataset") helper could be factored out. we already have two calls to "zfs get -Hp '...' $dataset" that could re-use such a helper. then we could add a single check in activate_storage (maybe just for the branch where we actually just imported the pool?) to verify that the stored mountpoint value is correct, and if not update it (or warn about the problem). that check could have a very low timeout, and errors could be ignored since it is just a double-check. ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >>> >>> I noticed that the $scfg parameter in zfs_request is never used. Should >>> I clean that up? Otherwise I'd need to include the $scfg parameter in >>> the helper function as well, where it also isn't needed. >> >> it's used by ZFS over iSCSI, where the plugin is derived from >> ZFSPoolPlugin, so it's not safe to remove ;) >> >> ___ >> pve-devel mailing list >> pve-devel@pve.proxmox.com >> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >> > > > I see, thanks. I'll remember to check whether the method is virtual next > time. > > There are two problems with doing the mount point configuration in > activate_storage. > First, we might get called by 'create' (API for config triggered by > 'pvesm add') which holds a lock on storage.cfg at that time, but we > might also get called by something else without such a lock. > Second, 'create' will write its own version of the config after calling > activate_storage anyways. > > It also doesn't help to only do the mount point configuration inside the > branch where we just imported, since the pool/dataset might be imported > already when we do 'pvesm add' and the second problem is still there. > > Now there is plugin->on_add_hook used by 'create' which would also be a > good place to configure the mount point. We could activate the storage > there, check for the mount point and then hijack the scfg that is passed > along to it (also not nice), so that it is written by 'create' later. In > activate_storage
Re: [pve-devel] [PATCH storage] fix #2085: Handle non-default mount point in path() by introducing new mountpoint property
On 11/7/19 12:59 PM, Fabian Grünbichler wrote: On November 7, 2019 12:52 pm, Fabian Ebner wrote: On 11/7/19 9:34 AM, Fabian Grünbichler wrote: On November 6, 2019 1:46 pm, Fabian Ebner wrote: A new mountpoint property is added to the schema for ZFSPool storages. When needed for the first time, the current mount point is determined and written to the storage config. Signed-off-by: Fabian Ebner --- PVE/Storage/ZFSPoolPlugin.pm | 25 +++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm index 16fb0d6..44c8123 100644 --- a/PVE/Storage/ZFSPoolPlugin.pm +++ b/PVE/Storage/ZFSPoolPlugin.pm @@ -32,6 +32,10 @@ sub properties { description => "use sparse volumes", type => 'boolean', }, + mountpoint => { + description => "mount point", + type => 'string', format => 'pve-storage-path', + }, }; } @@ -44,6 +48,7 @@ sub options { disable => { optional => 1 }, content => { optional => 1 }, bwlimit => { optional => 1 }, + mountpoint => { optional => 1 }, }; } @@ -148,11 +153,27 @@ sub path { my ($vtype, $name, $vmid) = $class->parse_volname($volname); my $path = ''; +my $mountpoint = $scfg->{mountpoint}; + +# automatically write mountpoint to storage config if it is not present +if (!$mountpoint) { + $mountpoint = $class->zfs_request($scfg, undef, 'get', '-o', 'value', + 'mountpoint', '-H', $scfg->{pool}); + chomp($mountpoint); + + eval { + PVE::Storage::lock_storage_config(sub { + my $cfg = PVE::Storage::config(); + $cfg->{ids}->{$storeid}->{mountpoint} = $mountpoint; + PVE::Storage::write_config($cfg); + }, "update storage config failed"); + }; + warn $@ if $@; +} if ($vtype eq "images") { if ($name =~ m/^subvol-/ || $name =~ m/^basevol-/) { - # fixme: we currently assume standard mount point?! - $path = "/$scfg->{pool}/$name"; + $path = "$mountpoint/$name"; } else { $path = "/dev/zvol/$scfg->{pool}/$name"; } it would be great if this "get mountpoint property of dataset" (or better: even "get arbitrary properties of dataset") helper could be factored out. we already have two calls to "zfs get -Hp '...' $dataset" that could re-use such a helper. then we could add a single check in activate_storage (maybe just for the branch where we actually just imported the pool?) to verify that the stored mountpoint value is correct, and if not update it (or warn about the problem). that check could have a very low timeout, and errors could be ignored since it is just a double-check. ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel I noticed that the $scfg parameter in zfs_request is never used. Should I clean that up? Otherwise I'd need to include the $scfg parameter in the helper function as well, where it also isn't needed. it's used by ZFS over iSCSI, where the plugin is derived from ZFSPoolPlugin, so it's not safe to remove ;) ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel I see, thanks. I'll remember to check whether the method is virtual next time. There are two problems with doing the mount point configuration in activate_storage. First, we might get called by 'create' (API for config triggered by 'pvesm add') which holds a lock on storage.cfg at that time, but we might also get called by something else without such a lock. Second, 'create' will write its own version of the config after calling activate_storage anyways. It also doesn't help to only do the mount point configuration inside the branch where we just imported, since the pool/dataset might be imported already when we do 'pvesm add' and the second problem is still there. Now there is plugin->on_add_hook used by 'create' which would also be a good place to configure the mount point. We could activate the storage there, check for the mount point and then hijack the scfg that is passed along to it (also not nice), so that it is written by 'create' later. In activate_storage we could still check whether configured and effective mount points match and warn if they don't. I don't see a clean way to do the automatic adding of the mount point property (doing it in path() is possible, but we need to assume that our caller doesn't hold the lock on storage.cfg). Maybe it's better to just do the warning in activate_storage and leave it to the user to configure the mount point properly? It feels a bit weird for the plugin code to modify the config; is there an inst
Re: [pve-devel] [PATCH 17/23] allow ticket in auth header as fallback
On 11/12/19 11:17 AM, Fabian Grünbichler wrote: > On November 12, 2019 11:05 am, Thomas Lamprecht wrote: >> On 11/12/19 10:46 AM, Fabian Grünbichler wrote: >>> On October 17, 2019 5:33 pm, Thomas Lamprecht wrote: On 10/17/19 3:14 PM, Fabian Grünbichler wrote: > @@ -1232,7 +1232,10 @@ sub unshift_read_header { > } elsif ($path =~ m/^\Q$base_uri\E/) { > my $token = $r->header('CSRFPreventionToken'); > my $cookie = $r->header('Cookie'); > - my $ticket = > PVE::APIServer::Formatter::extract_auth_cookie($cookie, > $self->{cookie_name}); > + my $auth_header = $r->header('Authorization'); > + my $ticket = > PVE::APIServer::Formatter::extract_auth_value($cookie, > $self->{cookie_name}); > + $ticket = > PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{cookie_name}) > + if !$ticket; can we do this a bit more separate like: if (!$ticket && (my $auth_header = $r->header('Authorization')) { $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name}); } >>> >>> this would then (with the next patch) become: >>> >>> my $api_token; >>> if (!$ticket && (my $auth_header = $r->header('Authorization')) { >>> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, >>> $self->{cookie_name}); >>> >>> if (!$ticket) { >>> $api_token = >>> PVE::APIServer::Formatter::extract_auth_value($auth_header, >>> $self->{apitoken_name}); >>> } >>> } >>> >> the inner (apitoken) "if" would be nicer to move a layer out below the other >> one.>> > > it needs $auth_header though, which would then also move (back out > again): > > my $auth_header = $r->header('Authorization'); > if (!$ticket) { > $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{cookie_name}); > } > my $api_token; > if (!$ticket) { > $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{apitoken_name}); > } > > which is basically the original version, modulo separate declaration of > $api_token: > > my $auth_header = $r->header('Authorization'); > > $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{cookie_name}) > if !$ticket; > > my $api_token; > $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{apitoken_name}) > if !$ticket; > Above with line spacing and comments would already be a big improvement. I feel that the real-nice-thing is still missing though, but no better idea myself ^^ ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] applied: [PATCH manager] ui: vm opts: add hint for spice foldersharing
On 11/6/19 1:17 PM, Aaron Lauterer wrote: > Spice foldersharing needs the webdavd daemon installed inside the guest. > This patch adds a hint to remind the user to install it in the VM. > > Signed-off-by: Aaron Lauterer > --- > www/manager6/form/SpiceEnhancementSelector.js | 13 + > 1 file changed, 13 insertions(+) > applied, thanks! ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH 17/23] allow ticket in auth header as fallback
On November 12, 2019 11:05 am, Thomas Lamprecht wrote: > On 11/12/19 10:46 AM, Fabian Grünbichler wrote: >> On October 17, 2019 5:33 pm, Thomas Lamprecht wrote: >>> On 10/17/19 3:14 PM, Fabian Grünbichler wrote: @@ -1232,7 +1232,10 @@ sub unshift_read_header { } elsif ($path =~ m/^\Q$base_uri\E/) { my $token = $r->header('CSRFPreventionToken'); my $cookie = $r->header('Cookie'); - my $ticket = PVE::APIServer::Formatter::extract_auth_cookie($cookie, $self->{cookie_name}); + my $auth_header = $r->header('Authorization'); + my $ticket = PVE::APIServer::Formatter::extract_auth_value($cookie, $self->{cookie_name}); + $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name}) + if !$ticket; >>> >>> can we do this a bit more separate like: >>> >>> if (!$ticket && (my $auth_header = $r->header('Authorization')) { >>> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, >>> $self->{cookie_name}); >>> } >> >> this would then (with the next patch) become: >> >> my $api_token; >> if (!$ticket && (my $auth_header = $r->header('Authorization')) { >> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, >> $self->{cookie_name}); >> >> if (!$ticket) { >> $api_token = >> PVE::APIServer::Formatter::extract_auth_value($auth_header, >> $self->{apitoken_name}); >> } >> } >> > the inner (apitoken) "if" would be nicer to move a layer out below the other > one.>> it needs $auth_header though, which would then also move (back out again): my $auth_header = $r->header('Authorization'); if (!$ticket) { $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name}); } my $api_token; if (!$ticket) { $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{apitoken_name}); } which is basically the original version, modulo separate declaration of $api_token: my $auth_header = $r->header('Authorization'); $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name}) if !$ticket; my $api_token; $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{apitoken_name}) if !$ticket; >>> or if you prefer: >>> >>> if (!$ticket) { >>> my $auth_header = $r->header('Authorization'); >>> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, >>> $self->{cookie_name}); >>> } >> >> my $api_token; >> if (!$ticket) { >> my $auth_header = $r->header('Authorization'); >> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, >> $self->{cookie_name}); >> >> if (!$ticket) { >> $api_token = >> PVE::APIServer::Formatter::extract_auth_value($auth_header, >> $self->{apitoken_name}); >> } >> } >> >>> >>> makes it slightly cleaner/easier to read, IMO >> >> which makes it harder to parse IMHO, but if you still prefer it I will >> rewrite it with your suggestion for v2? we could of course also add >> comments, like so: >> >> # check actual cookie > s/check/prefer/ > >> my $ticket = PVE::APIServer::Formatter::extract_auth_value($cookie, >> $self->{cookie_name}); >> >> # fallback to cookie in 'Authorization' header >> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, >> $self->{cookie_name}) >> if !$ticket; >> >> # fallback to API token >> my $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, >> $self->{apitoken_name}) >> if !$ticket; > NAK, do *not* declare variables guarded with a postfix if, that gets > bad results.[0][1] fair enough (I always forgot about this) - it's a separate issue from the question of whether to nest or not nest the if's though ;) > > [0]: https://pve.proxmox.com/pipermail/pve-devel/2018-June/032238.html > [1]: https://perldoc.perl.org/perlsyn.html#Statement-Modifiers > > ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH 17/23] allow ticket in auth header as fallback
On 11/12/19 10:46 AM, Fabian Grünbichler wrote: > On October 17, 2019 5:33 pm, Thomas Lamprecht wrote: >> On 10/17/19 3:14 PM, Fabian Grünbichler wrote: >>> @@ -1232,7 +1232,10 @@ sub unshift_read_header { >>> } elsif ($path =~ m/^\Q$base_uri\E/) { >>> my $token = $r->header('CSRFPreventionToken'); >>> my $cookie = $r->header('Cookie'); >>> - my $ticket = >>> PVE::APIServer::Formatter::extract_auth_cookie($cookie, >>> $self->{cookie_name}); >>> + my $auth_header = $r->header('Authorization'); >>> + my $ticket = >>> PVE::APIServer::Formatter::extract_auth_value($cookie, >>> $self->{cookie_name}); >>> + $ticket = >>> PVE::APIServer::Formatter::extract_auth_value($auth_header, >>> $self->{cookie_name}) >>> + if !$ticket; >> >> can we do this a bit more separate like: >> >> if (!$ticket && (my $auth_header = $r->header('Authorization')) { >> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, >> $self->{cookie_name}); >> } > > this would then (with the next patch) become: > > my $api_token; > if (!$ticket && (my $auth_header = $r->header('Authorization')) { > $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{cookie_name}); > > if (!$ticket) { > $api_token = > PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{apitoken_name}); > } > } > the inner (apitoken) "if" would be nicer to move a layer out below the other one.>> >> or if you prefer: >> >> if (!$ticket) { >> my $auth_header = $r->header('Authorization'); >> $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, >> $self->{cookie_name}); >> } > > my $api_token; > if (!$ticket) { > my $auth_header = $r->header('Authorization'); > $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{cookie_name}); > > if (!$ticket) { > $api_token = > PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{apitoken_name}); > } > } > >> >> makes it slightly cleaner/easier to read, IMO > > which makes it harder to parse IMHO, but if you still prefer it I will > rewrite it with your suggestion for v2? we could of course also add > comments, like so: > > # check actual cookie s/check/prefer/ > my $ticket = PVE::APIServer::Formatter::extract_auth_value($cookie, > $self->{cookie_name}); > > # fallback to cookie in 'Authorization' header > $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{cookie_name}) > if !$ticket; > > # fallback to API token > my $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{apitoken_name}) > if !$ticket; NAK, do *not* declare variables guarded with a postfix if, that gets bad results.[0][1] [0]: https://pve.proxmox.com/pipermail/pve-devel/2018-June/032238.html [1]: https://perldoc.perl.org/perlsyn.html#Statement-Modifiers ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH qemu-server] Use crm-command stop to allow shutdown with timeout and hard stop for HA
The minimum value for timeout in vm_shutdown is changed from 0 to 1, since a value of 0 would trigger a hard stop for HA managed VMs. Like this the API description stays valid for all cases. Signed-off-by: Fabian Ebner --- In vm_shutdown we'd like to pass along the timeout parameter to the HA stack, but the value 0 triggers a special behavior there, namely a hard stop. So either the description needs to be changed to mention this behavior or we just don't allow a timeout of 0 in vm_shutdown. Since a shutdown with timeout 0 doesn't really make sense anyways, I opted for the latter. It's the same situation for containers. PVE/API2/Qemu.pm | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index c31dd1d..16a7a04 100644 --- a/PVE/API2/Qemu.pm +++ b/PVE/API2/Qemu.pm @@ -2111,7 +2111,7 @@ __PACKAGE__->register_method({ print "Requesting HA stop for VM $vmid\n"; - my $cmd = ['ha-manager', 'set', "vm:$vmid", '--state', 'stopped']; + my $cmd = ['ha-manager', 'crm-command', 'stop', "vm:$vmid", '0']; PVE::Tools::run_command($cmd); return; }; @@ -2204,7 +2204,7 @@ __PACKAGE__->register_method({ timeout => { description => "Wait maximal timeout seconds.", type => 'integer', - minimum => 0, + minimum => 1, optional => 1, }, forceStop => { @@ -2267,12 +2267,13 @@ __PACKAGE__->register_method({ if (PVE::HA::Config::vm_is_ha_managed($vmid) && $rpcenv->{type} ne 'ha') { + my $timeout = $param->{timeout} // 60; my $hacmd = sub { my $upid = shift; print "Requesting HA stop for VM $vmid\n"; - my $cmd = ['ha-manager', 'set', "vm:$vmid", '--state', 'stopped']; + my $cmd = ['ha-manager', 'crm-command', 'stop', "vm:$vmid", "$timeout"]; PVE::Tools::run_command($cmd); return; }; -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH container] Use crm-command stop to allow shutdown with timeout and hard stop for HA
The minimum value for timeout in vm_shutdown is changed from 0 to 1, since a value of 0 would trigger a hard stop for HA managed containers. Like this the API description stays valid for all cases. Signed-off-by: Fabian Ebner --- src/PVE/API2/LXC/Status.pm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm index 1b7a71d..20b376a 100644 --- a/src/PVE/API2/LXC/Status.pm +++ b/src/PVE/API2/LXC/Status.pm @@ -243,7 +243,7 @@ __PACKAGE__->register_method({ print "Requesting HA stop for CT $vmid\n"; - my $cmd = ['ha-manager', 'set', "ct:$vmid", '--state', 'stopped']; + my $cmd = ['ha-manager', 'crm-command', 'stop', "ct:$vmid", '0']; PVE::Tools::run_command($cmd); }; @@ -291,7 +291,7 @@ __PACKAGE__->register_method({ timeout => { description => "Wait maximal timeout seconds.", type => 'integer', - minimum => 0, + minimum => 1, optional => 1, default => 60, }, @@ -325,7 +325,7 @@ __PACKAGE__->register_method({ print "Requesting HA stop for CT $vmid\n"; - my $cmd = ['ha-manager', 'set', "ct:$vmid", '--state', 'stopped']; + my $cmd = ['ha-manager', 'crm-command', 'stop', "ct:$vmid", "$timeout"]; PVE::Tools::run_command($cmd); }; -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH 17/23] allow ticket in auth header as fallback
On October 17, 2019 5:33 pm, Thomas Lamprecht wrote: > On 10/17/19 3:14 PM, Fabian Grünbichler wrote: >> based on idea & RFC by Tim Marx, incorporating feedback by Thomas >> Lamprecht. this will be extended to support API tokens in the >> Authorization header as well, so make it generic. >> >> Signed-off-by: Fabian Grünbichler >> --- >> >> Notes: >> semi-independent, could also leave extract_auth_cookie as alias/wrapper >> to >> avoid a change in PMG. but since we need to change other method >> signatures >> anyway for the token part, we could change this as well. >> >> as-is, needs a versioned breaks/depends on pve-manager and some part of >> PMG? >> >> PVE/APIServer/AnyEvent.pm | 5 - >> PVE/APIServer/Formatter.pm | 12 ++-- >> 2 files changed, 10 insertions(+), 7 deletions(-) >> >> diff --git a/PVE/APIServer/AnyEvent.pm b/PVE/APIServer/AnyEvent.pm >> index 9aba27d..c0b90ab 100644 >> --- a/PVE/APIServer/AnyEvent.pm >> +++ b/PVE/APIServer/AnyEvent.pm >> @@ -1232,7 +1232,10 @@ sub unshift_read_header { >> } elsif ($path =~ m/^\Q$base_uri\E/) { >> my $token = $r->header('CSRFPreventionToken'); >> my $cookie = $r->header('Cookie'); >> -my $ticket = >> PVE::APIServer::Formatter::extract_auth_cookie($cookie, >> $self->{cookie_name}); >> +my $auth_header = $r->header('Authorization'); >> +my $ticket = >> PVE::APIServer::Formatter::extract_auth_value($cookie, $self->{cookie_name}); >> +$ticket = >> PVE::APIServer::Formatter::extract_auth_value($auth_header, >> $self->{cookie_name}) >> +if !$ticket; > > can we do this a bit more separate like: > > if (!$ticket && (my $auth_header = $r->header('Authorization')) { > $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{cookie_name}); > } this would then (with the next patch) become: my $api_token; if (!$ticket && (my $auth_header = $r->header('Authorization')) { $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name}); if (!$ticket) { $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{apitoken_name}); } } > > or if you prefer: > > if (!$ticket) { > my $auth_header = $r->header('Authorization'); > $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, > $self->{cookie_name}); > } my $api_token; if (!$ticket) { my $auth_header = $r->header('Authorization'); $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name}); if (!$ticket) { $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{apitoken_name}); } } > > makes it slightly cleaner/easier to read, IMO which makes it harder to parse IMHO, but if you still prefer it I will rewrite it with your suggestion for v2? we could of course also add comments, like so: # check actual cookie my $ticket = PVE::APIServer::Formatter::extract_auth_value($cookie, $self->{cookie_name}); # fallback to cookie in 'Authorization' header $ticket = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{cookie_name}) if !$ticket; # fallback to API token my $api_token = PVE::APIServer::Formatter::extract_auth_value($auth_header, $self->{apitoken_name}) if !$ticket; > >> >> my ($rel_uri, $format) = &$split_abs_uri($path, >> $self->{base_uri}); >> if (!$format) { >> diff --git a/PVE/APIServer/Formatter.pm b/PVE/APIServer/Formatter.pm >> index 0c459bd..def1932 100644 >> --- a/PVE/APIServer/Formatter.pm >> +++ b/PVE/APIServer/Formatter.pm >> @@ -75,16 +75,16 @@ sub get_login_formatter { >> >> # some helper functions >> >> -sub extract_auth_cookie { >> -my ($cookie, $cookie_name) = @_; >> +sub extract_auth_value { >> +my ($header, $key) = @_; >> >> -return undef if !$cookie; >> +return undef if !$header; >> >> -my $ticket = ($cookie =~ /(?:^|\s)\Q$cookie_name\E=([^;]*)/)[0]; >> +my $value = ($header =~ /(?:^|\s)\Q$key\E(?:=| )([^;]*)/)[0]; >> >> -$ticket = uri_unescape($ticket) if $ticket; >> +$value = uri_unescape($value) if $value; >> >> -return $ticket; >> +return $value; >> } >> >> sub create_auth_cookie { >> > > > ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH docu] Fix #2459: qm: Make info about core limit clear
'assigning' could also mean that creating a VM with more cores than physically available is impossible. However, this is not the case. Using 'starting' instead is more precise and still easy to understand. Signed-off-by: Dominic Jäger --- qm.adoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qm.adoc b/qm.adoc index 100d315..b13f0f4 100644 --- a/qm.adoc +++ b/qm.adoc @@ -283,8 +283,8 @@ is greater than the number of cores on the server (e.g., 4 VMs with each 4 cores on a machine with only 8 cores). In that case the host system will balance the Qemu execution threads between your server cores, just like if you were running a standard multithreaded application. However, {pve} will prevent -you from assigning more virtual CPU cores than physically available, as this will -only bring the performance down due to the cost of context switches. +you from starting VMs with more virtual CPU cores than physically available, as +this will only bring the performance down due to the cost of context switches. [[qm_cpu_resource_limits]] Resource Limits -- 2.20.1 ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel