[ixia/Keysight] sample test cases and fixtures by abhijit-dhar · Pull Request #1819 · sonic-net/sonic-mgmt

abhijit-dhar · 2020-06-25T16:17:52Z

This directory should contain the Ixia ECN and RDMA test cases.
The lib folder contains
* fixtures required to run ixia test cases
* helper function required to write ixia test cases

Description of PR

Summary:
Fixes # (issue)

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

This directory should contain the Ixia ECN and RDMA test cases. The lib folder contains * fixtures required to run ixia test cases * helper function required to write ixia test cases

lgtm-com · 2020-06-25T16:29:05Z

This pull request introduces 3 alerts when merging 06cd811 into a25951a - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'
1 for Unused import
1 for Variable defined multiple times

yxieca · 2020-06-25T16:55:06Z

tests/ixia/test_ixia_traffic_restpy.py

+    traffic_item.Generate()
+    ixNetwork.Traffic.Apply()
+    ixNetwork.Traffic.Start()
+    time.sleep(10)


what is the purpose of this sleep?

I don't see IO being stopped, is that on purpose? If so, will it cause issue for next test?

Open IO was not intentional. I will modify the the code to add a line to stop traffic. Generally stop the traffic is good practice. But even if traffic is not stopped it wont effect the next test, because those ports will get rebooted during while getting configured fro the next test.

This issue is addressed. I have added ixNetwork.Traffic.Stop() ad the end of the script

I suggest use a helper with StartFor(5) seconds instead and get stats then if possible.

yxieca · 2020-06-25T16:55:47Z

tests/ixia/test_ixia_traffic_restpy.py

+    ixNetwork.Traffic.Apply()
+    ixNetwork.Traffic.Start()
+    time.sleep(10)
+    print(session.StatViewAssistant('Traffic Item Statistics'))


Please remove this debug code, or use logger to generate a log entry if necessary.

I will do that

I have address this issue. print statements are replaced by logger.info()

yxieca · 2020-06-25T16:56:09Z

tests/ixia/test_ixia_traffic_restpy.py

+    time.sleep(10)
+    print(session.StatViewAssistant('Traffic Item Statistics'))
+
+    print('passed')


Please remove this debug code

I will do that

I have removed this print statement

yxieca · 2020-06-25T16:56:19Z

tests/ixia/test_ixia_traffic_restpy.py

+
+    print('passed')
+
+    assert 1


Please remove this no-op code.

I will do that

yxieca · 2020-06-25T16:57:51Z

