Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: [Snappi] Different speed ingress and egress interface causes snappi-api to crash #12966

Closed
amitpawar12 opened this issue May 23, 2024 · 10 comments

Comments

@amitpawar12
Copy link
Contributor

Issue Description

When we try to have a ingress and egress with different speeds, the snappi_api crashes. There is assert in snappi_fixture to disallow ports of different speeds but if we really need to have let's say 400Gbps ingress and 100Gbps egress port test, there is no way to achieve it.

We need to support multi-speed interfaces via Snappi.

Current crash: 

snappi_tests/multidut/systest/files/pfcwd_multidut_helper.py:316: in run_pfc_test
snappi_extra_params=snappi_extra_params)
common/snappi_tests/traffic_generation.py:589: in run_sys_traffic
api.set_config(config)


self = <snappi_ixnetwork.snappi_api.Api object at 0x7f8161e4c810>, config = <snappi.snappi.Config object at 0x7f8161b5adc0>

def set_config(self, config):
    """Set or update the configuration"""
    try:
        if isinstance(config, (type(self._config_type), str)) is False:
            raise TypeError(
                "The content must be of type Union[Config, str]"
            )

        if isinstance(config, str) is True:
            config = self._config_type.deserialize(config)
        self.config_ixnetwork(config)
    except Exception as err:
      raise SnappiIxnException(err)

E SnappiIxnException: File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/snappi_api.py", line 214, in set_config
E self.config_ixnetwork(config)
E File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/snappi_api.py", line 258, in config_ixnetwork
E self.vport.config()
E File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/vport.py", line 157, in config
E self._set_location()
E File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/vport.py", line 293, in _set_location
E layer1_check = self._api.resource_group.set_group()
E File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/resourcegroup.py", line 68, in set_group
E "Please check the speed of these ports ", error_ports
E Port 1

config = <snappi.snappi.Config object at 0x7f8161b5adc0>
err = Exception('Please check the speed of these ports ', ['Port 1', 'Port 2'])
self = <snappi_ixnetwork.snappi_api.Api object at 0x7f8161e4c810>

Results you see

Crash reported above.

Results you expected to see

Test should allow multi-speed ingress and egress port selection.

Is it platform specific

generic

Relevant log output

No response

Output of show version

No response

Attach files (if any)

No response

@abdosi
Copy link
Contributor

abdosi commented Jun 12, 2024

@kamalsahu0001 / @selldinesh please check on this and update with ETA

@kamalsahu0001
Copy link
Contributor

Current workaround is to replace below code in snappi_fixtures.py to support multi-speed. Will raise PR once the confirmation is there from @amitpawar12

config.options.port_options.location_preemption = True
layer1 = config.layer1.layer1()[-1]
layer1.name = 'port settings'
# layer1.port_names = [port.name for port in config.ports]
# layer1.ieee_media_defaults = False
# layer1.auto_negotiation.rs_fec = True
# layer1.auto_negotiation.link_training = False
# layer1.speed = port_speed

for port in config.ports:
    for snappi_port in temp_tg_port:
        if snappi_port['location']==port.location:
            layer1.port_names  = [port.name]
            layer1.ieee_media_defaults = False
            layer1.auto_negotiation.rs_fec = True
            layer1.auto_negotiation.link_training = False
            layer1.speed = snappi_port['speed']
            layer1.auto_negotiate = False

@amitpawar12
Copy link
Contributor Author

@kamalsahu0001 - Where is temp_tg_port defined?

Please let me know.

Thanks,

@amitpawar12
Copy link
Contributor Author

@kamalsahu0001:

This is not working.

What I tried:
(1)

    for port in new_snappi_ports:
             l1_config.speed = 'speed_{}_gbps'.format(str(int(port['speed'])/1000))

Result:
The l1_config.speed takes the speed of the last interface in new_snappi_port which is 400Gbps and then crashes because it is expecting to see speed on 100Gbps interface as well.

Crash:

