[pve-devel] applied: [PATCH qemu-server] tests: add tests for various combinations of configs for usb

2022-11-11 Thread Thomas Lamprecht
Am 11/11/2022 um 07:59 schrieb Dominik Csapak:
> q35 + usb passthrough
> q35 + usb3 passthrough
> q35 + usb3 passthrough with new xhci controller
> old machine type + new usb config error
> old machine type + q35 + new usb config error
> old ostype (w2k) + new usb config error
> 
> Signed-off-by: Dominik Csapak 
> ---
>  test/cfg2cmd/ostype-usb13-error.conf| 13 ++
>  test/cfg2cmd/q35-usb13-error.conf   | 14 +++
>  test/cfg2cmd/q35-usb2.conf  | 13 ++
>  test/cfg2cmd/q35-usb2.conf.cmd  | 31 
>  test/cfg2cmd/q35-usb3.conf  | 13 ++
>  test/cfg2cmd/q35-usb3.conf.cmd  | 32 +
>  test/cfg2cmd/qemu-xhci-q35-7.1.conf | 12 ++
>  test/cfg2cmd/qemu-xhci-q35-7.1.conf.cmd | 29 ++
>  test/cfg2cmd/usb13-error.conf   | 13 ++
>  9 files changed, 170 insertions(+)
>  create mode 100644 test/cfg2cmd/ostype-usb13-error.conf
>  create mode 100644 test/cfg2cmd/q35-usb13-error.conf
>  create mode 100644 test/cfg2cmd/q35-usb2.conf
>  create mode 100644 test/cfg2cmd/q35-usb2.conf.cmd
>  create mode 100644 test/cfg2cmd/q35-usb3.conf
>  create mode 100644 test/cfg2cmd/q35-usb3.conf.cmd
>  create mode 100644 test/cfg2cmd/qemu-xhci-q35-7.1.conf
>  create mode 100644 test/cfg2cmd/qemu-xhci-q35-7.1.conf.cmd
>  create mode 100644 test/cfg2cmd/usb13-error.conf
> 
>

applied, with a small follow up to cover also the increased USB count in a 
expected
to succeed test too, thanks!


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



