-
Notifications
You must be signed in to change notification settings - Fork 817
[dash] Add Privatelink traffic test #14757
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
Conversation
Signed-off-by: Prabhat Aravind <[email protected]>
Signed-off-by: Lawrence Lee <[email protected]>
Signed-off-by: Lawrence Lee <[email protected]>
@mukeshmv to review |
tests/dash/test_dash_privatelink.py
Outdated
|
||
@pytest.fixture(scope="module") | ||
def dpu_ip(duthost, dpu_index): | ||
cmd = f"ip addr show | grep Ethernet-BP{dpu_index} | grep inet | awk '{{print $2}}'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface is vendor-specific. It should be taken from platform.json
file: https://github.com/sonic-net/SONiC/blob/master/doc/smart-switch/pmon/smartswitch-pmon.md#313-npu-to-dpu-data-port-mapping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example of Nvidia platform:
sonic-net/sonic-buildimage#20358
Signed-off-by: Lawrence Lee <[email protected]>
Signed-off-by: Lawrence Lee <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a test plan to explain the test? espeicallly for the privatelink_config,
What do the variables defined in the file of privatelink_config.py mean? If there is a chart to introduce the different ips in the test, it will be better.
Thanks
@theasianpianist the test misses the data plane interface configuration for both the NPU and DPU sides. There is a static route that the test adds on the NPU side to send traffic to the DPU, but no IP address configuration on the interfaces. Therefore, the test won’t work without some manual pre-configuration. My assumption is that the test should be atomic and apply the full configuration needed to pass. The test should be extended to set the IP address on the NPU, and the IP address and VIP address on the DPU. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As commented
tests/dash/test_dash_privatelink.py
Outdated
logger.info(messages) | ||
apply_messages(localhost, duthost, ptfhost, messages, dpu_index) | ||
|
||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no clean up of the dash configurations.
/azp run Azure.sonic-mgmt |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run Azure.sonic-mgmt |
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Lawrence Lee <[email protected]>
dash_info[LOCAL_PTF_INTF] = minigraph_facts["minigraph_ptf_indices"][intf] | ||
dash_info[LOCAL_PTF_MAC] = neigh_table["v4"][neigh_ip]["macaddress"] | ||
if topo == 'dpu-1' and REMOTE_PA_IP not in dash_info: | ||
if (topo == 'dpu-1' or topo == "t1-28-lag") and REMOTE_PA_IP not in dash_info: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to add smartswitch-t1 topo as well here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll do this later. Ok to merge for now.
Signed-off-by: Lawrence Lee <[email protected]>
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@oleksandrivantsiv , can you signoff? |
depends on sonic-net/sonic-buildimage#21553 |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Also depends on sonic-net/sonic-buildimage#21714 |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Commenter does not have sufficient privileges for PR 14757 in repo sonic-net/sonic-mgmt |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Lawrence Lee <[email protected]>
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Lawrence Lee <[email protected]>
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
[dash] Add Privatelink traffic test
@@ -23,7 +23,8 @@ def __init__(self, duthost): | |||
self.gnmi_client_cert = "gnmiclient.crt" | |||
self.gnmi_client_key = "gnmiclient.key" | |||
self.gnmi_server_start_wait_time = 30 | |||
self.enable_zmq = duthost.shell("netstat -na | grep -w 8100", module_ignore_errors=True)['rc'] == 0 | |||
# self.enable_zmq = duthost.shell("netstat -na | grep -w 8100", module_ignore_errors=True)['rc'] == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theasianpianist , why change dash test case always enable ZMQ on GNMI?
If someone run these test case on a device not enable ZMQ on orchagent side, some test case may failed, can you check this change?
Hi @theasianpianist |
Description of PR
Summary:
Fixes #14613
Adds outbound and inbound traffic tests for Privatelink
Type of change
Back port request
PR testes are failing, dependent on sonic-net/sonic-buildimage#21553
Approach
What is the motivation for this PR?
Add test cases to cover Privatelink traffic scenarios
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?
Smartswitch testbed only
Documentation