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.

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

Jump to bottom

[systemd-generator]: Fix dependency update for multi-asic platform #4820

Merged
merged 3 commits into from
Jun 29, 2020
Merged

[systemd-generator]: Fix dependency update for multi-asic platform #4820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0dcab04
[systemd-generator]: Fix the code to make sure that dependencies
SuvarnaMeenakshi Jun 19, 2020
f30b817
[systemd-generator]: Minor fix, remove debug code and
SuvarnaMeenakshi Jun 19, 2020
71c07f0
Minor fix to remove tabs.
SuvarnaMeenakshi Jun 19, 2020
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
4 changes: 2 additions & 2 deletions files/build_templates/sonic_debian_extension.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,10 +516,10 @@ sudo LANG=C cp $SCRIPTS_DIR/sonic-netns-exec $FILESYSTEM_ROOT/usr/bin/sonic-netn
# Copy systemd timer configuration
# It implements delayed start of services
sudo cp $BUILD_TEMPLATES/snmp.timer $FILESYSTEM_ROOT_USR_LIB_SYSTEMD_SYSTEM
sudo LANG=C chroot $FILESYSTEM_ROOT systemctl enable snmp.timer
echo "snmp.timer" | sudo tee -a $GENERATED_SERVICE_FILE
{% if enable_system_telemetry == 'y' %}
sudo cp $BUILD_TEMPLATES/telemetry.timer $FILESYSTEM_ROOT_USR_LIB_SYSTEMD_SYSTEM
sudo LANG=C chroot $FILESYSTEM_ROOT systemctl enable telemetry.timer
echo "telemetry.timer" | sudo tee -a $GENERATED_SERVICE_FILE
Comment on lines -519 to +522
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no longer calling systemctl enable on the timers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

systemctl enable will create a mapping of the service in /etc/systemd/system/.wants directory. This can cause issue in multi-asic platform. The dependencies that are created in systemd generator are present in /var/run/systemd/generator/.wants directory.
To explain better, I have directory structure from VS platform, with systemctl enable and without it.
Without enable: single asic VS platform

