[Fix 20284]: Enhance smartswitch environment variables parsing by Pterosaur · Pull Request #21209 · sonic-net/sonic-buildimage

Pterosaur · 2024-12-18T09:47:18Z

Why I did it

Fix #20284

In 202405 and above, two extra steps are added before the start of every container which checks NUM_DPU and IS_DPU_DEVICE by parsing the platform.json file using the jq tool. This is only relevant for Smartswitch. However, this is adding some delay during the reconciliation phase of WR/FR resulting

When there is load on CPU, both the jq calls are adding > 1 sec to the start of swss and almost 0.5 sec to the start of syncd. There are also present in teamd and bgp container start flow which may cause extra contention on the CPU.

Work item tracking

Microsoft ADO (number only):

How I did it

Set the environment variables for systemd by systemd-sonic-generator.

How to verify it

Check there is no jq command under the swss.sh start and syncd.sh start from the sonic-bootchart
Regression test:

echo '{"DPUS":{"dpu0":{}, "dpu1":{}}}' | sudo tee /usr/share/sonic/device/x86_64-kvm_x86_64-r0/platform.json
docker rm -f database
sudo reboot

...

admin@vlab-01:~$ sudo systemctl list-unit-files '*midplane*'
UNIT FILE                    STATE           PRESET
midplane-network-dpu.service disabled        enabled
midplane-network-npu.service enabled-runtime  enabled


# The DPU databases should be created
admin@vlab-01:~$ docker ps -a
CONTAINER ID   IMAGE                                COMMAND                  CREATED          STATUS         PORTS     NAMES
4455f7ab611c   docker-database:latest               "/usr/local/bin/dock…"   5 minutes ago    Up 5 minutes             databasedpu0
8e983b5beb27   docker-database:latest               "/usr/local/bin/dock…"   5 minutes ago    Up 5 minutes             databasedpu1

# Environment variables should be inserted to the service file
admin@vlab-01:~$ cat $(sudo find /etc -name 'swss*service')
[Unit]
Description=switch state service
Requires=database@dpu0.service

Requires=database@dpu1.service

After=database@dpu0.service

After=database@dpu1.service

Requires=config-setup.service
After=config-setup.service
BindsTo=sonic.target
After=sonic.target
Before=ntp-config.service
StartLimitIntervalSec=1200

StartLimitBurst=3

[Service]
Environment="NUM_DPU=2"
Environment="IS_DPU_DEVICE=false"
User=root
Environment=sonic_asic_platform=vs
ExecStartPre=/usr/local/bin/swss.sh start
ExecStart=/usr/local/bin/swss.sh wait
ExecStop=/usr/local/bin/swss.sh stop
RestartSec=30

[Install]
WantedBy=sonic.target

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

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)

Signed-off-by: Ze Gan <ganze718@gmail.com>

mssonicbld · 2024-12-18T09:50:23Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2024-12-18T09:50:37Z

Azure Pipelines successfully started running 1 pipeline(s).

Pterosaur · 2024-12-18T12:10:02Z

/azpw ms_conflict

Pterosaur · 2024-12-19T00:34:13Z

/azpw ms_conflict

liushilongbuaa · 2024-12-19T02:50:03Z

/azpw ms_conflict

vivekrnv · 2024-12-19T06:47:09Z

src/systemd-sonic-generator/systemd-sonic-generator.cpp

