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

env: make the new mount options opt-in #1183

Closed
wants to merge 1 commit into from

Conversation

davvid
Copy link
Collaborator

@davvid davvid commented Feb 21, 2025

Rocky 9.4 does not support the lowerdir+= option.
Make the new mount options an opt-in feature.

Related-to: #968
Related-to: #1164

@davvid davvid force-pushed the new-mount-options branch 3 times, most recently from f08ed1e to c0100ac Compare February 21, 2025 03:23
Copy link
Collaborator

@jrray jrray left a comment

Choose a reason for hiding this comment

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

Another consideration is if to make the way we determine the length limit configurable:

match nix::unistd::sysconf(nix::unistd::SysconfVar::PAGE_SIZE) {

256 should still be long enough in most cases, but will require more layer flattening. Can we determine the correct limit at runtime?

# https://github.com/spkenv/spk/issues/968
legacy-mount-options = []
new-mount-options = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider changing this name to mount_options_allow_append since we all know "new" things don't stay that way.

@rydrman
Copy link
Collaborator

rydrman commented Mar 5, 2025

Thoughts from the meeting today:

  1. We already have infrastructure setup to try and detect the supported overlayfs options, can we piggyback on that in any way to move this from a compile-time flag to a low-cost runtime check?
  2. re-iterating the same idea for how we determine the length of the supported args to ensure that our existing logic still works on newer kernels with different limits

(these could both be separate tickets/ follow ups)

Rocky 9.4 does not support the `lowerdir+` mount option.
Make the new mount options an opt-in feature.

Related-to: spkenv#968
Related-to: spkenv#1164
@davvid davvid force-pushed the new-mount-options branch from c0100ac to 5e673f9 Compare March 8, 2025 18:59
@davvid
Copy link
Collaborator Author

davvid commented Mar 8, 2025

Sounds good. I renamed the feature flag for now.

I can follow up in a separate PR with (1) and (2).

Since lowerdir+ is dependent on the kernel version, I can look at extending our parsing of modinfo overlay to read the version from there. That'll avoid an extra call to uname -r or similar since we're already calling that.

Once I do that, then we can reorient this from a feature flag to runtime behavior.

My other thought is that this approach can probably be then extended to #1181 to eliminate the need for (2) at all, since doing the direct syscalls should avoid any length limits whatsoever.

At that point, I kinda wonder whether the version check will even be needed. In theory, I may be able to get this working without any limits since we'd be using direct syscalls, and it's possible that it might work using the current lowerdir options supported by older kernels, so there might not be much benefit in using lowerdir+, even though the kernel supports it.

I'll have to test that. If using syscalls eliminates the errors w/out lowerdir+ then I think we might still want an option to opt-in to using syscalls, just because it's a fairly low-level thing, but I'm curious about your opinion on that since it does explode the combinatorics. I think the ultimate goal would be to squash the things down into a single way, so hopefully any feature flags or config options would be relatively short-lived.

So.. initially:

  • Feature flag for lowerdir+

Then, if syscalls works with just lowerdir, then we'd have:

  • An option to enable syscalls, and those syscalls only use lowerdir
  • A feature flag for the mount code paths for enabling lowerdir+

So that's three different configurations. Pseudocode:

if syscalls_option_is_enabled:
    do_syscalls
elif use_lowerdir_plus_feature_flag:
    do_mount_with_lowerdirplus
else:
    do_mount_with_lowerdir (current behavior)

I'm 🤞 that the syscalls can work out without having to have an extra branch in there for whether or not to use lowerdir+.

Let me know if that sounds good. For now, this PR has been updated with the review notes so it might be okay to merge as-is.

I'll plan to re-appropriate #1181 to implement the plan laid out above if we y'all agree. I'd do that after this is merged so I can rebase against this as its base.

@davvid
Copy link
Collaborator Author

davvid commented Mar 13, 2025

Superseded by #1192 where this has gone from a feature flag to a runtime probe.

@davvid davvid closed this Mar 13, 2025
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