add podman artifact extract #25238

Luap99 · 2025-02-05T18:13:22Z

Add a new command to extract the blob content of the artifact store to a local path.

Fixes https://issues.redhat.com/browse/RUN-2445

Does this PR introduce a user-facing change?

Added a new podman artifact extract command.

openshift-ci · 2025-02-05T18:13:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 · 2025-02-05T18:14:34Z

Tests are still missing, will work on it tomorrow. Opening this to allow some early review.

cc @baude @Honny1 @mtrmac

packit-as-a-service · 2025-02-05T18:21:44Z

Ephemeral COPR build failed. @containers/packit-build please check.

baude · 2025-02-05T18:40:21Z

docs/source/markdown/podman-artifact-extract.1.md

+must be used to select only a single blob. If the file already exists it will be
+overwritten.
+
+If the target is an directory (it must exist) all blobs will be copied to the


Suggested change

If the target is an directory (it must exist) all blobs will be copied to the

If the target is an directory (it must exist), all blobs will be copied to the

baude · 2025-02-05T18:41:56Z

docs/source/markdown/podman-artifact-extract.1.md

+overwritten.
+
+If the target is an directory (it must exist) all blobs will be copied to the
+target directory. As target file name the value from `org.opencontainers.image.title`


Seems like a typo here maybe ... did you mean "a" or "the" target file ?

baude · 2025-02-05T18:42:13Z

docs/source/markdown/podman-artifact-extract.1.md

+
+If the target is an directory (it must exist) all blobs will be copied to the
+target directory. As target file name the value from `org.opencontainers.image.title`
+annotation is used. If the annotation is missing the target file name will be


Suggested change

annotation is used. If the annotation is missing the target file name will be

annotation is used. If the annotation is missing, the target file name will be

baude · 2025-02-05T18:42:31Z

docs/source/markdown/podman-artifact-extract.1.md

+target directory. As target file name the value from `org.opencontainers.image.title`
+annotation is used. If the annotation is missing the target file name will be
+the digest of the blob (with `:` replaced by `-` in the name).
+If the target file already exists in the directory it will be overwritten.


Suggested change

If the target file already exists in the directory it will be overwritten.

If the target file already exists in the directory, it will be overwritten.

mtrmac

A fairly brief skim; ACK overall, some more design questions.

Note the ⚠️ .

docs/source/markdown/podman-artifact-extract.1.md

pkg/domain/entities/artifact.go

pkg/domain/infra/abi/artifact.go

pkg/libartifact/store/store.go

mtrmac · 2025-02-05T20:20:31Z

pkg/libartifact/store/store.go

+		title  string
+	)
+	for _, l := range arty.Manifest.Layers {
+		if options.Digest == l.Digest.String() {


Non-blocking: This probably can be structured as if layerMatchesOptions(…) { duplicate check; set digest/title }). This works fine as is, and it is short enough that it is not that necessary.

pkg/libartifact/store/store.go

docs/source/markdown/podman-artifact-extract.1.md

baude · 2025-02-06T15:27:53Z

pkg/libartifact/store/store.go

+	}
+	defer dest.Close()
+
+	// TODO use reflink is possible


is this a long term todo, or something you would do as part of this PR?

This is easy to do, but as usual I don't want to duplicate code and the c/image function is internal so I have to export it first then vendor again, etc... So not something I plan to do for this PR but I will be working on that soon enough.

Luap99 · 2025-02-06T16:29:08Z

I pushed 4 small artifacts for testing

quay.io/libpod/testartifact:20250206-single
quay.io/libpod/testartifact:20250206-multi
quay.io/libpod/testartifact:20250206-multi-no-title
quay.io/libpod/testartifact:20250206-evil

I leave the PR as draft for now and work to get them into our VM images to avoid flakes.

docs/source/markdown/podman-artifact-extract.1.md

pkg/domain/infra/abi/images.go

pkg/libartifact/store/store.go

test/e2e/artifact_test.go

Honny1

I have only a few minor comments, mostly LGTM

Honny1 · 2025-02-07T15:08:58Z

pkg/libartifact/store/store.go

