Skip to content

Create autofs-resolve mount option to pre-resolve paths before mounting a namespace#1706

Open
Abrar Quazi (AbrarQuazi) wants to merge 3 commits into
masterfrom
enable-autofs-resolution-mounting-option
Open

Create autofs-resolve mount option to pre-resolve paths before mounting a namespace#1706
Abrar Quazi (AbrarQuazi) wants to merge 3 commits into
masterfrom
enable-autofs-resolution-mounting-option

Conversation

@AbrarQuazi
Copy link
Copy Markdown
Contributor

Problem
Wakebox jobs fail when trying to bind mount autofs paths with "Too many levels of symbolic links" errors. This occurs because autofs resolution doesn't work inside user namespaces - as that is unsupported without proper mount propagation (which we don't want inorder to maintain our isolated namespace).

Solution
Add a new autofs-resolve mount operation type that pre-resolves autofs paths in the host namespace before entering the user namespace. This allows wakebox to trigger autofs mounts while still in the host namespace and having mounted filesystems before entering the isolated namespace where bind mounts can succeed.

Comment thread src/wakefs/fuse.cpp
Comment on lines +152 to +153
// Pre-resolve autofs paths through stating the paths on host machine
static bool resolve_autofs_mounts(const std::vector<mount_op> &mount_ops) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how I feel about having something specific to autofs at this level -- on the one hand, it seems like requiring users to know fine details about how the filesystem is laid out would from their perspective appear to be busywork, on the other this is sitting alongside methods like create-dir and tmpfs which require a similar level of knowledge about the disk layout. I think on the balance this is likely better than not, but I do wonder if there's any way to do this that's a bit more behind the scenes. Then again, that's a question we've been looking at for a while, so we'd probably have come up with at least the vague shape of one by now if there was.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we not just make this "eagerly_resolve_mounts" or "resolve_lazy_mounts"? The code itself doesn't have to do with autofs, its just going to touch/read each directory in the mount point before actually trying to mount and while that helps with autofs it doesn't seem like other FS's might not have similar issues?

Comment thread src/wakefs/fuse.cpp
Comment on lines +171 to +175
while (std::getline(ss, directory, '/')) {
if (!directory.empty()) { // Skip slashes
directories.push_back(directory);
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm also a bit concerned by the potential for bad structures here. Since it's all specifically under the autofs I guess we'd have to resolve it, and just rely on people building systems where Wake will be run being able to weigh the impact of autofs (and avoid the potential pathological setups), but for one example how does this handle a /usr/child -> /usr/long/path/from/parent/autofs/to/child/autofs symlink where both /usr/.../parent and /usr/child are listed in the autofs_paths? (And even if symlinks are realpathed, what about some similarly-degenerate bindmount?)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we can partially resolve this by only touching/reading the exact paths given to us by the user, rather than trying to do the entire hierarchical path.
This gives the user control and they can do resolve-bind-mount a/b, resolve-bin-mount a/b/c if they want more parts of the hierarchy to be resolved before bind mounting.

Comment thread src/wakefs/fuse.cpp
for (const auto &path : autofs_paths) {
struct stat st;
if (stat(path.c_str(), &st) == 0) {
std::cerr << "Pre-resolved autofs path: " << path << std::endl;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Where do debug prints like this land? If it's runner output then this might be all right, particularly if it's intermixed with similar prints from one or two of the other mount types, but if there's any runners which are tempted to put it in the Job stderr then it would probably be better to remove the temptation by not printing it in the first place. The failure case is good either way (if more ideal in the runner error).

Also, I recognize that this could easily be run by the wakebox tooling or something else external to Wake; we can't just send it to fd4 and call it good.

@colinschmidt
Copy link
Copy Markdown
Collaborator

Another thought I had was if we could just make all binds run this stat on the source first? Or if that is an unacceptable slow down?

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