+    env_vars["IS_DPU_DEVICE"] = (smart_switch_dpu ? "true" : "false");
+    env_vars["NUM_DPU"] = std::to_string(num_dpus);
+
+    for (const auto& [key, value] : env_vars) {


I think services files such as database, swss, syncd are enabled at build time.
Are we certain that, these will be started only after systemd-sonic-generator is run atleast once?

Please also verify the changes on smartswitch platform and make sure multiple database instances are created

Thanks for your comments.
Yes, the generator will be run only once and before all services started.
I did confirm the smartswitch scenario and pasted the results on the PR description.

Thanks for testing

vivekrnv · 2024-12-19T07:08:48Z

src/systemd-sonic-generator/systemd-sonic-generator.cpp

+
+    std::unordered_map<std::string, std::string> env_vars;
+    env_vars["IS_DPU_DEVICE"] = (smart_switch_dpu ? "true" : "false");
+    env_vars["NUM_DPU"] = std::to_string(num_dpus);


This is a good approach to solve this issue but i see one setback. If we need to add new env variables, ssg code should be updated. which IMO is not very flexible. I have an idea for generic solution, let me know what you think

Add a oneshot service very early in the boot. Read static env variables (Eg: $PLATFORM, $NUM_ASIC, $NUM_DPU, $SONIC_BOOT_TYPE etc) and write them to a common file /etc/sonic/static-env-variables

We can leverage ssg to write EnvironmentFile=/etc/sonic/static-env-variables option to all the services. making the ssg code minimal and flexible.

We can potentially clean a lot of code under docker_image_ctl with this approach

Good idea, I can have a try.

I tried it, but I feel it wasn't easy yet.

The shell of oneshot service you mentioned might look like the following. But at this time, the database service hasn't ready, so the sonic-cfggen would not work and I have to parse the /host/machine.conf as same as what systemd-sonic-generator did if I would like to get the variable. The SSG has done this by an efficient function, C function, why should I do it again by a shell?

SYSTEMD_ENV_FILE="/etc/sonic/static_env" # Load platform from sonic-cfggen PLATFORM=${PLATFORM:-`sonic-cfggen -H -v DEVICE_METADATA.localhost.platform`} echo "PLATFORM='${PLATFORM}'" >> "$SYSTEMD_ENV_FILE" # Parse environment from platform.json PLATFORM_JSON=/usr/share/sonic/device/$PLATFORM/platform.json if [ -f "$PLATFORM_JSON" ]; then # Environment variables for Smart Switch NUM_DPU=$(jq -r '.DPUS | length' $PLATFORM_JSON 2>/dev/null) if [[ -z "$NUM_DPU" ]]; then NUM_DPU=0 fi jq -e '.DPU' $PLATFORM_JSON >/dev/null if [[ $? -eq 0 ]]; then IS_DPU_DEVICE="true" else IS_DPU_DEVICE="false" fi echo "NUM_DPU='${NUM_DPU}'" >> "$SYSTEMD_ENV_FILE" echo "IS_DPU_DEVICE='${IS_DPU_DEVICE}'" >> "$SYSTEMD_ENV_FILE" fi

As your proposal, if we want to add new env variables, we have to update the shell script. I don't see any difference or challenge in doing this via SSG. If you feel that C code isn't flexible, I have to say we had an old SSG with python code previously, But we discarded it and rewrote it via C due to the efficiency issues.

Yes, script couldn't use DB just like SSG

Hmm, difference is we need to run the script once. Since the oneshot service runs before other services are even started, CPU should be fairly free and should be executed quickly. we do need to benchmark this solution to measure impact.

Advantage being load on SSG is less, all it does it to add EnvironmentFIle= and with some optimization we don't even need to edit and write to the .service file after the first

In any case, i'm okay with the current solution. We might move to the generic oneshot service if required in the future

If this PR looks good to you, could you please help to approve it.

vivekrnv · 2024-12-20T01:22:05Z

src/systemd-sonic-generator/systemd-sonic-generator.cpp

+    env_vars["NUM_DPU"] = std::to_string(num_dpus);
+
+    for (const auto& [key, value] : env_vars) {
+        tmp_file << "Environment=\"" << key << "=" << value << "\"" << std::endl;


Do we need to add Environment= option if the file doesn't have[Service] section in the file

I worry that the Service section might be introduced by other functions or modules in the future. And I don't see any side-effects if I define some environment variables at this point.

Please make sure, systemd won't throw an error if we add Environment= values after the last section without [Service]

If there is no Service section, I will add it.

Sounds good to me

Pterosaur · 2025-01-13T23:29:09Z

Hi @lguohan , Please help to review or merge this PR.

lguohan · 2025-01-19T19:26:42Z

@Pterosaur , can you put delay numbers in the description?

Fix sonic-net#20284 In 202405 and above, two extra steps are added before the start of every container which checks NUM_DPU and IS_DPU_DEVICE by parsing the platform.json file using the jq tool. This is only relevant for Smartswitch. However, this is adding some delay during the reconciliation phase of WR/FR resulting How I did it Set the environment variables for systemd by systemd-sonic-generator. Signed-off-by: Ze Gan <ganze718@gmail.com>

Optmized environment variable parser

93cfde1

Signed-off-by: Ze Gan <ganze718@gmail.com>

Pterosaur requested a review from lguohan as a code owner December 18, 2024 09:47

Pterosaur added Request for 202411 Branch Request for 202405 Branch labels Dec 18, 2024

Pterosaur mentioned this pull request Dec 18, 2024

Checking for DPUs in platform.json is adding delay to WR/FR reconciliation #20284

Closed

Pterosaur requested a review from vivekrnv December 18, 2024 10:53

Pterosaur added Issue for 202311 Request for 202311 Branch and removed Issue for 202311 labels Dec 19, 2024

mssonicbld added the Cherry Pick Conflict_202311 label Dec 19, 2024

Pterosaur removed Request for 202311 Branch Cherry Pick Conflict_202311 labels Dec 19, 2024

vivekrnv reviewed Dec 19, 2024

View reviewed changes

vivekrnv reviewed Dec 20, 2024

View reviewed changes

vivekrnv requested a review from Yakiv-Huryk December 20, 2024 06:27

vivekrnv removed the request for review from Yakiv-Huryk January 13, 2025 19:49

vivekrnv approved these changes Jan 13, 2025

View reviewed changes

lguohan merged commit 7e03462 into sonic-net:master Jan 19, 2025

Conversation

Pterosaur commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I did it

Work item tracking

How I did it

How to verify it

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

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)

Uh oh!

mssonicbld commented Dec 18, 2024

Uh oh!

azure-pipelines bot commented Dec 18, 2024

Uh oh!

Pterosaur commented Dec 18, 2024

Uh oh!

Pterosaur commented Dec 19, 2024

Uh oh!

liushilongbuaa commented Dec 19, 2024

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!

Pterosaur Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Pterosaur commented Jan 13, 2025

Uh oh!

lguohan commented Jan 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Pterosaur commented Dec 18, 2024 •

edited

Loading

Pterosaur Dec 19, 2024 •

edited

Loading