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

osbuild/systemd-unit-create: fix DefaultDependencies option #668

Merged

Conversation

achilleas-k
Copy link
Member

@achilleas-k achilleas-k commented May 7, 2024

The original purpose of this PR was to make the DefaultDependencies option an optional property that defaults to 'true'. The original PR also had a small fix for the mountpoint creation service. While working on updating the previous PR, I found that the mountpoint creation service for ostree-based images needed more work, including additions to the osbuild stage that creates it.

Replaces #608.

Changes to the systemd unit:

  • The unit must run after the ostree-remount.service.
  • The unit must run before the .mount units for each mountpoint (needs new option in systemd stage).
  • The chattr +i / command in ExecStartPost isn't run because RemainAfterExit is enabled, so the service doesn't "stop". RemainAfterExit should be disabled.
  • Instead of checking for composefs by grepping a binary, perhaps it's more reliable to check for the symptom, the thing we want to toggle, with lsattr -ld / | grep -q Immutable.

The last item was abandoned because we don't have a clean way of toggling the immutable flag back on without communicating info from the Pre command to the Post command.

This PR also updates the mountpoint policy for ostree-based systems to disallow paths that are top-level symlinks on a booted system.

Requires osbuild/osbuild#1782

@achilleas-k
Copy link
Member Author

This needs more work. The systemd service needs fixing both in the things it runs and its ordering.

@achilleas-k
Copy link
Member Author

achilleas-k commented May 8, 2024

The systemd unit in its current form doesn't do what we want. We need to be very specific about unit ordering for the mkdir unit to work. See the top comment for a check list of changes.

There's a simple (ish) way to experiment with the problem this service is trying to solve.

  1. Start an ostree container from CI (no need to build one):
podman run -d --rm -p42000:8080 registry.gitlab.com/redhat/services/products/image-builder/ci/images/rhel_9.4-x86_64-edge_container-empty:build-9018cfd56ba5f3b94ed50834e0ad64813704ed0be353abaeff95b18f5aa0b453
  1. Write this config.json (with your own ssh key and/or password):
{
  "name": "ostree-filesystem-customizations-42000",
  "blueprint": {
    "customizations": {
      "user": [
        {
          "name": "root",
          "key": "<SSHKEY>",
          "password": "<PASSWORD>"
        }
      ],
      "filesystem": [
        {
          "mountpoint": "/data",
          "minsize": 2147483648
        },
        {
          "mountpoint": "/data/secondary",
          "minsize": 2147483648
        },
        {
          "mountpoint": "/stuff",
          "minsize": 2147483648
        },
        {
          "mountpoint": "/var/mydata",
          "minsize": 1073741824
        }
      ]
    }
  },
  "options": {
    "ostree": {
      "ref": "rhel/9/x86_64/edge",
      "url": "http://127.0.0.1:42000/repo"
    }
  }
}
  1. Build an edge-raw image:
go build -o bin/build ./cmd/build && sudo ./bin/build --distro rhel-9.4 --image edge-raw --config ./config.json --output whatever
  1. Boot the image, ssh in, and break it
systemctl disable osbuild-ostree-mountpoints.service
chattr -i /
rmdir /stuff
chattr +i /
reboot

With the service disabled, the mount should fail on boot.

If the service looks like this though, everything should work (you can just plop it down in /etc/systemd/system/mountpoints.service and enable it):

[Unit]
Description=Ensure custom filesystem mountpoints exist
DefaultDependencies=False
After=ostree-remount.service
Before=stuff.mount
Before=data.mount
Before=var-mydata.mount

[Service]
Type=oneshot
RemainAfterExit=False
ExecStartPre=chattr -i /
ExecStartPre=ls -l /
ExecStopPost=chattr +i /
ExecStart=mkdir -pv /stuff /data /data/secondary /var/mydata

[Install]
WantedBy=sysinit.target

@achilleas-k achilleas-k force-pushed the systemd/default-dependencies/unset branch from c0692e7 to f40111c Compare May 13, 2024 18:36
@achilleas-k achilleas-k force-pushed the systemd/default-dependencies/unset branch from 8046905 to 3d3399f Compare May 22, 2024 14:11
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! This is very nice! Some quick drive-by feedback, I wanted to go full-review but only managed the first few lines and then got distracted. Feel free to ignore, it's mostly rambling.

say-paul and others added 10 commits June 4, 2024 14:56
The default value of DefaultDependencies in systemd is 'true' so we need
to make it a pointer with 'omitempty' to be able to represent the
'unset' state.

Signed-off-by: Sayan Paul <[email protected]>
Update the options of the org.osbuild.systemd.unit.create stage with the
new Before option.
Adding more paths that are denied for mountpoint creation for
ostree-based images.  In a booted ostree system, these paths are
symlinks, so it makes no sense (and will cause problems) to create
mountpoints at those locations.

The list has been sorted to make it simpler to modify consistently going
forward.
- The [ -z ... ] is redundant.  We can condition on the exit code of
  grep itself and don't need a subshell.
- Double quoting was wrong.
With RemainAfterExit enabled, the service is not considered "stopped"
when the execution ends, so the ExecStopPost is never executed.
Disabling RemainAfterExit ensures that ExecStopPost is run always,
even when the execution of the unit fails.
The mountpoint creation should happen after the ostree-remount.service
is run so that the ostree mounts are all set up and directories are
created in the correct location for the *live* system.
A new helper function that shells out to systemd-escape to determine the
mount unit name for a mountpoint.

See systemd-escape(1) for details.
- Add comment explaining why we disable DefaultDependencies.
- Drop the unnecessary [:] from the mountpoints slice.
@achilleas-k achilleas-k requested a review from mvo5 June 4, 2024 13:27
@achilleas-k achilleas-k force-pushed the systemd/default-dependencies/unset branch from 3d3399f to 49d8e81 Compare June 4, 2024 13:29
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!

@achilleas-k achilleas-k added this pull request to the merge queue Jun 6, 2024
Merged via the queue into osbuild:main with commit 9c903ab Jun 6, 2024
17 checks passed
@achilleas-k achilleas-k deleted the systemd/default-dependencies/unset branch February 27, 2025 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants