Skip to content

Conversation

@ndeloof
Copy link
Collaborator

@ndeloof ndeloof commented Jul 25, 2022

- What I did
Introduce GetImageWithDetails so that "image inspect" logic is not implement by router but backend, and we can re-implement using containerd snapshotter

relates to moby#43680

@ndeloof ndeloof force-pushed the image_inspect_ctrd branch from 3236817 to 3e175c0 Compare July 25, 2022 16:18
@ndeloof ndeloof requested review from rumpl, thaJeztah and vvoland and removed request for rumpl July 25, 2022 16:50
image/image.go Outdated
Comment on lines 117 to 125
Details *Details
}

// Details provides additional image data
type Details struct {
Size int64
Metadata map[string]string
Driver string
LastUpdated time.Time
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the new fields, make sure they are not serialised to JSON (json:"-")

not sure if it works to add that to Details, or it if's needed for the individual fields (or both) (haven't tested)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

makes me wonder: why do we define json rules for an engine type? This one is not part of the API

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think (but would have to double-check) these types are also used to serialise image information to disk (and as such could be part of, e.g., save/load. So in that case it's not for the API, but to prevent persisting this data to disk.

IIRC, this is also the reason why the V1Image is embedded, as that's what's used for save/load

@ndeloof ndeloof force-pushed the image_inspect_ctrd branch from fcdcb46 to bb4c1bb Compare July 26, 2022 10:16
@ndeloof ndeloof force-pushed the image_inspect_ctrd branch 4 times, most recently from bb43aa3 to fbb0432 Compare July 26, 2022 14:12
// Details provides additional image data
type Details struct {
Size int64
Metadata map[string]string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just realised that this is to store graphdriver metadata; wondering if that's clear enough from this type (at least could use a GoDoc), or was the intent to re-purpose this field for something else following the containerd integration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually wonder why graphdriver metadata are exposed to the API, seems like debugging data as a user would hardly use image's mergedir

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree. It looks like this was one of the Red Hat contributions, and I can only "guess" how they wanted to use this (possibly as part of some systemd hackery); moby#13198 (also related; moby#13130)

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants