Re: [PATCH 4/4] tests/qtest/migration: Add postcopy migration qtests to use 'channels' argument instead of uri

[Het Gala]

On 10/04/24 6:45 pm, Fabiano Rosas wrote:

> !-------------------------------------------------------------------|
>   CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> Het Gala <het.gala@nutanix.com> writes:
>
> > Add qtests to perform postcopy live migration by having list of
> > 'channels' argument as the starting point instead of uri string.
> > (Note: length of the list is restricted to 1 for now)
> >
> > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > ---
> >  tests/qtest/migration-test.c | 38 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index fa8a860811..599018baa0 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -1296,13 +1296,17 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
> >      migrate_ensure_non_converge(from);
> >  
> >      migrate_prepare_for_dirty_mem(from);
> > -    migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
> > +    if (args->connect_channels) {
> > +        migrate_incoming_qmp(to, NULL, args->connect_channels, "{}");
> > +    } else {
> > +        migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
> > +    }
> >       
> >From an interface perspective it's a bit unexpected to need this
> conditional when the migrate_qmp below doesn't need it.
>     
    I think, migrate_qmp has an unique advantage
      here. Please correct me If my understanding
      is not correct.
      1. test_migrate_start starts the qemu cmd line with either
      --incoming tcp:127.0.0.1:0
         or "defer". If tcp uri is provided, then migrate_incoming_qmp
      is not necessary
      2. If "defer" is passed, then only migrate_incoming_qmp is
      required with tcp uri string
      3. migrate_qmp can get the uri anyway either from
           test_migrate_start -> defer
      + migrate_incoming_qmp -> tcp:127.0.0.1:0
             test_migrate_start -> tcp:127.0.0.1:0
         with the help of migrate_get_connect_uri() with the correct
      migration port.
         Even if we always pass NULL, we are okay, But this is just for
      tcp transport, and
         not unix
      
      We can't have the unique situation for migrate_incoming_qmp, hence
      avoiding conditional
      earlier seemed to be necessary for me. But, with the hack
      suggested by you in previous
      patch, will prevent us from using conditional if else

> >  
> >      /* Wait for the first serial output from the source */
> >      wait_for_serial("src_serial");
> >      wait_for_suspend(from, &src_state);
> >  
> > -    migrate_qmp(from, to, NULL, NULL, "{}");
> > +    migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
> >  
> >      migrate_wait_for_dirty_mem(from, to);
> >  
> > @@ -1355,6 +1359,20 @@ static void test_postcopy(void)
> >      test_postcopy_common(&args);
> >  }
> >  
> > +static void test_postcopy_channels(void)
> > +{
> > +    MigrateCommon args = {
> > +        .listen_uri = "defer",
> > +        .connect_channels = "[ { 'channel-type': 'main',"
> > +                            "    'addr': { 'transport': 'socket',"
> > +                            "              'type': 'inet',"
> > +                            "              'host': '127.0.0.1',"
> > +                            "              'port': '0' } } ]",
> > +    };
> > +
> > +    test_postcopy_common(&args);
> > +}
> > +
> >  static void test_postcopy_suspend(void)
> >  {
> >      MigrateCommon args = {
> > @@ -1555,6 +1573,18 @@ static void test_postcopy_recovery(void)
> >      test_postcopy_recovery_common(&args);
> >  }
> >  
> > +static void test_postcopy_recovery_channels(void)
> > +{
> > +    MigrateCommon args = {
> > +        .connect_channels = "[ { 'channel-type': 'main',"
> > +                            "    'addr': { 'transport': 'socket',"
> > +                            "              'type': 'inet',"
> > +                            "              'host': '127.0.0.1',"
> > +                            "              'port': '0' } } ]",
> > +    };
> > +
> > +    test_postcopy_recovery_common(&args);
> > +}
> >  static void test_postcopy_recovery_compress(void)
> >  {
> >      MigrateCommon args = {
> > @@ -3585,8 +3615,12 @@ int main(int argc, char **argv)
> >  
> >      if (has_uffd) {
> >          migration_test_add("/migration/postcopy/plain", test_postcopy);
> > +        migration_test_add("/migration/postcopy/channels/plain",
> > +                           test_postcopy_channels);
> >          migration_test_add("/migration/postcopy/recovery/plain",
> >                             test_postcopy_recovery);
> > +        migration_test_add("/migration/postcopy/recovery/channels/plain",
> > +                           test_postcopy_recovery_channels);
> >          migration_test_add("/migration/postcopy/preempt/plain",
> >                             test_postcopy_preempt);
> >          migration_test_add("/migration/postcopy/preempt/recovery/plain",
> >       
>     
    Regards,
      Het Gala