From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.6 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 529E0C4332B for ; Mon, 23 Mar 2020 09:43:12 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1CA1C206F8 for ; Mon, 23 Mar 2020 09:43:12 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="KzoUAI60" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1CA1C206F8 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:59306 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jGJbz-00079P-8s for qemu-devel@archiver.kernel.org; Mon, 23 Mar 2020 05:43:11 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:55467) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jGJbL-0006hR-4z for qemu-devel@nongnu.org; Mon, 23 Mar 2020 05:42:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1jGJbJ-0007lr-6D for qemu-devel@nongnu.org; Mon, 23 Mar 2020 05:42:30 -0400 Received: from us-smtp-delivery-74.mimecast.com ([216.205.24.74]:20159) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1jGJbI-0007le-WB for qemu-devel@nongnu.org; Mon, 23 Mar 2020 05:42:29 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1584956548; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=rVzp5FgCwJWHJ3HfY4o/lRwpBPQ5rEqH+z/2nhW1+Ss=; b=KzoUAI60kQYKQ+1ZyA9PgqT8LrYdZOZJLVDkwGsOGGlfVF3Fu1i5FN83ek8zuKbNF9A0Mk xxstkVJSL3c4xxp87694d6drESYBkb/bw9TZw2RkYWb+ZgAH23DWw02dynWEDu8hMygy9J 6NLNw1cTC0UNbfcALLt41kRYvg7r7jI= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-7-zHdaJisbONmwrooqY-3GIg-1; Mon, 23 Mar 2020 05:42:25 -0400 X-MC-Unique: zHdaJisbONmwrooqY-3GIg-1 Received: by mail-wr1-f69.google.com with SMTP id q18so7082774wrw.5 for ; Mon, 23 Mar 2020 02:42:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=W5rTWkQ9Q0WFB+WucsYwF0erVXGngPH7/eCbGamiLY8=; b=LNxCJM02tiVZ2IHGb4GDy+QaMKaCUIfFEtDjaUp4TWeLsEJF1G3NsrPfLqkAtu9tpV KD/J0Ent9zsSaDUfsLpfMdWDkavKx2gTd2X6HYJSxzNB8SrDX928UowuNh9uQFEmb+HN gdYo0B+7+GGuRAaoaNIipyZ0PamkNNcLZ8eq/RF6n2MYYa/Dx5/ThcqA0OXTaIr68Tfx PQIQAZc4VjY4tfz/zEbudw7/PinQGB1/qLAoFv3wN0EIWQRTpvYICyDG+LWLul75jEND gvM0kNhSeF/BM1d1+BZwb93CLGtjkyR/1DX8jAHfeEf+kp/8+wlGOJLYE4fxfp1VluJh lGFQ== X-Gm-Message-State: ANhLgQ0D2ZL6BKeXUSgOCdVhI88FimydZnZhSAwqTYSJWaQ70il95HyA Qh9kplBd4gQc7084oKG/oqQm1Hh7p0eLyupj7NwTjodWg558pDmZ5Wg5lTUX/u/dn5J15251SCE PjxlHZ8wZOVn+i7h4n5unvUfZS3w/0jM= X-Received: by 2002:a1c:b0c3:: with SMTP id z186mr25436684wme.36.1584956544590; Mon, 23 Mar 2020 02:42:24 -0700 (PDT) X-Google-Smtp-Source: ADFU+vtQWo2XFqE+Z4Xk/8quaAyAd4nslGKoE67h/qe/AWC8uK9QdGWPs9YiNDDPePBsO1GwkT5nMHwNc9PRR5zxfsw= X-Received: by 2002:a1c:b0c3:: with SMTP id z186mr25436666wme.36.1584956544349; Mon, 23 Mar 2020 02:42:24 -0700 (PDT) MIME-Version: 1.0 References: <20200320151254.16766-1-ovoshcha@redhat.com> <20200320151254.16766-3-ovoshcha@redhat.com> In-Reply-To: From: Oksana Voshchana Date: Mon, 23 Mar 2020 11:42:13 +0200 Message-ID: Subject: Re: [PATCH v3 2/3] Acceptance test: provides new functions To: Willian Rampazzo X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="000000000000cf8d0f05a1827386" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 216.205.24.74 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Cleber Rosa , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= , qemu-devel , Wainer Moschetta , Eduardo Habkost Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000cf8d0f05a1827386 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Willian I totally agree with your comments I'll move those functions outside the class Let me update and repost the patch Thanks On Fri, Mar 20, 2020 at 6:22 PM Willian Rampazzo wrote: > Hi Oksana, > > On Fri, Mar 20, 2020 at 12:15 PM Oksana Vohchana > 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 > > --- > > 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 =3D service.ServiceManager() > > + rdma =3D 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 =3D 'rdma link show -j' > > + out =3D json.loads(process.getoutput(cmd)) > > + try: > > + for i in out: > > + if i['state'] =3D=3D '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 =3D LocalHost() > > + network_in =3D NetworkInterface(interface, local) > > + try: > > + ip =3D 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 =3D 'tcp:localhost:%u' % self._get_free_port() > > -- > > 2.21.1 > > > > > > Let me know if the comments do not make sense. > > Willian > > --000000000000cf8d0f05a1827386 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi=C2=A0Willian
I totally agree with your comments
= I'll move those functions outside the class
Let me update and repost= the patch
Thanks

On Fri, Mar 20, 2020 at 6:22 PM Willian Rampazzo = <wrampazz@redhat.com> wrot= e:
Hi Oksana,
On Fri, Mar 20, 2020 at 12:15 PM Oksana Vohchana <ovoshcha@redhat.com> 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 <ovoshcha@redhat.com>
> ---
>=C2=A0 tests/acceptance/migration.py | 30 +++++++++++++++++++++++++++++= +
>=C2=A0 1 file changed, 30 insertions(+)
>
> diff --git a/tests/acceptance/migration.py b/tests/acceptance/migratio= n.py
> index e4c39b85a1..a783f3915b 100644
> --- a/tests/acceptance/migration.py
> +++ b/tests/acceptance/migration.py
> @@ -11,12 +11,17 @@
>
>
>=C2=A0 import tempfile
> +import json
>=C2=A0 from avocado_qemu import Test
>=C2=A0 from avocado import skipUnless
>
>=C2=A0 from avocado.utils import network
>=C2=A0 from avocado.utils import wait
>=C2=A0 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
>
>
>=C2=A0 class Migration(Test):
> @@ -58,6 +63,31 @@ class Migration(Test):
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.cancel('Faile= d to find a free port')
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return port
>
> +=C2=A0 =C2=A0 def _if_rdma_enable(self):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 rdma_stat =3D service.ServiceManager() > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 rdma =3D rdma_stat.status('rdma')=
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return rdma

You can just `return rdma_stat.status('rdma')` here! Also, as you a= re
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.

> +
> +=C2=A0 =C2=A0 def _get_interface_rdma(self):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 cmd =3D 'rdma link show -j'
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 out =3D json.loads(process.getoutput(cmd)= )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 try:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 for i in out:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if i['sta= te'] =3D=3D 'ACTIVE':
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= return i['netdev']
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 except KeyError:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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?

> +
> +=C2=A0 =C2=A0 def _get_ip_rdma(self, interface):
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 local =3D LocalHost()
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 network_in =3D NetworkInterface(interface= , local)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 try:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ip =3D network_in._get_inte= rface_details()
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ip:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return ip[0][= 'addr_info'][0]['local']
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 except:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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.

>
>=C2=A0 =C2=A0 =C2=A0 def test_migration_with_tcp_localhost(self):
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dest_uri =3D 'tcp:localhost:%u&#= 39; % self._get_free_port()
> --
> 2.21.1
>
>

Let me know if the comments do not make sense.

Willian

--000000000000cf8d0f05a1827386--