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

[build]: Move Systemd service start to systemd generator #3172

Merged
merged 20 commits into from
Jul 29, 2019
Merged

[build]: Move Systemd service start to systemd generator #3172

merged 20 commits into from
Jul 29, 2019

Conversation

theasianpianist
Copy link
Contributor

@theasianpianist theasianpianist commented Jul 16, 2019 •
edited
Loading

- What I did

  • Move the enabling of Systemd services from sonic_debian_extension to a new systemd generator

- How I did it

  • Create a new systemd generator to manually create symlinks to enable systemd services
  • Add rules/Makefile to build generator
  • Add services to be enabled to /etc/sonic/generated_services.conf to be read by the generator at boot time

- How to verify it
Build, install, and start an image and ensure that all system services and docker containers run properly.

Output of docker ps after boot should be similar to the following:

admin@sonic:~$ docker ps
CONTAINER ID        IMAGE                             COMMAND                  CREATED             STATUS              PORTS               NAMES
c51475ca0653        docker-syncd-vs:latest            "/usr/bin/supervisord"   12 seconds ago      Up 11 seconds                           syncd
232e01bf384e        docker-dhcp-relay:latest          "/usr/bin/docker_ini…"   13 seconds ago      Up 11 seconds                           dhcp_relay
1a2b1fe59130        docker-teamd:latest               "/usr/bin/supervisord"   15 seconds ago      Up 13 seconds                           teamd
d4d144ef119c        docker-snmp-sv2:latest            "/usr/bin/supervisord"   15 seconds ago      Up 13 seconds                           snmp
a13a558645c8        docker-router-advertiser:latest   "/usr/bin/supervisord"   15 seconds ago      Up 13 seconds                           radv
88a23838dce0        docker-orchagent:latest           "/usr/bin/supervisord"   18 seconds ago      Up 16 seconds                           swss
065467d78f3c        docker-platform-monitor:latest    "/usr/bin/docker_ini…"   23 seconds ago      Up 22 seconds                           pmon
83868140ee1c        docker-lldp-sv2:latest            "/usr/bin/supervisord"   23 seconds ago      Up 22 seconds                           lldp
9b7c603859a7        docker-fpm-frr:latest             "/usr/bin/supervisord"   23 seconds ago      Up 22 seconds                           bgp
49c44f5b3139        docker-sonic-telemetry:latest     "/usr/bin/supervisord"   23 seconds ago      Up 22 seconds                           telemetry
7a8da33a617f        docker-database:latest            "/usr/bin/supervisord"   3 hours ago         Up 27 seconds                           database

All containers should be running or 'up'.
- Description for the changelog

Enable Systemd services in a Systemd generator to prepare for boot time platform detection and service configuration.

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

@lguohan lguohan requested a review from jleveque July 16, 2019 20:06
@lguohan
Copy link
Collaborator

lguohan commented Jul 16, 2019

please provide test log, can you get the docker ps output after boot?

@jleveque
Copy link
Contributor

The services that you moved shouldn't impact the docker ps output, as none of them start containers. Can you confirm that the moved services are all started upon the first boot? I'm concerned because rc.local is started by systemd (i.e. systemd has already started, and if you enable/disable services after systemd has started, the changes won't take effect until the next time you start systemd. I have a feeling that none of these services will get started on first boot.

@theasianpianist
Copy link
Contributor Author

@jleveque The services tied to docker containers are started in rc.local here, and the other explicitly named services get started as well.

Although you do bring up a good point, as I believe the build/boot process for the virtual switch image is slightly different than other images. @lguohan can probably speak more to this, but I believe for the virtual switch image (which is the only image I'm testing with), first boot actually happens during the build because of the way we generate the final image. So even though it works for the virtual switch, it might not work properly for other platforms. I'm not sure how to go about testing this.

@jleveque
Copy link
Contributor

You will need to build an image and install on an actual device to test.

So the Docker containers are part of the installer_services list? The name seems unclear/vague. Why the naming choice?

@theasianpianist
Copy link
Contributor Author

Not sure about the name, installer_services was the previously used name so I kept it.

@jleveque
Copy link
Contributor

OK. I guess I wasn't aware of that. 😕

@theasianpianist
Copy link
Contributor Author

@jleveque you were right, a reboot is needed for things to start up properly. I'm testing some changes now that hopefully fix that issue.

@lguohan
Copy link
Collaborator

lguohan commented Jul 17, 2019

it would be a no go if a reboot is required after installation.

@theasianpianist theasianpianist changed the title [build]: Move Systemd service start to rc.local [build]: Move Systemd service start to systemd generator Jul 19, 2019
@theasianpianist theasianpianist force-pushed the move-service-start branch 2 times, most recently from b050b13 to be62ff9 Compare July 19, 2019 22:54
@theasianpianist
Copy link
Contributor Author

retest vsimage please

@lguohan
Copy link
Collaborator

lguohan commented Jul 20, 2019 •
edited
Loading

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.

python is slow!

https://www.freedesktop.org/software/systemd/man/systemd.generator.html

I suggest follow the suggestion.

example:

https://github.com/systemd/systemd/blob/master/src/getty-generator/getty-generator.c

lguohan
lguohan requested changes Jul 20, 2019
View reviewed changes
Copy link
Collaborator

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

overall, i think it looks good.

since the boot up time is critical to us, I would like you to convert your generator to c program.

[platform]: add systemd generator as dependency for all images
52f1d74 
Signed-off-by: Lawrence Lee <t-lale@microsoft.com>
[systemd-sonic-generator]: address review comments
1d48819 
* remove install dependency from p4 docker
* use CONFIGURED_ARCH in Makefile

Signed-off-by: Lawrence Lee <t-lale@microsoft.com>
@theasianpianist
Copy link
Contributor Author

retest this please

@theasianpianist
Copy link
Contributor Author

retest vs please

@theasianpianist
Copy link
Contributor Author

retest this please

@theasianpianist
Copy link
Contributor Author

restest this please

@theasianpianist
Copy link
Contributor Author

retest vs please

1 similar comment
@theasianpianist
Copy link
Contributor Author

retest vs please

lguohan
lguohan requested changes Jul 28, 2019
View reviewed changes
Copy link
Collaborator

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

something is wrong, I cannot find the generator binary in the image.

@lguohan
Copy link
Collaborator

lguohan commented Jul 28, 2019

retest broadcom please

3 similar comments
@lguohan
Copy link
Collaborator

lguohan commented Jul 28, 2019

retest broadcom please

@lguohan
Copy link
Collaborator

lguohan commented Jul 28, 2019

retest broadcom please

@lguohan
Copy link
Collaborator

lguohan commented Jul 28, 2019

retest broadcom please

lguohan
lguohan reviewed Jul 28, 2019
View reviewed changes
platform/broadcom/raw-image.mk Outdated
@@ -4,6 +4,7 @@ SONIC_RAW_IMAGE = sonic-broadcom.raw
$(SONIC_RAW_IMAGE)_MACHINE = broadcom
$(SONIC_RAW_IMAGE)_IMAGE_TYPE = raw
$(SONIC_RAW_IMAGE)_INSTALLS += $(BRCM_OPENNSL_KERNEL)
$(SONIC_ONE_ABOOT_IMAGE)_INSTALLS += $(SYSTEMD_SONIC_GENERATOR)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is in correct, please fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you missed platform/broadcom/aboot-image.mk as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure which file you're referring to, I don't see broadcom/aboot-image.mk , did you mean one-aboot.mk?

[platform/broadcom]: address review comments
75002b4 
* fix Makefile typo

Signed-off-by: Lawrence Lee <t-lale@microsoft.com>
@theasianpianist
Copy link
Contributor Author

retest vs please

@theasianpianist
Copy link
Contributor Author

retest baseimage please

@theasianpianist
Copy link
Contributor Author

retest vs please

4 similar comments
@theasianpianist
Copy link
Contributor Author

retest vs please

@theasianpianist
Copy link
Contributor Author

retest vs please

@theasianpianist
Copy link
Contributor Author

retest vs please

@theasianpianist
Copy link
Contributor Author

retest vs please

@lguohan lguohan merged commit 7271fe5 into sonic-net:master Jul 29, 2019
@theasianpianist theasianpianist deleted the move-service-start branch May 13, 2021 23:11
StormLiangMS pushed a commit to StormLiangMS/sonic-buildimage that referenced this pull request Jul 18, 2022
@judyjoseph @abdosi
Merged PR 2925611: Adding Sonic Systemd generator for NBI images.
9d27458 
SYSTEMD_SONIC_GENERATOR was added earlier in public repo as part of PR sonic-net#3172 (sonic-net#3172)

This update was missed in the NBI image make file as this cisco image file ( one-nbi.mk ) is not present in public REPO
This is the module which fine tunes and creates the systemd files. This is needed for multi-asic dockers to come up on a 3164 coverted to SONIC with NBI image.

admin@NAMESPACE5:~$ docker ps
CONTAINER ID        IMAGE                             COMMAND                  CREATED             STATUS              PORTS               NAMES
71aa6d5cecc9        docker-teamd:latest               "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           teamd5
843d86e325d6        docker-syncd-brcm:latest          "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           syncd5
71e3db49929f        docker-teamd:latest               "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           teamd4
4c8b68200395        docker-orchagent:latest           "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           swss5
a99da03c65f6        docker-syncd-brcm:latest          "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           syncd4
43135b5ba83a        docker-teamd:latest               "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           teamd3
0d059f873315        docker-orchagent:latest           "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           swss4
388f1b4c8ab5        docker-syncd-brcm:latest          "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           syncd3
6a5a426454a8        docker-teamd:latest               "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           teamd2
489afc83e2b7        docker-orchagent:latest           "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           swss3
22f0f342b81b        docker-syncd-brcm:latest          "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           syncd2
67413f12cc0f        docker-teamd:latest               "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           teamd1
1f1dfcce6b23        docker-teamd:latest               "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           teamd0
c13644c642d8        docker-orchagent:latest           "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           swss2
6530eb85b150        docker-syncd-brcm:latest          "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           syncd1
e56e0712b2d1        docker-orchagent:latest           "/usr/bin/supervisord"   33 minutes ago      Up 33 minutes                           swss1
32728bcc249a        docker-syncd-brcm:latest          "/usr/bin/supervisord"   3 ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavel-shirshov pavel-shirshov pavel-shirshov left review comments

@lguohan lguohan lguohan approved these changes

@jleveque jleveque jleveque approved these changes

Assignees
No one assigned
Labels
Enhancement ➕
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@theasianpianist @lguohan @jleveque @pavel-shirshov @stcheng