convertor: support multi-arch images #266
Conversation
go.mod
Outdated
| @@ -10,17 +10,17 @@ require ( | |||
| github.com/moby/locker v1.0.1 | |||
| github.com/moby/sys/mountinfo v0.6.2 | |||
| github.com/opencontainers/go-digest v1.0.0 | |||
| github.com/opencontainers/image-spec v1.1.0-rc2.0.20221005185240-3a7f492d3f1b | |||
| github.com/opencontainers/image-spec v1.1.0-rc5 | |||
There was a problem hiding this comment.
image-spec 1.1.0 has been release, use v.1.1.0 instead of rc? https://pkg.go.dev/github.com/opencontainers/[email protected]/specs-go/v1
There was a problem hiding this comment.
Thanks for the heads up, I'll have a look at the differences between the 2 versions.
cmd/convertor/builder/builder.go
Outdated
| // (TODO) platform filter | ||
| manifestDescs = append(manifestDescs, index.Manifests...) |
There was a problem hiding this comment.
it may be possible that an index has another index which has other manifests? maybe good to handle ImageIndex case recursively?
There was a problem hiding this comment.
This is in fact explicitly valid according to the oci spec https://github.com/opencontainers/image-spec/blob/f5f87016de46439ccf91b5381cf76faaae2bc28f/image-index.md?plain=1#L46 I doubt its common common though or if wed want to support it in practice.
There was a problem hiding this comment.
Anyway, I rewrote a version of the implementation to handle the recursive index.
cmd/convertor/builder/builder.go
Outdated
| platform = platforms.Format(*mDesc.Platform) | ||
| rctx = log.WithLogger(gctx, log.G(ctx).WithField("platform", platform)) | ||
| } | ||
| workdir := filepath.Join(opt.WorkDir, strings.ReplaceAll(platform, "/", "-")) |
There was a problem hiding this comment.
Unfortunately, there is no guarantee that there is only one platform type per manifest as weird as that sounds and there is no requirement to include the platform in the index, its even possible to repeat the same manifest. To avoid any possible work folder collisions, we might want to add some additional value to the workdir e.g make it a guid or maybe add the manifest number in the index to the path?
There was a problem hiding this comment.
Changed to "{id}-{platform}-{digest}" format, id is an auto-incrementing key.
cmd/convertor/builder/builder.go
Outdated
| var mu sync.Mutex | ||
| var targetDescs []v1.Descriptor | ||
| g, gctx := errgroup.WithContext(ctx) | ||
| for _, _mDesc := range manifestDescs { |
There was a problem hiding this comment.
Following the introduced logic, we are basically creating an engine per manifest to convert each sub manifest. This introduces some concurrency concerns, especially because each conversion can potentially take up substantial storage space, we may want to add a limit to the number of manifests that can be converted at once and maybe make that configurable.
There was a problem hiding this comment.
add --concurrency-limit, default 4.
cmd/convertor/builder/builder.go
Outdated
|
|
||
| mu.Lock() | ||
| defer mu.Unlock() | ||
| targetDescs = append(targetDescs, desc) |
There was a problem hiding this comment.
Are there any annotations that we could add to denote that the artifacts are streamable ?
There was a problem hiding this comment.
I made a few changes to keep the artifacts in their original order
WaberZhuang
force-pushed
the
feat/multi-arch
branch
from
db1b78e
to
425df9e
Compare
WaberZhuang
force-pushed
the
feat/multi-arch
branch
from
425df9e
to
711655f
Compare
Signed-off-by: zhuangbowei.zbw <[email protected]>
WaberZhuang
force-pushed
the
feat/multi-arch
branch
from
711655f
to
db0aa5a
Compare
What this PR does / why we need it:
Add support for converting multi-arch images.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
#264
Please check the following list: