-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add ID-mapping capabilities #1392
Conversation
2d0cf6d
to
7f2fd08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for including the modified ctr as a individual commit. Makes it easy to revert if containerd accepts the upstream contribution later.
Overall nice work iterating on this. I would like to see the logging cleaned up a bit. Some considerations:
- Quite a bit of info logging occurring. Should some of these be reduced to debug or trace logs? Users running in production operate a large scale, so cost of logs increases quickly. So make each log count.
- Prefer structured logging over formatting. Think about the tech that gets paged and has a hard time finding where in code an error originated because it has been formatted.
7f2fd08
to
b87f962
Compare
New changes address logging concerns as well (EDIT: missed some) as moved the FUSE server setup to a new function. Also added a new internal patch which will make the diff look even bigger but it's just so we can store the patch locally. |
b87f962
to
79b17e1
Compare
FWIW, I tested these changes on the ubuntu runners and they all seemed to pass?? Not sure what it might be indicative of. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still reviewing, but I wanted to get my comments in so far before they drift too far.
3cb0543
to
28872ee
Compare
28872ee
to
02a1575
Compare
Looks like I undid a ton of changes and messed things up while rebasing 😓 Manually re-added everything I think but should double check. Will double-check multi-uid/gid mapping shenanigans tomorrow as well as using nerdctl instead of ctr but I'll have to see. |
02a1575
to
fe36d4d
Compare
63bb0a7
to
0728c4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure about the -pR change, we need to test more to approve this
if with -a the concerns about hardlink is indeed true then we can change, otherwise i will prefer not to change this option.
0728c4b
to
33c0b64
Compare
Ended up just using |
33c0b64
to
49f5d3b
Compare
Rebased with main |
nerdctl currently does not support ID mapping natively, so this commit adds the code for us to use a patched version of nerdctl that supports this. Once this is supported upstream natively, we can revert this. Signed-off-by: David Son <[email protected]>
Taken from containerd commit 83aaa89, this adds the necessary tools to add idmapping capabilities to SOCI. Signed-off-by: David Son <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tightening up the logging. It looks much better!
Change LGTM.
a536f13
to
c19f309
Compare
Newest changes should address most recent concerns with variable naming and also adds testing for files inside the container as well |
After this is merged I think documentation on how to actually use this feature would be nice. I can take it as a followup. |
This commit adds ID mapping functionality in SOCI. ID mapping is enabled if the correct labels are passed through. To avoid having containerd handle the ID mapping, we must declare in the containerd config file that the snapshotter supports ID mapping. Note that usage of this feature requires proxy plugins to have capabilities, which is only supported in containerd v1.7.23 onwards. Signed-off-by: David Son <[email protected]>
c19f309
to
4a2234d
Compare
Issue #, if available:
Description of changes:
Working off the POC at Kern--/idmap-v2, added id-mapping capabilities to the snapshotter.
The PR additionally has a commit to add a separate
ctr binarynerdctl binary (EDIT: later iterations changed this to a nerdctl binary) to the container. This is because containerd doesn't support id-mapping inctrnerdctl in an official release yet, so we need a specific binary to pass the proper labels. This is done by adding a new Makefile target and copying it in the Dockerfile, but I'm open to different approaches here.Testing performed:
make test
andmake integration
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.