I suggest move tests/ixia/lib/* to tests/common/ixia/*. test/common is where we put shared code. and I don't think explicit 'lib' in the path is required.

yxieca · 2020-06-25T16:59:38Z

You used lots of 'print' in your test. Can you change them into logger.debug(), logger.info() and/or logger.warning() instead?

baiwei0427 · 2020-06-25T23:39:47Z

tests/ixia/test_ixia_traffic_restpy.py

+from common.reboot import *
+from common.platform.device_utils import fanout_switch_port_lookup
+from common.helpers import assertions
+from ixnetwork_restpy import SessionAssistant, Files


Can you move all the low level IXIA API functions to ixia_helpers.py?

Thanks for the suggestion. In fact we are in process of designing those high-level wrapper function. See you soon in PRs and meeting and may be design discussion regarding this. Currently I suggest let it be there, to get the ball rolling. As soon we will formulate the new wrapper API we will remove these and replace these with new wrapper functions (which may be bit delayed). Till then let it be here

I have moved some of them to the ixia_fixture.py (mainly connection related). Rest of them is still there. Until we come up with some high level wrapper they suppose to be there. This is only a sample script. This will act as a templet for the engineers who will add test-case to act with ixia devices.
Purpose is the get the activity started. Later we may enhance/change or even remove it (after a considerable RDMA test case is added). Right now to stat the activity it is important.

I have also moved the ixia libraries to tests/ixia/lib/* and changed the import statements on the top of the script accordingly.

I have also moved the ixia libraries to "/sonic-mgmt/tests/common/ixia" and changed the import statements on the top of the script accordingly.

As per review comments given against the 1819 we have to keep the ixia libraries under sonic-mgmt/tests/common/ixia/

lgtm-com · 2020-06-26T12:04:18Z

This pull request introduces 6 alerts when merging b6a0963 into 815511c - view on LGTM.com

new alerts:

2 for Except block handles 'BaseException'
2 for Unused import
2 for Variable defined multiple times

This file should be in tests/common/ixia folder. So deleting it from tests/ixia/lib/ folder

This file should be under the folder tests/common/ixia Hence deleting it form tests/ixia/lib

This file should be under tests/common/ixia So deleting it from tests/ixia/lib

lgtm-com · 2020-06-26T12:31:10Z

This pull request introduces 3 alerts when merging b5316e8 into 815511c - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'
1 for Unused import
1 for Variable defined multiple times

There were many review comment against this file. Some of the comments I have implemented. But some of the comments may be implemented in future. Like "(Wei Bai) Can you move all the low level IXIA API functions to ixia_helpers.py?" or "I have a high level comment. can we design some IXIA wrapper functions (e.g., configure ports, create a PFC traffic item) and only call these high level wrapper functions in py.test scripts without exposing too many IXIA low level details? (Wei Bai)" This comment may be addressed in future PRs once we come up with the high level wrapper APIs.

lgtm-com · 2020-06-26T12:41:37Z

This pull request introduces 3 alerts when merging 663e920 into 815511c - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'
1 for Unused import
1 for Variable defined multiple times

neethajohn · 2020-06-26T16:28:33Z

tests/common/ixia/ixia_fixtures.py

+This fixture returns the hostnames and IP addresses of the IXIA chassis in the dictionary format.
+"""  
+@pytest.fixture(scope = "module")
+def ixia_dev(duthost, ansible_adhoc, conn_graph_facts, creds):


as discussed in the meeting, this needs to be part of the fanouthosts fixture and abstracted in common/devices.py. Please look at the EosHost or OnyxHost for examples

I have removed this fixture from ixia_fixtures.py, instead I am taking fanout_graph_facts as input and designed new class "IxiaFanoutManager" defined in ixia_helpers.py to handle and extract chassis IP address.

neethajohn · 2020-06-26T16:31:01Z

tests/ixia/test_ixia_traffic_restpy.py

+
+import time
+
+#pytestmark = [pytest.mark.disable_loganalyzer]


cleanup. remove

I have addressed this - the commented line is removed

neethajohn · 2020-06-26T16:34:00Z

tests/ixia/test_ixia_traffic_restpy.py

+    cardId    = getCard(ixiaports, 0)
+    PortId1   = getPort(ixiaports, 0)
+    PortId2   = getPort(ixiaports, 1)
+    PortId3   = getPort(ixiaports, 2)
+    PortId4   = getPort(ixiaports, 3)
+    PortId5   = getPort(ixiaports, 4)
+    PortId6   = getPort(ixiaports, 5)


Unless 6 ixia connections exist, this test cannot be used by others, can you make this num_of_ports configurable?

All this hard codings are removed. I have used "IxiaFanoutManager" class which can handle variable number of physical ports (as comes in fanout_graph_fact) and creates that number of virtual ports in the ixNetwork configuration. Also port assignments are done accordingly.

neethajohn · 2020-06-26T16:43:51Z

tests/ixia/test_ixia_traffic_restpy.py

+                               Password = Password,
+                               RestPort = RestPort)
+
+    sessionData = session.Session


all the test setup/cleanup code, should move to a fixture/separate method.

I have written a fixture ixia_api_server_session in the ixia_fixtures.py to handle session set up and initial clean up.

neethajohn · 2020-06-26T16:46:25Z

tests/common/ixia/ixia_helpers.py

+@param conn_graph_facts: testbed connectivity graph
+@return the port of its neighbor IXIA device or None if we cannot find it
+"""
+def get_neigh_ixia_port(intf, conn_graph_facts):


have a method to return a set of ports as well

neethajohn · 2020-06-26T16:48:09Z

tests/common/ixia/ixia_helpers.py

+@param ixia_dev: the mapping of hostname to IP address of IXIA devices
+@return the management IP address of its neighbor IXIA device or None if we cannot find it
+"""
+def get_neigh_ixia_mgmt_ip(intf, conn_graph_facts, ixia_dev):


use fanout_graph_facts intead of conn_graph_facts. that will return all the fanouts(ixia chassis) associated with the dut as a dict which will have all the details for each of the fanouts

I have used fanout_graph_facts to get chassis/card/port information.

I don't see any changes in this method. If we aren't using these, please remove them

Added a new fixture ixia_api_server_session

This class is added to efficiently manage ixia fanout devices

Making ixia directory a package, to access ixia_fixtures.py and ixia_helpers.py

This class is a place holder right now. Ixia fanout devices (chassis) do not require this abstraction layer. Ixia PTF (ixia API server) takes care of all the operation that we can do on the ixia fanout devices (chassis) and hence we execute them via API server only. But it is still nice to have all abstractions in the same location. So future users knows where to find them.

lgtm-com · 2020-07-01T11:09:33Z

This pull request introduces 3 alerts when merging 7bd3dbd into 1661316 - view on LGTM.com

new alerts:

1 for Missing call to __init__ during object initialization
1 for Syntax error
1 for 'import *' may pollute namespace

Added a new class that should make chassis port management easier

There was an typo

lgtm-com · 2020-07-01T11:32:02Z

This pull request introduces 2 alerts when merging 84f3201 into 1661316 - view on LGTM.com

new alerts:

1 for Missing call to __init__ during object initialization
1 for 'import *' may pollute namespace

Deletes accidentally

lgtm-com · 2020-07-01T12:01:06Z

This pull request introduces 6 alerts when merging b91dc5e into 1661316 - view on LGTM.com

new alerts:

3 for Unused import
1 for Unused local variable
1 for Missing call to __init__ during object initialization
1 for 'import *' may pollute namespace

tests/common/ixia/ixia_fixtures.py

baiwei0427 · 2020-07-20T05:31:31Z

tests/common/ixia/ixia_fixtures.py

+        duthost (pytest fixture): The duthost fixture.
+
+    Returns:
+        Ixia PTF server (API server) REST port.


REST port of Ixia API server

Removed the “PTF Server” from the comment.

baiwei0427 · 2020-07-20T05:32:41Z

tests/common/ixia/ixia_fixtures.py

+@pytest.fixture(scope = "module")
+def ixia_api_serv_port(duthost):
+    """
+    Return REST port of the ixia API server.


Can you elaborate REST port here? e.g., TCP port for REST API

I have changed the doc string: "This fixture returns the TCP port for REST API of the ixia API server."

baiwei0427 · 2020-07-20T05:33:17Z

tests/common/ixia/ixia_fixtures.py

+@pytest.fixture(scope = "module")
+def ixia_api_serv_session_id(duthost):
+    """
+    Ixia PTF can spawn multiple session on the same REST port.


Ixia PTF -> Ixia API server

Removed the “PTF Server” from the comment.

baiwei0427 · 2020-07-20T05:40:20Z

tests/common/ixia/ixia_fixtures.py

+@pytest.fixture(scope = "module")
+def ixia_dev(duthost, fanouthosts):
+    """
+    Returns the Ixia chassis IP


This function can return multiple IP addresses. We should emphasize this.

I have changed the comment section: "This fixture can return multiple IPs if multiple Ixia chassis are present in the test topology."

tests/common/ixia/ixia_fixtures.py

baiwei0427 · 2020-07-20T05:44:43Z

tests/common/ixia/ixia_fixtures.py

+        ixia_api_serv_passwd (pytest fixture): ixia_api_serv_passwd fixture.
+        ixia_api_serv_port (pytest fixture): ixia_api_serv_port fixture.
+        ixia_api_serv_session_id (pytest fixture): ixia_api_serv_session_id 
+            fixture.


No need to start a new line here

The comment was crossing 80 line boundary so I put an newline there. So I think no Change is needed here.

tests/common/ixia/ixia_fixtures.py

baiwei0427 · 2020-07-20T05:46:08Z

tests/common/ixia/ixia_fixtures.py

+
+@pytest.fixture(scope = "function")
+def ixia_api_server_session(ixia_api_serv_ip,
+    ixia_api_serv_user, ixia_api_serv_passwd, ixia_api_serv_port,


Arguments should be aligned.

I have corrected the alignment according to the PEP-8 guidelines.

baiwei0427 · 2020-07-20T05:46:25Z

tests/common/ixia/ixia_fixtures.py

+            fixture.
+
+    Returns:
+        IxNetwork Session Id


This function should return session instead of session ID

In fact this fixture is returning the IxNetwork "session". Return statement in the documentation requires change from "session ID" to session

Corrected the docstring
Returns: Returns:
IxNetwork Session Id --> IxNetwork Session

baiwei0427 · 2020-07-20T05:56:54Z

tests/common/ixia/ixia_fixtures.py

+    Returns:
+        Ixia PTF server (API server) session id.
+    """
+    return duthost.host.options['variable_manager']._hostvars[duthost.hostname]['secret_group_vars']['ixia_api_server']['session_id']


What if users do not specify session_id in configuration file? What will this function return?

Session Id is presumed as a mandatory parameter in the configuration file. So if it is not there we may get a key error (KeyError: 'session_Id'). If we don’t want to connect to particular session we give "None" as sessionID. No Change is needed.

baiwei0427 · 2020-07-20T05:57:22Z

tests/common/ixia/ixia_fixtures.py

+                               Password = ixia_api_serv_passwd,
+                               RestPort = ixia_api_serv_port,
+                               SessionId = ixia_api_serv_session_id,
+                               LogLevel='all')


Why loglevel is all?

LogLevel='all' is remove from the code.

How about adding loglevel as a function argument?

baiwei0427 · 2020-07-20T05:59:33Z

tests/common/ixia/ixia_fixtures.py

+    """
+
+    if (ixia_api_serv_session_id != "None") :
+        session = SessionAssistant(IpAddress = ixia_api_serv_ip,


Align function arguments

Taken care aligned the function arguments.

baiwei0427 · 2020-07-20T06:01:00Z

tests/common/ixia/ixia_fixtures.py

+        IxNetwork Session Id
+    """
+
+    if (ixia_api_serv_session_id != "None") :


Why this is "None" rather than None

We can give the value of session_id "None" there, in the configuration file. When session ID is not "None" we connect to given session ID. If it is none then we connect to the respective session ID.

baiwei0427 · 2020-07-20T06:40:21Z

tests/common/ixia/ixia_fixtures.py

+    ixNetwork   = session.Ixnetwork
+    ixNetwork.NewConfig()
+
+    yield session


It seems that ixia_api_server_session can be divided into a fixture and two functions: ixia_api_server_session (return a new session instance), clear_session (use NewConfig to clear this session) and remove_session. Using yield in this function affects the readability.

The “yield” was added based on earlier comments by Neeta, and I think it is good to have there

I think NewConfig() is unnecessary in this function.

baiwei0427 · 2020-07-20T06:54:16Z

tests/common/ixia/ixia_helpers.py

+        a integer index (starting from 0)
+
+        Args:
+           fanout_data (dict): the dictionary returned by fanout_graph_fact


Can you give a simple example of fanout_data here?

I have provided an example of fan-out data in the comment section.

baiwei0427 · 2020-07-20T06:56:16Z

tests/common/ixia/ixia_helpers.py

+        Args:
+            This function takes no argument.
+
+        Returns:


Returns -> Return

Google style docstring recommends “Returns” instead of “Return”(https://google.github.io/styleguide/pyguide.html). So, I kept it as it is.

baiwei0427 · 2020-07-20T07:00:00Z

tests/common/ixia/ixia_helpers.py

+            Dictionary of chassis card port information.
+        """
+        retval = []
+        for ports in self.current_ixia_port_list:   


ports -> port

I have change the name from ports to port.

baiwei0427 · 2020-07-20T07:21:51Z

tests/common/ixia/ixia_helpers.py

+    name='Topology 1', 
+    ip_type='ipv4', 
+    ip_start='10.0.0.1', 
+    ip_incr_step='0.0.1.0', 


ip_incr_step should be '0.0.0.1'

Implemented.

baiwei0427 · 2020-07-20T07:22:01Z

tests/common/ixia/ixia_helpers.py

+    ip_start='10.0.0.1', 
+    ip_incr_step='0.0.1.0', 
+    gw_start='10.0.0.2', 
+    gw_incr_step='0.0.1.0'):


This should be '0.0.0.0'

Implemented

baiwei0427 · 2020-07-20T07:32:28Z

tests/common/ixia/ixia_helpers.py

+        src_start_port (int): The start port number.
+        src_port_count (int): The number of ports involved in sending traffic 
+            starting from src_start_port number. Example, if the start port is 
+            port2 and port2 to port5 is sending traffic then src_start_port = 2


I think it should be 'port2 to port4 are sending'

baiwei0427 · 2020-07-20T07:38:52Z

tests/common/ixia/ixia_helpers.py

+           'example_traffic'.
+        traffic_type (str, optional): Type of the ip source and destination
+        (ipv4/ipv6). Default traffic_type is 'ipv4'.
+


The meanings of 'src_first_route_index', 'src_route_count', 'dst_fist_route_index' and 'dst_route_count' are still unclear to me.

I have updated these comments

lgtm-com · 2020-07-21T12:07:37Z

This pull request introduces 3 alerts when merging b3970a9 into 64e6f08 - view on LGTM.com

new alerts:

1 for Unused import
1 for Wrong number of arguments in a call
1 for Wrong number of arguments in a class instantiation

neethajohn

Some cosmetic comments. Overall looks good. Can you also address the LGTM alerts

tests/common/ixia/common_helpers.py

tests/common/ixia/ixia_fixtures.py

tests/common/ixia/ixia_helpers.py

lgtm-com · 2020-07-22T05:52:51Z

This pull request introduces 3 alerts when merging fc12785 into 84f6687 - view on LGTM.com

new alerts:

2 for Wrong name for an argument in a class instantiation
1 for Unused import

57de13b [config] fix interface IPv6 address removal. (sonic-net#1819) (sonic-net#1909)

[ixia/Keysight] sample test cases and fixtures

06cd811

This directory should contain the Ixia ECN and RDMA test cases. The lib folder contains * fixtures required to run ixia test cases * helper function required to write ixia test cases

yxieca reviewed Jun 25, 2020

View reviewed changes

baiwei0427 reviewed Jun 25, 2020

View reviewed changes

[ixia/Keysight] adding ixia libraries

b6a0963

As per review comments given against the 1819 we have to keep the ixia libraries under sonic-mgmt/tests/common/ixia/

abhijit-dhar added 3 commits June 26, 2020 17:45

[ixia/Keysight] This file should be in common/ixia

2a8ad2b

This file should be in tests/common/ixia folder. So deleting it from tests/ixia/lib/ folder

[ixia/Keysight] lib/ixia_fixtures.py

694ae89

This file should be under the folder tests/common/ixia Hence deleting it form tests/ixia/lib

[ixia/Keysight] deleting ixia_helpers.py

b5316e8

This file should be under tests/common/ixia So deleting it from tests/ixia/lib

neethajohn reviewed Jun 26, 2020

View reviewed changes

abhijit-dhar added 5 commits July 1, 2020 16:07

Update test_ixia_traffic_restpy.py

61ff293

[ixia/Keysight] Changed as per review comments

721b6b1

Added a new fixture ixia_api_server_session

[ixia/Keysight] added new IxiaFanoutManager class

4833513

This class is added to efficiently manage ixia fanout devices

[ixia/Keysight] Adding __init__().py

b0f1bf6

Making ixia directory a package, to access ixia_fixtures.py and ixia_helpers.py

abhijit-dhar added 2 commits July 1, 2020 16:42

[ixia/Keysight] corrected typo added a new class

cbf0235

Added a new class that should make chassis port management easier

[ixia/Keysight] Deleted from the review

84f3201

There was an typo

abhijit-dhar added 2 commits July 1, 2020 17:08

[ixia/Keysight] adding ixia_fixtures

f0098d1

Deletes accidentally

[ixia/Keysight] updated ixia_fixtures.py

b91dc5e

baiwei0427 reviewed Jul 20, 2020

View reviewed changes

tests/common/ixia/ixia_fixtures.py Show resolved Hide resolved

baiwei0427 reviewed Jul 20, 2020

View reviewed changes

tests/common/ixia/ixia_fixtures.py Show resolved Hide resolved

baiwei0427 reviewed Jul 20, 2020

View reviewed changes

tests/common/ixia/ixia_fixtures.py Show resolved Hide resolved

baiwei0427 reviewed Jul 20, 2020

View reviewed changes

Implementing the comments of PR1819

b3970a9

neethajohn reviewed Jul 22, 2020

View reviewed changes

Updating the files as per the comment made on PR1819

fc12785

neethajohn approved these changes Jul 22, 2020

View reviewed changes

yxieca merged commit ca0ffd0 into sonic-net:master Jul 25, 2020

kazinator-arista pushed a commit to kazinator-arista/sonic-mgmt that referenced this pull request Mar 4, 2026

Update sonic-utilities submodule

5f2d926

57de13b [config] fix interface IPv6 address removal. (sonic-net#1819) (sonic-net#1909)

Conversation

abhijit-dhar commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

Uh oh!

lgtm-com bot commented Jun 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yxieca commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yxieca commented Jun 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Jun 26, 2020

Uh oh!

lgtm-com bot commented Jun 26, 2020

Uh oh!

lgtm-com bot commented Jun 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhijit-dhar commented Jun 25, 2020 •

edited

Loading

yxieca commented Jun 25, 2020 •

edited

Loading