-
Notifications
You must be signed in to change notification settings - Fork 528
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
Start to flesh out crane optimize. #879
Conversation
This is a hidden command, which roundtrips a remote image to a target image through `tarball.LayerFromOpener(layer.Uncompressed)`. Right now this does nothing to force estargz (still need `GGCR_EXPERIMENT_ESTARGZ=1`) or prioritize files (need `estargz.WithPrioritizedFiles(foo)`), but want to start the convo. Fixes: google#878
bae7180
to
b6faed6
Compare
Codecov Report
@@ Coverage Diff @@
## master #879 +/- ##
==========================================
- Coverage 74.33% 72.76% -1.58%
==========================================
Files 107 108 +1
Lines 4523 4655 +132
==========================================
+ Hits 3362 3387 +25
- Misses 661 758 +97
- Partials 500 510 +10
Continue to review full report at Codecov.
|
Alright, I also have:
... working to prioritize cc @ktock @AkihiroSuda FYI |
|
||
cmd := &cobra.Command{ | ||
Use: "optimize SRC DST", | ||
Hidden: true, |
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.
Because this is hidden, I don't really care that much, but I would rather this be called something like [e]stargz[ify]
(and documented as such), because "optimize" doesn't mean much.
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.
I'll let you and @imjasonh fight about this. That was my original proposal and I care almost not at all.
Hidden: true, | ||
Aliases: []string{"opt"}, | ||
Short: "Optimize a remote container image from src to dst", | ||
Args: cobra.ExactArgs(2), |
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.
It would be nice if this allowed one arg and would just retag the optimized version, with an optional -t
flag for the dst if you want to leave src alone.
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.
IDK how I feel about in-place optimization, but most of the places I want this for are things that aren't yet producing these images.
Descriptor: v1.Descriptor{ | ||
URLs: desc.URLs, | ||
MediaType: desc.MediaType, | ||
Annotations: desc.Annotations, | ||
Platform: desc.Platform, | ||
}, |
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.
We could probably do something in mutate
to make this a little nicer.
return remote.Write(dstRef, oimg, o.remote...) | ||
} | ||
|
||
func optimizeImage(img v1.Image, prioritize []string) (v1.Image, error) { |
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.
What you're doing follows the patterns from crane.Copy
, which makes sense, but it would be nice to expose this separately as well, maybe OptimizeImage
, so it can be used with non-remote images.
It might also make sense for some of this boilerplate to live in mutate
.
) | ||
|
||
// Optimize optimizes a remote image or index from src to dst. | ||
func Optimize(src, dst string, prioritize []string, opt ...Option) error { |
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.
My "as long as it's hidden" concession for the command also kind of applies here. I don't think we should expose this in pkg/crane until we're happy with exposing it in the CLI as well.
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.
Given how things are structured, that isn't particularly feasible (without deviating significantly from the broader style). What did you have in mind?
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.
You could keep this under cmd/crane/cmd, which is what I've done for gcrane
stuff I don't want to expose.
return remote.WriteIndex(dstRef, oidx, o.remote...) | ||
} | ||
|
||
func optimizeIndex(idx v1.ImageIndex, prioritize []string) (v1.ImageIndex, error) { |
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.
We really need a:
func Map(f func(v1.Image) (v1.Image, error), i v1.ImageOrIndex) (v1.ImageOrIndex, error)
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.
I believe I have one downstream somewhere 😇
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.
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.
Oh yeah you cced me on this earlier. My only issue is the pushing bit, we are missing an abstraction here for an ImageOrIndex thing (strawman here: #835)
|
||
cmd := &cobra.Command{ | ||
Use: "optimize SRC DST", | ||
Hidden: true, |
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.
I'll let you and @imjasonh fight about this. That was my original proposal and I care almost not at all.
Hidden: true, | ||
Aliases: []string{"opt"}, | ||
Short: "Optimize a remote container image from src to dst", | ||
Args: cobra.ExactArgs(2), |
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.
IDK how I feel about in-place optimization, but most of the places I want this for are things that aren't yet producing these images.
return remote.WriteIndex(dstRef, oidx, o.remote...) | ||
} | ||
|
||
func optimizeIndex(idx v1.ImageIndex, prioritize []string) (v1.ImageIndex, error) { |
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.
) | ||
|
||
// Optimize optimizes a remote image or index from src to dst. | ||
func Optimize(src, dst string, prioritize []string, opt ...Option) error { |
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.
Given how things are structured, that isn't particularly feasible (without deviating significantly from the broader style). What did you have in mind?
pkg/crane/optimize.go
Outdated
|
||
olayers = append(olayers, mutate.Addendum{ | ||
Layer: olayer, | ||
History: cfg.History[i], |
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.
I may drop this as I'm not sure it is actually correct.
Moving this down from inline comment so it doesn't get lost...
Given that it's ostensibly backward-compatible, I don't see a huge issue, but I understand the reservations. If we do in-place, one thing that would be nice for one-liners is to print the resulting image to stdout:
So you could do stuff like:
That might not make sense for in-place by tag, but:
Would be kinda nice? I like the pattern in general for mutation stuff, especially if you don't want to write me a compiler so this stuff only works in-registry. I'd be okay with needing to set a flag to overwrite tags, as well. |
BTW, I think being able to collect missed files (containerd/stargz-snapshotter#216) will be very useful for surfacing user errors with the prioritized file stuff (thanks @ktock!). |
This is a hidden command, which roundtrips a remote image to a target image through
tarball.LayerFromOpener(layer.Uncompressed)
.Right now this does nothing to force estargz (still need
GGCR_EXPERIMENT_ESTARGZ=1
) or prioritize files (needestargz.WithPrioritizedFiles(foo)
), but want to start the convo.Fixes: #878