[PATCH] Make 'uri' optional for migrate QAPI

[PATCH] Make 'uri' optional for migrate QAPI

[Het Gala]

'uri' argument should be optional, as 'uri' and 'channels'
arguments are mutally exclusive in nature.

Fixes: 074dbce5fcce (migration: New migrate and
migrate-incoming argument 'channels')
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 qapi/migration.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index eb2f883513..197d3faa43 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1757,7 +1757,7 @@
 #
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str',
+  'data': {'*uri': 'str',
            '*channels': [ 'MigrationChannel' ],
            '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
            '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
-- 
2.22.3

Re: [PATCH] Make 'uri' optional for migrate QAPI

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]


On 23/01/24 12:12 pm, Het Gala wrote:
> 'uri' argument should be optional, as 'uri' and 'channels'
> arguments are mutally exclusive in nature.

Also verified by running the qemu CI/CD pipeline. ref: 
https://gitlab.com/galahet/Qemu/-/pipelines/1147048455

> Fixes: 074dbce5fcce (migration: New migrate and
> migrate-incoming argument 'channels')
> Signed-off-by: Het Gala<het.gala@nutanix.com>
> ---
>   qapi/migration.json | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index eb2f883513..197d3faa43 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1757,7 +1757,7 @@
>   #
>   ##
>   { 'command': 'migrate',
> -  'data': {'uri': 'str',
> +  'data': {'*uri': 'str',
>              '*channels': [ 'MigrationChannel' ],
>              '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
>              '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },

Regards,

Het Gala

[-- Attachment #2: Type: text/html, Size: 1688 bytes --]

Re: [PATCH] Make 'uri' optional for migrate QAPI

[Daniel P. Berrangé]

On Tue, Jan 23, 2024 at 06:42:19AM +0000, Het Gala wrote:
> 'uri' argument should be optional, as 'uri' and 'channels'
> arguments are mutally exclusive in nature.
> 
> Fixes: 074dbce5fcce (migration: New migrate and
> migrate-incoming argument 'channels')
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  qapi/migration.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index eb2f883513..197d3faa43 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1757,7 +1757,7 @@
>  #
>  ##
>  { 'command': 'migrate',
> -  'data': {'uri': 'str',
> +  'data': {'*uri': 'str',
>             '*channels': [ 'MigrationChannel' ],
>             '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
>             '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },

Hmm, this mistake shows a lack of coverage in migration-test.c for
the 'channels' argument. I thought the original series adding 'channels'
included the tests for this. Either way, this needs to come with test
coverage for use of 'channels', with 'uri' omitted.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] Make 'uri' optional for migrate QAPI

[Peter Xu]

On Tue, Jan 23, 2024 at 08:21:55AM +0000, Daniel P. Berrangé wrote:
> On Tue, Jan 23, 2024 at 06:42:19AM +0000, Het Gala wrote:
> > 'uri' argument should be optional, as 'uri' and 'channels'
> > arguments are mutally exclusive in nature.
> > 
> > Fixes: 074dbce5fcce (migration: New migrate and
> > migrate-incoming argument 'channels')
> > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > ---
> >  qapi/migration.json | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index eb2f883513..197d3faa43 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1757,7 +1757,7 @@
> >  #
> >  ##
> >  { 'command': 'migrate',
> > -  'data': {'uri': 'str',
> > +  'data': {'*uri': 'str',
> >             '*channels': [ 'MigrationChannel' ],
> >             '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
> >             '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
> 
> Hmm, this mistake shows a lack of coverage in migration-test.c for
> the 'channels' argument. I thought the original series adding 'channels'
> included the tests for this. Either way, this needs to come with test
> coverage for use of 'channels', with 'uri' omitted.

Agreed.  Het, do you plan to provide a test case?

-- 
Peter Xu

Re: [PATCH] Make 'uri' optional for migrate QAPI

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]


On 24/01/24 7:13 am, Peter Xu wrote:
> On Tue, Jan 23, 2024 at 08:21:55AM +0000, Daniel P. Berrangé wrote:
>> On Tue, Jan 23, 2024 at 06:42:19AM +0000, Het Gala wrote:
>>> 'uri' argument should be optional, as 'uri' and 'channels'
>>> arguments are mutally exclusive in nature.
>>>
>>> Fixes: 074dbce5fcce (migration: New migrate and
>>> migrate-incoming argument 'channels')
>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>> ---
>>>   qapi/migration.json | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index eb2f883513..197d3faa43 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1757,7 +1757,7 @@
>>>   #
>>>   ##
>>>   { 'command': 'migrate',
>>> -  'data': {'uri': 'str',
>>> +  'data': {'*uri': 'str',
>>>              '*channels': [ 'MigrationChannel' ],
>>>              '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
>>>              '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
>> Hmm, this mistake shows a lack of coverage in migration-test.c for
>> the 'channels' argument. I thought the original series adding 'channels'
>> included the tests for this. Either way, this needs to come with test
>> coverage for use of 'channels', with 'uri' omitted.
> Agreed.  Het, do you plan to provide a test case?

Yes, will provide a test case patch for 'channels' soon.

Regards,

Het Gala

[-- Attachment #2: Type: text/html, Size: 2305 bytes --]

Re: [PATCH] Make 'uri' optional for migrate QAPI

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

On 24/01/24 10:03 am, Het Gala wrote:
>
>
> On 24/01/24 7:13 am, Peter Xu wrote:
>> On Tue, Jan 23, 2024 at 08:21:55AM +0000, Daniel P. Berrangé wrote:
>>> On Tue, Jan 23, 2024 at 06:42:19AM +0000, Het Gala wrote:
>>>> 'uri' argument should be optional, as 'uri' and 'channels'
>>>> arguments are mutally exclusive in nature.
>>>>
>>>> Fixes: 074dbce5fcce (migration: New migrate and
>>>> migrate-incoming argument 'channels')
>>>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>>>> ---
>>>>   qapi/migration.json | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index eb2f883513..197d3faa43 100644
>>>> --- a/qapi/migration.json
>>>> +++ b/qapi/migration.json
>>>> @@ -1757,7 +1757,7 @@
>>>>   #
>>>>   ##
>>>>   { 'command': 'migrate',
>>>> -  'data': {'uri': 'str',
>>>> +  'data': {'*uri': 'str',
>>>>              '*channels': [ 'MigrationChannel' ],
>>>>              '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
>>>>              '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
>>> Hmm, this mistake shows a lack of coverage in migration-test.c for
>>> the 'channels' argument. I thought the original series adding 'channels'
>>> included the tests for this. Either way, this needs to come with test
>>> coverage for use of 'channels', with 'uri' omitted.
>> Agreed.  Het, do you plan to provide a test case?
> Yes, will provide a test case patch for 'channels' soon.

Hi everyone, I was trying to wrap around on how to write a migration test or to mock migration.
I see there are a couple of migration tests already written, but most of them focuses on just getting the uri and parsing uri to start the migration.
I have a couple of questions for starters like me who is attempting to write test cases for the first time:

1. Do I need to make a whole new test or just edit one of the tests that is using uri, and instead send in 'MigrateChannel' struct and parse the necessary information out of it ?

2. Do I need to add tests for unix, fd too with the modified syntax ?

3. Do I also need to add test to ensure - uri and channels both cannot be used simultaneously ? (based on the above patch)

4. Is there updated document in Qemu to follow latest practices on how to write migration tests?

Regards,

Het Gala

[-- Attachment #2: Type: text/html, Size: 3555 bytes --]

Re: [PATCH] Make 'uri' optional for migrate QAPI

[Daniel P. Berrangé]

On Fri, Jan 26, 2024 at 07:40:12PM +0530, Het Gala wrote:
> Hi everyone, I was trying to wrap around on how to write a migration test or to mock migration.
> I see there are a couple of migration tests already written, but most of them focuses on just getting the uri and parsing uri to start the migration.
> I have a couple of questions for starters like me who is attempting to write test cases for the first time:
> 
> 1. Do I need to make a whole new test or just edit one of the tests that is using uri, and instead send in 'MigrateChannel' struct and parse the necessary information out of it ?

I think this option is best. We have two code paths - 'uri' and
'MigrateChannel', we we just need coverage of that new path.
So modifying some of the existing test cases to use MigrateChannel
gives us that coverage without harming existing coverage. This is
more time efficient than adding extra tests.

> 2. Do I need to add tests for unix, fd too with the modified syntax ?

I don't think so. When using the legacy 'uri' syntax (which all tests
already do), we convert to MigrateChannel internally, then the rest
of migration uses the MigrateChannel.  IOW, we already have coverage
of unix/fd/etc.

All we're lacking is validation that the very first entrypoint allows
MigrateChannel. We can prove that with a single test that uses
MigrateChannel

> 3. Do I also need to add test to ensure - uri and channels both
> cannot be used simultaneously ? (based on the above patch)

Yes, its a worthwhile sanity check. There are a few intentional
failure tests in migrate-test.c.

> 4. Is there updated document in Qemu to follow latest practices on how to write migration tests?

Not that I know of

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] Make 'uri' optional for migrate QAPI

[Michael Tokarev]

23.01.2024 09:42, Het Gala:
> 'uri' argument should be optional, as 'uri' and 'channels'
> arguments are mutally exclusive in nature.
> 
> Fixes: 074dbce5fcce (migration: New migrate and
> migrate-incoming argument 'channels')
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>   qapi/migration.json | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index eb2f883513..197d3faa43 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1757,7 +1757,7 @@
>   #
>   ##
>   { 'command': 'migrate',
> -  'data': {'uri': 'str',
> +  'data': {'*uri': 'str',
>              '*channels': [ 'MigrationChannel' ],
>              '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
>              '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },

This seems like a stable material too, - please let me know if it is not.

Thanks,

/mjt

Re: [PATCH] Make 'uri' optional for migrate QAPI

[Fabiano Rosas]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 23.01.2024 09:42, Het Gala:
>> 'uri' argument should be optional, as 'uri' and 'channels'
>> arguments are mutally exclusive in nature.
>> 
>> Fixes: 074dbce5fcce (migration: New migrate and
>> migrate-incoming argument 'channels')
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>>   qapi/migration.json | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index eb2f883513..197d3faa43 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1757,7 +1757,7 @@
>>   #
>>   ##
>>   { 'command': 'migrate',
>> -  'data': {'uri': 'str',
>> +  'data': {'*uri': 'str',
>>              '*channels': [ 'MigrationChannel' ],
>>              '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
>>              '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
>
> This seems like a stable material too, - please let me know if it is not.
>

Yes, those API changes went into 8.2.

Thanks

Re: [PATCH] Make 'uri' optional for migrate QAPI

[Peter Xu]

On Mon, Jan 29, 2024 at 11:30:53PM +0300, Michael Tokarev wrote:
> 23.01.2024 09:42, Het Gala:
> > 'uri' argument should be optional, as 'uri' and 'channels'
> > arguments are mutally exclusive in nature.
> > 
> > Fixes: 074dbce5fcce (migration: New migrate and
> > migrate-incoming argument 'channels')
> > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > ---
> >   qapi/migration.json | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index eb2f883513..197d3faa43 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1757,7 +1757,7 @@
> >   #
> >   ##
> >   { 'command': 'migrate',
> > -  'data': {'uri': 'str',
> > +  'data': {'*uri': 'str',
> >              '*channels': [ 'MigrationChannel' ],
> >              '*blk': { 'type': 'bool', 'features': [ 'deprecated' ] },
> >              '*inc': { 'type': 'bool', 'features': [ 'deprecated' ] },
> 
> This seems like a stable material too, - please let me know if it is not.

Yes it is. I used to be more careful on copying stable at least in the
commit message when I post patches, but forgot to do so when start picking
up..

Note that it's already merged in master 57fd4b4e10, while there should be a
test case to land later when ready (which won't need to copy stable).

Since at it, just to double check how stable works for us: as long as the
commit has "Cc: qemu-stable@nongnu.org" when merge should work, even
without the need to reply the patch copying stable list, am I right?

Thanks,

-- 
Peter Xu

Re: [PATCH] Make 'uri' optional for migrate QAPI

[Michael Tokarev]

30.01.2024 04:35, Peter Xu:
..
>> This seems like a stable material too, - please let me know if it is not.
> 
> Yes it is. I used to be more careful on copying stable at least in the
> commit message when I post patches, but forgot to do so when start picking
> up..
> 
> Note that it's already merged in master 57fd4b4e10, while there should be a
> test case to land later when ready (which won't need to copy stable).

I already picked it up from master yesterday, --
https://gitlab.com/mjt0k/qemu/-/commits/staging-8.2/ . Sometimes I pick up
the test cases too, especially (not in this case) when the actual change
makes existing tests to fail.  And yes, I've seen subsequent discussion
in the original thread (to which I replied) about adding the test case.

> Since at it, just to double check how stable works for us: as long as the
> commit has "Cc: qemu-stable@nongnu.org" when merge should work, even
> without the need to reply the patch copying stable list, am I right?

Well, basically, yes.  When you send a pull request with a patch which
has Cc: qemu-stable@, it will be Cc'd there by git send-email already.
Also, sometimes I scan all commits applied to master grepping for
Fixes:/Resolves: and similar patterns, and check if the change found
this way is relevant for stable or not, - but obviously this is much
less reliable (compared with when the actual patch author who understands
the situation much better marks the change explicitly) and often
requires extra confirmation round-trip.  Cc'ing qemu-stable@ in the
middle of discussion in qemu-devel@ will do the trick too.  The key
here is to mark the changes *somehow*.

My own work here is based on my local qemu-stable mailbox, plus the
commits scanning I mention above.

Thanks,

/mjt