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

using device mapper name for systemd unit instead #224

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Luiz-Monad
Copy link

@Luiz-Monad Luiz-Monad commented May 14, 2022

Why don't we use the mapper name in the systemd unit alias, the uuid is kinda ugly when I do systemctl status.

Also, I don't have uuidparse on my system.

I also needed the multi-user.target patch because I use lvm and my btrfs is on top of it. (this should be a separe commit, I can fix it later)
Nevertheless it seems to be working in my build system machine. Thanks for the nice project I use for saving space of my stupid .o objects.

Copy link
Contributor

@kakra kakra left a comment

Choose a reason for hiding this comment

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

It looks fine apart from some nitpicks and oversights.

But this makes me think if we should rather use a systemd-generator in the long term which generates systemd units on the fly for each btrfs partition found and configured in the config file?

@@ -1,7 +1,8 @@
[Unit]
Description=Bees (%i)
Documentation=https://github.com/Zygo/bees
After=sysinit.target
#After=sysinit.target
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say, simply remove the commented line. After=local-fs.target is actually correct here. I'm using the same dependency in my custom crafted unit file.

Copy link
Author

Choose a reason for hiding this comment

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

I think it was a mistake perhaps. I don't know why I wanted it before the plymouth start, perhaps I was testing from the frame-buffer console and it was crashing because my space ran out.
If it wants local-fs.target that's a dependency of sysinit.target, then it shouldn't matter if it starts before or after sysinit.target.

systemctl list-dependencies sysinit.target
sysinit.target
● ├─apparmor.service
● ├─blk-availability.service
● ├─debug-shell.service
● ├─dev-hugepages.mount
● ├─dev-mqueue.mount
● ├─haveged.service
● ├─keyboard-setup.service
● ├─kmod-static-nodes.service
● ├─lvm2-lvmpolld.socket
● ├─lvm2-monitor.service
○ ├─plymouth-read-write.service
● ├─plymouth-start.service
● ├─proc-sys-fs-binfmt_misc.automount
● ├─resolvconf.service
● ├─sys-fs-fuse-connections.mount
● ├─sys-kernel-config.mount
● ├─sys-kernel-debug.mount
● ├─sys-kernel-tracing.mount
○ ├─systemd-ask-password-console.path
○ ├─systemd-binfmt.service
○ ├─systemd-boot-system-token.service
○ ├─systemd-journal-flush.service
● ├─systemd-journald.service
○ ├─systemd-machine-id-commit.service
● ├─systemd-modules-load.service
○ ├─systemd-pstore.service
○ ├─systemd-random-seed.service
○ ├─systemd-repart.service
● ├─systemd-sysctl.service
● ├─systemd-sysusers.service
● ├─systemd-timesyncd.service
● ├─systemd-tmpfiles-setup-dev.service
● ├─systemd-tmpfiles-setup.service
● ├─systemd-udev-trigger.service
● ├─systemd-udevd.service
● ├─systemd-update-utmp.service
● ├─cryptsetup.target
● ├─integritysetup.target
● ├─local-fs.target
● │ ├─-.mount
● │ ├─boot.mount
● │ ├─home.mount
● │ ├─systemd-remount-fs.service
● │ ├─tmp.mount
● │ ├─var-data\x2dcache.mount
● │ └─var.mount
● ├─swap.target
● │ ├─dev-mapper-data\x2d\x2dvg\x2dswap0.swap
● │ └─dev-mapper-data\x2d\x2dvg\x2dswap1.swap
● └─veritysetup.target

fi
if [ ! -z "$(blkid /dev/mapper/$arg -o value | head -1)" ]; then
UUID="$(blkid /dev/mapper/$arg -o value | head -1)"
INFO "Found $UUID for $arg"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

This misses the "help" branch condition which has been there previously. Please convert to using elif and finally end with help.

Copy link
Author

Choose a reason for hiding this comment

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

fair enough

Copy link
Author

Choose a reason for hiding this comment

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

isn't it going to fall to

[ -z "$UUID" ] && help
either way as UUID is never set if we don't find it ?

@@ -56,4 +57,4 @@ AmbientCapabilities=CAP_DAC_OVERRIDE CAP_DAC_READ_SEARCH CAP_FOWNER CAP_SYS_ADMI
NoNewPrivileges=true

[Install]
WantedBy=basic.target
WantedBy=multi-user.target
Copy link
Contributor

@kakra kakra Jun 7, 2022

Choose a reason for hiding this comment

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

I'm not sure about this... While it may fix your problem (and isn't actually incorrect), I rather think that it solves your problem only by accident. After=local-fs.target should be enough for your lvm setup to become ready, otherwise we may need another After= line maybe?

Don't get me wrong: I think this change should go into the commit (actually, that's what my custom-crafted unit file also uses). But I think it doesn't reliably solve your problem. I think in case of lvm, there may be another synchronization point missing. What's the output of systemd-analyze critical-chain?

PS: I wonder why the bees-provided unit file still contains the old broken dependencies. I'm pretty sure this has been discussed before.

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.

2 participants