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

Remove zpool altroot when creating datasets. #1018

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

Conversation

kostaslambda
Copy link

poudriere breaks if the zpool it's given to use has the altroot property set, as its internal dataset mountpoints are different than the path they were actually mounted on.

Specifically, if the zpool has altroot set, all mounted datasets will have altroot prepended to their mountpoint. When setting BASEFS, the user would naturally take that into account and include altroot in the path. The setting would look something like: BASEFS=<altroot>/rest/of/path. However, what happens is that the BASEFS mountpoint is actually <altroot><altroot>/rest/of/path, while poudriere thinks it's <altroot>/rest/of/path.

To fix the problem, simply check if altroot is set and remove it from the mount path at the point of the dataset creation. The dataset will be prepended with altroot, which will match the path poudriere knows about.

@kostaslambda
Copy link
Author

ping?

[ $# -ne 1 ] && eargs remove_altroot mountpoint
local mountpoint altroot
mountpoint="$1"
altroot="$([ -z "${NO_ZFS}" ] && zpool get -Ho value altroot "${ZPOOL}")"
Copy link
Member

Choose a reason for hiding this comment

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

Why the $() for this? We don't need a subshell for the test; we may avoid a fork if NO_ZFS is set.
We need to set a default value or unset altroot too for NO_ZFS.

altroot=
if [ -z "${NO_ZFS}" ]; then
  altroot=$(zpool get -Ho value altroot "${ZPOOL}")
fi

Or

altroot=
[ -n "${NO_ZFS}" ] || altroot=$(zpool get -Ho value altroot "${ZPOOL}")

The test && test will exit with set -e, even if in a subshell. Using || won't exit.

# sh -c 'set -e; foo=$(false && true); echo here'; echo $?
1
# sh -c 'set -e; foo=$(false && true) || :; echo here'; echo $?
here
0
# sh -c 'set -e; foo=$(false || true); echo here'; echo $?
here
0

Copy link
Author

Choose a reason for hiding this comment

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

Done. Thanks for the detailed example. My shell-fu is not very strong :)

@bdrewery
Copy link
Member

bdrewery commented Nov 4, 2022

altroot seems like a temporary thing. What's the use case for something like this?

@kostaslambda kostaslambda force-pushed the bugfix/zpool_altroot branch 2 times, most recently from 969b9b1 to cfa99e9 Compare November 5, 2022 12:21
@kostaslambda
Copy link
Author

I came across this while trying to use poudriere on TrueNAS, which sets altroot=/mnt for all pools. Maybe it's uncommon for people to use that property, but it could still happen.

I've also changed the commit message, as it was wrong and maybe a bit convoluted. Let me know what you think.

If `${ZPOOL}` has `altroot` set, `${ZROOTFS}` can only be mounted
under `altroot`. This means `${BASEFS}` _must_ begin with `altroot`.

In `poudriere.conf`, the setting would look something like:
`BASEFS=<altroot>/rest/of/path`.

However, `altroot` is always prepended to whatever path is set to
`mountpoint` at the time of dataset creation. What happens is that
`${ZROOTFS}` is actually mounted on `<altroot>${BASEFS}`, while
`poudriere` thinks it's on `${BASEFS}`.

To fix the problem, simply check if `altroot` is set and remove it
from `mountpoint` when creating datasets. Their `mountpoint`s will
then be prepended with `altroot`, which matches `poudriere`'s internal
records.
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