[systemd-generator]: Fix dependency update for multi-asic platform #4820
Conversation
of host services are generated correctly for multi-asic platforms. Add code to make sure that systemd timer files are also modified to add the correct service dependency for multi-asic platforms. Signed-off-by: SuvarnaMeenakshi <[email protected]>
remove unused variable. Signed-off-by: SuvarnaMeenakshi <[email protected]>
Signed-off-by: SuvarnaMeenakshi <[email protected]>
|
Interesting. So the strtok() call must be getting interrupted somehow. Should we also change all the other strtok() calls to strtok_r() as well to be safe? |
From my understanding, strtok() invokes strtok_r() internally. There will be an issue only if there is nested strtok(), else it should be fine. |
| 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 |
There was a problem hiding this comment.
Why no longer calling systemctl enable on the timers?
There was a problem hiding this comment.
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
|
retest vsimage please |
| fputs(line,fp_tmp); | ||
| } else { | ||
| line_copy = strdup(line); | ||
| token = strtok(line_copy, "="); | ||
| while ((word = strtok(NULL, " "))){ | ||
| token = strtok_r(line_copy, "=", &save_ptr1); |
There was a problem hiding this comment.
strtok_r [](start = 20, length = 8)
Rewrite everything by python?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for sharing the history discussion.
-
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.
-
Can we remove explicit knowledge on ASIC numbers, just like depending on all [email protected] ?
-
Can we use C++ (as high performance as C) string and stream to split strings?
In reply to: 443869780 [](ancestors = 443869780)
There was a problem hiding this comment.
- 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. - 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.
- 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?
There was a problem hiding this comment.
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.
…4820) * [systemd-generator]: Fix the code to make sure that dependencies of host services are generated correctly for multi-asic platforms. Add code to make sure that systemd timer files are also modified to add the correct service dependency for multi-asic platforms. Signed-off-by: SuvarnaMeenakshi <[email protected]> * [systemd-generator]: Minor fix, remove debug code and remove unused variable.
- Why I did it
First Issue: Issue seen while updating .service file for host services in multi-asic platform:
if host service file has the line:
For multi-asic platform, with 3 asics, where there are 3 syncd and swss services, the dependency should be modified as:
But currently, the updated service files looks like:
On VS platform, the string formed is not clean and corrupted characters are getting added.
second issue: Currently .timer files are not updated with the right dependencies for multi-asic platform. Like snmp.timer has WantedBy swss.service, this should get updated as WantedBy [email protected], [email protected] etc.
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)