-
Notifications
You must be signed in to change notification settings - Fork 112
Add OCI index detection for Nydus alternatives #669
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
base: main
Are you sure you want to change the base?
Add OCI index detection for Nydus alternatives #669
Conversation
e7e433c to
b7a0864
Compare
| export AUTH_TYPE='${{ inputs.auth-type }}' | ||
| export INDEX_DETECT='${{ inputs.index-detect }}' |
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.
export are needed so that the env variables get picked up in kind.sh. Without it, they won't. Which makes me think that the auth-type: cri actually did not work as expected previously 😅
Implement index detection feature that automatically discovers Nydus alternative manifests within OCI index manifests, similar to what SOCI snashotter is able to do. This enables transparent fallback to optimized Nydus images when available while keeping the original index manifest OCI compliant as non-nydus client can simply pull the regular OCI image. The feature is heavily based on the already existing ReferrerDetect feature which looks for nydus manifests using the referrer API. However, the referrer API is not supported by all registries and as it relies on changes happening outside the manifest being pulled it's a bit dangerous from a supply chain perspective. On the other hand, this new IndexDetect feature is supported everywhere and does not have the supply chain issue as everyting is packed in a single manifest. Building a manifest compatible with this feature can be easily done with the `--merge-platform` flag in `nydusify convert`. Key changes: - Add EnableIndexDetect config option to experimental features - Implement index package with manifest finding logic. Very similar to referrer package. The detection is based on the `platform.os.feature` (nydus.remoteimage.v1) or the `artifactType` (application/vnd.nydus.image.manifest.v1+json) field in the index manifest - Result is cached to avoid multiple API calls for the same digest - Integrate index detection into filesystem and snapshot layers. The index detection is done before the referrer detection: if both exist, we prefer to use the index detection for the supply chain issues mentionned above. - Add e2e test for index detection - Add documentation for the new feature
b7a0864 to
fb0aac1
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #669 +/- ##
==========================================
- Coverage 20.62% 20.59% -0.03%
==========================================
Files 122 125 +3
Lines 13709 13942 +233
==========================================
+ Hits 2827 2872 +45
- Misses 10564 10750 +186
- Partials 318 320 +2
🚀 New features to boost your workflow:
|
|
@Fricounet Thanks for submitting such a helpful PR, overall it's quite good! |
|
@imeoer after doing more tests with this feature, I actually noticed an issue with the way nydus manifests are built currently on certain clients without nydus support. After digging a bit into why this doesn't happen with soci implementation, I discovered that it's because we need to change the Do you think it would be fine to modify the config mediaType to let's say the same value as the |
|
And the change to the config would need to only be applied for images that use This is tricky to handle 😬 |
|
@imeoer So, I digged more into the possible solutions and here's what I got:
As a result I think what we can do is:
What do you think about this solution? Anything I'm missing? |
@Fricounet Thanks for the detailed investigation, agree with this solution. containerd primarily uses the |
|
@Fricounet Thanks! Tagged nydus-snapshotter v0.15.3. |
|
@imeoer thanks! Here's the acceleration-service PR goharbor/acceleration-service#351 |
Done! Tagged acceleration-service v0.2.20 |
|
Thanks and here is the final nydusify PR dragonflyoss/nydus#1756 |
Signed-off-by: Baptiste Girard-Carrabin <[email protected]>
b5192d8 to
52d1e58
Compare
|
@Fricounet nydus v2.3.6 released! |
|
Thanks for all the bumps @imeoer 🙇 I've now discovered a new tricky issue though: If image A has been
I'm trying to figure out how we can separate the non-nydus image pull from the nydus one so that this situation doesn't happen but I don't have an easy solution for now:
The value for the snapshot key is computed from the |
|
I believe something cleaner would be that the nydus snapshotter is able to only mount the relevant layer and not the complete original image when containerd calls Mounts but that's something that would need to be done within the nydus format itself and I'm not familiar enough with it to know if that's even possible. Maybe with the |
|
Hi @Fricounet By the code
Can we only find the index digest and return from the iteration which the snapshot matches The nydus bootstrap (metadata) layer have a different digest (or diff_id) for different images. |
|
By the way, regarding our internal nydus usage, in cluster each node is deployed nydus snapshotter, and also deployed a separate kubernetes webhook component, based on pod info, determines whether to replace the image name |
The issue with this approach (if I understood correctly) is that with the But even if we were somehow getting it, the fact that containerd doesn't try to unpack the common layer when it pulls image B because it thinks the layer is unpacked is the real killer here I think. It means that when we mount the layer, we can only give it the |
|
@imeoer Currently I think that the clean way to solve this issue would be to be able to mount intermediate layers. I was looking at the content of |
This is my fallback idea but I'd rather just have an image that's pure OCI compliant if we can as it feels cleaner than rewriting user image refs under the hood. Also, this approach doesn't quite work when users specify digests directly when pulling images. As you can't know which tag the digest refers to |
Sorry for the misunderstanding earlier. The nydus image always has one more bootstrap layer than an OCI image, this reduces the number of FUSE mountpoints (since each layer mounted separately increases instability) by merging all layer filesystem metadata into a single bootstrap via overlay like, this approach makes data (chunk) deduplication between layers / images easier and benefits filesystem analysis, such as security scanning, which only requires pulling the bootstrap, mounting each layer individually also complicate the snapshotter implementation. |
|
From the code, one thing I'm confused about is that the common layer should have different snapshot key in different images, since the chainID is computed from the diff_ids of previous layers, why the containerd think the common layer in image B has already been prepared? |
Sorry, this part is probably confusing because it's a behavior that's introduced by the nydusify changes in v2.3.6:
Does that make more sense? |
|
By the way, I'm fairly sure the issue of layers being reused which leads to the full filesystem of the nydus image being mounted in the OCI one is also an issue with the existing |
Yes that makes sense to not want to mount each individual layer in it's own FUSE. I don't propose we change this for the general case. |
|
Hi @Fricounet, the nydus blob format is the structure that arranges data as follows:
To implement this logic, the snapshotter process might become quite complex. |
|
Thanks a lot for the explanation, I think I understand the situation better now:
Are you fundamentally against me trying to make this work? I understand that it adds complexity however, it would bring a few benefits outside of this PR:
But if you think that's a truly bad idea, I'll drop it |
|
Hi @Fricounet,
Yes it's correct understanding, the
We can fetch and extract just the layered-bootstrap from a remote blob, instead of fetching the entire blob, so the
The blob data ( EntryBlob:
It's okay for me, it's very useful to mount layer independently, but it may require separating the logic from the normal merged bootstrap process in the snapshotter, nydusd also supports mounting multiple bootstraps by mount API (pseudo mounts). |
If that's done in an overlay-like way, I assure the merge process is not easily reversible then? Meaning it's not possible to get the individual bootstraps file the constitute the final bootstrap file if we only have the final bootstrap?
Are you talking about this implementation for a remote instance?
Gotcha, I'll have a stab at it to see if I can make this work without too much changes On the other hand, I do have an alternative way of fixing this (code POC) but this involves changing the original OCI image that's referenced in the final index to modify it's layers so that they can't be considered shared by containerd anymore. But this only fix the issue for the merge-platform+indexDetect case, not for the with-referrer+referrerDetect case since the OCI manifest is unchanged in the latter. |
Yes, the merged bootstrap can't be reverted to layered-bootstraps since it doesn't preserve whiteout information (such as deleted or opaque files).
Yes!
👍, Does the empty layer with a fixed digest cause the share issue? |
Quite the opposite, it solves the layer sharing issue between merged-manifest images and pure OCI ones. Adding an empty layer at the beginning of the OCI image present in the
The main drawback of this approach is the we have to rewrite part of the original OCI image (so its digest changes) which means that the OCI image in the final merged manifest will not be exactly the same as the original. If you think this change is reasonable I can clean up the code a make a PR with it in the acceleration service |
Thanks for the explanation, LGTM! This approach might feel a bit hacky, but it brings greater benefit when combining OCI v1 and nydus into a single image reference. |
Indeed, since the merge-platform is useless today, it might be better to have something that works albeit a bit ugly compared to nothing at all. I've opened goharbor/acceleration-service#352 |
|
Hi @Fricounet, do you have a slack account? I just sent a direct message. :) |