admin@sonic:**/var/run/systemd/generator**$ ls timers.target.wants/
snmp.timer  telemetry.timer
admin@sonic:**/var/run/systemd/generator**$ ls swss.service.wants/
nat.service  snmp.timer
admin@sonic:/etc/systemd/system/timers.target.wants$ ls
apt-daily.timer          logrotate.timer
apt-daily-upgrade.timer  process-reboot-cause.timer
admin@sonic:/var/run/systemd/generator/swss.service.wants$ 
.timer
â—� snmp.timer - Delays snmp container until SONiC has started
   Loaded: loaded (/lib/systemd/system/snmp.timer; **enabled-runtime;** vendor prese
   Active: inactive (dead) since Mon 2020-06-22 00:35:35 UTC; 4min 40s ago
multi-asic:

admin@sonic:**/var/run/systemd/generator$ ls [email protected]/**
nat.service  snmp.timer
admin@sonic:/var/run/systemd/generator$ ls timers.target.wants/
snmp.timer  telemetry.timer

Though timers.target.wants directory is present in /etc/systemd/system and /var/run/systemd/generator, systemctl -r shows all under timers.target:

timers.target             loaded active     active          Timers      
---------------------------------------------------------------------------------       
apt-daily-upgrade.timer   loaded active     waiting         Daily apt upgrade a
apt-daily.timer           loaded active     waiting         Daily apt download 
fstrim.timer              loaded active     waiting         Discard unused bloc
logrotate.timer           loaded active     waiting         Daily rotation of l
process-reboot-cause.timer loaded active     waiting         Delays process-rebo
snmp.timer                loaded active     waiting         Delays snmp contain
systemd-tmpfiles-clean.timer loaded active     waiting         Daily Cleanup of 
telemetry.timer           loaded active     waiting         Delays telemetry co

If we keep sysctl enable, then in mutli-asic platform, /etc/systemd/system/swss.service.wants directory is created with snmp.timer in it. /var/run/systemd/generator/[email protected] also has snmp.timer. So, this is not inline with the required dependency.

admin@sonic:/etc/systemd/system/timers.target.wants$ ls
apt-daily.timer          logrotate.timer             snmp.timer
apt-daily-upgrade.timer  process-reboot-cause.timer  telemetry.timer
admin@sonic:/etc/systemd/system/swss.service.wants$ ls
snmp.timer

{% endif %}

sudo LANG=C DEBIAN_FRONTEND=noninteractive chroot $FILESYSTEM_ROOT apt-get purge -y python-dev
Expand Down
27 changes: 16 additions & 11 deletions src/systemd-sonic-generator/systemd-sonic-generator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,16 @@ static void replace_multi_inst_dep(char *src) {
char *line_copy;
char *service_name;
char *type;
char *save_ptr1 = NULL;
char *save_ptr2 = NULL;
ssize_t nread;
bool section_done = false;
char tmp_file_path[PATH_MAX];

/* assumes that the service files has 3 sections,
/* Assumes that the service files has 3 sections,
* in the order: Unit, Service and Install.
* Assumes that the timer file has 3 sectiosn,
* in the order: Unit, Timer and Install.
* Read service dependency from Unit and Install
* sections, replace if dependent on multi instance
* service.
Expand All @@ -162,30 +166,31 @@ static void replace_multi_inst_dep(char *src) {
fp_tmp = fopen(tmp_file_path, "w");

while ((nread = getline(&line, &len, fp_src)) != -1 ) {
if (strstr(line, "[Service]") != NULL) {
if ((strstr(line, "[Service]") != NULL) ||
(strstr(line, "[Timer]") != NULL)) {
section_done = true;
fputs(line,fp_tmp);
} else if (strstr(line, "[Install]") != NULL) {
} else if (strstr(line, "[Install]") != NULL) {
section_done = false;
fputs(line,fp_tmp);
} else if ((strstr(line, "[Unit]") != NULL) ||
(strstr(line, "Description") != NULL) ||
(section_done == true)){
(section_done == true)) {
fputs(line,fp_tmp);
} else {
line_copy = strdup(line);
token = strtok(line_copy, "=");
while ((word = strtok(NULL, " "))){
token = strtok_r(line_copy, "=", &save_ptr1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

strtok_r [](start = 20, length = 8)

Rewrite everything by python?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial change to add systemd-generator had a discussion on this after which it was decided that C will be used. Guohan had pointed to a reference: "If you are careful, you can implement generators in shell scripts. We do recommend C code however, since generators are executed synchronously and hence delay the entire boot if they are slow." https://www.freedesktop.org/software/systemd/man/systemd.generator.html.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for sharing the history discussion.

  1. I don't understand why it is on boot critical path. Could you elaborate? It seems relating the ASIC numbers, and only need to run once on first cold boot.

  2. Can we remove explicit knowledge on ASIC numbers, just like depending on all [email protected] ?

  3. Can we use C++ (as high performance as C) string and stream to split strings?

In reply to: 443869780 [](ancestors = 443869780)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. systemd-generator runs before systemd starts, hence it is on boot critical path.
    Yes, we will ideally need to run this only once on first cold boot. Currently this is always run upon each boot before systemd starts.
  2. systemd generator does 2 things based on number of asics: 1. create the number of instances to be started, ex: swss@0, swss@1 etc. 2. Change the dependency of host services to reflect this list of actual running services. if we specify syncd@N - systemd does not convert that to 0 - n-1, instead looks for syncd@N service, which is not running.
  3. Agreed that c++ will provide in built functions for string, but as service files are going to have some set of strings which will not vary much, c string functions should satisfy that requirement. Was there any other reason to use c++ other than ease of use with string functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did a comparison on runtime between c and c++. On a mulit-asic platform, C program takes 2643 micro seconds and c++ take 2651 micro seconds. As there is not much difference, we can move to c++. Will raise another PR to move to code c++ with stream functions for string operations to make it less error prone.

while ((word = strtok_r(NULL, " ", &save_ptr1))) {
if((strchr(word, '.') == NULL) ||
(strchr(word, '@') != NULL)) {
snprintf(buf, MAX_BUF_SIZE,"%s=%s\n",token, word);
fputs(buf,fp_tmp);
} else {
service_name = strdup(word);
service_name = strtok(service_name, ".");
type = strtok(NULL, " ");
service_name = strtok_r(service_name, ".", &save_ptr2);
type = strtok_r(NULL, " ", &save_ptr2);
if (is_multi_instance_service(word)) {
for(i = 0; i < num_asics; i++){
for(i = 0; i < num_asics; i++) {
snprintf(buf, MAX_BUF_SIZE, "%s=%s@%d.%s\n",
token, service_name, i, type);
fputs(buf,fp_tmp);
Expand Down Expand Up @@ -513,7 +518,7 @@ static int get_num_of_asic() {

while ((nread = getline(&line, &len, fp)) != -1) {
if ((strstr(line, "onie_platform") != NULL) ||
(strstr(line, "aboot_platform") != NULL)) {
(strstr(line, "aboot_platform") != NULL)) {
token = strtok(line, "=");
platform = strtok(NULL, "=");
strip_trailing_newline(platform);
Expand Down Expand Up @@ -580,7 +585,7 @@ int main(int argc, char **argv) {

// For each unit file, get the installation targets and install the unit
for (int i = 0; i < num_unit_files; i++) {
unit_instance = strdup(unit_files[i]);
unit_instance = strdup(unit_files[i]);
if ((num_asics == 1) && strstr(unit_instance, "@") != NULL) {
prefix = strtok(unit_instance, "@");
suffix = strtok(NULL, "@");
Expand Down