Re: [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning

2022-11-11 Thread DERUMIER, Alexandre
Le mercredi 09 novembre 2022 à 15:19 +0100, Mira Limbeck a écrit :
> 
> 
> Why not add the bridge in the pve-bridge script as well? This way
> there 
> would be no need for patch 2 for qemu-server since we always add the
> MAC 
> address to the FDB whenever the tap device is plugged.
> 
> If we then remove it in the pve-bridgedown script, there should be no
> need for patch 3, right?
> 
> 

ok, I review the whole patch serie, the current behaviour is good.
(Seem the workflow I have send in my previous comment on patch3)


We can't add fdb entry inconditionally in pve-bridge,as on live
migration, we don't want to add fdb entry until the migration is
finished.

(If we do that, the others vms on the target host, will not be able to
communicate with source vms during the migration)




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


[pve-devel] applied: [PATCH V4 storage 1/3] fix #3967: enable ZFS dRAID creation via API

2022-11-11 Thread Thomas Lamprecht
Am 10/11/2022 um 14:24 schrieb Stefan Hrdlicka:
> It is possible to set the number of spares and the size of
> data stripes via draidspares & dreaddata parameters.
> 
> Signed-off-by: Stefan Hrdlicka 
> ---
>  PVE/API2/Disks/ZFS.pm | 55 ++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
> 

applied with Lukas' T-b tag and some stylistic follow ups (see below), thanks!

> diff --git a/PVE/API2/Disks/ZFS.pm b/PVE/API2/Disks/ZFS.pm
> index a4d4aa2..d1bcbcb 100644
> --- a/PVE/API2/Disks/ZFS.pm
> +++ b/PVE/API2/Disks/ZFS.pm
> @@ -285,6 +285,19 @@ __PACKAGE__->register_method ({
>   return $pool;
>  }});
>  
> +my $draid_config_format = {
> +spares => {
> + type => 'integer',
> + minimum => 0,
> + description => 'Number of dRAID spares.',
> +},
> +data => {
> + type => 'integer',
> + minimum => 1,
> + description => 'The number of data devices per redundancy group. 
> (dRAID)',

the code handles either as optional but this isn't encoded in the schema,
I added it, please re-check if that was OK.

> +},
> +};
> +
>  __PACKAGE__->register_method ({
>  name => 'create',
>  path => '',
> @@ -303,12 +316,21 @@ __PACKAGE__->register_method ({
>   raidlevel => {
>   type => 'string',
>   description => 'The RAID level to use.',
> - enum => ['single', 'mirror', 'raid10', 'raidz', 'raidz2', 
> 'raidz3'],
> + enum => [
> + 'single', 'mirror',
> + 'raid10', 'raidz', 'raidz2', 'raidz3',
> + 'draid', 'draid2', 'draid3',
> + ],
>   },
>   devices => {
>   type => 'string', format => 'string-list',
>   description => 'The block devices you want to create the zpool 
> on.',
>   },
> + 'draid-config' => {
> + type => 'string',
> + format => $draid_config_format,
> + optional => 1,
> + },
>   ashift => {
>   type => 'integer',
>   minimum => 9,
> @@ -344,6 +366,13 @@ __PACKAGE__->register_method ({
>   my $devs = [PVE::Tools::split_list($param->{devices})];
>   my $raidlevel = $param->{raidlevel};
>   my $compression = $param->{compression} // 'on';
> + my $draid_config = {};
> + if (exists $param->{'draid-config'}) {

I moved the "draid-config but no draid level" assertion here, we don't need to 
check
spare/raid settings then explicitly, which would have been prone to forget to 
updating
the check below if draid-config ever got more parameters.

> + $draid_config = PVE::JSONSchema::parse_property_string(
> + $draid_config_format, $param->{'draid-config'});
> + }
> + my $draid_data = $draid_config->{data};
> + my $draid_spares = $draid_config->{spares};

I dropped those intermediate variables, location of declaration and usage was 
pretty
split and it didn't felt like we'd gain much over just using the hash directly 
here in
their current usage.



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



Re: [pve-devel] [PATCH V3 qemu-server 1/3] tap_plug: add support for bridge disable learning

2022-11-11 Thread Mira Limbeck

On 11/11/22 09:36, DERUMIER, Alexandre wrote:

Le mercredi 09 novembre 2022 à 15:19 +0100, Mira Limbeck a écrit :


Why not add the bridge in the pve-bridge script as well? This way
there
would be no need for patch 2 for qemu-server since we always add the
MAC
address to the FDB whenever the tap device is plugged.

If we then remove it in the pve-bridgedown script, there should be no
need for patch 3, right?



ok, I review the whole patch serie, the current behaviour is good.
(Seem the workflow I have send in my previous comment on patch3)


We can't add fdb entry inconditionally in pve-bridge,as on live
migration, we don't want to add fdb entry until the migration is
finished.

(If we do that, the others vms on the target host, will not be able to
communicate with source vms during the migration)

Wouldn't it still send the traffic to the source VM while the entry is 
still alive?


But since you're a lot more knowledgeable about networks, I trust your 
judgement in this case.



In this case consider the 3 qemu-server patches:

Reviewed-by: Mira Limbeck 

Tested-by: Mira Limbeck 


The pve-network patch has only been tested without SDN for now, I'll try 
to set something up to test it with SDN as well.


The pve-container patch is on my todo list as well.



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


Re: [pve-devel] [PATCH ha-manager 09/11] manager: use static resource scheduler when configured

2022-11-11 Thread Fiona Ebner
Am 10.11.22 um 15:37 schrieb Fiona Ebner:
> @@ -206,11 +207,30 @@ my $valid_service_states = {
>  sub recompute_online_node_usage {
So I was a bit worried that recompute_online_node_usage() would become
too inefficient with the new add_service_usage_to_node() overhead from
needing to read the guest configs. I now tested it with ~300 HA services
(minimal containers) running on my virtual test cluster.

Timings with 'basic' mode were between 0.0004 - 0.001 seconds
Timings with 'static' mode were between 0.007 - 0.012 seconds

While about a 10-fold increase, it's not too dramatic at least. I guess
that's what the caching of cfs files is for :)

Still, the function is currently not only called in the main loop in
manage(), but also in next_state_recovery() and change_service_state().

With, say, 400 HA services each on 5 nodes, if a node fails there's
400 calls from changing to freeze
400 calls from changing to recovery
400 calls in next_state_recovery
400 calls from changing to started
If we take a generous estimate that each call takes 0.1 seconds (there's
2000 services in total), that's 40+80+40 seconds in 3 bursts during the
fencing and recovery period.

Is that acceptable? Should I try to optimize how often the function is
called?


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



Re: [pve-devel] [PATCH pve-guest-common/pve-docs 0/1]

2022-11-11 Thread Daniel Tschlatscher
The hookscripts for pre- and post- snapshot work as intended.
I tested creating snapshots in the GUI and on the CLI without finding
any problems.
When the hookscript fails the behavior was as I would expect it: An
error in 'pre-hookscript' fails the whole task. An error in the
'post-hookscript' states that an error in the hookscript occured, but
still ends with 'TASK OK', with the snapshot being created.


One general suggestion:
When the 'post-snapshot' hook fails it would be nice if a warning could
be printed. As the snapshot still succeeds in this case, just looking at
the task log list, it looks like the task finished without any problems.

This would require some changes in 'exec_hookscript' and probably for
other 'post-x' hooks as well, though, this is not in the scope of these
patches. I just thought to put it somewhere.


So, consider this series

Tested-by: Daniel Tschlatscher 

On 9/22/22 13:54, Stefan Hanreich wrote:
> This patch adds hooks that run when the user creates a snapshot from the Web 
> UI
> / CLI. I have tested this with both VMs/CTs via Web UI and CLI. Are there any
> other places where the hook should get triggered that I missed?
> 
> pve-guest-common:
> 
> Stefan Hanreich (1):
>   add pre/post-snapshot hooks
> 
>  src/PVE/AbstractConfig.pm | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> pve-docs:
> 
> Stefan Hanreich (1):
>   add pre/post snapshot events to example hookscript
> 
>  examples/guest-example-hookscript.pl | 14 ++
>  1 file changed, 14 insertions(+)
> 


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



Re: [pve-devel] [PATCH manager v2 1/2] fix #3719: gui: expose LXC MTU option in web UI

2022-11-11 Thread Stefan Hanreich

Tested on an Alpine Linux Container (3.16):

- setting MTU > 65535 or < 576 via UI (doesn't work)
- setting MTU to several values >= 576 and <= 65535 via UI (works)
- setting MTU to >= 64 and < 576 via config then starting (works, but 
this is apparently intended otherwise it would be a backwards breaking 
change)
- setting MTU >= 576 and <= 65535 via Config and then starting the 
container (works)
- setting MTU > 65535 or < 64 via Config and then starting the container 
(doesn't work)


Some Notes:
- Setting the MTU while the container is running, does not update the 
MTU of the running container. If this is intended behavior it might be 
smart to document it somewhere or throw a warning. This also doesn't 
work for the VM patch. Not sure if this is even possible at runtime tbh.
- Adding a network device when the Container is running with a specific, 
valid MTU (e.g. 1234) does add the network device to the container, BUT 
it has a MTU of 1500. Upon reboot the correct MTU is set. Reloading the 
Network config does not change anything. Maybe just an LXC limitation?


Code LGTM - small nit: there is still a gettext('MTU') left in the 
NetworkView, but it has been changed in the NetworkInputPanel.


Reviewed-by: Stefan Hanreich 
Tested-by: Stefan Hanreich 

On 11/3/22 16:38, Daniel Tschlatscher wrote:

The option to set the mtu parameter for lxc containers already exists
in the backend. It just has to be exposed in the web UI as well.

Signed-off-by: Daniel Tschlatscher 
---
Changes from v1:
* fieldLabel for MTU textfield no longer uses gettext()
* text field has an emptyText now
* the minimum value for the MTU is 576 in the frontend now

  www/manager6/lxc/Network.js | 18 ++
  1 file changed, 18 insertions(+)

diff --git a/www/manager6/lxc/Network.js b/www/manager6/lxc/Network.js
index 7b6437c5..c6c40934 100644
--- a/www/manager6/lxc/Network.js
+++ b/www/manager6/lxc/Network.js
@@ -282,6 +282,18 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
},
];
  
+	me.advancedColumn1 = [

+   {
+   xtype: 'proxmoxintegerfield',
+   fieldLabel: 'MTU',
+   emptyText: gettext('Same as bridge'),
+   name: 'mtu',
+   value: cdata.mtu,
+   minValue: 576,
+   maxValue: 65535,
+   },
+   ];
+
me.callParent();
  },
  });
@@ -519,6 +531,11 @@ Ext.define('PVE.lxc.NetworkView', {
}
},
},
+   {
+   header: gettext('MTU'),
+   width: 80,
+   dataIndex: 'mtu',
+   },
],
listeners: {
activate: me.load,
@@ -543,6 +560,7 @@ Ext.define('PVE.lxc.NetworkView', {
'gw6',
'tag',
'firewall',
+   'mtu',
],
  });
  });



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



Re: [pve-devel] [PATCH pve-docs 1/1] add pre/post-restore events to example hookscript

2022-11-11 Thread Daniel Tschlatscher
On 11/10/22 16:33, Stefan Hanreich wrote:
> Signed-off-by: Stefan Hanreich 
> ---
>  examples/guest-example-hookscript.pl | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/examples/guest-example-hookscript.pl 
> b/examples/guest-example-hookscript.pl
> index adeed59..19fe213 100755
> --- a/examples/guest-example-hookscript.pl
> +++ b/examples/guest-example-hookscript.pl
> @@ -54,6 +54,20 @@ if ($phase eq 'pre-start') {
>  
>  print "$vmid stopped. Doing cleanup.\n";
>  
> +} elsif ($phase eq 'pre-restore') {
> +
> +# Phase 'pre-restore' will be executed before restoring a backup. (via 
> UI or
> +# CLI)
> +
> +print "Restoring $vmid from backup.\n";
> +
> +} elsif ($phase eq 'post-restore') {
> +
> +# Phase 'pre-restore' will be after before restoring a backup. (via UI or
> +# CLI)

Nit: should say "'post-restore' will be executed after [...]"

> +
> +print "$vmid finished restoring from backup.\n";
> +
>  } else {
>  die "got unknown phase '$phase'\n";
>  }


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



Re: [pve-devel] [PATCH v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks

2022-11-11 Thread Daniel Tschlatscher
The new hookscript example works nicely out of the box.

I tested restore for both VMs and containers via the GUI, the restore
and create commands in the respective CLI commands and with the API.

One thing which might some more consideration:
When restoring a backup that does not configure a hookscript, the
'pre-restore' hook will run, however, the 'post-restore' will not. This
was very confusing at first.
Similarly, if the config does not include a hookscript, but the backup
does, then the 'pre-restore' will not run but the 'post-restore' will.
While this is not breaking, it is definitely very unexpected for an
unsuspecting user.


Otherwise, the core part of the series works as intended, therefore:

Tested-by: Daniel Tschlatscher 


On 11/10/22 16:33, Stefan Hanreich wrote:
> This patch adds hooks that run when the user restores a backup from the Web UI
> / CLI. I have tested this with both VMs/CTs via Web UI and CLI. Are there any
> other places where the hook should get triggered that I missed?
> 
> Changes compared to v1:
> - slightly moved the call site of the exec_hookscript in qemu-server and
> pve-container, so necessary checks are run before the hookscript runs.
> 
> 
> pve-container:
> 
> Stefan Hanreich (1):
>   Add pre/post-restore hooks to CTs
> 
>  src/PVE/API2/LXC.pm | 7 +++
>  1 file changed, 7 insertions(+)
> 
> 
> pve-docs:
> 
> Stefan Hanreich (1):
>   add pre/post-restore events to example hookscript
> 
>  examples/guest-example-hookscript.pl | 14 ++
>  1 file changed, 14 insertions(+)
> 
> 
> qemu-server:
> 
> Stefan Hanreich (1):
>   Add pre/post-restore hooks to VMs
> 
>  PVE/API2/Qemu.pm | 10 --
>  1 file changed, 8 insertions(+), 2 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 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks

2022-11-11 Thread Stefan Hanreich



On 11/11/22 14:58, Daniel Tschlatscher wrote:

The new hookscript example works nicely out of the box.

I tested restore for both VMs and containers via the GUI, the restore
and create commands in the respective CLI commands and with the API.

One thing which might some more consideration:
When restoring a backup that does not configure a hookscript, the
'pre-restore' hook will run, however, the 'post-restore' will not. This
was very confusing at first.
Similarly, if the config does not include a hookscript, but the backup
does, then the 'pre-restore' will not run but the 'post-restore' will.
While this is not breaking, it is definitely very unexpected for an
unsuspecting user.


Yes, it might be smarter to use the old config for both pre/post-restore 
and not mix both configurations I think.


Because of this and the minor issues with the example hookscript I will 
create a v3. Some input on whether to use the old configuration for both 
pre/post-restore or not would be much appreciated


Ty for the review!



Otherwise, the core part of the series works as intended, therefore:

Tested-by: Daniel Tschlatscher 


On 11/10/22 16:33, Stefan Hanreich wrote:

This patch adds hooks that run when the user restores a backup from the Web UI
/ CLI. I have tested this with both VMs/CTs via Web UI and CLI. Are there any
other places where the hook should get triggered that I missed?

Changes compared to v1:
- slightly moved the call site of the exec_hookscript in qemu-server and
pve-container, so necessary checks are run before the hookscript runs.


pve-container:

Stefan Hanreich (1):
   Add pre/post-restore hooks to CTs

  src/PVE/API2/LXC.pm | 7 +++
  1 file changed, 7 insertions(+)


pve-docs:

Stefan Hanreich (1):
   add pre/post-restore events to example hookscript

  examples/guest-example-hookscript.pl | 14 ++
  1 file changed, 14 insertions(+)


qemu-server:

Stefan Hanreich (1):
   Add pre/post-restore hooks to VMs

  PVE/API2/Qemu.pm | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)



___
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 v2 pve-container/qemu-server/pve-docs] Add pre/post-restore hooks

2022-11-11 Thread Daniel Tschlatscher


On 11/11/22 15:02, Stefan Hanreich wrote:
> 
> On 11/11/22 14:58, Daniel Tschlatscher wrote:
>> The new hookscript example works nicely out of the box.
>>
>> I tested restore for both VMs and containers via the GUI, the restore
>> and create commands in the respective CLI commands and with the API.
>>
>> One thing which might some more consideration:
>> When restoring a backup that does not configure a hookscript, the
>> 'pre-restore' hook will run, however, the 'post-restore' will not. This
>> was very confusing at first.
>> Similarly, if the config does not include a hookscript, but the backup
>> does, then the 'pre-restore' will not run but the 'post-restore' will.
>> While this is not breaking, it is definitely very unexpected for an
>> unsuspecting user.
> 
> Yes, it might be smarter to use the old config for both pre/post-restore
> and not mix both configurations I think.

+1 for not mixing the configurations

Based on our discussion off-list:
I feel like it might be even better to make this setting config-version
agnostic. Or at least, to not overwrite/include it when making or
restoring backups, and to keep it the same each time.
At least that feels like the way I would expect it to work intuitively.

> 
> Because of this and the minor issues with the example hookscript I will
> create a v3. Some input on whether to use the old configuration for both
> pre/post-restore or not would be much appreciated
> 
> Ty for the review!
> 
>>
>> Otherwise, the core part of the series works as intended, therefore:
>>
>> Tested-by: Daniel Tschlatscher 
>>
>>
>> On 11/10/22 16:33, Stefan Hanreich wrote:
>>> This patch adds hooks that run when the user restores a backup from
>>> the Web UI
>>> / CLI. I have tested this with both VMs/CTs via Web UI and CLI. Are
>>> there any
>>> other places where the hook should get triggered that I missed?
>>>
>>> Changes compared to v1:
>>> - slightly moved the call site of the exec_hookscript in qemu-server and
>>> pve-container, so necessary checks are run before the hookscript runs.
>>>
>>>
>>> pve-container:
>>>
>>> Stefan Hanreich (1):
>>>    Add pre/post-restore hooks to CTs
>>>
>>>   src/PVE/API2/LXC.pm | 7 +++
>>>   1 file changed, 7 insertions(+)
>>>
>>>
>>> pve-docs:
>>>
>>> Stefan Hanreich (1):
>>>    add pre/post-restore events to example hookscript
>>>
>>>   examples/guest-example-hookscript.pl | 14 ++
>>>   1 file changed, 14 insertions(+)
>>>
>>>
>>> qemu-server:
>>>
>>> Stefan Hanreich (1):
>>>    Add pre/post-restore hooks to VMs
>>>
>>>   PVE/API2/Qemu.pm | 10 --
>>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>
>> ___
>> 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 docs v2 2/2] added Memory Encryption documentation

