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

Add configuration optimization to run SONiC on DPU #18807

Merged
merged 1 commit into from
May 16, 2024

Conversation

oleksandrivantsiv
Copy link
Collaborator

Why I did it

  • The Appliance DPU supports a limited subset of the configuration defined in the SONiC DASH HLD.

  • Configure SWSS and GMNI to communicate on the Appliance DPU.

  • Add a sample configuration for the appliance topology.

Work item tracking
  • Microsoft ADO (number only):

How I did it

Disable configuration unsupported by the Appliance DPU based on the device type.

How to verify it

Run VNET-to-VNET sonic-mgmt test.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@@ -86,6 +86,9 @@ if [[ x"${LOCALHOST_SUBTYPE}" == x"SmartSwitch" ]]; then
else
ORCHAGENT_ARGS+=" -q tcp://127.0.0.1:8100"
fi
# Enable ZMQ for DPU Appliance
elif [[ x"${LOCALHOST_SUBTYPE}" == x"Appliance" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a subtype 'Appliance'. It should be 'SmartSwitch' for both npu and dpu. @Pterosaur

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Appliance is not needed. I removed the changes.

LOCALHOST_SUBTYPE=`sonic-db-cli CONFIG_DB hget "DEVICE_METADATA|localhost" "subtype"`
if [[ x"${LOCALHOST_SUBTYPE}" == x"SmartSwitch" ]]; then
if [[ x"${LOCALHOST_SUBTYPE}" == x"SmartSwitch" || x"${LOCALHOST_SUBTYPE}" == x"Appliance" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, please remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Appliance is not needed. I removed the changes.

def generate_appliance_config(data):
data['DEVICE_METADATA']['localhost']['hostname'] = 'sonic'
data['DEVICE_METADATA']['localhost']['switch_type'] = 'dpu'
data['DEVICE_METADATA']['localhost']['type'] = 'SonicHost'
Copy link
Contributor

Choose a reason for hiding this comment

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

this is SonicDpu

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed this file from the PR. I'll raise another PR after we close the question with the default topology name for the DPU.

@oleksandrivantsiv oleksandrivantsiv changed the title Add configuration optimization to run SONiC on Appliance DPU Add configuration optimization to run SONiC on DPU May 3, 2024
Copy link
Contributor

@prabhataravind prabhataravind left a comment

Choose a reason for hiding this comment

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

lgtm

prsunny
prsunny previously approved these changes May 6, 2024
@liat-grozovik
Copy link
Collaborator

@prsunny please help to review the ms_conflict checker and please let us know what is the next step to have this PR included

@prsunny
Copy link
Contributor

prsunny commented May 9, 2024

@liat-grozovik , @oleksandrivantsiv , working on the conflict. Will merge after the conflict is resolved

@@ -18,13 +18,15 @@
[
{
"SWITCH_TABLE:switch": {
{% if DEVICE_METADATA.localhost.switch_type != "dpu" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add modify to this - {% if DEVICE_METADATA.localhost.switch_type and DEVICE_METADATA.localhost.switch_type != "dpu" %}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By default, this configuration should be enabled. We want to disable it only for DPU.
The switch_type attribute in DEVICE_METADATA table is optional. When the attribute is missing the assumption is that the type is switch. The condition for this should be the following:
{% if not DEVICE_METADATA.localhost.switch_type or DEVICE_METADATA.localhost.switch_type != "dpu" %}
@prsunny can you please confirm this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@oleksandrivantsiv , I feel thats still not going to achieve the intended functionality with 'or'. One condition will be true when swtich_type is present even with 'dpu' and continues with setting attributes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The condition is 'if not DEVICE_METADATA.localhost.switch_type or DEVICE_METADATA.localhost.switch_type != "dpu"'.

It should hit in two cases. When the switch_type is not present in the table:
image
Or when switch_type is not equal to dpu
image
And should not hit when the switch_type is dpu
image

@lguohan
Copy link
Collaborator

lguohan commented May 12, 2024

@prsunny , please faciltate to address ms_conflict test failure.

@Pterosaur
Copy link
Contributor

/azpw ms_conflict

@Pterosaur
Copy link
Contributor

@prsunny , please faciltate to address ms_conflict test failure.

@lguohan The ms_conflict is passed.

@liat-grozovik liat-grozovik merged commit c5a2eba into sonic-net:master May 16, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants