[pve-devel] [PATCH qemu-server] fix #3581: pass size via argument for memory-backend-ram qmp call

[pve-devel] [PATCH qemu-server] fix #3581: pass size via argument for memory-backend-ram qmp call

[Constantin Herold]

Signed-off-by: Constantin Herold <proxmox8914@herold.me>
---
 PVE/QemuServer/Memory.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer/Memory.pm b/PVE/QemuServer/Memory.pm
index f3e15f1..e34727f 100644
--- a/PVE/QemuServer/Memory.pm
+++ b/PVE/QemuServer/Memory.pm
@@ -155,7 +155,7 @@ sub qemu_memory_hotplug {
 		    eval { hugepages_update_locked($code); };
 
 		} else {
-		    eval { mon_cmd($vmid, "object-add", 'qom-type' => "memory-backend-ram", id => "mem-$name", props => { size => int($dimm_size*1024*1024) } ) };
+		    eval { mon_cmd($vmid, "object-add", 'qom-type' => "memory-backend-ram", id => "mem-$name", size => int($dimm_size*1024*1024) ) };
 		}
 
 		if (my $err = $@) {
-- 
2.23.0

Re: [pve-devel] [PATCH qemu-server] fix #3581: pass size via argument for memory-backend-ram qmp call

[Wolfgang Bumiller]

Hi,

On Thu, Aug 12, 2021 at 08:16:42PM +0200, Constantin Herold wrote:
> Signed-off-by: Constantin Herold <proxmox8914@herold.me>
> ---
>  PVE/QemuServer/Memory.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer/Memory.pm b/PVE/QemuServer/Memory.pm
> index f3e15f1..e34727f 100644
> --- a/PVE/QemuServer/Memory.pm
> +++ b/PVE/QemuServer/Memory.pm
> @@ -155,7 +155,7 @@ sub qemu_memory_hotplug {
>  		    eval { hugepages_update_locked($code); };
>  
>  		} else {
> -		    eval { mon_cmd($vmid, "object-add", 'qom-type' => "memory-backend-ram", id => "mem-$name", props => { size => int($dimm_size*1024*1024) } ) };
> +		    eval { mon_cmd($vmid, "object-add", 'qom-type' => "memory-backend-ram", id => "mem-$name", size => int($dimm_size*1024*1024) ) };

Looks fine.

There's another `object-add` command a few lines higher up which also
still uses `props` and needs to be changed in the same way.

Care to include it in a v2?
It would also be nice to include the cause in the commit message,
something like:

    `props` was deprecated with qemu 5.0 and removed in qemu 6.0.
    The commands are now generally flattened.

Re: [pve-devel] [PATCH qemu-server] fix #3581: pass size via argument for memory-backend-ram qmp call

[Constantin Herold]

Sure no problem.

In that case the patch will probably break memory hotplug for any VM's 
tagged to Qemu <=4.2

Since VM's by default are tagged to >= 5.x that shouldn't be a issue though.


Am 17.08.2021 um 15:21 schrieb Wolfgang Bumiller:
> Hi,
>
> On Thu, Aug 12, 2021 at 08:16:42PM +0200, Constantin Herold wrote:
>> Signed-off-by: Constantin Herold <proxmox8914@herold.me>
>> ---
>>   PVE/QemuServer/Memory.pm | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/PVE/QemuServer/Memory.pm b/PVE/QemuServer/Memory.pm
>> index f3e15f1..e34727f 100644
>> --- a/PVE/QemuServer/Memory.pm
>> +++ b/PVE/QemuServer/Memory.pm
>> @@ -155,7 +155,7 @@ sub qemu_memory_hotplug {
>>   		    eval { hugepages_update_locked($code); };
>>   
>>   		} else {
>> -		    eval { mon_cmd($vmid, "object-add", 'qom-type' => "memory-backend-ram", id => "mem-$name", props => { size => int($dimm_size*1024*1024) } ) };
>> +		    eval { mon_cmd($vmid, "object-add", 'qom-type' => "memory-backend-ram", id => "mem-$name", size => int($dimm_size*1024*1024) ) };
> Looks fine.
>
> There's another `object-add` command a few lines higher up which also
> still uses `props` and needs to be changed in the same way.
>
> Care to include it in a v2?
> It would also be nice to include the cause in the commit message,
> something like:
>
>      `props` was deprecated with qemu 5.0 and removed in qemu 6.0.
>      The commands are now generally flattened.
>

Re: [pve-devel] [PATCH qemu-server] fix #3581: pass size via argument for memory-backend-ram qmp call

[Wolfgang Bumiller]

On Tue, Aug 17, 2021 at 05:32:00PM +0200, Constantin Herold wrote:
> Sure no problem.
> 
> In that case the patch will probably break memory hotplug for any VM's
> tagged to Qemu <=4.2

I think it's okay as it's just the command interface to the running qemu
process.

* pve6 has qemu 5.2 (supports both variants)
* pve7 has qemu 6.0 (supports only the new one)

For clusters we recommend migrating running machines to different nodes
when upgrading, which means they'll be running on pve6+qemu5.2 and then get
migrated to pve7-qemu6, afterwards hotplug should be working once this
patch lands.

For in-place major-upgrades with machines running this doesn't really
*break* a running machine, just prevents hotplug until it was either
stop-started or a live-snapshot&rollback is performed.

If you want to go the extra mile, the running qemu version could be
queried, but I don't think we need to insist on doing this, as it just
adds code complexity for backward-compatibility across a major-version
boundary.