2022-11-11 Thread Markus Frank
added AMD SEV documentation for "[PATCH qemu-server] QEMU AMD SEV
enable"

Signed-off-by: Markus Frank 
---
 qm.adoc | 113 
 1 file changed, 113 insertions(+)

diff --git a/qm.adoc b/qm.adoc
index e7d0c07..5ba43a2 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -598,6 +598,119 @@ systems.
 When allocating RAM to your VMs, a good rule of thumb is always to leave 1GB
 of RAM available to the host.
 
+[[qm_memory_encryption]]
+Memory Encryption
+~
+
+[[qm_memory_encryption_sev]]
+AMD SEV
+^^^
+
+Memory Encryption per VM using AES-128 Encryption and the AMD Secure Processor.
+See https://developer.amd.com/sev/[AMD SEV]
+
+*Host-Requirements:*
+
+* AMD EPYC/Ryzen PRO CPU
+* configured SEV BIOS settings on Host Machine
+* add "kvm_amd.sev=1" to kernel parameters if not enabled by default
+* add "mem_encrypt=on" to kernel parameters if you want encrypt memory on the
+host (SME)
+see https://www.kernel.org/doc/Documentation/x86/amd-memory-encryption.txt
+* maybe increase SWIOTLB see https://github.com/AMDESE/AMDSEV#faq-4
+
+To check if SEV is enabled on Host-Machine search for `sev` in dmesg
+and print out the sev kernel parameter of kvm_amd:
+
+
+# dmesg | grep -i sev
+[...] ccp :45:00.1: sev enabled
+[...] ccp :45:00.1: SEV API: 
+[...] SEV supported:  ASIDs
+[...] SEV-ES supported:  ASIDs
+# cat /sys/module/kvm_amd/parameters/sev
+Y
+
+
+*Guest-VM-Requirements:*
+
+* edk2-OVMF
+* advisable to use Q35
+* The guest operating system inside the VM must contain SEV-support
+* if there are problems while booting (stops at blank/splash screen or "Guest 
has not
+initialized the display (yet)") try to add virtio-rng and/or set "freeze: 1"
+so that you wait a few seconds before you click on *Resume* to boot.
+
+*Limitations:*
+
+* Because the memory is encrypted the memory usage on host is always wrong
+* Operations that involve saving or restoring memory like snapshots
+& live migration do not work yet or are attackable
+https://github.com/PSPReverse/amd-sev-migration-attack
+* KVM is unsupported when running as an SEV guest
+* PCI passthrough is not supported
+
+Example Configuration:
+
+
+# qm set  -memory_encryption 
type=sev,cbitpos=47,policy=0x0001,reduced-phys-bits=1
+
+
+*SEV Parameters*
+
+*type* defines the encryption technology ("type=" is not necessary):
+currently-supported: *sev*
+and in the future: sev-snp, mktme
+
+*reduced-phys-bios*, *cbitpos* and *policy* correspond to the variables with 
the
+same name in qemu.
+
+*reduced-phys-bios* and *cbitpos* are system specific and can be read out
+with QMP. If not set, qm starts a dummy-vm to read QMP
+for these variables out and saves them to config.
+
+*policy* can be calculated with
+https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf[AMD
 SEV API Specification Chapter 3]
+
+To use SEV-ES (CPU register encryption) the *policy* should be set
+somewhere between 0x4 and 0x7 or 0xC and 0xF, etc.
+(Bit-2 has to be set 1 (LSB 0 bit numbering))
+
+*Check if SEV is working on the Guest*
+
+Method 1 - dmesg:
+
+Output should look like this.
+
+
+# dmesg | grep -i sev
+AMD Memory Encryption Features active: SEV
+
+
+Method 2 - MSR 0xc0010131 (MSR_AMD64_SEV):
+
+Output should be 1.
+
+
+# apt install msr-tools
+# modprobe msr
+# rdmsr -a 0xc0010131
+1
+
+
+Links:
+
+* https://github.com/AMDESE/AMDSEV
+* https://www.qemu.org/docs/master/system/i386/amd-memory-encryption.html
+* https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf
+* https://documentation.suse.com/sles/15-SP1/html/SLES-amd-sev/index.html
+
+// Commented because cannot be tested without new EPYC-CPU
+// AMD SEV-SNP
+// ^^^
+// * SEV-SNP needs EPYC 7003 "Milan" processors.
+// * SEV-SNP should in Kernel 5.19:
+// 
https://www.phoronix.com/scan.php?page=news_item&px=AMD-SEV-SNP-Arrives-Linux-5.19
 
 [[qm_network_device]]
 Network Device
-- 
2.30.2



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



[pve-devel] [PATCH qemu-server v2 1/2] QEMU AMD SEV enable

2022-11-11 Thread Markus Frank
This Patch is for enabling AMD SEV (Secure Encrypted
Virtualization) support in QEMU and enabling future
memory encryption technologies like INTEL MKTME
(Multi-key Total Memory Encryption) and SEV-SNP.

Config-Example:
memory_encryption: type=sev,cbitpos=47,policy=0x0001,reduced-phys-bits=1

reduced-phys-bios, cbitpos and policy correspond to the varibles with the
same name in qemu.

reduced-phys-bios and cbitpos are system specific and can be read out
with QMP. If not set by the user, a dummy-vm gets started to read QMP
for these variables out and save them to config. Afterwards the dummy-vm gets
stopped.

policy can be calculated with the links in comments & description.
To test SEV-ES (CPU register encryption) the policy should be set
somewhere between 0x4 and 0x7 or 0xC and 0xF, etc.
(Bit-2 has to be set 1 (LSB 0 bit numbering))

SEV needs edk2-OVMF to work.

Signed-off-by: Markus Frank 
---
 PVE/QemuServer.pm | 133 ++
 1 file changed, 133 insertions(+)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 513a248..2ea8abd 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -175,6 +175,58 @@ my $agent_fmt = {
 },
 };
 