common/snappi_tests/traffic_generation.py:847: in run_sys_traffic
    api.set_config(config)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <snappi_ixnetwork.snappi_api.Api object at 0x7f42f5893050>, config = <snappi.snappi.Config object at 0x7f42f5c4e230>

    def set_config(self, config):
        """Set or update the configuration"""
        try:
            if isinstance(config, (type(self._config_type), str)) is False:
                raise TypeError(
                    "The content must be of type Union[Config, str]"
                )
    
            if isinstance(config, str) is True:
                config = self._config_type.deserialize(config)
            self.config_ixnetwork(config)
        except Exception as err:
>           raise SnappiIxnException(err)
E           SnappiIxnException:   File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/snappi_api.py", line 214, in set_config
E               self.config_ixnetwork(config)
E             File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/snappi_api.py", line 258, in config_ixnetwork
E               self.vport.config()
E             File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/vport.py", line 157, in config
E               self._set_location()
E             File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/vport.py", line 293, in _set_location
E               layer1_check = self._api.resource_group.set_group()
E             File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/resourcegroup.py", line 68, in set_group
E               "Please check the speed of these ports ", error_ports
E           Port 0

(2)
Trying to set the speed attribute in API as a list:

l1_config.speed = ['speed_{}_gbps'.format(str(int(port['speed'])/1000)) for port in new_snappi_ports]

Result:
I see that it does not like the list part and expects a single variable.

Crash:

self = <snappi_ixnetwork.snappi_api.Api object at 0x7f5370783390>, config = <snappi.snappi.Config object at 0x7f537c4970a0>

    def set_config(self, config):
        """Set or update the configuration"""
        try:
            if isinstance(config, (type(self._config_type), str)) is False:
                raise TypeError(
                    "The content must be of type Union[Config, str]"
                )
    
            if isinstance(config, str) is True:
                config = self._config_type.deserialize(config)
            self.config_ixnetwork(config)
        except Exception as err:
>           raise SnappiIxnException(err)
E           SnappiIxnException:   File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/snappi_api.py", line 214, in set_config
E               self.config_ixnetwork(config)
E             File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/snappi_api.py", line 253, in config_ixnetwork
E               self.validation.validate_config()
E             File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/validation.py", line 16, in validate_config
E               self.__check_config_objects(self._api.snappi_config, item_ids=[])
E             File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/validation.py", line 61, in __check_config_objects
E               self.__check_config_objects(item, item_ids)
E             File "/usr/local/lib/python2.7/dist-packages/snappi_ixnetwork/validation.py", line 23, in __check_config_objects
E               config_item.validate()
E             File "/usr/local/lib/python2.7/dist-packages/snappi/snappi.py", line 688, in validate
E               self._validate_types(key, value)
E             File "/usr/local/lib/python2.7/dist-packages/snappi/snappi.py", line 636, in _validate_types
E               self.__class__,
E            property speed shall be one of these ['speed_10_fd_mbps', 'speed_10_hd_mbps', 'speed_100_fd_mbps', 'speed_100_hd_mbps', 'speed_1_gbps', 'speed_10_gbps', 'speed_25_gbps', 'speed_40_gbps', 'speed_50_gbps', 'speed_100_gbps', 'speed_200_gbps', 'speed_400_gbps'] enum, but got ['speed_100_gbps', 'speed_400_gbps'] at <class 'snappi.snappi.Layer1'>

config     = <snappi.snappi.Config object at 0x7f537c4970a0>
err        = TypeError("property speed shall be one of these ['speed_10_fd_mbps', 'speed_10...t got ['speed_100_gbps', 'speed_400_gbps'] at <class 'snappi.snappi.Layer1'>",)
self       = <snappi_ixnetwork.snappi_api.Api object at 0x7f5370783390>

@kamalsahu0001
Copy link
Contributor

@amitpawar12 Can you giving the port numbers as specified below for multi-speed. Instead of Card1/Port2 for 400G port, you can use directly Port2 for 400G.

StartDevice,StartPort,EndDevice,EndPort,BandWidth,VlanID,VlanMode
sonic-s6100-dut1,Ethernet0,snappi-sonic,Port1.1,100000,2,Access
sonic-s6100-dut1,Ethernet8,snappi-sonic,Port2, 400000,2,Access

@amitpawar12
Copy link
Contributor Author

@kamalsahu0001 ,

Thanks for the update. I tried these changes. I needed to update the snappi_dut_base_config to iterate over the ports as well.

The test ran well. However, I ran into issue when I tried the Pause Frames.

I am seeing this warning message in IxNetwork for Pause Frames:

02:01:04 snappi_api.info                          L1132 INFO   | IxNet - Port does not support given protocol stack when not in data center mode.

I ignored the issue and went ahead with test. I saw that one of the IXIA transmitting interfaces was receiving the PFCs but it was quietly dropping the PFCs frames and, hence ignoring it.

Way to reproduce this:

  • Configure the ports as you have described in connection_graph.xml.
  • Modify the snappi_dut_base_config to iterate over the ports and add L1 config per port.
  • Ensure that test has 2 ingress interfaces and 1 egress interface ( of any speed).
  • Ensure that test has PFC frames.

The test should throw the above warning and one of the ingress interfaces should be receiving PFCs but ignoring it.

Thanks,
-A

@kamalsahu0001
Copy link
Contributor

kamalsahu0001 commented Jul 31, 2024 via email

@amitpawar12
Copy link
Contributor Author

Yeah. I went through the code again and seems like this piece of the code is one making the difference, not sure how though!

    config.options.port_options.location_preemption = True
    l1_config = config.layer1.layer1()[-1]
    l1_config.name = 'L1 config'
    for port in config.ports:
        for snappi_port in new_snappi_ports:
            if snappi_port['location']==port.location:
                #import pdb; pdb.set_trace()
                l1_config.port_names = [port.name]
                l1_config.speed = 'speed_'+str(int(int(snappi_port['speed'])/1000))+'_gbps'
                l1_config.ieee_media_defaults = False
                l1_config.auto_negotiate = False
                l1_config.auto_negotiation.link_training = False
                l1_config.auto_negotiation.rs_fec = True

This seems to be enabling the FCOE only for the last port in the list (of the ports). Rest of the ports do not have FCOE enabled.

Please let me know if u know, why that would happen.

Thanks,
-A

@kamalsahu0001
Copy link
Contributor

@amitpawar12 .. Can you please move the lines after that line inside the for loop.

for port in config.ports:
    for index,snappi_port in enumerate(new_snappi_ports):
        if snappi_port['location']==port.location:
            l1_config = config.layer1.layer1()[-1]
            l1_config.name = 'L1 config {}'.format(index)
            l1_config.port_names = [port.name]
            l1_config.speed = 'speed_'+str(int(int(snappi_port['speed'])/1000))+'_gbps'
            l1_config.ieee_media_defaults = False
            l1_config.auto_negotiate = False
            l1_config.auto_negotiation.link_training = False
            l1_config.auto_negotiation.rs_fec = True
            pfc = l1_config.flow_control.ieee_802_1qbb
            pfc.pfc_delay = 0
        #[setattr(pfc, 'pfc_class_{}'.format(i), i) for i in range(8)]
        if pfcQueueGroupSize == 8:
            pfc.pfc_class_0 = 0
            pfc.pfc_class_1 = 1
            pfc.pfc_class_2 = 2
            pfc.pfc_class_3 = 3
            pfc.pfc_class_4 = 4
            pfc.pfc_class_5 = 5
            pfc.pfc_class_6 = 6
            pfc.pfc_class_7 = 7
        elif pfcQueueGroupSize == 4:
            pfc.pfc_class_0 = pfcQueueValueDict[0]
            pfc.pfc_class_1 = pfcQueueValueDict[1]
            pfc.pfc_class_2 = pfcQueueValueDict[2]
            pfc.pfc_class_3 = pfcQueueValueDict[3]
            pfc.pfc_class_4 = pfcQueueValueDict[4]
            pfc.pfc_class_5 = pfcQueueValueDict[5]
            pfc.pfc_class_6 = pfcQueueValueDict[6]
            pfc.pfc_class_7 = pfcQueueValueDict[7]
        else:
            pytest_assert(False, 'pfcQueueGroupSize value is not 4 or 8')