+	stat, err := os.Stat(target)
+	if err == nil {


Suggested change

stat, err := os.Stat(target)

if err == nil {

if stat, err := os.Stat(target); err == nil {

Honny1 · 2025-02-07T15:13:34Z

pkg/libartifact/store/store.go

+		err = copyImageBlobToFile(ctx, imgSrc, l.Digest, filepath.Join(target, filename))
+		if err != nil {


Suggested change

err = copyImageBlobToFile(ctx, imgSrc, l.Digest, filepath.Join(target, filename))

if err != nil {

if err := copyImageBlobToFile(ctx, imgSrc, l.Digest, filepath.Join(target, filename)); err != nil {

Honny1 · 2025-02-07T16:05:48Z

cmd/podman/artifact/extract.go

+	err := registry.ImageEngine().ArtifactExtract(registry.Context(), args[0], args[1], &extractOpts)
+	if err != nil {


Suggested change

err := registry.ImageEngine().ArtifactExtract(registry.Context(), args[0], args[1], &extractOpts)

if err != nil {

if err := registry.ImageEngine().ArtifactExtract(registry.Context(), args[0], args[1], &extractOpts); err != nil {

mtrmac

LGTM

This is needed by containers/podman#25238 To avoid flakes we need to have the test artifacts in the cache registry. Signed-off-by: Paul Holzinger <[email protected]>

Add a new command to extract the blob content of the artifact store to a local path. Fixes https://issues.redhat.com/browse/RUN-2445 Signed-off-by: Paul Holzinger <[email protected]>

Luap99 · 2025-02-11T14:19:22Z

This is good to go, rebased on main with the image update to include the artifacts so we shoul dnot get any new pull flakes from it.
I work on the reflink change in another PR.

baude · 2025-02-11T18:42:33Z

/lgtm

openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 5, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2025

baude reviewed Feb 5, 2025

View reviewed changes

mtrmac reviewed Feb 5, 2025

View reviewed changes

baude reviewed Feb 6, 2025

View reviewed changes

Luap99 force-pushed the artifact-extract branch 2 times, most recently from 3bde1e7 to 73f058f Compare February 6, 2025 16:27

mtrmac reviewed Feb 6, 2025

View reviewed changes

Luap99 force-pushed the artifact-extract branch from 73f058f to ed5bb02 Compare February 7, 2025 14:05

Honny1 reviewed Feb 7, 2025

View reviewed changes

mtrmac reviewed Feb 7, 2025

View reviewed changes

add podman artifact extract

3925a30

Add a new command to extract the blob content of the artifact store to a local path. Fixes https://issues.redhat.com/browse/RUN-2445 Signed-off-by: Paul Holzinger <[email protected]>

Luap99 force-pushed the artifact-extract branch from ed5bb02 to 3925a30 Compare February 11, 2025 13:37

Luap99 marked this pull request as ready for review February 11, 2025 14:18

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2025

openshift-ci bot assigned baude Feb 11, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2025

openshift-merge-bot bot merged commit 2f261d1 into containers:main Feb 11, 2025
83 checks passed

Luap99 deleted the artifact-extract branch February 11, 2025 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add podman artifact extract #25238

add podman artifact extract #25238

Luap99 commented Feb 5, 2025

openshift-ci bot commented Feb 5, 2025

Luap99 commented Feb 5, 2025

packit-as-a-service bot commented Feb 5, 2025

baude Feb 5, 2025

baude Feb 5, 2025

baude Feb 5, 2025

baude Feb 5, 2025

mtrmac left a comment

mtrmac Feb 5, 2025

baude Feb 6, 2025

Luap99 Feb 6, 2025

Luap99 commented Feb 6, 2025

Honny1 left a comment

Honny1 Feb 7, 2025

Honny1 Feb 7, 2025

Honny1 Feb 7, 2025

mtrmac left a comment

Luap99 commented Feb 11, 2025

baude commented Feb 11, 2025

	If the target is an directory (it must exist) all blobs will be copied to the
	If the target is an directory (it must exist), all blobs will be copied to the

	annotation is used. If the annotation is missing the target file name will be
	annotation is used. If the annotation is missing, the target file name will be

	If the target file already exists in the directory it will be overwritten.
	If the target file already exists in the directory, it will be overwritten.

	stat, err := os.Stat(target)
	if err == nil {
	if stat, err := os.Stat(target); err == nil {

		err = copyImageBlobToFile(ctx, imgSrc, l.Digest, filepath.Join(target, filename))
		if err != nil {

	err = copyImageBlobToFile(ctx, imgSrc, l.Digest, filepath.Join(target, filename))
	if err != nil {
	if err := copyImageBlobToFile(ctx, imgSrc, l.Digest, filepath.Join(target, filename)); err != nil {

		err := registry.ImageEngine().ArtifactExtract(registry.Context(), args[0], args[1], &extractOpts)
		if err != nil {

	err := registry.ImageEngine().ArtifactExtract(registry.Context(), args[0], args[1], &extractOpts)
	if err != nil {
	if err := registry.ImageEngine().ArtifactExtract(registry.Context(), args[0], args[1], &extractOpts); err != nil {

add podman artifact extract #25238

add podman artifact extract #25238

Conversation

Luap99 commented Feb 5, 2025

Does this PR introduce a user-facing change?

openshift-ci bot commented Feb 5, 2025

Luap99 commented Feb 5, 2025

packit-as-a-service bot commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 commented Feb 6, 2025

Honny1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Luap99 commented Feb 11, 2025

baude commented Feb 11, 2025