Skip to content

Use systemd mount unit file for extra volume mount in compat mode #245

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

Closed
rhvgoyal wants to merge 1 commit into from
Closed

Use systemd mount unit file for extra volume mount in compat mode #245

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 84 additions & 2 deletions container-storage-setup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,7 +1054,7 @@ extra_lv_mountpoint() {
local vg=$1
local lv_name=$2
local mount_dir=$3
mounts=$(findmnt -n -o TARGET --source /dev/$vg/$lv_name | grep $mount_dir)
mounts=$(findmnt -n -o TARGET --source /dev/$vg/$lv_name | grep "^$mount_dir")
echo $mounts
}

Expand Down Expand Up @@ -1109,6 +1109,88 @@ setup_extra_volume() {
fi
}

# This is used only in compatibility mode. We are still using systemd
# mount unit only for compatibility mode. Reason being that upon service
# restart, we run into races and device might not yet be up when we try
# to mount it. And we don't know if this is first time start and we need
# to create volume or this is restart and we need to wait for device. So
# continue to use systemd mount unit for compatibility mode.
setup_systemd_mount_unit_compat() {
local filename
local vg=$1
local lv_name=$2
local mount_dir=$3
local unit_file_path

# filename must match the path ${mount_dir}.
# e.g if ${mount_dir} is /var/lib/containers
# then filename will be var-lib-containers.mount
filename=$(systemd_escaped_filename ${mount_dir})
unit_file_path="/etc/systemd/system/$filename"
Copy link
Member

@cgwalters cgwalters Jun 2, 2017

Choose a reason for hiding this comment

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

I'm not certain about re-engineering this now, but doing things like this is exactly why systemd generators were created. This is fresh on my mind since I recently wrote one for ostree.

Copy link
Collaborator Author

@rhvgoyal rhvgoyal Jun 2, 2017

Choose a reason for hiding this comment

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

So I will have to write a generator of my own? How it is different than what I am doing now? I am also generating a unit file. Just that outside the systemd mechanism to call generators.

Also I am assuming that I need to drop a generator binary somewhere (instead of script) and we don't compile anything in this package.

So for this use case, it feels like that switching to generator mechanism might be work and less gain.

Copy link
Member

@cgwalters cgwalters Jun 2, 2017

Choose a reason for hiding this comment

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

Generators can be shell scripts, just look at:

# rpm -qf /usr/lib/systemd/system-generators/selinux-autorelabel-generator.sh 
policycoreutils-2.5-20.fc25.x86_64
# cat /usr/lib/systemd/system-generators/selinux-autorelabel-generator.sh
#!/bin/sh

# This systemd.generator(7) detects if SELinux is running and if the
# user requested an autorelabel, and if so sets the default target to
# selinux-autorelabel.target, which will cause the filesystem to be
# relabelled and then the system will reboot again and boot into the
# real default target.

PATH=/usr/sbin:$PATH
unitdir=/usr/lib/systemd/system

# If invoked with no arguments (for testing) write to /tmp.
earlydir="/tmp"
if [ -n "$2" ]; then
    earlydir="$2"
fi

set_target ()
{
    ln -sf "$unitdir/selinux-autorelabel.target" "$earlydir/default.target"
}

if selinuxenabled; then
    if test -f /.autorelabel; then
        set_target
    elif grep -sqE "\bautorelabel\b" /proc/cmdline; then
        set_target
    fi
fi

Copy link
Member

@cgwalters cgwalters Jun 2, 2017

Choose a reason for hiding this comment

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

The argument for writing a generator is that we're plugging into a well-understood mechanism designed for generating unit files.

To repeat again, it handles the daemon reload thing for us. Another thing is that the generated units are explicitly runtime only, stored in /run. That means it's much clearer that the source config files are the canonical thing. If it's in /etc a sysadmin might think they should edit it.

Actually, that's another thing - if a sysadmin wants to override or extend the unit, then units stored in /etc will take precendence over it. Which is another issue that I fixed with the ostree generator for /var.


# If unit file already exists, nothing to do.
[ -f "$unit_file_path" ] && return 0
Copy link
Member

@cgwalters cgwalters Jun 2, 2017

Choose a reason for hiding this comment

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

Pet peeve: this isn't atomic - if the system crashes in the middle we could have a partially written mount unit. Use a temporary file and rename into place. (Perhaps someday bash will support O_TMPFILE?)

Copy link
Collaborator Author

@rhvgoyal rhvgoyal Jun 2, 2017

Choose a reason for hiding this comment

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

Sure. other parts of scripts are already doing this logic. Will use it for unit file too.


cat <<EOF > "${unit_file_path}.tmp"
# WARNING: This file was auto generated by container-storage-setup. Do not
# edit it. In the future, this file might be moved to a different location.
[Unit]
Description=Mount $lv_name on $mount_dir directory.
Before=docker-storage-setup.service

[Mount]
What=/dev/$vg/$lv_name
Where=${mount_dir}
Type=xfs
Options=defaults

[Install]
WantedBy=docker-storage-setup.service
EOF
mv "${unit_file_path}.tmp" "$unit_file_path"
systemctl daemon-reload
systemctl enable $filename >/dev/null 2>&1
systemctl start $filename
}

