Skip to content
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

RFC: copy: add --multi-arch=sparse:... #1987

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 123 additions & 17 deletions cmd/skopeo/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ import (
"fmt"
"io"
"os"
"runtime"
"strings"

"github.com/containerd/containerd/platforms"
commonFlag "github.com/containers/common/pkg/flag"
"github.com/containers/common/pkg/retry"
"github.com/containers/image/v5/copy"
Expand All @@ -19,6 +21,8 @@ import (
"github.com/containers/image/v5/transports/alltransports"
encconfig "github.com/containers/ocicrypt/config"
enchelpers "github.com/containers/ocicrypt/helpers"
"github.com/opencontainers/go-digest"
imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/spf13/cobra"
)

Expand Down Expand Up @@ -99,24 +103,122 @@ See skopeo(1) section "IMAGE NAMES" for the expected format
}

// parseMultiArch parses the list processing selection
// It returns the copy.ImageListSelection to use with image.Copy option
func parseMultiArch(multiArch string) (copy.ImageListSelection, error) {
switch multiArch {
case "system":
return copy.CopySystemImage, nil
case "all":
return copy.CopyAllImages, nil
// There is no CopyNoImages value in copy.ImageListSelection, but because we
// don't provide an option to select a set of images to copy, we can use
// CopySpecificImages.
case "index-only":
return copy.CopySpecificImages, nil
// We don't expose CopySpecificImages other than index-only above, because
// we currently don't provide an option to choose the images to copy. That
// could be added in the future.
// It returns the copy.ImageListSelection, instance list, and platform list to use in image.Copy option
func parseMultiArch(globalOptions *globalOptions, multiArch string) (copy.ImageListSelection, []digest.Digest, []imgspecv1.Platform, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Eventually, it might be cleaner for this to update fields of copy.Options in-place instead of returning a list of values that need to be applied in the caller. For now, the three values have different types, which protects us against mistakes just fine.)

switch {
case multiArch == "system":
return copy.CopySystemImage, nil, nil, nil
case multiArch == "all":
return copy.CopyAllImages, nil, nil, nil
case multiArch == "index-only":
// There is no CopyNoImages value in copy.ImageListSelection per se, but
// we can get the desired effect by using CopySpecificImages.
return copy.CopySpecificImages, nil, nil, nil
case strings.HasPrefix(multiArch, "sparse:"):
sparseArg := strings.TrimPrefix(multiArch, "sparse:")
platformList, instanceList, err := parseMultiArchSparse(globalOptions, sparseArg)
if err != nil {
return copy.CopySpecificImages, nil, nil, err
}
return copy.CopySpecificImages, instanceList, platformList, nil
default:
return copy.CopySystemImage, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'all', or 'index-only'", multiArch)
return copy.CopySystemImage, nil, nil, fmt.Errorf("unknown multi-arch option %q. Choose one of the supported options: 'system', 'sparse:...', 'all', or 'index-only'", multiArch)
}
}

func parseMultiArchSparse(globalOptions *globalOptions, sparseArg string) ([]imgspecv1.Platform, []digest.Digest, error) {
var platformList []imgspecv1.Platform
var instanceList []digest.Digest
remainder := "," + sparseArg
for remainder != "" {
parseArg := func(argStart string, argEnd string, fn func(argVal string) (bool, error)) (bool, error) {
if newRemainder, isArg := strings.CutPrefix(remainder, ","+argStart); isArg {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CutPrefix was added in Go 1.20; currently we declare Go 1.18.

(It is also very neat, I can’t wait for this to be available…)

We are not necessarily stuck on 1.18, and updating is an option. But looking at https://bodhi.fedoraproject.org/updates/?packages=golang , we might be limited to 1.19.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh, good catch.

if !isArg {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unreachable right now. (Aesthetically, I’d weakly prefer to flip the control flow so that the !isArg code exits early at the top, instead of at the very bottom of the function.)

return false, nil
}
var argSpec string
if argEnd != "" {
var foundArgEnd bool
argSpec, newRemainder, foundArgEnd = strings.Cut(newRemainder, argEnd)
if !foundArgEnd {
return false, fmt.Errorf("--multi-arch=sparse:%s: end of argument marker %s not found", argStart, argEnd)
}
}
handled, err := fn(argSpec)
if err != nil {
return false, err
}
if handled {
remainder = newRemainder
return true, nil
}
return false, nil
}
return false, nil
}
if isSystem, err := parseArg("system", "", func(string) (bool, error) {
systemPlatform := imgspecv1.Platform{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I continue to be worried about the design that copy.Options.InstancePlatforms can contain “partial” platform specifications with some fields filled in, with the rest defaulted to runtime.GO*.

E.g. we can’t represent “find the entry with arch="arm", variant="" if Variant: "" meant “the current runtime variant”. (Now, this special case already exists in the underlying ChooseInstance, but do we need to document that in copy.Options.InstancePlatforms?)

I’d prefer a copy.InstancePlatformIncludeSystem bool and let c/image handle that, including how “system” interacts with SystemContext = globalOptions.override*.

OS: globalOptions.overrideOS,
Architecture: globalOptions.overrideArch,
Variant: globalOptions.overrideVariant,
}
platformList = append(platformList, systemPlatform)
return true, nil
}); err != nil {
return nil, nil, err
} else if isSystem {
continue
}
if isDigest, err := parseArg("digest=[", "]", func(digestSpecList string) (bool, error) {
Comment on lines +167 to +172
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is probably very premature… Let’s settle on the UI and API first before beautifying the implementation.)

It seems to me that something vaguely like

keyword, remainder, err := findKeyword(remainder)
switch keyword {
case "system": …
case "digest": ourArgs, remainder, err = getArgs(remainder, "=[", "]"); …
case "arch": ourArgs, remainder, err = getArgs(remainder, "=[", "]"); …
}

should be possible to do; centralizing the remainder = newRemainder code in a single place does help us not to forget to update remainder, but it ends up with these copy&pasted sequences instead, which seems not too be worth it to me. Alternatively, we could end up with a whole type argDescriptor struct { start, end, handler … } which also seem like an overkill.

for _, digestSpec := range strings.Split(digestSpecList, ",") {
instanceDigest, err := digest.Parse(digestSpec)
if err != nil {
return false, fmt.Errorf("while parsing instance digest %q: %w", digestSpec, err)
}
instanceList = append(instanceList, instanceDigest)
}
return true, nil
}); err != nil {
return nil, nil, err
} else if isDigest {
continue
}
if isArch, err := parseArg("arch=[", "]", func(archSpecList string) (bool, error) {
wantedOS := runtime.GOOS
if globalOptions.overrideOS != "" {
wantedOS = globalOptions.overrideOS
}
Comment on lines +187 to +190
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Mac users must use --override-os=linux --multi-arch=sparse:arch=[amd64,arm64], that’s not really any simpler than --multi-arch=sparse:platform=[linux/amd64,linux/arm64].

I think if for an arch, separate and distinct from platform, to be worth adding (extra complexity in implementation and for users to read about), it should be an arch-only filter, not restricting the OS at all.

I also don’t think we need to build that arch-only filter at all for the first version… see the top-level comment.

for _, archSpec := range strings.Split(archSpecList, ",") {
p := strings.SplitN(archSpec, "/", 2)
if len(p) > 1 {
platformList = append(platformList, imgspecv1.Platform{OS: wantedOS, Architecture: p[0], Variant: p[1]})
} else {
platformList = append(platformList, imgspecv1.Platform{OS: wantedOS, Architecture: p[0]})
}
}
return true, nil
}); err != nil {
return nil, nil, err
} else if isArch {
continue
}
if isPlatform, err := parseArg("platform=[", "]", func(platformSpecList string) (bool, error) {
for _, platformSpec := range strings.Split(platformSpecList, ",") {
p, err := platforms.Parse(platformSpec)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

platforms.Parse, if I’m reading it correctly, turns "linux" into linux/$GOARCH. Or, alternatively, "amd64" into $GOOS/amd64.

Hum, do we want that? Especially for Mac users?


Actually, should the UI be built around the OCI “platform” triplet at all, or should we have OS/arch filters separately? (And what does that mean for Zstd compression? Probably nothing…)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

platforms.Parse, if I’m reading it correctly, turns "linux" into linux/$GOARCH. Or, alternatively, "amd64" into $GOOS/amd64.

Hum, do we want that? Especially for Mac users?

Hmm, for people on GOOS="darwin" I could actually see defaulting to "linux" as being reasonable, but for people running on GOOS="windows" I think it would be surprising.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more option is to just provide the full-platform filter now (with a more strict parser requiring a full specification?), and leave a nicer arch-filter-only UI for a future improvement — as long as we can see a path to making that possible, and, uh, --multi-arch=sparse2:… is a last-resort option we always have.

“A nice arch-filter-only UI” is not what the primary motivating issue is asking for right now…

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2175 also suggests using platforms.Parse, with the same potential surprises.

Both PRs should probably end up with the same decision.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are to ever have a separate arch= and platform=, at most one of them (possibly neither) should default to runtime.GOOS.

Requiring the user to always provide os/arch (with an optional variant) here, using a custom parser instead of platforms.Parse, would for now provide a clearly deterministic feature, while still giving us the flexibility to addd that defaulting to GOOS in the future.

if err != nil {
return false, fmt.Errorf("while parsing platform specifier %q: %w", platformSpec, err)
}
platformList = append(platformList, p)
}
return true, nil
}); err != nil {
return nil, nil, err
} else if isPlatform {
continue
}
return nil, nil, fmt.Errorf("--multi-arch=sparse: unrecognized value %q", strings.TrimPrefix(remainder, ","))
}
return platformList, instanceList, nil
}

func (opts *copyOptions) run(args []string, stdout io.Writer) (retErr error) {
Expand Down Expand Up @@ -186,11 +288,13 @@ func (opts *copyOptions) run(args []string, stdout io.Writer) (retErr error) {
}

imageListSelection := copy.CopySystemImage
var instanceDigests []digest.Digest
var instancePlatforms []imgspecv1.Platform
if opts.multiArch.Present() && opts.all {
return fmt.Errorf("Cannot use --all and --multi-arch flags together")
}
if opts.multiArch.Present() {
imageListSelection, err = parseMultiArch(opts.multiArch.Value())
imageListSelection, instanceDigests, instancePlatforms, err = parseMultiArch(opts.global, opts.multiArch.Value())
if err != nil {
return err
}
Expand Down Expand Up @@ -296,6 +400,8 @@ func (opts *copyOptions) run(args []string, stdout io.Writer) (retErr error) {
DestinationCtx: destinationCtx,
ForceManifestMIMEType: manifestType,
ImageListSelection: imageListSelection,
Instances: instanceDigests,
InstancePlatforms: instancePlatforms,
PreserveDigests: opts.preserveDigests,
OciDecryptConfig: decConfig,
OciEncryptLayers: encLayers,
Expand Down
Loading