Lokasi ngalangkungan proxy:   [ UP ]  
[Ngawartoskeun bug]   [Panyetelan cookie]                
Skip to content

c8d: Add image events#46405

Merged
rumpl merged 3 commits into
moby:masterfrom
rumpl:image-events
Sep 6, 2023
Merged

c8d: Add image events#46405
rumpl merged 3 commits into
moby:masterfrom
rumpl:image-events

Conversation

@rumpl

@rumpl rumpl commented Sep 5, 2023

Copy link
Copy Markdown
Member

- What I did

Added image events for

  • push
  • pull
  • save

- How I did it

With my hands

- How to verify it

Change line 84 in hack/make/.integration-test-helpers to say

flags+=" -test.run ${TEST_FILTER}/"

and run:

$ make DOCKER_GRAPHDRIVER=overlayfs TEST_FILTER=DockerCLIEventSuite TEST_INTEGRATION_USE_SNAPSHOTTER=1 test-integration

All the tests should pass.

- Description for the changelog

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

Signed-off-by: Djordje Lukic djordje.lukic@docker.com

@rumpl rumpl added status/2-code-review area/images Image Service containerd-integration Issues and PRs related to containerd integration labels Sep 5, 2023
@rumpl rumpl added this to the 25.0.0 milestone Sep 5, 2023
@rumpl rumpl changed the title c8d: Add image push events c8d: Add image events Sep 5, 2023
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

}).Debug("export image without name")
}

i.LogImageEvent(target.Digest.String(), target.Digest.String(), events.ActionSave)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SO confusing that it's "image expert", but actually "image save" 😞

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had to check 3 times to make sure it's the right one...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The image is not exported at this point yet. We should log the events after Export returns a nil error.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the way graph drivers work too. Also you could export multiple images…

so it’s more “ready to export this image” rather than “this image was exported”

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, right. That's weird though.. but if that's how it worked with the graphdrivers then it should stay that way. Sorry!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is weird. I’ll give you that. I even did what you said at one point but reverted. It would be too big of a change. Maybe we can fix this in a followup, fix both implementations if we think it’s useful

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, we should map what events we trigger "where" and with what information. Many of them where just added "willy-nilly" in the code, but not all were properly defined.

Should be slightly easier to find all locations now that we have the consts defined, but it could use some tweaking (and documenting) overall

}).Debug("export image without name")
}

i.LogImageEvent(target.Digest.String(), target.Digest.String(), events.ActionSave)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The image is not exported at this point yet. We should log the events after Export returns a nil error.

@rumpl rumpl merged commit 2c95ddf into moby:master Sep 6, 2023
@rumpl rumpl deleted the image-events branch September 6, 2023 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/images Image Service containerd-integration Issues and PRs related to containerd integration status/2-code-review

Projects

Development

Successfully merging this pull request may close these issues.

3 participants