+my $memory_encryption_fmt = {
+type => {
+   type => 'string',
+   default_key => 1,
+   description => "Memory Encryption Type:"
+   ." for AMD SEV -> 'memory_encryption: type=sev';"
+   ." for AMD SEV-ES -> use 'sev' and change policy to between 0x4 and 
0x7;"
+   ." (Bit-2 has to be set 1 (LSB 0 bit numbering))"
+   #. "for AMD SEV-SNP -> 'memory_encryption: type=sev-snp'"
+   ." (sev requires edk2-ovmf & sev kernel support by guest operating 
system &"
+   ." on host: add kernel-parameters 'mem_encrypt=on kvm_amd.sev=1')"
+   ." see https://github.com/AMDESE/AMDSEV &"
+   ." 
https://documentation.suse.com/sles/15-SP1/html/SLES-amd-sev/index.html";,
+   format_description => "qemu-memory-encryption-type",
+   # TODO enable sev-snp option when feature can be tested on 3rd-gen EPYC
+   # https://www.phoronix.com/news/AMD-SEV-SNP-Arrives-Linux-5.19
+   # enum => ['sev','sev-snp','mktme'],
+   enum => ['sev'],
+   maxLength => 10,
+},
+'reduced-phys-bits' => {
+   description => "Number of bits the physical address space is reduced 
by. System dependent",
+   type => 'integer',
+   default => 1,
+   optional => 1,
+   minimum => 0,
+   maximum => 100,
+},
+cbitpos => {
+   description => "C-bit: marks if a memory page is protected. System 
dependent",
+   type => 'integer',
+   default => 47,
+   optional => 1,
+   minimum => 0,
+   maximum => 100,
+},
+policy => {
+   description => "SEV Guest Policy"
+   . "see Capter 3:"
+   . 
"https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf";
+   . "& 
https://www.qemu.org/docs/master/system/i386/amd-memory-encryption.html";,
+
+   format_description => "qemu-memory-encryption-policy",
+   type => 'string',
+   default => '0x',
+   pattern => '0[xX][0-9a-fA-F]{1,4}',
+   optional => 1,
+   maxLength => 6,
+},
+};
+PVE::JSONSchema::register_format('pve-qemu-memory-encryption-fmt', 
$memory_encryption_fmt);
+
 my $vga_fmt = {
 type => {
description => "Select the VGA type.",
@@ -349,6 +401,12 @@ my $confdesc = {
minimum => 16,
default => 512,
 },
+memory_encryption => {
+   description => "Memory Encryption",
+   optional => 1,
+   format => 'pve-qemu-memory-encryption-fmt',
+   type => 'string',
+},
 balloon => {
optional => 1,
type => 'integer',
@@ -2113,6 +2171,17 @@ sub parse_guest_agent {
 return $res;
 }
 
+sub parse_memory_encryption {
+my ($value) = @_;
+
+return if !$value;
+
+my $res = eval { parse_property_string($memory_encryption_fmt, $value) };
+warn $@ if $@;
+return $res;
+}
+
+
 sub get_qga_key {
 my ($conf, $key) = @_;
 return undef if !defined($conf->{agent});
@@ -4085,6 +4154,70 @@ sub config_to_command {
 }
 push @$machineFlags, "type=${machine_type_min}";
 
+# Memory Encryption
+my $memory_encryption = 
parse_memory_encryption($conf->{'memory_encryption'});
+
+# Die if bios is not ovmf
+if (
+   $memory_encryption->{'type'}
+   && $memory_encryption->{type} eq 'sev'
+   && $conf->{bios} ne 'ovmf'
+) {
+   die "SEV needs ovmf\n";
+}
+
+# AMD SEV
+if ($memory_encryption->{'type'} && $memory_encryption->{type} =~ 
/(sev|sev-snp)/) {
+   # Get reduced-phys-bits & cbitpos from QMP, if not set
+   if (
+   !$memory_encryption->{'reduced-phys-bits'}
+   || !$memory_encryption->{cbitpos}
+   ) {
+   my $fakevmid = -1;
+   my $qemu_cmd = get_command_for_arch($arch);
+   my $pidfile = PVE::QemuServer::Helpers:

[pve-devel] [PATCH qemu-server/docs v2 0/2] AMD SEV

2022-11-11 Thread Markus Frank
qemu-server:

v2:
* spelling of minimum
* !$conf->{bios} eq 'ovmf' changed to $conf->{bios} ne 'ovmf'

Markus Frank (1):
  QEMU AMD SEV enable

 PVE/QemuServer.pm | 133 ++
 1 file changed, 133 insertions(+)


docs:

v2:
* added more details for host & clients
* moved things from Limitations to Requirements
* changed order of text

Markus Frank (1):
  added Memory Encryption documentation

 qm.adoc | 113 
 1 file changed, 113 insertions(+)

-- 
2.30.2



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



Re: [pve-devel] [PATCH docs v2 2/2] added Memory Encryption documentation

2022-11-11 Thread Matthias Heiserer

Inline, I have some suggestions regarding wording and spelling

On 11.11.2022 15:27, Markus Frank wrote:

added AMD SEV documentation for "[PATCH qemu-server] QEMU AMD SEV
enable"

Signed-off-by: Markus Frank 
---
  qm.adoc | 113 
  1 file changed, 113 insertions(+)

diff --git a/qm.adoc b/qm.adoc
index e7d0c07..5ba43a2 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -598,6 +598,119 @@ systems.
  When allocating RAM to your VMs, a good rule of thumb is always to leave 1GB
  of RAM available to the host.
  
+[[qm_memory_encryption]]

+Memory Encryption
+~
+
+[[qm_memory_encryption_sev]]
+AMD SEV
+^^^
+
+Memory Encryption per VM using AES-128 Encryption and the AMD Secure Processor.
+See https://developer.amd.com/sev/[AMD SEV]
+
+*Host-Requirements:*
+
+* AMD EPYC/Ryzen PRO CPU
+* configured SEV BIOS settings on Host Machine
+* add "kvm_amd.sev=1" to kernel parameters if not enabled by default
+* add "mem_encrypt=on" to kernel parameters if you want encrypt memory on the

"to encrypt" or "encrypted"

+host (SME)
+see https://www.kernel.org/doc/Documentation/x86/amd-memory-encryption.txt
+* maybe increase SWIOTLB see https://github.com/AMDESE/AMDSEV#faq-4
+
+To check if SEV is enabled on Host-Machine search for `sev` in dmesg

I think "on the host" would be the best wording

+and print out the sev kernel parameter of kvm_amd:
+
+
+# dmesg | grep -i sev
+[...] ccp :45:00.1: sev enabled
+[...] ccp :45:00.1: SEV API: 
+[...] SEV supported:  ASIDs
+[...] SEV-ES supported:  ASIDs
+# cat /sys/module/kvm_amd/parameters/sev
+Y
+
+
+*Guest-VM-Requirements:*
Also here, we spell guest/host without hyphen. Maybe just drop the VM, 
i.e. "Guest Requirements"

+
+* edk2-OVMF
+* advisable to use Q35
+* The guest operating system inside the VM must contain SEV-support
+* if there are problems while booting (stops at blank/splash screen or "Guest 
has not
+initialized the display (yet)") try to add virtio-rng and/or set "freeze: 1"
+so that you wait a few seconds before you click on *Resume* to boot.

There's inconsistent capitalization

+
+*Limitations:*
+
+* Because the memory is encrypted the memory usage on host is always wrong
+* Operations that involve saving or restoring memory like snapshots
+& live migration do not work yet or are attackable
+https://github.com/PSPReverse/amd-sev-migration-attack
+* KVM is unsupported when running as an SEV guest
+* PCI passthrough is not supported
+
+Example Configuration:
+
+
+# qm set  -memory_encryption 
type=sev,cbitpos=47,policy=0x0001,reduced-phys-bits=1
+
+
+*SEV Parameters*

I'd use the same format as e.g. in "10.13.3. Options".
Regarding the following sentences: Consistently end them with a dot or 
without, don't mix.

+
+*type* defines the encryption technology ("type=" is not necessary):
+currently-supported: *sev*
+and in the future: sev-snp, mktme
+
+*reduced-phys-bios*, *cbitpos* and *policy* correspond to the variables with 
the
+same name in qemu.
+
+*reduced-phys-bios* and *cbitpos* are system specific and can be read out
+with QMP. If not set, qm starts a dummy-vm to read QMP
+for these variables out and saves them to config.
+
+*policy* can be calculated with
+https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf[AMD
 SEV API Specification Chapter 3]
+
+To use SEV-ES (CPU register encryption) the *policy* should be set
+somewhere between 0x4 and 0x7 or 0xC and 0xF, etc.
+(Bit-2 has to be set 1 (LSB 0 bit numbering))
+
+*Check if SEV is working on the Guest*
+
+Method 1 - dmesg:
+
+Output should look like this.
+
+
+# dmesg | grep -i sev
+AMD Memory Encryption Features active: SEV
+
+
+Method 2 - MSR 0xc0010131 (MSR_AMD64_SEV):
+
+Output should be 1.
+
+
+# apt install msr-tools
+# modprobe msr
+# rdmsr -a 0xc0010131
+1
+
+
+Links:
+
+* https://github.com/AMDESE/AMDSEV
+* https://www.qemu.org/docs/master/system/i386/amd-memory-encryption.html
+* https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf
+* https://documentation.suse.com/sles/15-SP1/html/SLES-amd-sev/index.html
In case you haven't done so, you could make a snapshot of these pages 
with archive.org or similarly, so there's a backup if they ever get removed.

+
+// Commented because cannot be tested without new EPYC-CPU
+// AMD SEV-SNP
+// ^^^
+// * SEV-SNP needs EPYC 7003 "Milan" processors.
+// * SEV-SNP should in Kernel 5.19:
+// 
https://www.phoronix.com/scan.php?page=news_item&px=AMD-SEV-SNP-Arrives-Linux-5.19
  
  [[qm_network_device]]

  Network Device




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



[pve-devel] [PATCH docs v2 2/2] asciidoc-pve: ignore anchor names in curly braces

2022-11-11 Thread Stefan Sterz
previously the scanner would detect some `onlineHelp` keys that are
set via CBind as anchor names. this would cause it to fail, as they
cannot be present anywhere in the documentation. no valid anchor name
can be wrapped in curly braces, as they need to be valid xml names.
hence it should be safe to just ignore all keys wrapped in curly
braces.

Signed-off-by: Stefan Sterz 
---
 asciidoc-pve.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/asciidoc-pve.in b/asciidoc-pve.in
index d638a38..c536371 100644
--- a/asciidoc-pve.in
+++ b/asciidoc-pve.in
@@ -465,7 +465,7 @@ sub scan_extjs_file {
 debug("scan-extjs $filename");
 
 while(defined(my $line = <$fh>)) {
-   if ($line =~ m/\s+onlineHelp:\s*[\'\"](.*?)[\'\"]/) {
+   if ($line =~ m/\s+onlineHelp:\s*[\'\"]([^{].*?[^}])[\'\"]/) {
my $blockid = $1;
my $link = $fileinfo->{blockid_target}->{default}->{$blockid};
die "undefined blockid '$blockid' ($filename, line $.)\n"
-- 
2.30.2



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



[pve-devel] [PATCH manager v2 1/1] fix #4328: ui: add widget toolkit to dependencies of OnlineHelpInfo.js

2022-11-11 Thread Stefan Sterz
previously the widget toolkit was not scanned when creating the
mapping between `onlineHelp` keys and pve-doc anchors. this could
lead to cases where help buttons didn't work because the necessary
mapping wasn't present in `OnlineHelpInfo.js`.

Signed-off-by: Stefan Sterz 
---
i took the liberty here to remove "/usr/bin/asciidoc-pve" as a
prerequisite of the affected make target. imo it isn't prerequisite
any more than "eslint" is for the `lint` target. it should already be
handled by the build dependencies anyway afaiu.

if that's not correct, i'll send a v3.

 debian/control| 1 +
 www/manager6/Makefile | 6 --
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/debian/control b/debian/control
index 35c9eee3..712d65e8 100644
--- a/debian/control
+++ b/debian/control
@@ -19,6 +19,7 @@ Build-Depends: debhelper (>= 12~),
libtemplate-perl,
libtest-mockmodule-perl,
lintian,
+   proxmox-widget-toolkit (>= 3.4-9),
pve-cluster,
pve-container,
pve-doc-generator (>= 7.0-4),
diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 5938c7f5..2802cbac 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -288,6 +288,8 @@ JSSRC=  
\
Workspace.js\
 # end of JSSRC list
 
+WIDGETKIT=/usr/share/javascript/proxmox-widget-toolkit/proxmoxlib.js
+
 all:
 
 .lint-incremental: ${JSSRC}
@@ -304,8 +306,8 @@ pvemanagerlib.js: .lint-incremental OnlineHelpInfo.js 
${JSSRC}
cat OnlineHelpInfo.js ${JSSRC} >$@.tmp
mv $@.tmp $@
 
-OnlineHelpInfo.js: /usr/bin/asciidoc-pve ${JSSRC}
-   /usr/bin/asciidoc-pve scan-extjs ${JSSRC} >$@.tmp
+OnlineHelpInfo.js: ${JSSRC} ${WIDGETKIT}
+   /usr/bin/asciidoc-pve scan-extjs $^ >$@.tmp
mv $@.tmp $@
 
 .PHONY: install
-- 
2.30.2



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



[pve-devel] [PATCH docs v2 1/2] pveum: add the "user_mgmt" reference to the documentation

2022-11-11 Thread Stefan Sterz
since this key was missing from the PVE documentation, the TFA ui's
help buttons didn't work as they relied on it.

Signed-off-by: Stefan Sterz 
---
 pveum.adoc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pveum.adoc b/pveum.adoc
index cbd553a..52de14d 100644
--- a/pveum.adoc
+++ b/pveum.adoc
@@ -1,4 +1,7 @@
 [[chapter_user_management]]
+
+[[user_mgmt]]
+
 ifdef::manvolnum[]
 pveum(1)
 
-- 
2.30.2



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



[pve-devel] [PATCH docs v2 0/2] pve fix help buttons by accounting for keys in widget toolkit

2022-11-11 Thread Stefan Sterz
this patch series fixes an issue where `onlineHelp` keys, that were
only used by components in the widget toolkit, were not properly
mapped to the corresponding anchors in the docs. this lead to
affected help buttons only showing an error message.

this series first fixes `pve-docs` by adding keys that were already
used by the widget toolkit's components. it also makes `asciidoc-pve`
a bit less aggressiv in order to avoid keys that are set by cbind.
then it adds the toolkit as a build dependency to `pve-manager` and
scans the toolkit for keys there when building the front-end.


changes from v1 @ Thomas Lamprecht:

* scan the widget toolkit too and not just components in
  `pve-manager`


Stefan Sterz (2):
  pveum: add the "user_mgmt" reference to the documentation
  asciidoc-pve: ignore anchor names in curly braces

 asciidoc-pve.in | 2 +-
 pveum.adoc  | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

Stefan Sterz (1):
  fix #4328: ui: add widget toolkit to dependencies of OnlineHelpInfo.js

 debian/control| 1 +
 www/manager6/Makefile | 6 --
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.30.2



___
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] fix #4201: delete cloud-init disk on rollback

2022-11-11 Thread Stefan Hanreich

Test Setup:
I created a new VM, without any Cloud-Init drive, and immediately 
created a snapshot. Then I setup a Cloud-Init drive according to the PVE 
documentation via CLI. I created another snapshot of this state with a 
Cloud-Init drive.


What I tested:
- Rolling back to the older snapshot without Cloud-Init drive, it got 
successfully removed.
- Rolling back to the newer snapshot that includes a Cloud-Init drive, 
it successfully showed up afterwards.


I also tested both of those cases with Cloud-Init drives created from 
the Web UI, in case there are any differences.


Some additional tests including normal CD-ROM drives I did as well - 
just in case:
- Rolled back from a snapshot with only a Cloud-Init drive to a snapshot 
that contains only a CD-ROM drive, the CD-ROM drive showed up afterwards 
and removed the Cloud-Init drive successfully
- Rolled back from a snapshot with only a CD-ROM drive to a state that 
contains only a Cloud-Init drive, the Cloud-Init drive showed up 
afterwards and removed the CD-ROM drive successfully



Code LGTM


Reviewed-by: Stefan Hanreich 
Tested-by: Stefan Hanreich 

On 10/20/22 17:22, Mira Limbeck wrote:

If the config doesn't contain the cloud-init disk anymore after the
rollback, we have to clean it up since otherwise no further disk can be
attached unless the one still existing on the storage is deleted.

Signed-off-by: Mira Limbeck 
---
v2:
  - chose the add_unused_volume way as @fiona recommended, the
implementation is a lot cleaner, but contains a cloudinit regex

  - removed the 2nd patch for reusing already existing disks when
adding a cloudinit disk

  PVE/QemuConfig.pm | 17 +++--
  1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 482c7ab..9fb8e9f 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -498,7 +498,7 @@ sub __snapshot_rollback_get_unused {
  $class->foreach_volume($conf, sub {
my ($vs, $volume) = @_;
  
-	return if PVE::QemuServer::drive_is_cdrom($volume);

+   return if PVE::QemuServer::drive_is_cdrom($volume, 1);
  
  	my $found = 0;

my $volid = $volume->{file};
@@ -507,7 +507,7 @@ sub __snapshot_rollback_get_unused {
my ($ds, $drive) = @_;
  
  	return if $found;

-   return if PVE::QemuServer::drive_is_cdrom($drive);
+   return if PVE::QemuServer::drive_is_cdrom($drive, 1);
  
  	$found = 1

if ($drive->{file} && $drive->{file} eq $volid);
@@ -519,6 +519,19 @@ sub __snapshot_rollback_get_unused {
  return $unused;
  }
  
+sub add_unused_volume {

+my ($class, $config, $volid) = @_;
+
+if ($volid =~ m/vm-\d+-cloudinit/) {
+   print "found unused cloudinit disk '$volid', removing it\n";
+   my $storecfg = PVE::Storage::config();
+   PVE::Storage::vdisk_free($storecfg, $volid);
+   return undef;
+} else {
+return $class->SUPER::add_unused_volume($config, $volid);
+}
+}
+
  # END implemented abstract methods from PVE::AbstractConfig
  
  sub has_cloudinit {



___
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] fix #4201: delete cloud-init disk on rollback

2022-11-11 Thread Mira Limbeck

On 11/11/22 16:18, Stefan Hanreich wrote:

Test Setup:
I created a new VM, without any Cloud-Init drive, and immediately 
created a snapshot. Then I setup a Cloud-Init drive according to the 
PVE documentation via CLI. I created another snapshot of this state 
with a Cloud-Init drive.


What I tested:
- Rolling back to the older snapshot without Cloud-Init drive, it got 
successfully removed.
- Rolling back to the newer snapshot that includes a Cloud-Init drive, 
it successfully showed up afterwards.


I also tested both of those cases with Cloud-Init drives created from 
the Web UI, in case there are any differences.


Some additional tests including normal CD-ROM drives I did as well - 
just in case:
- Rolled back from a snapshot with only a Cloud-Init drive to a 
snapshot that contains only a CD-ROM drive, the CD-ROM drive showed up 
afterwards and removed the Cloud-Init drive successfully
- Rolled back from a snapshot with only a CD-ROM drive to a state that 
contains only a Cloud-Init drive, the Cloud-Init drive showed up 
afterwards and removed the CD-ROM drive successfully



Code LGTM


Reviewed-by: Stefan Hanreich 
Tested-by: Stefan Hanreich 

Thanks for testing! Since it requires a rebase, I'll send a rebased v3 
with your R-b and T-b tags.



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



[pve-devel] [PATCH v3 qemu-server] fix #4201: delete cloud-init disk on rollback

2022-11-11 Thread Mira Limbeck
If the config doesn't contain the cloud-init disk anymore after the
rollback, we have to clean it up since otherwise no further disk can be
attached unless the one still existing on the storage is deleted.

Signed-off-by: Mira Limbeck 
Reviewed-by: Stefan Hanreich 
Tested-by: Stefan Hanreich 
---
v3:
 - rebased on top of master
 - added R-b/T-b tags from v2
v2:
 - chose the add_unused_volume way as @fiona recommended, the
   implementation is a lot cleaner, but contains a cloudinit regex

 - removed the 2nd patch for reusing already existing disks when
   adding a cloudinit disk

 PVE/QemuConfig.pm | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index 0939538..051382c 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -498,7 +498,7 @@ sub __snapshot_rollback_get_unused {
 $class->foreach_volume($conf, sub {
my ($vs, $volume) = @_;
 
-   return if PVE::QemuServer::drive_is_cdrom($volume);
+   return if PVE::QemuServer::drive_is_cdrom($volume, 1);
 
my $found = 0;
my $volid = $volume->{file};
@@ -507,7 +507,7 @@ sub __snapshot_rollback_get_unused {
my ($ds, $drive) = @_;
 
return if $found;
-   return if PVE::QemuServer::drive_is_cdrom($drive);
+   return if PVE::QemuServer::drive_is_cdrom($drive, 1);
 
$found = 1
if ($drive->{file} && $drive->{file} eq $volid);
@@ -519,6 +519,19 @@ sub __snapshot_rollback_get_unused {
 return $unused;
 }
 
+sub add_unused_volume {
+my ($class, $config, $volid) = @_;
+
+if ($volid =~ m/vm-\d+-cloudinit/) {
+   print "found unused cloudinit disk '$volid', removing it\n";
+   my $storecfg = PVE::Storage::config();
+   PVE::Storage::vdisk_free($storecfg, $volid);
+   return undef;
+} else {
+return $class->SUPER::add_unused_volume($config, $volid);
+}
+}
+
 sub load_current_config {
 my ($class, $vmid, $current) = @_;
 
-- 
2.30.2


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



Re: [pve-devel] [PATCH docs v2 2/2] asciidoc-pve: ignore anchor names in curly braces

2022-11-11 Thread Thomas Lamprecht
Am 11/11/2022 um 16:05 schrieb Stefan Sterz:
> previously the scanner would detect some `onlineHelp` keys that are
> set via CBind as anchor names. this would cause it to fail, as they
> cannot be present anywhere in the documentation. no valid anchor name
> can be wrapped in curly braces, as they need to be valid xml names.
> hence it should be safe to just ignore all keys wrapped in curly
> braces.
> 
> Signed-off-by: Stefan Sterz 
> ---
>  asciidoc-pve.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/asciidoc-pve.in b/asciidoc-pve.in
> index d638a38..c536371 100644
> --- a/asciidoc-pve.in
> +++ b/asciidoc-pve.in
> @@ -465,7 +465,7 @@ sub scan_extjs_file {
>  debug("scan-extjs $filename");
>  
>  while(defined(my $line = <$fh>)) {
> - if ($line =~ m/\s+onlineHelp:\s*[\'\"](.*?)[\'\"]/) {
> + if ($line =~ m/\s+onlineHelp:\s*[\'\"]([^{].*?[^}])[\'\"]/) {

IIUC this indirectly raised the minimum length of references to two characters,
not a deal breaker IMO as I don't really expect two characters to be used 
anytime
soon (maybe with unicode 🤔🧠💭 x)), but maybe hint it in the commit message.

>   my $blockid = $1;
>   my $link = $fileinfo->{blockid_target}->{default}->{$blockid};
>   die "undefined blockid '$blockid' ($filename, line $.)\n"



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


[pve-devel] applied: [PATCH docs v2 1/2] pveum: add the "user_mgmt" reference to the documentation

2022-11-11 Thread Thomas Lamprecht
Am 11/11/2022 um 16:05 schrieb Stefan Sterz:
> since this key was missing from the PVE documentation, the TFA ui's
> help buttons didn't work as they relied on it.
> 
> Signed-off-by: Stefan Sterz 
> ---
>  pveum.adoc | 3 +++
>  1 file changed, 3 insertions(+)
> 
>

applied, thanks!

btw. for my reply on patch 2 I can just amend the commit message myself,
so no need for a v3; just confirm if I indeed understood correctly.


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



[pve-devel] applied: [PATCH v3 qemu-server] fix #4201: delete cloud-init disk on rollback

2022-11-11 Thread Thomas Lamprecht
Am 11/11/2022 um 16:46 schrieb Mira Limbeck:
> If the config doesn't contain the cloud-init disk anymore after the
> rollback, we have to clean it up since otherwise no further disk can be
> attached unless the one still existing on the storage is deleted.
> 
> Signed-off-by: Mira Limbeck 
> Reviewed-by: Stefan Hanreich 
> Tested-by: Stefan Hanreich 
> ---
> v3:
>  - rebased on top of master
>  - added R-b/T-b tags from v2
> v2:
>  - chose the add_unused_volume way as @fiona recommended, the
>implementation is a lot cleaner, but contains a cloudinit regex
> 
>  - removed the 2nd patch for reusing already existing disks when
>adding a cloudinit disk
> 
>  PVE/QemuConfig.pm | 17 +++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
>

applied, thanks to both of you!


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