Overview
Please briefly describe the changes your pull request makes.
Implement index detection feature that automatically discovers Nydus alternative manifests within OCI index manifests, similar to what SOCI snashotter is able to do. This enables transparent fallback to optimized Nydus images when available while keeping the original index manifest OCI compliant as non-nydus client can simply pull the regular OCI image. The feature is heavily based on the already existing ReferrerDetect feature which looks for nydus manifests using the referrer API. However, the referrer API is not supported by all registries and as it relies on changes happening outside the manifest being pulled it's a bit dangerous from a supply chain perspective. On the other hand, this new IndexDetect feature is supported everywhere and does not have the supply chain issue as everyting is packed in a single manifest. Building a manifest compatible with this feature can be easily done with the
--merge-platformflag innydusify convert.Related Issues
Please link to the relevant issue. For example:
Fix #123orRelated #456.Comes from discussions we had in dragonflyoss/nydus#1692 (comment). Additionally, this will help with dragonflyoss/nydus#463 since it will be easier to have OCI-compliant nydus images
Change Details
Please describe your changes in detail:
Key changes:
platform.os.feature(nydus.remoteimage.v1) or theartifactType(application/vnd.nydus.image.manifest.v1+json) field in the index manifestTest Results
If you have any relevant screenshots or videos that can help illustrate your changes, please add them here.
Added an e2e test and some unit tests to cover this new feature.
I have not added a test in the
integration/directory mainly because there is no image built withmerge-platforminghcr.io/dragonflyoss/image-service/wordpress. If one is added, I can add an integration test as well.Change Type
Please select the type of change your pull request relates to:
Self-Checklist
Before submitting a pull request, please ensure you have completed the following: