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.

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

systemd.unit.create: support the After option in the Unit section (HMS-3814) #1741

Merged
merged 3 commits into from
Apr 18, 2024

Conversation

achilleas-k
Copy link
Member

@achilleas-k achilleas-k commented Apr 16, 2024

stage/systemd.unit.create: add After option

Support the After option in the Unit section of the unit file.


test/systemd_unit_create: Wants, Requires, After

Add test values for Wants, Requires, and After.
Adding multiple values to test that arrays work and made sure they're
all different.
The units need to be valid, real unit names otherwise the
'systemd-analyze verify' check will fail.


stage/systemd.unit.create: move systemd-analyze verify to tests

Verifying the systemd unit also checks if any referred systemd units
(Wants, Requires, After) exist and if all commands in Exec exist and are
executable. Without '--root', the systemd-analyze verify command is
testing this against files in the build root, which isn't valid.

Units and binaries might not exist in the build root when referenced in
the image root tree, making the unit fail when when it's valid.
Conversely, the verification can succeed by finding executables in the
build root that don't exist in the image root tree when it should be
failing.

When verifying user units, systemd expects runtime directories.

All of this makes it quite difficult to verify systemd units properly
when building an image. The call is useful for making sure the unit is
structured properly, but the user unit verification setup is difficult
to accomplish in a general way while building.

Remove the systemd-analyze verify step from the stage. Move it to the
unit test so that we have some assurance that our unit file structure is
correct and things work as expected. Create referenced unit files and
commands to make the unit valid.


Support the After option in the Unit section of the unit file.
Add test values for Wants, Requires, and After.
Adding multiple values to test that arrays work and made sure they're
all different.
The units need to be valid, real unit names otherwise the
'systemd-analyze verify' check will fail.
@achilleas-k achilleas-k changed the title Support the After option in the Unit section of the unit file (HMS-3814) systemd.unit.create: support the After option in the Unit section (HMS-3814) Apr 16, 2024
@ezr-ondrej
Copy link

Quite cool addition 🧡

@achilleas-k
Copy link
Member Author

Needs one more fix. The systemd-analyze verify in the stage should be run with the --root option because it checks if the units it references are valid (Wants etc) and that executables in Exec exist and are executable. That also needs some fiddling in the unit test to work.

@achilleas-k
Copy link
Member Author

The change in cd79140 makes the systemd-analyze verify step in the stage verify against the root tree, so units and commands must exist in the target tree for it to be pass (which is the Right Thing). However, this doesn't seem to work with user unit files.

The test manifest in test/data/stages/systemd.unit.create/b.json for example fails with:

Failed to lookup RuntimeDirectory path: No such device or address
Failed to initialize manager: No such device or address

@achilleas-k achilleas-k force-pushed the systemd-after branch 3 times, most recently from 1315b62 to 73a894b Compare April 17, 2024 14:47
@achilleas-k
Copy link
Member Author

achilleas-k commented Apr 17, 2024

I ended up removing the systemd-analyze verify step from the stage and added it to the unit test but only for system services. The reasons are described in the last commit (ping @mvo5).

I kept a version of some of the things I tried for getting it right here: main...achilleas-k:osbuild:systemd-after-verify-stuff

The summary is that it gets very tricky to verify user units in a non-booted system. We could skip verification only for user units, but it feels weird to have it only for certain situations. User unit verification did work for me when I would bind mount the build root's /run into the tree, but I don't know if there are any cases that would make this fail unexpectedly. Adding --user to the verify call always fails with the errors I mentioned in the previous comment. Same for --global.

If anyone feels strongly about adding the verification back, I can add it with the /run bind mount (and figure out how to get around it in the unit test).

@achilleas-k achilleas-k force-pushed the systemd-after branch 2 times, most recently from 3a1fcfc to 4053242 Compare April 17, 2024 14:55
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

This looks very nice, thank you! Some small suggestion inline but feel free to ignore if you disagree/prefer the other way.

stages/org.osbuild.systemd.unit.create Show resolved Hide resolved
stages/test/test_systemd_unit_create.py Outdated Show resolved Hide resolved
stages/test/test_systemd_unit_create.py Show resolved Hide resolved
stages/test/test_systemd_unit_create.py Outdated Show resolved Hide resolved
mvo5
mvo5 previously approved these changes Apr 18, 2024
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you!

stages/test/test_systemd_unit_create.py Show resolved Hide resolved
stages/test/test_systemd_unit_create.py Show resolved Hide resolved
Verifying the systemd unit also checks if any referred systemd units
(Wants, Requires, After) exist and if all commands in Exec exist and are
executable.  Without '--root', the systemd-analyze verify command is
testing this against files in the build root, which isn't valid.

Units and binaries might not exist in the build root when referenced in
the image root tree, making the unit fail when when it's valid.
Conversely, the verification can succeed by finding executables in the
build root that don't exist in the image root tree when it should be
failing.

When verifying user units, systemd expects runtime directories.

All of this makes it quite difficult to verify systemd units properly
when building an image.  The call is useful for making sure the unit is
structured properly, but the user unit verification setup is difficult
to accomplish in a general way while building.

Remove the systemd-analyze verify step from the stage.  Move it to the
unit test so that we have some assurance that our unit file structure is
correct and things work as expected.  Create referenced unit files and
commands to make the unit valid.
@croissanne croissanne enabled auto-merge (rebase) April 18, 2024 15:20
@croissanne croissanne merged commit f255fba into osbuild:main Apr 18, 2024
44 checks passed
@achilleas-k achilleas-k deleted the systemd-after branch April 18, 2024 15:21
achilleas-k added a commit to achilleas-k/images that referenced this pull request Apr 18, 2024
achilleas-k added a commit to achilleas-k/images that referenced this pull request Apr 19, 2024
achilleas-k added a commit to achilleas-k/images that referenced this pull request Apr 19, 2024
achilleas-k added a commit to achilleas-k/images that referenced this pull request Apr 19, 2024
achilleas-k added a commit to achilleas-k/images that referenced this pull request Apr 22, 2024
achilleas-k added a commit to achilleas-k/images that referenced this pull request Apr 23, 2024
achilleas-k added a commit to achilleas-k/images that referenced this pull request Apr 23, 2024
achilleas-k added a commit to achilleas-k/images that referenced this pull request Apr 23, 2024
achilleas-k added a commit to achilleas-k/images that referenced this pull request Apr 23, 2024
github-merge-queue bot pushed a commit to osbuild/images that referenced this pull request Apr 25, 2024
github-merge-queue bot pushed a commit to osbuild/images that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants