Skip to content

ctr pull should unpack for default platform when transfer service is used#11086

Merged
fuweid merged 1 commit intocontainerd:mainfrom
djdongjin:ctr-pull-transfer-service
Dec 11, 2024
Merged

ctr pull should unpack for default platform when transfer service is used#11086
fuweid merged 1 commit intocontainerd:mainfrom
djdongjin:ctr-pull-transfer-service

Conversation

@djdongjin
Copy link
Member

@djdongjin djdongjin commented Dec 3, 2024

Currently if neither --all-platforms nor --platform is specified in ctr i pull, it will pull using the default platform, but skip the unpack step (if using transfer service). However, we should apply the default platform to unpack as well.

# in main branch, no any snapshot is generated
containerd [main]  ➜ sudo ctr i pull --snapshotter=overlayfs docker.io/library/alpine:latest
...
containerd [main]  ➜ sudo ctr snapshot ls
KEY PARENT KIND
# with this fix, default platform is used for pull and unpack.
containerd [ctr-pull-transfer-service]  ➜ sudo ctr i pull --snapshotter=overlayfs docker.io/library/alpine:latest
...
containerd [ctr-pull-transfer-service]  ➜ sudo ctr snapshot ls
KEY                                                                     PARENT KIND
sha256:75654b8eeebd3beae97271a102f57cdeb794cc91e442648544963a7e951e9558        Committed

@dosubot dosubot bot added the area/distribution Image Distribution label Dec 3, 2024
@djdongjin djdongjin changed the title ctr pull unpack for default platform using transfer service ctr pull should unpack for default platform when transfer service is used Dec 3, 2024
@djdongjin
Copy link
Member Author

djdongjin commented Dec 4, 2024

hi @dmcgowan could you PTAL this PR when you get some time? thanks.

This makes --snapshotter take effect when no platform is passed (use default platform), when using transfer service.

chrishenzie
chrishenzie reviewed Dec 5, 2024
View reviewed changes
cmd/ctr/commands/images/pull.go
sopts = append(sopts, image.WithUnpack(platform, cliContext.String("snapshotter")))
allPlatforms := cliContext.Bool("all-platforms")
if len(p) > 0 && allPlatforms {
return errors.New("cannot specify both --platform and --all-platforms")
Copy link
Contributor

@chrishenzie chrishenzie Dec 5, 2024

Choose a reason for hiding this comment

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

This is probably out-of-scope for this PR, but are there other subcommands using all-platforms that would benefit from this check?

Copy link
Member Author

@djdongjin djdongjin Dec 5, 2024

Choose a reason for hiding this comment

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

Other commands just ignore --platforms if both are specified, e.g., in Fetch:

containerd/cmd/ctr/commands/content/fetch.go

Lines 137 to 143 in e514bae

if !cliContext.Bool("all-platforms") {
p := cliContext.StringSlice("platform")
if len(p) == 0 {
p = append(p, platforms.DefaultString())
}
config.Platforms = p
}

I think error early is better than ignore one flag, but am okay to keep it the same with other commands

@dmcgowan dmcgowan added the cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch label Dec 6, 2024
@fuweid fuweid added this pull request to the merge queue Dec 11, 2024
Merged via the queue into containerd:main with commit edfa911 Dec 11, 2024
@djdongjin
Copy link
Member Author

djdongjin commented Dec 11, 2024

/cherrypick release/2.0

@k8s-infra-cherrypick-robot
Copy link

k8s-infra-cherrypick-robot commented Dec 11, 2024

@djdongjin: new pull request created: #11139

Details

In response to this:

/cherrypick release/2.0

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Dec 11, 2024
Merged
@dmcgowan dmcgowan added cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch and removed cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch labels Dec 12, 2024
@azr
Copy link
Contributor

azr commented Dec 17, 2024 •
edited
Loading

nice, will remove that fix from my PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrishenzie chrishenzie chrishenzie left review comments

@dmcgowan dmcgowan dmcgowan approved these changes

@fuweid fuweid fuweid approved these changes

Assignees

No one assigned

Labels

area/distribution Image Distribution cherry-picked/2.0.x PR commits are cherry picked into the release/2.0 branch kind/bug size/S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@djdongjin @k8s-infra-cherrypick-robot @azr @dmcgowan @chrishenzie @fuweid @k8s-ci-robot