Re: [PATCH v2 REPOST] Acceptance test: provides to use different transport for migration

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Oksana,

On 1/30/20 5:42 PM, Oksana Vohchana wrote:
> Along with VM migration via TCP, we can use migration through EXEC
> and UNIX transport protocol
> 
> Signed-off-by: Oksana Vohchana <ovoshcha@redhat.com>
> ---
> v2:
>    - Removes unnecessary symbols and unused method
> ---
>   tests/acceptance/migration.py | 51 ++++++++++++++++++++++++-----------
>   1 file changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/acceptance/migration.py b/tests/acceptance/migration.py
> index a44c1ae58f..1f6a674843 100644
> --- a/tests/acceptance/migration.py
> +++ b/tests/acceptance/migration.py
> @@ -10,10 +10,13 @@
>   # later.  See the COPYING file in the top-level directory.
>   
>   
> +import tempfile
>   from avocado_qemu import Test
> +from avocado import skipUnless
>   
>   from avocado.utils import network
>   from avocado.utils import wait
> +from avocado.utils.path import find_command
>   
>   
>   class Migration(Test):
> @@ -24,6 +27,26 @@ class Migration(Test):
>       def migration_finished(vm):
>           return vm.command('query-migrate')['status'] in ('completed', 'failed')
>   

It is easier to read if you declare assert_migration() before do_migrate().

> +    def do_migrate(self, dest_uri, src_uri=None):
> +        source_vm = self.get_vm()
> +        dest_vm = self.get_vm('-incoming', dest_uri)
> +        dest_vm.launch()
> +        if src_uri is None:
> +            src_uri = dest_uri
> +        source_vm.launch()
> +        source_vm.qmp('migrate', uri=src_uri)
> +        self.assert_migration(source_vm, dest_vm)
> +
> +    def assert_migration(self, source_vm, dest_vm):
> +        wait.wait_for(self.migration_finished,
> +                      timeout=self.timeout,
> +                      step=0.1,
> +                      args=(source_vm,))
> +        self.assertEqual(source_vm.command('query-migrate')['status'], 'completed')
> +        self.assertEqual(dest_vm.command('query-migrate')['status'], 'completed')
> +        self.assertEqual(dest_vm.command('query-status')['status'], 'running')
> +        self.assertEqual(source_vm.command('query-status')['status'], 'postmigrate')
> +
>       def _get_free_port(self):
>           port = network.find_free_port()
>           if port is None:
> @@ -32,19 +55,17 @@ class Migration(Test):
>   
>   
>       def test_migration_with_tcp_localhost(self):
> -        source_vm = self.get_vm()
>           dest_uri = 'tcp:localhost:%u' % self._get_free_port()
> -        dest_vm = self.get_vm('-incoming', dest_uri)
> -        dest_vm.launch()
> -        source_vm.launch()
> -        source_vm.qmp('migrate', uri=dest_uri)
> -        wait.wait_for(
> -            self.migration_finished,
> -            timeout=self.timeout,
> -            step=0.1,
> -            args=(source_vm,)
> -        )
> -        self.assertEqual(dest_vm.command('query-migrate')['status'], 'completed')
> -        self.assertEqual(source_vm.command('query-migrate')['status'], 'completed')
> -        self.assertEqual(dest_vm.command('query-status')['status'], 'running')
> -        self.assertEqual(source_vm.command('query-status')['status'], 'postmigrate')
> +        self.do_migrate(dest_uri)

I would split this patch in 2:
- Extract assert_migration() and do_migrate(),
- Add 'unix' socket test
- Add 'exec' test

> +
> +    def test_migration_with_unix(self):
> +        with tempfile.TemporaryDirectory(prefix='socket_') as socket_path:
> +            dest_uri = 'unix:%s/qemu-test.sock' % socket_path
> +            self.do_migrate(dest_uri)
> +
> +    @skipUnless(find_command('nc', default=False), "nc command not found on the system")
> +    def test_migration_with_exec(self):
> +        free_port = self._get_free_port()
> +        dest_uri = 'exec:nc -l localhost %u' % free_port

Can you check and add a comment that the argument used is handled 
similarly by both netcat-traditional / netcat-openbsd packages?

> +        src_uri = 'exec:nc localhost %u' % free_port
> +        self.do_migrate(dest_uri, src_uri)
> 

Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>