| Message ID | 20240410111541.188504-2-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tests/qtest/migration: Add postcopy qtests for introducing 'channels' argument with new QAPI syntax | expand |
Het Gala <het.gala@nutanix.com> writes: > This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb > > After addition of 'channels' as the starting argument of new QAPI > syntax inside postcopy test, even if the user entered the old QAPI > syntax, test used the new syntax. > It was a temporary patch added to have some presence of the new syntax > since the migration qtest framework lacked any logic for introducing > 'channels' argument. That wasn't clear to me when we merged that. Was that really the case? > > Now that the qtest framework has logic to introduce uri and channel > argument separately, we can remove this temporary change. > > Signed-off-by: Het Gala <het.gala@nutanix.com> Anyway, I can see the point of this. Reviewed-by: Fabiano Rosas <farosas@suse.de>
On Wed, Apr 10, 2024 at 10:04:33AM -0300, Fabiano Rosas wrote: > Het Gala <het.gala@nutanix.com> writes: > > > This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb > > > > After addition of 'channels' as the starting argument of new QAPI > > syntax inside postcopy test, even if the user entered the old QAPI > > syntax, test used the new syntax. > > It was a temporary patch added to have some presence of the new syntax > > since the migration qtest framework lacked any logic for introducing > > 'channels' argument. > > That wasn't clear to me when we merged that. Was that really the case? Yeah these look all a bit confusing.. I'm wondering whether do we need the new interface to cover both precopy and postcopy, or one would suffice? Both should share the same interface. I think it means if we covered the channels interface in precopy, then perhaps we don't need to test anywhere else, as we got the code paths all covered. We actually do the same already for all kinds of channels for postcopy, where we stick with either tcp/unix but don't cover the rest.
On 10/04/24 6:34 pm, Fabiano Rosas wrote: > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > Het Gala<het.gala@nutanix.com> writes: > >> This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb >> >> After addition of 'channels' as the starting argument of new QAPI >> syntax inside postcopy test, even if the user entered the old QAPI >> syntax, test used the new syntax. >> It was a temporary patch added to have some presence of the new syntax >> since the migration qtest framework lacked any logic for introducing >> 'channels' argument. > That wasn't clear to me when we merged that. Was that really the case? Yes, I had little to no experience on how to introduce 'channels' as a new argument in the migration QAPIs back then. IIRC, while trying to merge the series (migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration), I was adviced to just modify one of the qtest with the new QAPI syntax rather than writing a new qtest altogether. So, I just renamed the old syntax with the new one. >> Now that the qtest framework has logic to introduce uri and channel >> argument separately, we can remove this temporary change. >> >> Signed-off-by: Het Gala<het.gala@nutanix.com> > Anyway, I can see the point of this. Thanks for the support for building that framework. Today we can independently call channel or uri easily for 'migrate' QAPI > Reviewed-by: Fabiano Rosas <farosas@suse.de Regards, Het Gala
On 10/04/24 8:23 pm, Peter Xu wrote: > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > On Wed, Apr 10, 2024 at 10:04:33AM -0300, Fabiano Rosas wrote: >> Het Gala <het.gala@nutanix.com> writes: >> >>> This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb >>> >>> After addition of 'channels' as the starting argument of new QAPI >>> syntax inside postcopy test, even if the user entered the old QAPI >>> syntax, test used the new syntax. >>> It was a temporary patch added to have some presence of the new syntax >>> since the migration qtest framework lacked any logic for introducing >>> 'channels' argument. >> That wasn't clear to me when we merged that. Was that really the case? > Yeah these look all a bit confusing.. > > I'm wondering whether do we need the new interface to cover both precopy > and postcopy, or one would suffice? > > Both should share the same interface. I think it means if we covered the > channels interface in precopy, then perhaps we don't need to test anywhere > else, as we got the code paths all covered. > > We actually do the same already for all kinds of channels for postcopy, > where we stick with either tcp/unix but don't cover the rest. Do we want to add other transports too (vsock, exec, rdma) with the new interface ? I believe we have tests for fd based migration Regards, Het Gala
On Thu, Apr 11, 2024 at 07:45:21PM +0530, Het Gala wrote: > > On 10/04/24 8:23 pm, Peter Xu wrote: > > !-------------------------------------------------------------------| > > CAUTION: External Email > > > > |-------------------------------------------------------------------! > > > > On Wed, Apr 10, 2024 at 10:04:33AM -0300, Fabiano Rosas wrote: > > > Het Gala <het.gala@nutanix.com> writes: > > > > > > > This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb > > > > > > > > After addition of 'channels' as the starting argument of new QAPI > > > > syntax inside postcopy test, even if the user entered the old QAPI > > > > syntax, test used the new syntax. > > > > It was a temporary patch added to have some presence of the new syntax > > > > since the migration qtest framework lacked any logic for introducing > > > > 'channels' argument. > > > That wasn't clear to me when we merged that. Was that really the case? > > Yeah these look all a bit confusing.. > > > > I'm wondering whether do we need the new interface to cover both precopy > > and postcopy, or one would suffice? > > > > Both should share the same interface. I think it means if we covered the > > channels interface in precopy, then perhaps we don't need to test anywhere > > else, as we got the code paths all covered. > > > > We actually do the same already for all kinds of channels for postcopy, > > where we stick with either tcp/unix but don't cover the rest. > Do we want to add other transports too (vsock, exec, rdma) with the new > interface ? > I believe we have tests for fd based migration Het, What I meant is we used to do white box testing for migration, trying to cover all the code paths would suffice for us in that case. It means maybe we don't need the postcopy test to cover the channels interface as long as precopy has covered that and also if that covered all the "channels" abi then we should be safe. What I worry is we keep extending the test matrix but we're actually testing the same code paths. Then the test runs slower each time, we burn more cpus for each CI kick, but without a real beneift. Thanks,
On 11/04/24 7:56 pm, Peter Xu wrote: > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > On Thu, Apr 11, 2024 at 07:45:21PM +0530, Het Gala wrote: >> On 10/04/24 8:23 pm, Peter Xu wrote: >>> !-------------------------------------------------------------------| >>> CAUTION: External Email >>> >>> |-------------------------------------------------------------------! >>> >>> On Wed, Apr 10, 2024 at 10:04:33AM -0300, Fabiano Rosas wrote: >>>> Het Gala <het.gala@nutanix.com> writes: >>>> >>>>> This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb >>>>> >>>>> After addition of 'channels' as the starting argument of new QAPI >>>>> syntax inside postcopy test, even if the user entered the old QAPI >>>>> syntax, test used the new syntax. >>>>> It was a temporary patch added to have some presence of the new syntax >>>>> since the migration qtest framework lacked any logic for introducing >>>>> 'channels' argument. >>>> That wasn't clear to me when we merged that. Was that really the case? >>> Yeah these look all a bit confusing.. >>> >>> I'm wondering whether do we need the new interface to cover both precopy >>> and postcopy, or one would suffice? >>> >>> Both should share the same interface. I think it means if we covered the >>> channels interface in precopy, then perhaps we don't need to test anywhere >>> else, as we got the code paths all covered. >>> >>> We actually do the same already for all kinds of channels for postcopy, >>> where we stick with either tcp/unix but don't cover the rest. >> Do we want to add other transports too (vsock, exec, rdma) with the new >> interface ? >> I believe we have tests for fd based migration > Het, > > What I meant is we used to do white box testing for migration, trying to > cover all the code paths would suffice for us in that case. > > It means maybe we don't need the postcopy test to cover the channels > interface as long as precopy has covered that and also if that covered all > the "channels" abi then we should be safe. > > What I worry is we keep extending the test matrix but we're actually > testing the same code paths. Then the test runs slower each time, we burn > more cpus for each CI kick, but without a real beneift. Yeah, I understood your concern Peter. Yes, since precopy and postcopy tests cover the same code path, adding 'channels' postcopy qtests does not make much sense after already having introduced in precopy qtests. I just wanted to highlight couple of pointers: 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI, we use the old uri for 'migrate-incoming' QAPI. 2. We do not cover other 'channels' abi, only have tcp path tested. So, the TO-DOs could be: 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels' interface OR have 'channels' interface with other than tcp transport (file, exec, vsock, etc) so as to cover different code paths. 2. Extend channels interface to migrate-incoming QAPI for precopy qtests Regards, Het Gala
On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote: > I just wanted to highlight couple of pointers: > 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI, > we > use the old uri for 'migrate-incoming' QAPI. > 2. We do not cover other 'channels' abi, only have tcp path tested. > > So, the TO-DOs could be: > 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels' > interface OR have 'channels' interface with other than tcp transport > (file, exec, vsock, etc) so as to cover different code paths. > 2. Extend channels interface to migrate-incoming QAPI for precopy qtests You can see whether Fabiano has anything to say, but what you proposed looks good to me. Thanks!
Peter Xu <peterx@redhat.com> writes: > On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote: >> I just wanted to highlight couple of pointers: >> 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI, >> we >> use the old uri for 'migrate-incoming' QAPI. >> 2. We do not cover other 'channels' abi, only have tcp path tested. >> >> So, the TO-DOs could be: >> 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels' >> interface OR have 'channels' interface with other than tcp transport >> (file, exec, vsock, etc) so as to cover different code paths. >> 2. Extend channels interface to migrate-incoming QAPI for precopy qtests > > You can see whether Fabiano has anything to say, but what you proposed > looks good to me. Ok, so what about we convert some of the 'plain' tests into channels to cover all transports? - tcp: test_multifd_tcp_none (this one we already did) - file: test_precopy_file - unix: test_precopy_unix_plain - exec: test_analyze_script - fd: test_migrate_precopy_fd_socket Those^, plus the validate_uri that's already in next should cover everything. We don't need to do this at once, by the way. Moreover: - leave all test strings untouched to preserve bisecting; - let's not bother adding "channels" and "uri" to the test string anymore. The channels API should be taken for granted at this point, I don't expect we start hitting bugs that will require us to run either foo/uri/plain or foo/channels/plain, so there's not much point in making the distinction.
On Thu, Apr 11, 2024 at 04:41:16PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote: > >> I just wanted to highlight couple of pointers: > >> 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI, > >> we > >> use the old uri for 'migrate-incoming' QAPI. > >> 2. We do not cover other 'channels' abi, only have tcp path tested. > >> > >> So, the TO-DOs could be: > >> 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels' > >> interface OR have 'channels' interface with other than tcp transport > >> (file, exec, vsock, etc) so as to cover different code paths. > >> 2. Extend channels interface to migrate-incoming QAPI for precopy qtests > > > > You can see whether Fabiano has anything to say, but what you proposed > > looks good to me. > > Ok, so what about we convert some of the 'plain' tests into channels to > cover all transports? > > - tcp: test_multifd_tcp_none (this one we already did) > - file: test_precopy_file > - unix: test_precopy_unix_plain > - exec: test_analyze_script > - fd: test_migrate_precopy_fd_socket > > Those^, plus the validate_uri that's already in next should cover > everything. > > We don't need to do this at once, by the way. > > Moreover: > > - leave all test strings untouched to preserve bisecting; > > - let's not bother adding "channels" and "uri" to the test string > anymore. The channels API should be taken for granted at this point, I > don't expect we start hitting bugs that will require us to run either > foo/uri/plain or foo/channels/plain, so there's not much point in > making the distinction. Do you mean we can put "uri:" aside? Maybe I misunderstood.. The matrix previously was (I think.. when this series posted): [tcp, unix, file, exec, fd] x [uri, channels] x [precopy, postcopy] Drop postcopy as doesn't seem to have any special paths: [tcp, unix, file, exec, fd] x [uri, channels] So logically we should still cover these, right? Thanks,
Peter Xu <peterx@redhat.com> writes: > On Thu, Apr 11, 2024 at 04:41:16PM -0300, Fabiano Rosas wrote: >> Peter Xu <peterx@redhat.com> writes: >> >> > On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote: >> >> I just wanted to highlight couple of pointers: >> >> 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI, >> >> we >> >> use the old uri for 'migrate-incoming' QAPI. >> >> 2. We do not cover other 'channels' abi, only have tcp path tested. >> >> >> >> So, the TO-DOs could be: >> >> 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels' >> >> interface OR have 'channels' interface with other than tcp transport >> >> (file, exec, vsock, etc) so as to cover different code paths. >> >> 2. Extend channels interface to migrate-incoming QAPI for precopy qtests >> > >> > You can see whether Fabiano has anything to say, but what you proposed >> > looks good to me. >> >> Ok, so what about we convert some of the 'plain' tests into channels to >> cover all transports? >> >> - tcp: test_multifd_tcp_none (this one we already did) >> - file: test_precopy_file >> - unix: test_precopy_unix_plain >> - exec: test_analyze_script >> - fd: test_migrate_precopy_fd_socket >> >> Those^, plus the validate_uri that's already in next should cover >> everything. >> >> We don't need to do this at once, by the way. >> >> Moreover: >> >> - leave all test strings untouched to preserve bisecting; >> >> - let's not bother adding "channels" and "uri" to the test string >> anymore. The channels API should be taken for granted at this point, I >> don't expect we start hitting bugs that will require us to run either >> foo/uri/plain or foo/channels/plain, so there's not much point in >> making the distinction. > > Do you mean we can put "uri:" aside? Maybe I misunderstood.. I mean the test name does not need to specify "channels" vs. "uri" because that should never be broken to the point that we actually need to go fetch those tests by name. We'd still have at least 1 test for each transport with channels and (existing) at least 1 test for each transport with uri. > > The matrix previously was (I think.. when this series posted): > > [tcp, unix, file, exec, fd] x [uri, channels] x [precopy, postcopy] > > Drop postcopy as doesn't seem to have any special paths: > > [tcp, unix, file, exec, fd] x [uri, channels] > > So logically we should still cover these, right? Right, I'm just suggesting we convert some tests to use channels, one for each transport, to test the channels API in full. The rest of the existing tests as well as future tests need not have a uri (or channel) variant. Just one of them is enough.
On Fri, Apr 12, 2024 at 11:58:43AM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Thu, Apr 11, 2024 at 04:41:16PM -0300, Fabiano Rosas wrote: > >> Peter Xu <peterx@redhat.com> writes: > >> > >> > On Thu, Apr 11, 2024 at 11:31:08PM +0530, Het Gala wrote: > >> >> I just wanted to highlight couple of pointers: > >> >> 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI, > >> >> we > >> >> use the old uri for 'migrate-incoming' QAPI. > >> >> 2. We do not cover other 'channels' abi, only have tcp path tested. > >> >> > >> >> So, the TO-DOs could be: > >> >> 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels' > >> >> interface OR have 'channels' interface with other than tcp transport > >> >> (file, exec, vsock, etc) so as to cover different code paths. > >> >> 2. Extend channels interface to migrate-incoming QAPI for precopy qtests > >> > > >> > You can see whether Fabiano has anything to say, but what you proposed > >> > looks good to me. > >> > >> Ok, so what about we convert some of the 'plain' tests into channels to > >> cover all transports? > >> > >> - tcp: test_multifd_tcp_none (this one we already did) > >> - file: test_precopy_file > >> - unix: test_precopy_unix_plain > >> - exec: test_analyze_script > >> - fd: test_migrate_precopy_fd_socket > >> > >> Those^, plus the validate_uri that's already in next should cover > >> everything. > >> > >> We don't need to do this at once, by the way. > >> > >> Moreover: > >> > >> - leave all test strings untouched to preserve bisecting; > >> > >> - let's not bother adding "channels" and "uri" to the test string > >> anymore. The channels API should be taken for granted at this point, I > >> don't expect we start hitting bugs that will require us to run either > >> foo/uri/plain or foo/channels/plain, so there's not much point in > >> making the distinction. > > > > Do you mean we can put "uri:" aside? Maybe I misunderstood.. > > I mean the test name does not need to specify "channels" vs. "uri" > because that should never be broken to the point that we actually need > to go fetch those tests by name. We'd still have at least 1 test for > each transport with channels and (existing) at least 1 test for each > transport with uri. > > > > > The matrix previously was (I think.. when this series posted): > > > > [tcp, unix, file, exec, fd] x [uri, channels] x [precopy, postcopy] > > > > Drop postcopy as doesn't seem to have any special paths: > > > > [tcp, unix, file, exec, fd] x [uri, channels] > > > > So logically we should still cover these, right? > > Right, I'm just suggesting we convert some tests to use channels, one > for each transport, to test the channels API in full. The rest of the > existing tests as well as future tests need not have a uri (or channel) > variant. Just one of them is enough. Ah so that's the "test string"; sounds all good.
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 5d6d8cd634..27a1066ae4 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -1297,12 +1297,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr, migrate_prepare_for_dirty_mem(from); qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming'," - " 'arguments': { " - " 'channels': [ { 'channel-type': 'main'," - " 'addr': { 'transport': 'socket'," - " 'type': 'inet'," - " 'host': '127.0.0.1'," - " 'port': '0' } } ] } }"); + " 'arguments': { 'uri': 'tcp:127.0.0.1:0' }}"); /* Wait for the first serial output from the source */ wait_for_serial("src_serial");
This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb After addition of 'channels' as the starting argument of new QAPI syntax inside postcopy test, even if the user entered the old QAPI syntax, test used the new syntax. It was a temporary patch added to have some presence of the new syntax since the migration qtest framework lacked any logic for introducing 'channels' argument. Now that the qtest framework has logic to introduce uri and channel argument separately, we can remove this temporary change. Signed-off-by: Het Gala <het.gala@nutanix.com> --- tests/qtest/migration-test.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)