Fix converter to be able to push manifests to ECR #639
Conversation
This field is not supported by certain registries like ECR and is not really needed according to dragonflyoss/nydus#1691.
This field is causing issues when attempting to push multi-arch images to ECR because the feature field is not supported in index manifests. According to dragonflyoss/nydus#1692, the goal of this feature is to help the container runtime pick the nydus image in an index manifest if there are multiple entries for the same architecture. However, as this is a feature that is not supported in any container runtime yet and the only way to get a manifest with both architecture is to use `nydusify convert --merge-platform`, which uses harbor acceleration-service to add the feature in the manifest, this means that we can remove it from the converter.
|
If the PR is merged, a new release will have to be cut for the snapshotter in order to use it in nydusify right? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #639 +/- ##
==========================================
+ Coverage 21.36% 21.39% +0.02%
==========================================
Files 122 122
Lines 13632 13614 -18
==========================================
Hits 2913 2913
+ Misses 10398 10380 -18
Partials 321 321
🚀 New features to boost your workflow:
|
| // Skip the manifest which is not modified. | ||
| continue | ||
| } | ||
| manifest.Platform.OSFeatures = append(manifest.Platform.OSFeatures, ManifestOSFeatureNydus) |
There was a problem hiding this comment.
Why is it not sufficient to remove only the line manifest.Platform.OSFeatures = append(manifest.Platform.OSFeatures, ManifestOSFeatureNydus) ? I'm concerned that omitting index.Manifests[i] = manifest might introduce unexpected issues elsewhere.
There was a problem hiding this comment.
@BraveY just removing the line you mentioned would indeed work. I removed the whole block because functionally, it now becomes:
for i, manifest := range index.Manifests {
index.Manifests[i] = manifest
}
which, in my opinion does not serve much purpose 😅 except complicating the code. If you really prefer that we keep it, I can add it back if you want. What do you prefer?
There was a problem hiding this comment.
@BraveY just removing the line you mentioned would indeed work. I removed the whole block because functionally, it now becomes:
for i, manifest := range index.Manifests { index.Manifests[i] = manifest }which, in my opinion does not serve much purpose 😅 except complicating the code. If you really prefer that we keep it, I can add it back if you want. What do you prefer?
Appreciate the clarification! My mistake stemmed from assuming index was empty, but since readJSON already initializes it, the loop adds no value. Agreed—let’s remove the redundant code.
Once the PR is merged, a new release of the snapshotter will indeed need to be published to use it in nydusify. For testing purposes, you can temporarily pin the commit hash directly in go.mod instead of referencing a tag. |
For testing purposes, let’s proceed with pinning the snapshotter’s commit hash in go.mod (using replace directive) to validate the proposed changes in nydusify. |
pkg/converter/convert_unix.go
Outdated
| @@ -922,36 +922,18 @@ func ConvertHookFunc(opt MergeOption) converter.ConvertHookFunc { | |||
| } | |||
| } | |||
|
|
|||
| // convertIndex modifies the original index by appending "nydus.remoteimage.v1" | |||
| // to the Platform.OSFeatures of each modified manifest descriptors. | |||
| // convertIndex modifies the original index converting it to manifest directly if it contains only one manifest. | |||
| func convertIndex(ctx context.Context, cs content.Store, orgDesc ocispec.Descriptor, newDesc *ocispec.Descriptor) (*ocispec.Descriptor, error) { | |||
| var orgIndex ocispec.Index | |||
There was a problem hiding this comment.
It seems we can remove the useless codes, also for the // Update image index in content store. below.
There was a problem hiding this comment.
Very good point, I hadn't caught that. Fixed in 6542a89
This code is useless now since we return the first manifest in the index directly without updating the index itself
Sure, I'm currently struggling in go version incompatibility in the go mod of nydusify (which is also present on the latest release of the snapshotter), but I'll try to get this done asap |
|
Let's tag the next release as |
This attempts to solve both dragonflyoss/nydus#1691 and dragonflyoss/nydus#1692.
Attempting to push some ECR manifests has 2 issues currently:
with-referrerfails because thesubjectfield in the manifest has aplatformfield. Although this is ok from an OCI spec perspective, ECR doesn't support it and it is not really needed so we can remove itplatformfails when attempting to build multi-arch manifests because theos.featurefield in the manifest is not supported either. We can remove it because this is not really supported yet in any container runtime. However, we still keep it when building images throughmerge-platformas it is the only way to differentiate a regular OCI image from a nydus one in that case. Fortunately, the merge-platform code goes through a different code path that uses Harbor's acceleration service and the os.feature field is set there.I did test this by rebuilding nydusify using the fixed converter and tried to push both images to ECR, which worked.
Additionally, I made sure that the
merge-platformfield was still working: