libartifact: introduce handling of artifact store events by nimdrak · Pull Request #768 · containers/container-libs

nimdrak · 2026-04-15T08:45:47Z

fixed #465

packit-as-a-service · 2026-04-15T08:48:27Z

Packit jobs failed. @containers/packit-build please check.

Signed-off-by: Byounguk Lee <nimdrak@gmail.com>

- Introduced withLockedLayout and copyArtifact helpers to resolve dupl lint issues. - Maintained the original thread-locking scope for as.storePath access. - Fixed a resource leak by ensuring the copier is always closed using defer and capturing the close error. Signed-off-by: Byounguk Lee <nimdrak@gmail.com>

…annel - TestArtifactStore_EventChannel deals with the cases: AddSuccessAndAddError, RemoveSuccess, PushError, PullError Signed-off-by: Byounguk Lee <nimdrak@gmail.com>

baude · 2026-04-20T13:22:57Z

+	// EventTypeArtifactPull represents an artifact pull.
+	EventTypeArtifactPull
+	// EventTypeArtifactPullError represents a failed artifact pull.
+	EventTypeArtifactPullError


Is this new? Do we report errors as events?

Yes, some of these are new.
Our podman/libpod and container-libs/common/libimage only handles PullError

I thought it would be beneficial to notify clients about other errors as well, such as AddError, RemoveError, and PushError. And I also add

However, if there's a specific reason why we only track PullError, I am completely open to removing these new additions.

What do you think?

While I introduced AddEvent, I intentionally left out other events like Extract and Inspect. These are new too, but I only added AddEvent since it's the only one that actually modifies the artifact store.

I think the rest of the team should weigh in on this. I can understand the attraction to add more and some users have def asked for more, but it is a definite departure from what is logged by other libraries...

@mheon @Luap99 wdyt ?

I lean towards having events for the errors being unnecessary. Whatever is calling Podman is already going to be seeing the error.

I objected the purpose of pullError when it was added so I will object any other Error type events, I really do not think we should spam the event channel with ton of errors.

I think we have consensus: let's pull the error events. I think this is basically fine once they're out.

baude · 2026-04-29T12:38:12Z

i realize this is not break changes but id like to see if we can get this in ... @nimdrak lets remove the additional events for now. I'm more concerned about you getting in the event handling, etc.

Luap99 · 2026-04-29T15:21:12Z

i realize this is not break changes but id like to see if we can get this in ... @nimdrak lets remove the additional events for now. I'm more concerned about you getting in the event handling, etc.

I have 0 capacity to review this or other larger non 6 work right now so I rather not see this blocking 6 here...

mtrmac

Just some notes… I suppose maintaining symmetry with libimage is more valuable, and anyway we don’t want to delay for any of this.

I didn’t read the tests.

mtrmac · 2026-04-29T17:26:39Z

-		return "", err
-	}
+// Execute fn while holding the store lock and providing a reference to the local layout.
+func (as *ArtifactStore) withLockedLayout(localName string, fn func(localRef types.ImageReference) (digest.Digest, error)) (digest.Digest, error) {


I’d rather prefer using a typed ArtifactReference instead of plain strings whenever possible.

mtrmac · 2026-04-29T17:33:15Z

-		return "", err
-	}
+// Execute fn while holding the store lock and providing a reference to the local layout.
+func (as *ArtifactStore) withLockedLayout(localName string, fn func(localRef types.ImageReference) (digest.Digest, error)) (digest.Digest, error) {


I’m not too convinced that this helper is worth it… it’s pretty specific to the two call sites and it does not read idiomatically, combining very unrelated lock handling and reference creation.

An universal withLockHeld would make a bit more sense to me, but that’s

a bit annoying to do in Go (we can use generics for the return value, but there is no void type)

clearly out of scope of this PR

mtrmac · 2026-04-29T17:33:57Z

+// EventChannel creates a buffered channel for events that the ArtifactStore will use
+// to write events to. Callers are expected to read from the channel in a
+// timely manner.
+// Can be called once for a given ArtifactStore.


(This restriction seems unnecessary, given the implementation. Is this a general principle we want to hold? Yes, libimage says the same thing.)

mtrmac · 2026-04-29T17:36:58Z

+	// Time of the event.
+	Time time.Time
+	// Type of the event.
+	Type EventType


(To the extent this is an ~enum discriminator, should it go first? OTOH this matches libimage.)

mtrmac · 2026-04-29T17:38:24Z

+
+// writeEvent writes the specified event to the store's event channel. The
+// event is discarded if no event channel has been registered (yet).
+func (as *ArtifactStore) writeEvent(event *Event) {


Non-blocking: I wouldn’t mind something like writeEvent(type …, digest digest.Digest, ref ArtifactStoreReference), to decrease the .String() and time.Now() tedium at call sites.

mtrmac · 2026-04-29T17:39:01Z

+	ID string
+	// Name of the object (e.g., artifact name "quay.io/foobar/artifact:special")
+	Name string


Should these be more strictly typed? I have no idea.

github-actions Bot added the common Related to "common" package label Apr 15, 2026

nimdrak force-pushed the introduce_an_event_channel_for_artifactstore branch from 4066258 to 0afac95 Compare April 15, 2026 08:47

nimdrak changed the title ~~Introduce handling of artifact store events~~ [WIP] Introduce handling of artifact store events Apr 15, 2026

nimdrak changed the title ~~[WIP] Introduce handling of artifact store events~~ [WIP] libartifact: Introduce handling of artifact store events Apr 15, 2026

nimdrak changed the title ~~[WIP] libartifact: Introduce handling of artifact store events~~ [WIP] libartifact: introduce handling of artifact store events Apr 15, 2026

nimdrak force-pushed the introduce_an_event_channel_for_artifactstore branch from 0afac95 to 3ee8036 Compare April 15, 2026 09:00

nimdrak added 2 commits April 15, 2026 09:50

libartifact: introduce handling of artifact store events

a61bb36

Signed-off-by: Byounguk Lee <nimdrak@gmail.com>

libartifact: use pointer receivers

6362450

Signed-off-by: Byounguk Lee <nimdrak@gmail.com>

nimdrak force-pushed the introduce_an_event_channel_for_artifactstore branch from 4f53335 to 6362450 Compare April 15, 2026 09:55

nimdrak force-pushed the introduce_an_event_channel_for_artifactstore branch from ec94ce0 to 1770e3d Compare April 20, 2026 08:04

nimdrak marked this pull request as ready for review April 20, 2026 09:37

nimdrak changed the title ~~[WIP] libartifact: introduce handling of artifact store events~~ libartifact: introduce handling of artifact store events Apr 20, 2026

libartifact: add tests for copyArtifact, withLockedLayout and EventCh…

8afbf4d

…annel - TestArtifactStore_EventChannel deals with the cases: AddSuccessAndAddError, RemoveSuccess, PushError, PullError Signed-off-by: Byounguk Lee <nimdrak@gmail.com>

nimdrak force-pushed the introduce_an_event_channel_for_artifactstore branch from 1770e3d to 8afbf4d Compare April 20, 2026 10:13

baude reviewed Apr 20, 2026

View reviewed changes

baude added the podman 6 breaking changes that should go only into podman 6 only label Apr 29, 2026

mtrmac reviewed Apr 29, 2026

View reviewed changes

Conversation

nimdrak commented Apr 15, 2026

Uh oh!

packit-as-a-service Bot commented Apr 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nimdrak Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nimdrak Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baude commented Apr 29, 2026

Uh oh!

Luap99 commented Apr 29, 2026

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nimdrak Apr 21, 2026 •

edited

Loading

nimdrak Apr 21, 2026 •

edited

Loading