Skip to content

[Fix 20284]: Enhance smartswitch environment variables parsing #21209

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.

Sign up for GitHub

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
lguohan merged 1 commit into from
Jan 19, 2025
Merged

[Fix 20284]: Enhance smartswitch environment variables parsing #21209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions files/build_templates/docker_image_ctl.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,18 +372,6 @@ start() {
source $PLATFORM_ENV_CONF
fi

# Parse the platform.json file to get the platform specific information
PLATFORM_JSON=/usr/share/sonic/device/$PLATFORM/platform.json
if [ -f "$PLATFORM_JSON" ]; then
NUM_DPU=$(jq -r '.DPUS | length' $PLATFORM_JSON 2>/dev/null)
jq -e '.DPU' $PLATFORM_JSON >/dev/null
if [[ $? -eq 0 ]]; then
IS_DPU_DEVICE="true"
else
IS_DPU_DEVICE="false"
fi
fi

{%- if sonic_asic_platform == "broadcom" %}
{%- if docker_container_name == "syncd" %}
# Set the SYNCD_SHM_SIZE if this variable not defined
Expand Down
31 changes: 30 additions & 1 deletion src/systemd-sonic-generator/ssg-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ class SsgMainTest : public SsgFunctionTest {
while (getline(file, line) && !found) {
if (str == line) {
found = true;
break;
}
}
return found;
Expand Down Expand Up @@ -364,6 +365,32 @@ class SsgMainTest : public SsgFunctionTest {
"system", !cfg.is_smart_switch_npu && !cfg.is_smart_switch_dpu, cfg.num_dpus, false);
}

void validate_environment_variable(const SsgMainConfig &cfg) {
std::unordered_map<std::string, std::string> env_vars;
env_vars["IS_DPU_DEVICE"] = (cfg.is_smart_switch_dpu ? "true" : "false");
env_vars["NUM_DPU"] = std::to_string(cfg.num_dpus);

std::vector<std::string> checked_service_list;

checked_service_list.insert(checked_service_list.end(), common_service_list.begin(), common_service_list.end());
if (cfg.num_dpus > 0) {
checked_service_list.insert(checked_service_list.end(), npu_service_list.begin(), npu_service_list.end());
}
if (cfg.num_asics > 1) {
checked_service_list.insert(checked_service_list.end(), multi_asic_service_list.begin(), multi_asic_service_list.end());
}

for (const auto &target: checked_service_list) {
if (find_string_in_file("[Service]", target)) {
for (const auto& item : env_vars) {
std::string str = "Environment=\"" + item.first + "=" + item.second + "\"";
EXPECT_EQ(find_string_in_file(str, target), true)
<< "Error validating " + str + " in " + target;
}
}
}
}

/* ssg_main test routine.
* input: num_asics number of asics
*/
Expand Down Expand Up @@ -431,6 +458,9 @@ class SsgMainTest : public SsgFunctionTest {

/* Validate Test Unit file for dependency creation. */
validate_depedency_in_unit_file(cfg);

/* Validate environment variables */
validate_environment_variable(cfg);
}

/* Save global variables before running tests */
Expand Down Expand Up @@ -719,7 +749,6 @@ TEST_F(SsgMainTest, ssg_main_argv) {
std::vector<std::string> arguments = {
"ssg_main",
};

/* Create argv list for ssg_main. */
for (const auto& arg : arguments) {
argv_.push_back((char*)arg.data());
Expand Down
55 changes: 55 additions & 0 deletions src/systemd-sonic-generator/systemd-sonic-generator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
#include <string>
#include <sstream>
#include <unordered_set>
#include <fstream>
#include <unordered_map>
#include <regex>

#define MAX_NUM_TARGETS 48
#define MAX_NUM_INSTALL_LINES 48
Expand Down Expand Up @@ -385,6 +388,56 @@ static void replace_multi_inst_dep(const char *src) {
rename(tmp_file_path, src);
}

static void update_environment(const std::string &src)
{
std::ifstream src_file(src);
std::ofstream tmp_file(src + ".tmp");
bool has_service_section = false;
std::string line;

// locate the [Service] section
while (std::getline(src_file, line)) {
tmp_file << line << std::endl;
if (line.find("[Service]") != std::string::npos) {
has_service_section = true;
break;
}
}

if (!has_service_section) {
tmp_file << "[Service]" << std::endl;
}

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);
Copy link
Contributor

@vivekrnv vivekrnv Dec 19, 2024

Choose a reason for hiding this comment

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

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

  1. 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
  2. We can leverage ssg to write EnvironmentFile=/etc/sonic/static-env-variables option to all the services. making the ssg code minimal and flexible.
  3. We can potentially clean a lot of code under docker_image_ctl with this approach

Copy link
Contributor Author

@Pterosaur Pterosaur Dec 19, 2024

Choose a reason for hiding this comment

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

Good idea, I can have a try.

Copy link
Contributor Author

@Pterosaur Pterosaur Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

@vivekrnv vivekrnv Dec 20, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@vivekrnv vivekrnv Dec 20, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Pterosaur Pterosaur Dec 20, 2024

Choose a reason for hiding this comment

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

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


for (const auto& [key, value] : env_vars) {
Copy link
Contributor

@vivekrnv vivekrnv Dec 19, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@vivekrnv vivekrnv Dec 19, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Pterosaur Pterosaur Dec 19, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@vivekrnv vivekrnv Dec 20, 2024

Choose a reason for hiding this comment

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

Thanks for testing

tmp_file << "Environment=\"" << key << "=" << value << "\"" << std::endl;
Copy link
Contributor

@vivekrnv vivekrnv Dec 20, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Pterosaur Pterosaur Dec 20, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@vivekrnv vivekrnv Dec 20, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Pterosaur Pterosaur Dec 20, 2024

Choose a reason for hiding this comment

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

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

image

Copy link
Contributor

@vivekrnv vivekrnv Dec 20, 2024

Choose a reason for hiding this comment

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

Sounds good to me

}

// copy the rest of the file
if (has_service_section) {
const static std::regex env_var_regex("Environment=\"?(\\w+)");
while (std::getline(src_file, line)) {
// skip the existing environment variables
std::smatch match;
if (std::regex_search(line, match, env_var_regex)) {
if (env_vars.find(match[1]) != env_vars.end()) {
continue;
}
}
tmp_file << line << std::endl;
}
}

src_file.close();
tmp_file.close();
// remove the original file and rename the temporary file
remove(src.c_str());
rename((src + ".tmp").c_str(), src.c_str());
}

int get_install_targets(std::string unit_file, char* targets[]) {
/***
Returns install targets for a unit file
Expand Down Expand Up @@ -1155,6 +1208,8 @@ int ssg_main(int argc, char **argv) {
free(targets[j]);
}

update_environment(get_unit_file_prefix() + unit_instance);

free(unit_files[i]);
}

Expand Down