Re: [PATCH v3 2/3] Acceptance test: provides new functions

[Willian Rampazzo]

Hi Oksana,

On Fri, Mar 20, 2020 at 12:15 PM Oksana Vohchana <address@hidden> wrote:
>
> Provides new functions related to the rdma migration test
> Adds functions to check if service RDMA is enabled and gets
> the ip address on the interface where it was configured
>
> Signed-off-by: Oksana Vohchana <address@hidden>
> ---
>  tests/acceptance/migration.py | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/tests/acceptance/migration.py b/tests/acceptance/migration.py
> index e4c39b85a1..a783f3915b 100644
> --- a/tests/acceptance/migration.py
> +++ b/tests/acceptance/migration.py
> @@ -11,12 +11,17 @@
>
>
>  import tempfile
> +import json
>  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
> +from avocado.utils.network.interfaces import NetworkInterface
> +from avocado.utils.network.hosts import LocalHost
> +from avocado.utils import service
> +from avocado.utils import process
>
>
>  class Migration(Test):
> @@ -58,6 +63,31 @@ class Migration(Test):
>              self.cancel('Failed to find a free port')
>          return port
>
> +    def _if_rdma_enable(self):
> +        rdma_stat = service.ServiceManager()
> +        rdma = rdma_stat.status('rdma')
> +        return rdma

You can just `return rdma_stat.status('rdma')` here! Also, as you are
not using any of the class attributes or methods, if you make this
method static, you don't need to call it with `None` as you did on
patch 3 of this series.

> +
> +    def _get_interface_rdma(self):
> +        cmd = 'rdma link show -j'
> +        out = json.loads(process.getoutput(cmd))
> +        try:
> +            for i in out:
> +                if i['state'] == 'ACTIVE':
> +                    return i['netdev']
> +        except KeyError:
> +            return None

Same comment about making this method static.

Actually, if you are not using any of the attributes or methods from
the Migration class on these two methods, I think you can define them
as functions, outside of the Class. Does it make sense?

> +
> +    def _get_ip_rdma(self, interface):
> +        local = LocalHost()
> +        network_in = NetworkInterface(interface, local)
> +        try:
> +            ip = network_in._get_interface_details()
> +            if ip:
> +                return ip[0]['addr_info'][0]['local']
> +        except:
> +            self.cancel("Incorrect interface configuration or device name")
> +

If you change the logic a bit and raise an exception here, instead of
doing a `self.cancel`, you can also make this method static, or move
it outside of the class. The cancel can be handled in the test, with
the exception raised here.

>
>      def test_migration_with_tcp_localhost(self):
>          dest_uri = 'tcp:localhost:%u' % self._get_free_port()
> --
> 2.21.1
>
>

Let me know if the comments do not make sense.

Willian