Add load/save image event support

[HackToday]

- What I did
make docker load/save emit events
- How I did it
move related logic to daemon directory(it follows other docker operation style)
- How to verify it
Have integration test for that

- A picture of a cute animal (not mandatory but encouraged)

This change moves related load/save logic to daemon dir,
which keep same as other docker operations(import, export etc.)

For every docker load and save operations, it would log related
image events.

Closes: #21973

Signed-off-by: Kai Qiang Wu(Kennan) [email protected]

[coolljt0725]

IMO, I don't think we should move image/tarexport/load.go and image/tarexport/save.go to daemon

[HackToday]

Hi @coolljt0725

1. I did not find good reason to just put tarexport/load.go and tarexport/save.go in separate directory. Maybe other cores who wrote the code before can specify why put it there.

2. Also if you use old layout, how could you use daemon.LogImageEvent from tarexport package?

I'd like to hear specific suggestions. Thanks

[HackToday]

also ping @calavera who comment on issue #21973 before Wdyt? Thanks

[coolljt0725]

@HackToday I think just add a LogImageEvent interface to struct tarexporter can solve the problem. this is an example

+++ b/daemon/daemon.go
@@ -973,7 +973,7 @@ func isBrokenPipe(e error) bool {
 // the same tag are exported. names is the set of tags to export, and
 // outStream is the writer which the images are written to.
 func (daemon *Daemon) ExportImage(names []string, outStream io.Writer) error {
-       imageExporter := tarexport.NewTarExporter(daemon.imageStore, daemon.layerStore, daemon.referenceStore)
+       imageExporter := tarexport.NewTarExporter(daemon.imageStore, daemon.layerStore, daemon.referenceStore, daemon)
        return imageExporter.Save(names, outStream)
 }

@@ -1052,7 +1052,7 @@ func (daemon *Daemon) LookupImage(name string) (*types.ImageInspect, error) {
 // complement of ImageExport.  The input stream is an uncompressed tar
 // ball containing images and metadata.
 func (daemon *Daemon) LoadImage(inTar io.ReadCloser, outStream io.Writer, quiet bool) error {
-       imageExporter := tarexport.NewTarExporter(daemon.imageStore, daemon.layerStore, daemon.referenceStore)
+       imageExporter := tarexport.NewTarExporter(daemon.imageStore, daemon.layerStore, daemon.referenceStore, daemon)
        return imageExporter.Load(inTar, outStream, quiet)
 }

diff --git a/image/tarexport/load.go b/image/tarexport/load.go
index 42eaa40..69c518e 100644
--- a/image/tarexport/load.go
+++ b/image/tarexport/load.go
@@ -122,6 +122,7 @@ func (l *tarexporter) Load(inTar io.ReadCloser, outStream io.Writer, quiet bool)
                }

                parentLinks = append(parentLinks, parentLink{imgID, m.Parent})
+               l.e.LogImageEvent(imgID.String(), imgID.String(), "load")
        }

        for _, p := range validatedParentLinks(parentLinks) {
diff --git a/image/tarexport/tarexport.go b/image/tarexport/tarexport.go
index 5e20877..45f7aae 100644
--- a/image/tarexport/tarexport.go
+++ b/image/tarexport/tarexport.go
@@ -21,17 +21,23 @@ type manifestItem struct {
        Parent   image.ID `json:",omitempty"`
 }

+type LogImageEvent interface {
+       LogImageEvent(imageID, refName, action string)
+}
+
 type tarexporter struct {
        is image.Store
        ls layer.Store
        rs reference.Store
+       e  LogImageEvent
 }

 // NewTarExporter returns new ImageExporter for tar packages
-func NewTarExporter(is image.Store, ls layer.Store, rs reference.Store) image.Exporter {
+func NewTarExporter(is image.Store, ls layer.Store, rs reference.Store, e LogImageEvent) image.Exporter {
        return &tarexporter{
                is: is,
                ls: ls,
                rs: rs,
+               e:  e,
        }
 }

[HackToday]

Thanks @coolljt0725 I would to hear more docker-cores inputs about which is better.
As I still think save and load is daemon specific operations, So put it in right place seems have benefits.

[tonistiigi]

This code removes busybox. Meaning it will be repulled from instead of using the frozen image. Image pulled from hub doesn't have a parent chain.

[HackToday]

Thanks @tonistiigi for your input!
I would use another way to test the load/save case

[calavera]

I agree with @coolljt0725.

[HackToday]

@calavera OK add another interface could have less code, and I would follow that.

but I did not find any good reason put such daemon operation in separate package make sense. Could you help explain why ?

[HackToday]

The jenkins failed seems unrelated with my change

[thaJeztah]

@HackToday I think they're related;

05:25:11 These files are not properly gofmt'd:
05:25:11  - image/tarexport/tarexport.go
05:25:11 
05:25:11 Please reformat the above files using "gofmt -s -w" and commit the result.

[HackToday]

please test it

[HackToday]

@calavera and @cpuguy83 PTAL, I think it would be ok for code review now. Thanks

[calavera]

LGTM

[calavera]

We should add those events to the documentation

[HackToday]

@calavera docs updated

ping @thaJeztah PTAL if docs OK. Thanks

[coolljt0725]

LGTM

[vdemeester]

LGTM 🐯