yejianquan pushed a commit that referenced this issue Dec 12, 2024
…and egress tests. (#14856)

Description of PR
Summary:
Existing snappi_dut_base_config in tests/common/snappi_tests/snappi_fixtures.py has an assert in case, mixed-speed ingress and egress interfaces are selected.

Since the interface speeds were same, the L1 configuration was done ONLY once.

With mixed-speed interfaces being used as ingress and egress, the assert needs to be removed.

Second issue with existing snappi_multi_base_config was that speed was set to ONLY one of the interfaces being used for the test. This was incorrect for mixed speed interfaces, causing Snappi API itself to crash.

Fixes # (issue)
#12966

Approach
What is the motivation for this PR?
Existing snappi_dut_base_config asserts when ingress and egress interface speeds are different.

Furthermore, snappi framework itself did not support mixed-speed interfaces for the test and crashed (Please see issue #12966 ) for the same.

How did you do it?
Added a new function - snappi_sys_base_config which replaces the assert with info level log indicating that interfaces are of different speeds.

The L1 configuration is done for all the snappi_ports and set appropriate speed for all the snappi_ports. Ideally, existing snappi_dut_base_config could be modified with additional argument mixed-speed=NONE, and then selectively run the code for the mixed-speed=TRUE. However, this being frequently used function, I will keep it as is, and add a new function to ensure, existing function is not broken.

How did you verify/test it?
Ran on the local clone with mixed and same speed interfaces. No issues seen.

Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation

co-authorized by: jianquanye@microsoft.com
mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this issue Dec 12, 2024
…and egress tests. (sonic-net#14856)

Description of PR
Summary:
Existing snappi_dut_base_config in tests/common/snappi_tests/snappi_fixtures.py has an assert in case, mixed-speed ingress and egress interfaces are selected.

Since the interface speeds were same, the L1 configuration was done ONLY once.

With mixed-speed interfaces being used as ingress and egress, the assert needs to be removed.

Second issue with existing snappi_multi_base_config was that speed was set to ONLY one of the interfaces being used for the test. This was incorrect for mixed speed interfaces, causing Snappi API itself to crash.

Fixes # (issue)
sonic-net#12966

Approach
What is the motivation for this PR?
Existing snappi_dut_base_config asserts when ingress and egress interface speeds are different.

Furthermore, snappi framework itself did not support mixed-speed interfaces for the test and crashed (Please see issue sonic-net#12966 ) for the same.

How did you do it?
Added a new function - snappi_sys_base_config which replaces the assert with info level log indicating that interfaces are of different speeds.

The L1 configuration is done for all the snappi_ports and set appropriate speed for all the snappi_ports. Ideally, existing snappi_dut_base_config could be modified with additional argument mixed-speed=NONE, and then selectively run the code for the mixed-speed=TRUE. However, this being frequently used function, I will keep it as is, and add a new function to ensure, existing function is not broken.

How did you verify/test it?
Ran on the local clone with mixed and same speed interfaces. No issues seen.

Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation

co-authorized by: jianquanye@microsoft.com
mssonicbld pushed a commit that referenced this issue Dec 12, 2024
…and egress tests. (#14856)

Description of PR
Summary:
Existing snappi_dut_base_config in tests/common/snappi_tests/snappi_fixtures.py has an assert in case, mixed-speed ingress and egress interfaces are selected.

Since the interface speeds were same, the L1 configuration was done ONLY once.

With mixed-speed interfaces being used as ingress and egress, the assert needs to be removed.

Second issue with existing snappi_multi_base_config was that speed was set to ONLY one of the interfaces being used for the test. This was incorrect for mixed speed interfaces, causing Snappi API itself to crash.

Fixes # (issue)
#12966

Approach
What is the motivation for this PR?
Existing snappi_dut_base_config asserts when ingress and egress interface speeds are different.

Furthermore, snappi framework itself did not support mixed-speed interfaces for the test and crashed (Please see issue #12966 ) for the same.

How did you do it?
Added a new function - snappi_sys_base_config which replaces the assert with info level log indicating that interfaces are of different speeds.

The L1 configuration is done for all the snappi_ports and set appropriate speed for all the snappi_ports. Ideally, existing snappi_dut_base_config could be modified with additional argument mixed-speed=NONE, and then selectively run the code for the mixed-speed=TRUE. However, this being frequently used function, I will keep it as is, and add a new function to ensure, existing function is not broken.

How did you verify/test it?
Ran on the local clone with mixed and same speed interfaces. No issues seen.

Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation

co-authorized by: jianquanye@microsoft.com
@amitpawar12
Copy link
Contributor Author

This issue is resolved via PR #14856 . Hence closing this issue.

Thanks,

mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this issue Jan 2, 2025
…and egress tests. (sonic-net#14856)

Description of PR
Summary:
Existing snappi_dut_base_config in tests/common/snappi_tests/snappi_fixtures.py has an assert in case, mixed-speed ingress and egress interfaces are selected.

Since the interface speeds were same, the L1 configuration was done ONLY once.

With mixed-speed interfaces being used as ingress and egress, the assert needs to be removed.

Second issue with existing snappi_multi_base_config was that speed was set to ONLY one of the interfaces being used for the test. This was incorrect for mixed speed interfaces, causing Snappi API itself to crash.

Fixes # (issue)
sonic-net#12966

Approach
What is the motivation for this PR?
Existing snappi_dut_base_config asserts when ingress and egress interface speeds are different.

Furthermore, snappi framework itself did not support mixed-speed interfaces for the test and crashed (Please see issue sonic-net#12966 ) for the same.

How did you do it?
Added a new function - snappi_sys_base_config which replaces the assert with info level log indicating that interfaces are of different speeds.

The L1 configuration is done for all the snappi_ports and set appropriate speed for all the snappi_ports. Ideally, existing snappi_dut_base_config could be modified with additional argument mixed-speed=NONE, and then selectively run the code for the mixed-speed=TRUE. However, this being frequently used function, I will keep it as is, and add a new function to ensure, existing function is not broken.

How did you verify/test it?
Ran on the local clone with mixed and same speed interfaces. No issues seen.

Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation

co-authorized by: jianquanye@microsoft.com
mssonicbld pushed a commit that referenced this issue Jan 3, 2025
…and egress tests. (#14856)

Description of PR
Summary:
Existing snappi_dut_base_config in tests/common/snappi_tests/snappi_fixtures.py has an assert in case, mixed-speed ingress and egress interfaces are selected.

Since the interface speeds were same, the L1 configuration was done ONLY once.

With mixed-speed interfaces being used as ingress and egress, the assert needs to be removed.

Second issue with existing snappi_multi_base_config was that speed was set to ONLY one of the interfaces being used for the test. This was incorrect for mixed speed interfaces, causing Snappi API itself to crash.

Fixes # (issue)
#12966

Approach
What is the motivation for this PR?
Existing snappi_dut_base_config asserts when ingress and egress interface speeds are different.

Furthermore, snappi framework itself did not support mixed-speed interfaces for the test and crashed (Please see issue #12966 ) for the same.

How did you do it?
Added a new function - snappi_sys_base_config which replaces the assert with info level log indicating that interfaces are of different speeds.

The L1 configuration is done for all the snappi_ports and set appropriate speed for all the snappi_ports. Ideally, existing snappi_dut_base_config could be modified with additional argument mixed-speed=NONE, and then selectively run the code for the mixed-speed=TRUE. However, this being frequently used function, I will keep it as is, and add a new function to ensure, existing function is not broken.

How did you verify/test it?
Ran on the local clone with mixed and same speed interfaces. No issues seen.

Any platform specific information?
Supported testbed topology if it's a new test case?
Documentation

co-authorized by: jianquanye@microsoft.com
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

4 participants