-
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
base: main
Are you sure you want to change the base?
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.
Makefile
Outdated
# Use a custom fork for testing ID-mapping as containerd doesn't fully support this yet. | ||
git clone https://github.com/sondavidb/containerd.git tempfolder | ||
cd tempfolder && \ | ||
git checkout multi-uid-gid && \ |
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.
Instead of taking a dependency on a developer fork, can we carry a patch in SOCI and pull a known good tag from upstream as a git submodule.
fs/fs.go
Outdated
@@ -455,6 +459,79 @@ func (fs *filesystem) getSociContext(ctx context.Context, imageRef, indexDigest, | |||
return c, err | |||
} | |||
|
|||
func (fs *filesystem) getIDMappedMountpoint(mountpoint, activeLayerKey string) string { |
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.
Function doesn't use any state from filesystem. Could this function be standalone?
fs/fs.go
Outdated
|
||
func (fs *filesystem) IDMapMount(ctx context.Context, mountpoint, activeLayerKey string, idmapper idtools.IDMap) (string, error) { | ||
newMountpoint := fs.getIDMappedMountpoint(mountpoint, activeLayerKey) | ||
log.G(ctx).WithField("mountpoint", newMountpoint).Info("creating remote id-mapped mount") |
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.
It looks like most logs in this function add this field. Perhaps something like this would be more concise.
log.G(ctx).WithField("mountpoint", newMountpoint).Info("creating remote id-mapped mount") | |
logger := log.G(ctx).WithField("mountpoint", newMountPoint) | |
logger.Info("creating remote id-mapped mount") |
log.G(ctx).WithField("mountpoint", newMountpoint).Info("failed to create remote id-mapped mount") | ||
return "", errdefs.ErrNotFound | ||
} | ||
fs.layer[newMountpoint] = l |
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.
So this adds the new mountpoint, but does not remove the "old" mountpoint. Is that intentional?
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.
It just reuses the same layer and builds on top of it AFAIK. I don't believe we need to unassign this in any way?
fs/fs.go
Outdated
if _, err := exec.LookPath(fusermountBin); err == nil { | ||
mountOpts.Options = []string{"suid"} // option for fusermount; allow setuid inside container | ||
} else { | ||
log.G(ctx).WithError(err).Infof("%s not installed; trying direct mount", fusermountBin) |
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.
Prefer structured logging instead of formatting. (handful of other places if you can get those as well)
log.G(ctx).WithError(err).Infof("%s not installed; trying direct mount", fusermountBin) | |
logger.WithField("binary", fusermountBin).WithError(err).Info("fuse binary not found; trying direct mount") |
snapshot/snapshot.go
Outdated
for i := range s.ParentIDs { | ||
id := s.ParentIDs[i] | ||
if _, ok := o.idmapped[s.ID]; ok { | ||
id = fmt.Sprintf("%s_%s", s.ParentIDs[i], s.ID) |
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.
Can use id
here instead of reading again.
snapshot/snapshot.go
Outdated
} | ||
|
||
func (o *snapshotter) createIDMapMounts(ctx context.Context, s storage.Snapshot, idmap idtools.IDMap) error { | ||
log.G(ctx).Infof("mapping ids") |
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.
Formatting is not required, but also is this logging too verbose? Perhaps consider removing or reducing to trace.
snapshot/snapshot.go
Outdated
// s.ID is the shortest unique identifier for each new container, | ||
// so append it to the end of the new mountpoint | ||
_, err := o.fs.IDMapMount(ctx, path, id, idmap) | ||
if err == errdefs.ErrNotFound { |
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.
For readability, let's use errdefs.IsNotFound(err)
here instead.
fs/fs.go
Outdated
return "", err | ||
} | ||
|
||
go server.Serve() |
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.
Does this goroutine need to be tethered to the passed in context? If not, it looks like the passed in context is only being used for logging.
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.
Is it a bad thing to tether logging to the context?
@@ -808,7 +812,9 @@ func entryToAttr(ino uint64, e metadata.Attr, out *fuse.Attr) fusefs.StableAttr | |||
mtime := e.ModTime | |||
out.SetTimes(nil, &mtime, nil) | |||
out.Mode = fileModeToSystemMode(e.Mode) | |||
out.Owner = fuse.Owner{Uid: uint32(e.UID), Gid: uint32(e.GID)} | |||
// Potentially dangerous casting int -> uint32? But probably fine. |
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.
?
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 generally cautious of casting from a signed int to an unsigned int, perhaps overly so 😅 I'd feel a bit more comfortable if we have some sort of bounds check before this?
fs/fs.go
Outdated
return "", err | ||
} | ||
|
||
rawFS := fusefs.NewNodeFS(node, &fusefs.Options{ |
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.
Is this all the same as in fs.Mount
? Can we have a helper function instead?
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]>
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. |
Signed-off-by: David Son <[email protected]>
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.
@@ -692,7 +770,8 @@ func (fs *filesystem) Unmount(ctx context.Context, mountpoint string) error { | |||
l, ok := fs.layer[mountpoint] | |||
if !ok { | |||
fs.layerMu.Unlock() | |||
return fmt.Errorf("specified path %q isn't a mountpoint", mountpoint) | |||
log.G(ctx).Errorf("specified path %q isn't a remote mount", mountpoint) |
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.
Can we wrap the error instead? We always try to Unmount
before UnmountLocal
so this is going to cause a lot of log messages.
@@ -692,7 +770,8 @@ func (fs *filesystem) Unmount(ctx context.Context, mountpoint string) error { | |||
l, ok := fs.layer[mountpoint] | |||
if !ok { | |||
fs.layerMu.Unlock() | |||
return fmt.Errorf("specified path %q isn't a mountpoint", mountpoint) | |||
log.G(ctx).Errorf("specified path %q isn't a remote mount", mountpoint) | |||
return errdefs.ErrNotFound | |||
} | |||
delete(fs.layer, mountpoint) // unregisters the corresponding layer | |||
l.Done() |
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.
Since we're reusing layers, I think this invalidates the entire image when any idmapped layer is cleaned up.
@@ -706,6 +785,10 @@ func (fs *filesystem) Unmount(ctx context.Context, mountpoint string) error { | |||
return syscall.Unmount(mountpoint, syscall.MNT_FORCE) | |||
} | |||
|
|||
func (fs *filesystem) UnmountLocal(ctx context.Context, mountpoint string) error { |
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.
What does it mean to unmount a local mount?
I think both idmapped and non-idmapped layers are handled in Unmount
.
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 binary to the container. This is because containerd doesn't support id-mapping in ctr 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.