Update erofs snapshotter to use mount manager #12333
Conversation
|
Skipping CI for Draft Pull Request. |
3a31d36 to
6f2aa9c
Compare
6f2aa9c to
6245053
Compare
Signed-off-by: Derek McGowan <derek@mcg.dev>
Signed-off-by: Derek McGowan <derek@mcg.dev>
6245053 to
8c0456a
Compare
9f53900 to
fbedc23
Compare
fbedc23 to
92d4f19
Compare
92d4f19 to
22c4e73
Compare
Extend the mount manager to support more transformers than format. The transformers allow altering the mount before it is passed to the mount handlers. These could be one-time actions which are needed to perform the mount. Adds mkdir and mkfs actions which can be used to prepare the arguments for a mount. The actions can be limited to actions within the target mount directory or plugin directories. Signed-off-by: Derek McGowan <derek@mcg.dev>
I believe these are failing on windows because this is using |
Adding a close method allows the mount manager to close any open file descriptors. The method will also be called automatically by containerd on shutdown. Ensure the tests call Close to avoid leaking file descriptors or errors on Windows cleaning up directories that are in use. Signed-off-by: Derek McGowan <derek@mcg.dev>
Reduce the size of the test files. Even using sparse files, the reported large size may cause issues in some test environments. Signed-off-by: Derek McGowan <derek@mcg.dev>
| return fmt.Errorf("failed to remove snapshot %s: %w", key, err) | ||
| } | ||
|
|
||
| removals, err = s.getCleanupDirectories(ctx) |
There was a problem hiding this comment.
we can introduce asyncRemove flag for this.
|
One flake in the loopback test I'll look into |
Signed-off-by: Derek McGowan <derek@mcg.dev>
|
Added one more commit to address the flakiness in the loopback tests. I saw that flake outside of this PR as well, but it flaked in merge queue so should just include it here. |
This commit integrates dm-verity integrity verification into the EROFS snapshotter, adapted for the new mount manager architecture introduced in PR containerd#12333. Key changes: - Add enableDmverity configuration option to SnapshotterConfig - Add WithDmverity() option function for enabling dm-verity - Implement formatDmverityLayer() to format EROFS blobs with dm-verity hash trees during commit phase - Store dm-verity root hash in .roothash file for each layer - Modify mount specifications to use 'dmverity/erofs' mount type when dm-verity is enabled, delegating device setup to mount manager - Add plugin configuration option (enable_dmverity) - Import internal/dmverity package for dm-verity operations Architecture: Instead of the snapshotter directly managing dm-verity devices, this implementation uses the mount manager pattern where: 1. During Commit: format the EROFS blob with hash tree, save root hash 2. During mount: return mount spec with type 'dmverity/erofs' and X-containerd.dmverity.roothash-file option 3. Mount manager (via mount transformer) handles device setup/teardown Benefits: - Cleaner separation of concerns - Runtime handles mounting (not snapshotter) - No device lifecycle management in snapshotter - Follows new architecture patterns - Compatible with graceful shutdown Note: This requires a corresponding mount transformer to handle the 'dmverity/erofs' mount type (to be implemented separately).
|
@dmcgowan I've been playing around with erofs the last few days, and this PR is causing issues if I've got:
Without it the pod starts fine and user namespaces are indeed used. I think that code path needs to learn about the mount manager? |
should we apply the similar logic in pkg/oci/spec_opts.go:withReadonlyFS() to |
|
I probably need to better understand the id-mapped mount code path we have. My understanding is that we should be able to avoid the expensive snapshotter remapping logic. We could add the similar logic to those functions to make it complete, however, it is still slow and not the best way to achieve this. Especially with erofs, it is way better to just remap the metadata and avoid the costly mount->chown->commit. That inefficient approach was only intended as a stop gap until id-mapped mounts support or native snapshotter support. |
Yeah, agreed. I think idmap support is fine but this feature can only be used for newer kernels. Also for older kernels, mkfs.erofs has Yet if fsmerge feature is off and kernels don't have idmap support, I'm not sure if we need such compat logic. |
Oh wow, I thought we were using idmap support for the container in the end and it was just a particular call path that was bugging out and using the remap chown path. I realize now looking at my test setup that we're not doing the idmap at all. I need to revisit that all in the morning, I thought since (at least prior to this series) the Mount::Type was overlay for the erofs snapshotter that we'd hit this path during mount and end up doing the idmap bind mount for the lower dirs still, but that doesn't seem to be the case. We should definitely switch things over to using idmap support for erofs too, but I do wonder how we should handle cases like this where it seems the snapshotter expects mount manager to handle things but the call path isn't setup for that. To be honest I also need to look more at the mount manager integration too to give any useful suggestions. |
The erofs snapshotter doesn't adapt containerd idmapped mount support initially, but the kernel already supports this feature. Anyway, if we really need idmapped mount (in addition to old chown), it needs some extra development anyway. |
I've worked on this some today and think I got something going, I'll try and post a PR soon. Need to rebase with the mount manager support and see if that breaks anything, etc. |
Updates the erofs snapshotter to use the mount manager. With this change, the erofs snapshotter no longer needs to mount erofs block files, but can pass it along to the runtime to mount either when a task is created or inside the shim. This add a couple new features along side this, including..
This change is based on #12063