-
Notifications
You must be signed in to change notification settings - Fork 5
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
update default build args, make pull use them #82
Conversation
This looks like it removes Wouldn't squashing the file ownership during the build/pull result in losing the separation in the image if you push it up and want to use it somewhere else? This particularly might be an issue if users are building images here and pushing them to use elsewhere. It would be better for users to specify a more appropriate id mapping at build time. |
It does add Since we've seen errors without |
What I'm afraid of is how the man page describes the effect of
So when the image gets pulled, it no longer has ownership separation between files in the image. Dan also says (emphasis mine):
... which to me suggests this makes sense for cases where users are not receiving a range of subordinate ids in If I understand the the errors correctly, the cases where we still need it is if users want to pull or build a container with an id above 65536. In those cases, if the user is doing a Or am I misunderstanding the permanence of the id squashing effect of |
As far as I understand the squashing to a single user from using I think it would be better to leave it on as default for our novice and intermediate users, and leave a note in our docs advising expert users how to turn it off if desired via specifying their own |
After some thought I think it makes sense in this PR, so I'll work on it. |
Ok, what do you think about this:
This gives users the ability to turn off the defaults and would require that they specify their own. I think this is reasonable for expert users. This is pretty straightforward for To disable
The resulting config looks like:
Note this will also require some changes in the @danfulton and @scanon what do you think of this proposed structure? |
Yeah, agree it makes sense to include here. I think I'm confusing myself about the diff though. Maybe we can discuss at containers meeting today. |
Ok, this makes the changes that @danfulton and I discussed yesterday:
What do you think? |
Looks good to me! Thanks for the fixes. |
Addresses
#81
#80
#76
Adjusts the base set of default args. Forces
pull
to use the default args (it wasn't before, and was ignoring log-level) and append extra args as needed. More in line with howshared_run
was already handling args.Have completed some preliminary testing on Muller- looks ok so far.
For example, from build:
DEBU[0037] Called build.PersistentPostRunE(/usr/bin/podman build --root /tmp/75313_hpc/storage --runroot /tmp/75313_hpc --storage-opt mount_program=/usr/bin/fuse-overlayfs-wrap --storage-opt ignore_chown_errors=true --cgroup-manager cgroupfs --hooks-dir /usr/share/containers/oci/hooks.d --env PODMANHPC_MODULES_DIR=/etc/podman_hpc/modules.d --annotation podman_hpc.hook_tool=true --log-level debug -t test:test .)
From pull:
DEBU[0001] Called pull.PersistentPostRunE(/usr/bin/podman pull --root /tmp/75313_hpc/storage --runroot /tmp/75313_hpc --storage-opt mount_program=/usr/bin/fuse-overlayfs-wrap --storage-opt ignore_chown_errors=true --cgroup-manager cgroupfs --log-level debug ubuntu:latest) INFO: Migrating image to /mscratch/sd/s/stephey/storage
From run:
time="2023-07-28T15:02:20-07:00" level=debug msg="Called run.PersistentPostRunE(/usr/bin/podman run --root /tmp/75313_hpc/storage --runroot /tmp/75313_hpc --storage-opt mount_program=/usr/bin/fuse-overlayfs-wrap --storage-opt ignore_chown_errors=true --cgroup-manager cgroupfs --storage-opt additionalimagestore=/mscratch/sd/s/stephey/storage --hooks-dir /usr/share/containers/oci/hooks.d --env PODMANHPC_MODULES_DIR=/etc/podman_hpc/modules.d --annotation podman_hpc.hook_tool=true --security-opt seccomp=unconfined --log-level debug --rm registry.nersc.gov/library/nersc/mpi4py:3.1.3 date)"