Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Add mount and umount service units #47

Merged
merged 3 commits into from
Mar 26, 2019
Merged

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Feb 27, 2019

Still cobbling together the FCOS side of this so will likely need some more tweaks, but this is what I'm working with right now.

Requires: #46

@dustymabe dustymabe added the WIP PR still being worked on label Mar 4, 2019
@jlebon jlebon force-pushed the pr/mount-umount branch from f3d7b77 to 4b4b112 Compare March 5, 2019 21:27
@jlebon
Copy link
Member Author

jlebon commented Mar 5, 2019

Woohoo! With the above I successfully booted FCOS with a separate /var mountpoint! 🎉

Ignition file:

{
    "ignition": {
        "config": {},
        "security": {
            "tls": {}
        },
        "timeouts": {},
        "version": "3.0.0-experimental"
    },
    "passwd": {
        "users": [
            {
                "name": "root",
                "sshAuthorizedKeys": [
                    "ssh-rsa AAA..."
                ]
            }
        ]
    },
    "storage": {
        "disks": [
            {
                "device": "/dev/vdb",
                "wipeTable": true,
                "partitions": [
                    {
                        "label": "VAR",
                        "number": 0,
                        "sizeMiB": 0,
                        "wipePartitionEntry": true
                    }
                ]
            }
        ],
        "filesystems": [
            {
                "device": "/dev/vdb1",
                "path": "/var",
                "format": "xfs",
                "wipeFilesystem": true,
                "label": "VAR"
            }
        ],
        "files": [
            {
                "path": "/etc/fstab",
                "append": true,
                "contents": {
                    "source": "data:,%0A/dev/vdb1%20/var%20xfs%20defaults%200%200%0A"
                }
            },
            {
                "path": "/var/lib/hello.txt",
                "contents": {
                    "source": "data:,hello%20world"
                }
            }
        ]
    },
    "systemd": {}
}

Result:

[root@localhost ~]# grep /var /etc/fstab
/dev/vdb1 /var xfs defaults 0 0
[root@localhost ~]# findmnt /var
TARGET SOURCE    FSTYPE OPTIONS
/var   /dev/vdb1 xfs    rw,relatime,seclabel,attr2,inode64,noquota
[root@localhost ~]# cat /var/lib/hello.txt
hello world[root@localhost ~]#

@jlebon
Copy link
Member Author

jlebon commented Mar 5, 2019

Two high-level questions:

  1. Where should the distro-specific units live (those are the coreos-* files)? Strawman: rather than create yet another package, let's put it in https://github.com/coreos/fedora-coreos-config and teach coreos-assembler to create RPMs? IIRC, the latter bit was suggested already somewhere to also help with emptying the manifest of hacks.
  2. Right now, the coreos-* services are getting manually tagged into the transaction. But we need a generic way of getting distro-specific units pulled in only when Ignition is activated (i.e. first boot or ignition.firstboot=1 is passed). We could add a symlink in e.g. /run/systemd/system/ignition-files.service.requires though I'm thinking we could add a clean ignition.target instead. This would also allow us to collapse our add_requires calls into a single unit we enable.

@dustymabe
Copy link
Member

dustymabe commented Mar 6, 2019

Where should the distro-specific units live (those are the coreos-* files)? Strawman: rather than create yet another package, let's put it in https://github.com/coreos/fedora-coreos-config and teach coreos-assembler to create RPMs? IIRC, the latter bit was suggested already somewhere to also help with emptying the manifest of hacks.

half of hates it and half of me loves it. If we go that route I think I'd almost rather have it done in rpm-ostree itself rather than coreos-assembler (i.e. another stanza in the manifest)

@ajeddeloh
Copy link
Contributor

ajeddeloh commented Mar 6, 2019

half of hates it and half of me loves it.

Yup. I feel the same

I feel like the root of this problem is that dracut is really awkward to use for this since it requires all it's units in /usr/lib/dracut/modules.d. This gets into the earlier discussions we had about "Can we have a distro independent set of dracut modules for Ignition". I still think the answer is "no, not really" and we should just have a distro-dependent set of modules.

Right now, the coreos-* services are getting manually tagged into the transaction. But we need a generic way of getting distro-specific units pulled in only when Ignition is activated (i.e. first boot or ignition.firstboot=1 is passed). We could add a symlink in e.g. /run/systemd/system/ignition-files.service.requires though I'm thinking we could add a clean ignition.target instead. This would also allow us to collapse our add_requires calls into a single unit we enable.

