| Message ID | 20240123064219.40514-1-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make 'uri' optional for migrate QAPI | expand |
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
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
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?
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
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
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
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
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
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,
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
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' ] },
'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(-)