setup_extra_lv_fs_compat() {
[ -z "$_RESOLVED_MOUNT_DIR_PATH" ] && return 0
if ! setup_extra_dir $_RESOLVED_MOUNT_DIR_PATH; then
return 1
fi
# If we are restarting, then extra volume should exist. This unit
# file is dependent on extra volume mount unit file. That means this
# code should run after mount unit has activated successfully. That
# means after extra volume has come up.

# We had got rid of this logic and reintroducing it back. That means
# there can be configurations out there which have extra volume but
# don't have unit file. So in such case, drop a unit file now. This
# is still racy though. There is no guarantee that volume will be
# up by the time this code runs when unit file is not present already.
if extra_volume_exists $CONTAINER_ROOT_LV_NAME $VG; then
if ! setup_systemd_mount_unit_compat "$VG" "$CONTAINER_ROOT_LV_NAME" "$_RESOLVED_MOUNT_DIR_PATH"; then
Copy link
Member

@cgwalters cgwalters Jun 2, 2017

Choose a reason for hiding this comment

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

If the admin changes the config file, we won't regenerate the mount unit, right?

Copy link
Collaborator Author

@rhvgoyal rhvgoyal Jun 2, 2017

Choose a reason for hiding this comment

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

If admin changes config file to start mounting on a different path, then a mount unit will be generated. Ideally I am expecting user to "reset" existing configuration and then make changes to config file. Otherwise there are many possibilities and many things can go wrong.

On a side note, to cover for all these corner cases, I have written new commands for container-storage-setup and at some point of time we should make use of it. These commands store their
own metadata and don't allow arbitrary changes to existing configuration.

Fatal "Failed to setup systemd mount unit for extra volume $CONTAINER_ROOT_LV_NAME."
fi
return 0
fi

if [ -z "$CONTAINER_ROOT_LV_SIZE" ]; then
Fatal "Specify a valid value for CONTAINER_ROOT_LV_SIZE."
fi
if ! check_data_size_syntax $CONTAINER_ROOT_LV_SIZE; then
Fatal "CONTAINER_ROOT_LV_SIZE value $CONTAINER_ROOT_LV_SIZE is invalid."
fi
# Container runtime extra volume does not exist. Create one.
if ! setup_extra_volume $CONTAINER_ROOT_LV_NAME $_RESOLVED_MOUNT_DIR_PATH $CONTAINER_ROOT_LV_SIZE; then
Fatal "Failed to setup extra volume $CONTAINER_ROOT_LV_NAME."
fi

if ! setup_systemd_mount_unit_compat "$VG" "$CONTAINER_ROOT_LV_NAME" "$_RESOLVED_MOUNT_DIR_PATH"; then
Fatal "Failed to setup systemd mount unit for extra volume $CONTAINER_ROOT_LV_NAME."
fi
}

setup_extra_lv_fs() {
[ -z "$_RESOLVED_MOUNT_DIR_PATH" ] && return 0
if ! setup_extra_dir $_RESOLVED_MOUNT_DIR_PATH; then
Expand Down Expand Up @@ -1246,7 +1328,7 @@ setup_storage_compat() {

# Set up a separate named ($CONTAINER_ROOT_LV_NAME) volume
# for $CONTAINER_ROOT_LV_MOUNT_PATH.
if ! setup_extra_lv_fs; then
if ! setup_extra_lv_fs_compat; then
Error "Failed to setup logical volume for $CONTAINER_ROOT_LV_MOUNT_PATH."
return 1
fi
Expand Down