I'm in favor of an ignition-complete.target that requires all the ignition bits and lets the units themselves order themselves with After= and Before=

@jlebon
Copy link
Member Author

jlebon commented Mar 6, 2019

This gets into the earlier discussions we had about "Can we have a distro independent set of dracut modules for Ignition". I still think the answer is "no, not really" and we should just have a distro-dependent set of modules.

Hmm, I thought that's what this repo was about? Are you suggesting just keeping the coreos-* files here?

If we go that route I think I'd almost rather have it done in rpm-ostree itself rather than coreos-assembler (i.e. another stanza in the manifest)

Yeah, I can imagine moving that functionality to rpm-ostree eventually and superseding add-files, though to start off, teaching coreos-assembler to do rpmbuild -ba would be easier.

Again, this would also allow us to give the manifest a major diet (right now we've got ~140 lines of postprocessing which mostly just creates files). Basically consider the RPM an "integration layer" which contains all the bits that make FCOS FCOS. (Though I suppose one could argue this belongs better in the release package, but having the RPM dynamically constructed from sources that sit alongside the manifest is much nicer to hack on and test).

Is there a similar package in CL for this? Was it something that gave more trouble than it was worth?

@ajeddeloh
Copy link
Contributor

On CL we have the init[1][2] package which is probably the closest analog, but it doesn't include any dracut modules. We really need a way to build packages within cosa though. I don't want to have to either manually set things up to build them or deal with submitting 1000 jobs to koji as I test things.

[1] https://github.com/coreos/init
[2] https://github.com/coreos/coreos-overlay/tree/master/coreos-base/coreos-init

@jlebon
Copy link
Member Author

jlebon commented Mar 6, 2019

though I'm thinking we could add a clean ignition.target instead

OK, I opened #55 for this!

I'm in favor of an ignition-complete.target that requires all the ignition bits and lets the units themselves order themselves with After= and Before=

And now I'm rereading this comment more carefully. I do like the ignition-complete.target name. Will rename it.

@jlebon
Copy link
Member Author

jlebon commented Mar 15, 2019

Where should the distro-specific units live (those are the coreos-* files)? Strawman: rather than create yet another package, let's put it in https://github.com/coreos/fedora-coreos-config and teach coreos-assembler to create RPMs? IIRC, the latter bit was suggested already somewhere to also help with emptying the manifest of hacks.

coreos/fedora-coreos-config#66

Copy link
Contributor

@ajeddeloh ajeddeloh left a comment

Choose a reason for hiding this comment

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

Probably worth running shellcheck over the shell scripts as well

dracut/30ignition/coreos-populate-var.sh Outdated Show resolved Hide resolved
dracut/30ignition/coreos-populate-var.sh Outdated Show resolved Hide resolved
@jlebon
Copy link
Member Author

jlebon commented Mar 19, 2019

Probably worth running shellcheck over the shell scripts as well

Done!

@jlebon
Copy link
Member Author

jlebon commented Mar 19, 2019

This is blocked on coreos/fedora-coreos-config#66 now, which is blocked on coreos/coreos-assembler#414.

@ajeddeloh
Copy link
Contributor

Is this still WIP?

@jlebon
Copy link
Member Author

jlebon commented Mar 21, 2019

Is this still WIP?

Yes, though we're getting there! I split out the FCOS-specific bits into coreos/fedora-coreos-config#70. Still testing that combination.

(Though this PR now also requires #61).

Right now, we enable our services by adding a link in
`initrd.target.requires`. However, it seems like systemd doesn't
necessarily fail the boot even if `initrd.target` fails. That unit has:

    OnFailure=emergency.target
    OnFailureJobMode=replace-irreversibly

But looking at the logs, it seems like that can get overridden by the
`systemctl isolate initrd-switch-root.target` call that
`initrd-cleanup.service` does.

Let's just be really explicit here and tell systemd we want to
immediately switch to `emergency.target` if any of our units fail.
Add support for the new Ignition `mount` and `umount` stages. The
`mount` stage runs *after* the `disks` stage and
`initrd-root-fs.target`, since we need to be able to mount under
`/sysroot`, but *before* the `files` stage, which will of course drop
files under those mounts.

Combined with coreos/fedora-coreos-config#70,
this allows us to drop all OSTree-specific bits and no longer rely on
`ostree-prepare-root.service` to mount `/var` for us.
@jlebon jlebon changed the title WIP: Add mount and umount service units Add mount and umount service units Mar 25, 2019
@jlebon
Copy link
Member Author

jlebon commented Mar 25, 2019

Dropped WIP on this. See status updates in coreos/fedora-coreos-config#70.

Maybe let's merge it soon-ish, so it's easier for others to test it out before we switch FCOS/RHCOS to spec 3.0.0? Hmm, we should probably set up a COPR or something to track master.

Copy link
Contributor

@ajeddeloh ajeddeloh left a comment

Choose a reason for hiding this comment

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

Still LGTM with the fcos-config changes

@dustymabe dustymabe removed the WIP PR still being worked on label Mar 26, 2019
@dustymabe
Copy link
Member

Maybe let's merge it soon-ish

I defer to andrew - feel free to merge at your convenience

@ajeddeloh
Copy link
Contributor

Synced with jlebon: in the interest of making it easier to package for the time being we'll be bringing the modules back from the fcos config

This module is the integration point between Ignition and Fedora CoreOS.
Notably, we add two new systemd services in the initrd which mesh with
Ignition's own systemd services:

1. `coreos-mount-var.service`, which finds the `/var` stateroot bind
   mount and mounts it under `/sysroot/var` before `ignition-mount`
   potentially adds more mounts underneath (or even shadowing `/var`
   itself).
2. `coreos-populate-var.service`, which sets up `/var` on first boot
   using `systemd-tmpfiles` before `ignition-files` runs.

NOTE: this is a temporary holding place to make packaging easier.
Eventually, we will move this module to the FCOS overlay:
coreos/fedora-coreos-config#70
@jlebon
Copy link
Member Author

jlebon commented Mar 26, 2019

Was hitting issues doing a final sanity check of this on FCOS and RHCOS.

On FCOS, I noticed some avc denied issues I hadn't seen before which I think is from journald racing against /var getting relabeled (likely another instance of systemd/systemd#11903, see https://github.com/jlebon/ignition-dracut/blob/fd254750aec187ee75ea4ea0c56355e120ce1c56/dracut/30ignition/coreos-populate-var.sh#L33). Though it does succeed eventually and there are no failed services! Will look into it after, though I don't think it should block this.

On RHCOS, the situation is more serious. ignition-disks doesn't work at all. If fails with:

Mar 26 15:43:35 localhost ignition[586]: disks: createPartitions: op(1): [failed]   waiting for devices [/dev/vdb]: dbus: authentication protocol error

This is not a regression from this PR or Ignition v2.0.0-alpha: specifying any extra disk on the latest dev RHCOS will fail similarly. @ajeddeloh mentioned it's likely systemd/systemd#9625. Testing that now.

Copy link
Contributor

@ajeddeloh ajeddeloh left a comment

Choose a reason for hiding this comment

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

LGTM

@jlebon
Copy link
Member Author

jlebon commented Mar 26, 2019

OK, yup, I can confirm we were hitting systemd/systemd#9625. With that patch, RHCOS is working fine here as well!

Let's merge this and discuss the systemd backport situation OOB?

@ajeddeloh
Copy link
Contributor

SGTM. Merge away

@jlebon
Copy link
Member Author

jlebon commented Mar 26, 2019

I don't have merging privileges on this repo.

@dustymabe
Copy link
Member

I don't have merging privileges on this repo.

you should now

@jlebon
Copy link
Member Author

jlebon commented Mar 28, 2019

On FCOS, I noticed some avc denied issues I hadn't seen before which I think is from journald racing against /var getting relabeled (likely another instance of systemd/systemd#11903

OK, I can confirm this. The basic issue here is that using tmpfiles.d to relabel /var leaves us open to any sort of services that run after var.mount but before systemd-tmpfiles-setup.service (see that systemd pull for discussions on that), which is the case here for systemd-journal-flush.service.

Mind you, it doesn't actually seem to cause any failures. The journal does get flushed to /var eventually. But it makes for a lot of denials in the journal. I'm still trying to understand though why systemd-tmpfiles-setup.service doesn't run before systemd-journal-flush.service in the first place (echoes from https://bugzilla.redhat.com/show_bug.cgi?id=1265295).

@jlebon
Copy link
Member Author

jlebon commented Mar 28, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants