[v2] qemu: support renaming domains with snapshots/checkpoints

[PATCH v2 00/10] qemu: support renaming domains with snapshots/checkpoints

Posted by Nikolay Shirokovskiy 3 years, 5 months ago

This is basically just rebase of [1] as it was not get any attention at that
time.

[1] [PATCH 0/8] qemu: support renaming domains with snapshots/checkpoints
https://www.redhat.com/archives/libvir-list/2020-March/msg00018.html

Nikolay Shirokovskiy (10):
  qemu: qemuDomainRenameCallback: fix sending false undefined event
  qemu: rename: send events only on success
  qemu: rename: return instead of goto if no cleanup required
  qemu: remove duplicate code for removing remnant files
  qemu: rename: support renaming snapshots directory
  qemu: rename: support renaming checkpoints directory
  qemu: update name on reverting from snapshot
  qemu: rename: remove snapshot/checkpoint restriction
  qemu: qemuDomainDefineXMLFlags: move cleanup logic to cleanup section
  qemu: remove possible garbage left from previous rename/undefine

 src/qemu/qemu_checkpoint.c |   2 +-
 src/qemu/qemu_checkpoint.h |   6 ++
 src/qemu/qemu_domain.c     |  43 ++++++++++++
 src/qemu/qemu_domain.h     |   5 ++
 src/qemu/qemu_driver.c     | 158 ++++++++++++++++++++++++++-------------------
 src/qemu/qemu_migration.c  |   3 +
 src/qemu/qemu_snapshot.c   |  10 +++
 7 files changed, 160 insertions(+), 67 deletions(-)

--
1.8.3.1

Re: [PATCH v2 00/10] qemu: support renaming domains with snapshots/checkpoints

Posted by Daniel Henrique Barboza 3 years, 5 months ago


On 11/3/20 8:59 AM, Nikolay Shirokovskiy wrote:
> This is basically just rebase of [1] as it was not get any attention at that
> time.
> 
> [1] [PATCH 0/8] qemu: support renaming domains with snapshots/checkpoints
> https://www.redhat.com/archives/libvir-list/2020-March/msg00018.html

Code LGTM:

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


Shouldn't you add some test cases for this new behavior though? I'm a bit
nervous with pushing this upstream without any coverage.


Thanks,


DHB

> 
> Nikolay Shirokovskiy (10):
>    qemu: qemuDomainRenameCallback: fix sending false undefined event
>    qemu: rename: send events only on success
>    qemu: rename: return instead of goto if no cleanup required
>    qemu: remove duplicate code for removing remnant files
>    qemu: rename: support renaming snapshots directory
>    qemu: rename: support renaming checkpoints directory
>    qemu: update name on reverting from snapshot
>    qemu: rename: remove snapshot/checkpoint restriction
>    qemu: qemuDomainDefineXMLFlags: move cleanup logic to cleanup section
>    qemu: remove possible garbage left from previous rename/undefine
> 
>   src/qemu/qemu_checkpoint.c |   2 +-
>   src/qemu/qemu_checkpoint.h |   6 ++
>   src/qemu/qemu_domain.c     |  43 ++++++++++++
>   src/qemu/qemu_domain.h     |   5 ++
>   src/qemu/qemu_driver.c     | 158 ++++++++++++++++++++++++++-------------------
>   src/qemu/qemu_migration.c  |   3 +
>   src/qemu/qemu_snapshot.c   |  10 +++
>   7 files changed, 160 insertions(+), 67 deletions(-)
> 
> --
> 1.8.3.1
>

Re: [PATCH v2 00/10] qemu: support renaming domains with snapshots/checkpoints

Posted by Nikolay Shirokovskiy 3 years, 5 months ago

On 09.11.2020 16:51, Daniel Henrique Barboza wrote:
> 
> 
> On 11/3/20 8:59 AM, Nikolay Shirokovskiy wrote:
>> This is basically just rebase of [1] as it was not get any attention at that
>> time.
>>
>> [1] [PATCH 0/8] qemu: support renaming domains with snapshots/checkpoints
>> https://www.redhat.com/archives/libvir-list/2020-March/msg00018.html
> 
> Code LGTM:
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> 
> Shouldn't you add some test cases for this new behavior though? I'm a bit
> nervous with pushing this upstream without any coverage.
> 
> 

So basically we need to test how qemuDomainRenameCallback works. Currently
there is no test for this function or virDomainRename. I spent some time
looking at existing tests that need to mock driver/vm objects and it looks like
it requires a good deal of effort in order to prepare the test in this case.
At the same time those tests has many inputs so it looks like worth heavy
preparation. In case of rename the code we test does not depend greatly on
inputs so may be it does not worth adding such test given heavy preparation we
have to do.

Of course I give the patch series some manual testing.

Nikolay

Re: [PATCH v2 00/10] qemu: support renaming domains with snapshots/checkpoints

Posted by Nikolay Shirokovskiy 3 years, 5 months ago


On 10.11.2020 10:58, Nikolay Shirokovskiy wrote:
> 
> 
> On 09.11.2020 16:51, Daniel Henrique Barboza wrote:
>>
>>
>> On 11/3/20 8:59 AM, Nikolay Shirokovskiy wrote:
>>> This is basically just rebase of [1] as it was not get any attention at that
>>> time.
>>>
>>> [1] [PATCH 0/8] qemu: support renaming domains with snapshots/checkpoints
>>> https://www.redhat.com/archives/libvir-list/2020-March/msg00018.html
>>
>> Code LGTM:
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>
>>
>> Shouldn't you add some test cases for this new behavior though? I'm a bit
>> nervous with pushing this upstream without any coverage.
>>
>>
> 
> So basically we need to test how qemuDomainRenameCallback works. Currently
> there is no test for this function or virDomainRename. I spent some time
> looking at existing tests that need to mock driver/vm objects and it looks like
> it requires a good deal of effort in order to prepare the test in this case.
> At the same time those tests has many inputs so it looks like worth heavy
> preparation. In case of rename the code we test does not depend greatly on
> inputs so may be it does not worth adding such test given heavy preparation we
> have to do.
> 
> Of course I give the patch series some manual testing.
> 

Thanx for the review!
Pushed with next hunk squashed into the last patch:

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c                  
index c831ae6..fef0be6 100644                                                       
--- a/src/qemu/qemu_migration.c                                                     
+++ b/src/qemu/qemu_migration.c                                                     
@@ -5137,7 +5137,7 @@ qemuMigrationDstPersist(virQEMUDriverPtr driver,              
                                                priv->qemuCaps)))                   
         goto error;                                                                
                                                                                    
-    if (!oldDef && qemuDomainNamePathsCleanup(cfg, vmdef->name, false) < 0)        
+    if (!oldPersist && qemuDomainNamePathsCleanup(cfg, vmdef->name, false) < 0)    
         goto error;                                                                
                                                                                    
     if (virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir) < 0 &&      

Nikolay