Skip to content

[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

Merged
merged 15 commits into from
Feb 21, 2025
Merged

Conversation

theasianpianist
Copy link
Contributor

@theasianpianist theasianpianist commented Sep 26, 2024

Description of PR

Summary:
Fixes #14613

Adds outbound and inbound traffic tests for Privatelink

Type of change

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

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405

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

@prsunny
Copy link
Contributor

prsunny commented Sep 26, 2024

@mukeshmv to review


@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}}'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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]>
Copy link
Contributor

@JibinBao JibinBao left a 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

@oleksandrivantsiv
Copy link
Contributor

@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.

Copy link
Contributor

@oleksandrivantsiv oleksandrivantsiv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As commented

logger.info(messages)
apply_messages(localhost, duthost, ptfhost, messages, dpu_index)

return
Copy link
Contributor

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.

@StormLiangMS
Copy link
Collaborator

/azp run Azure.sonic-mgmt

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StormLiangMS
Copy link
Collaborator

/azp run Azure.sonic-mgmt

Copy link

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:
Copy link
Contributor

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.

Copy link
Contributor

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.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

prsunny
prsunny previously approved these changes Feb 1, 2025
@prsunny
Copy link
Contributor

prsunny commented Feb 1, 2025

@oleksandrivantsiv , can you signoff?

@prsunny
Copy link
Contributor

prsunny commented Feb 3, 2025

depends on sonic-net/sonic-buildimage#21553

@prsunny
Copy link
Contributor

prsunny commented Feb 10, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prsunny
Copy link
Contributor

prsunny commented Feb 13, 2025

Also depends on sonic-net/sonic-buildimage#21714

@prsunny
Copy link
Contributor

prsunny commented Feb 13, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@theasianpianist
Copy link
Contributor Author

/azp run

Copy link

Commenter does not have sufficient privileges for PR 14757 in repo sonic-net/sonic-mgmt

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lawrence Lee <[email protected]>
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lawrence Lee <[email protected]>
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kperumalbfn kperumalbfn merged commit 1628450 into sonic-net:master Feb 21, 2025
17 checks passed
nnelluri-cisco pushed a commit to nnelluri-cisco/sonic-mgmt that referenced this pull request Mar 15, 2025
@@ -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
Copy link
Contributor

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?

@JibinBao
Copy link
Contributor

JibinBao commented May 6, 2025

Hi @theasianpianist
Since there is no code to configure the dpu ip, npu ip and route, how do you run the test automatically?
Are they configured by the PR #17596?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Test Gap][dash] Privatelink packet transformation tests