migrate Podman to containers/common/libimage #10147

vrothberg · 2021-04-27T12:41:15Z

Migrate the Podman code base over to common/libimage which replaces
libpod/image and a lot of glue code entirely.

Note that I tried to leave bread crumbs for changed tests.

Miscellaneous changes:

Some errors yield different messages which required to alter some
tests.
I fixed some pre-existing issues in the code. Others were marked as
//TODOs to prevent the PR from exploding.
The NamesHistory of an image is returned as is from the storage.
Previously, we did some filtering which I think is undesirable.
Instead we should return the data as stored in the storage.
Touched handlers use the ABI interfaces where possible.
Local image resolution: previously Podman would match "foo" on
"myfoo". This behaviour has been changed and Podman will now
only match on repository boundaries such that "foo" would match
"my/foo" but not "myfoo". I consider the old behaviour to be a
bug, at the very least an exotic corner case.
Futhermore, "foo:none" does not resolve to a local image "foo"
without tag anymore. It's a hill I am (almost) willing to die on.
image prune prints the IDs of pruned images. Previously, in some
cases, the names were printed instead. The API clearly states ID,
so we should stick to it.
Compat endpoint image removal with force deletes the entire not
only the specified tag.

Signed-off-by: Valentin Rothberg [email protected]

vrothberg · 2021-04-27T12:42:56Z

Note: this does not yet implement an event system in libimage. I want to tackle that after this PR has been merged. Then, I'll update Buildah to the latest libimage.

vrothberg · 2021-04-27T13:06:49Z

@mheon @baude PTAL.

I'm now massaging the changes until they pass CI. But early feedback is good feedback.

Update Buildah to the latest libimage. Migrating Podman over to libimage entailed a number of fixes and changes to libimage which we need to account for in Buildah. Most notably: * `(*Runtime).LookupImage()` now returns `storage.ErrImageUnknown` instead of `nil` in case no matching image is found. * `(*Runtime).LookupImage()` now does quite a bit more work finding a local image and will also look at the repotags (or digests) of all local images if needed. * The signature of `(*Runtime).RemoveImages()` was changed and now returns a slice of reports and errors. The reports aggregate the data of a removed image which allows the function to be used by `podman image prune` which is also interested in the size of the removed data. The slice of errors is also needed in Podman which needs to have a closer look at _all_ rmi errors in order to determine the appropriate exit code (Docker compat). * `libimage/types` has been removed. Pull policies have been merged into already existing logic in `pkg/config`. Please refer to containers/podman/pull/10147 for a more detailed changelog. Signed-off-by: Valentin Rothberg <[email protected]>

Update Buildah to the latest libimage. Migrating Podman over to libimage entailed a number of fixes and changes to libimage which we need to account for in Buildah. Most notably: * `(*Runtime).LookupImage()` now returns `storage.ErrImageUnknown` instead of `nil` in case no matching image is found. * `(*Runtime).LookupImage()` now does quite a bit more work finding a local image and will also look at the repotags (or digests) of all local images if needed. * The signature of `(*Runtime).RemoveImages()` was changed and now returns a slice of reports and errors. The reports aggregate the data of a removed image which allows the function to be used by `podman image prune` which is also interested in the size of the removed data. The slice of errors is also needed in Podman which needs to have a closer look at _all_ rmi errors in order to determine the appropriate exit code (Docker compat). * `libimage/types` has been removed. Pull policies have been merged into already existing logic in `pkg/config`. Please refer to containers/podman/pull/10147 for a more detailed changelog. [NO NEW TESTS NEEDED] Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg · 2021-04-30T17:00:46Z

@jwhonce if you have some cycles to check why podman/test/e2e/create_test.go:634 is failing, I'd love to have your eyes on it.

I cannot reproduce locally and this seems to be the last remote fart.

Update Buildah to the latest libimage. Migrating Podman over to libimage entailed a number of fixes and changes to libimage which we need to account for in Buildah. Most notably: * `(*Runtime).LookupImage()` now returns `storage.ErrImageUnknown` instead of `nil` in case no matching image is found. * `(*Runtime).LookupImage()` now does quite a bit more work finding a local image and will also look at the repotags (or digests) of all local images if needed. * The signature of `(*Runtime).RemoveImages()` was changed and now returns a slice of reports and errors. The reports aggregate the data of a removed image which allows the function to be used by `podman image prune` which is also interested in the size of the removed data. The slice of errors is also needed in Podman which needs to have a closer look at _all_ rmi errors in order to determine the appropriate exit code (Docker compat). * `libimage/types` has been removed. Pull policies have been merged into already existing logic in `pkg/config`. Please refer to containers/podman/pull/10147 for a more detailed changelog. [NO NEW TESTS NEEDED] Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg · 2021-05-04T17:57:45Z

Looks fine to me

Thanks for checking!

vrothberg · 2021-05-04T17:59:20Z

@containers/podman-maintainers should turn green now

edsantiago · 2021-05-04T18:34:57Z

test/system/070-build.bats

-    is "${lines[4]}"  "...  ID: [0-9a-f]\{12\} Size: .* Top Layer of: \[$IMAGE]" \
+    is "${lines[4]}"  ".* ID: [0-9a-f]\{12\} Size: .* Top Layer of: \[localhost/build_test:latest]" \
       "image tree: first layer line"
-    is "${lines[-1]}" "...  ID: [0-9a-f]\{12\} Size: .* Top Layer of: \[localhost/build_test:latest]" \
+    is "${lines[-1]}" ".* ID: [0-9a-f]\{12\} Size: .* Top Layer of: \[$IMAGE]" \
       "image tree: last layer line"


Just a comment, not a change request: this seems weird, like a deliberate reordering/reversal of image tree. Isn't this a breaking change? Will users be affected?

Sorry for not mentioning that in the commit message here. I only mentioned it in the last libimage commit (containers/common@c53283f).

It was a deliberate reordering. I had to rewrite image tree to make it use the internal layer tree (used to compute parent-child relationships). When looking closer at it, the previous order did not feel intuitive as the top layer was at the very bottom. In practice, however, layers are put on top of each other. It also came with the nice benefit of making the code much simpler.

I see the output of image tree as something like the pull/push progress bars which may change over time (rather cosmetic) but others may feel different.

I will make it a PS in today's meeting.

I did some more detailed testing of image tree and found some other issues in --whatrequires. I will address them in a follow-up PR.

rhatdan · 2021-05-04T19:18:36Z

Fixes: #10116

edsantiago · 2021-05-04T20:51:58Z

Fix for bud test:

diff --git a/test/buildah-bud/apply-podman-deltas b/test/buildah-bud/apply-podman-deltas
index 9f6f38190..57cca7a04 100755
--- a/test/buildah-bud/apply-podman-deltas
+++ b/test/buildah-bud/apply-podman-deltas
@@ -137,7 +137,8 @@ skip "podman requires a directory, not a Dockerfile" \

 # ...or due to Ed's laziness
 skip "Too much effort to spin up a local registry" \
-     "bud with encrypted FROM image"
+     "bud with encrypted FROM image" \
+     "bud --authfile"

 # ...or due to a fundamental arg-parsing difference between buildah and podman
 # which we could and perhaps should fix in the buildah repo via:
@@ -146,6 +147,10 @@ skip "Too much effort to spin up a local registry" \
 skip "FIXME FIXME FIXME: argument-order incompatible with podman" \
      "bud-squash-hardlinks"

+skip "FIXME FIXME FIXME we'll figure these out later" \
+     "bud-multi-stage-nocache-nocommit" \
+     "bud with --cgroup-parent"
+
 ###############################################################################
 # BEGIN tests which are skipped due to actual podman bugs.
 skip "FIXME: podman #9915" \

[or, download buildah-bud-skip.diff.txt]

The --authfile one is a permanent skip. The other two will have to be fixed in buildah itself: both are newly-added or -edited tests, and I guessI didn't review them properly for working under podman. That is a problem for later (I will take it on).

vrothberg · 2021-05-05T06:43:12Z

Thank you very much, @edsantiago!

vrothberg · 2021-05-05T08:17:15Z

[+0100s] not ok 8 bud with --layers and --no-cache flags
[...]
[+0100s] # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
[+0100s] # #|     FAIL: buildah inspect --format {{index .Docker.History 2}} test1
[+0100s] # #| expected: 'FROM localhost/alpine:latest'
[+0100s] # #|   actual: '{2021-05-05 07:37:21.000305695 +0000 UTC  /bin/sh -c mkdir /hello FROM docker.io/library/alpine:latest false}'

One last to go. Once Podman is updated in the Buildah CI VMs, we'll run into the same issue there. Some background: previously podman save would tag alpine as localhost/alpine in the tar archive (even if the local image is actually docker.io/library/alpine). This has been fixed in libimage, hence the error.

While it changes previous behavior, I see it as fixing a bug.

Migrate the Podman code base over to `common/libimage` which replaces `libpod/image` and a lot of glue code entirely. Note that I tried to leave bread crumbs for changed tests. Miscellaneous changes: * Some errors yield different messages which required to alter some tests. * I fixed some pre-existing issues in the code. Others were marked as `//TODO`s to prevent the PR from exploding. * The `NamesHistory` of an image is returned as is from the storage. Previously, we did some filtering which I think is undesirable. Instead we should return the data as stored in the storage. * Touched handlers use the ABI interfaces where possible. * Local image resolution: previously Podman would match "foo" on "myfoo". This behaviour has been changed and Podman will now only match on repository boundaries such that "foo" would match "my/foo" but not "myfoo". I consider the old behaviour to be a bug, at the very least an exotic corner case. * Futhermore, "foo:none" does *not* resolve to a local image "foo" without tag anymore. It's a hill I am (almost) willing to die on. * `image prune` prints the IDs of pruned images. Previously, in some cases, the names were printed instead. The API clearly states ID, so we should stick to it. * Compat endpoint image removal with _force_ deletes the entire not only the specified tag. Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg · 2021-05-05T12:15:48Z

Ready to be merged

rhatdan · 2021-05-05T12:27:48Z

libpod/define/errors.go

@@ -12,15 +12,6 @@ var (
 	// ErrNoSuchPod indicates the requested pod does not exist
 	ErrNoSuchPod = errors.New("no such pod")

-	// ErrNoSuchImage indicates the requested image does not exist
-	ErrNoSuchImage = errors.New("no such image")


This is a breaking change to the API, should we atlease set these constants to point at the libimage constants?

Which API do you mean?

libimage forwards all errors from containers/storage, so it would be storage.ErrImageUnknown but there's really no user of that error in libpod/Podman.

If we change this then we should bump the version of podman/libpod to v3, if we are following the rules.

rhatdan

LGTM
Except for constants in define directory.

openshift-ci-robot · 2021-05-05T12:30:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, vrothberg

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~~ [rhatdan,vrothberg]

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

rhatdan · 2021-05-05T12:33:23Z

As Nancy Pelosi famously said ""We have to pass the bill so that you can find out what is in it."

Meaning this PR is so huge we probably have to merge it and hope that the CI/CD system was good enough to catch regressions, then we need to cut a beta release and request as many users as possible use it, to find if we have regressions.

vrothberg · 2021-05-05T12:35:16Z

As Nancy Pelosi famously said ""We have to pass the bill so that you can find out what is in it."

Meaning this PR is so huge we probably have to merge it and hope that the CI/CD system was good enough to catch regressions, then we need to cut a beta release and request as many users as possible use it, to find if we have regressions.

Totally agree. It's too big to review entirely. I think we can cut a first RC next week. I want the team to test manually before.

mheon · 2021-05-05T13:19:47Z

/lgtm

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 27, 2021

vrothberg force-pushed the new-image-package branch 13 times, most recently from 666d0d6 to 27d74f3 Compare April 30, 2021 06:31

vrothberg mentioned this pull request Apr 30, 2021

update to latest libimage containers/buildah#3194

Merged

vrothberg force-pushed the new-image-package branch 2 times, most recently from 121a1e9 to 999c31e Compare April 30, 2021 10:19

vrothberg force-pushed the new-image-package branch 3 times, most recently from d206b87 to e4c790a Compare April 30, 2021 13:14

vrothberg force-pushed the new-image-package branch from e4c790a to ff4bbf5 Compare May 1, 2021 16:54

vrothberg force-pushed the new-image-package branch from 58d0496 to d272e46 Compare May 4, 2021 17:57

vrothberg changed the title ~~WIP - migrate Podman to containers/common/libimage~~ migrate Podman to containers/common/libimage May 4, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2021

edsantiago reviewed May 4, 2021

View reviewed changes

vrothberg force-pushed the new-image-package branch from d272e46 to 6726543 Compare May 5, 2021 06:42

vrothberg force-pushed the new-image-package branch from 6726543 to 9c519c9 Compare May 5, 2021 08:32

vrothberg mentioned this pull request May 5, 2021

podman system prune: Inconsistent "Total reclaimed space" #10215

Closed

vrothberg force-pushed the new-image-package branch from 9c519c9 to 0f7d54b Compare May 5, 2021 09:30

rhatdan reviewed May 5, 2021

View reviewed changes

rhatdan approved these changes May 5, 2021

View reviewed changes

openshift-ci-robot assigned mheon May 5, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2021

openshift-merge-robot merged commit a278195 into containers:master May 5, 2021

vrothberg deleted the new-image-package branch May 5, 2021 13:21

mtrmac mentioned this pull request Sep 22, 2021

Fix an issue with podman save trying to store signatures #11669

Closed

vrothberg mentioned this pull request Sep 23, 2021

podman save - signature support #11718

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate Podman to containers/common/libimage #10147

migrate Podman to containers/common/libimage #10147

vrothberg commented Apr 27, 2021 •

edited

Loading

vrothberg commented Apr 27, 2021

vrothberg commented Apr 27, 2021 •

edited

Loading

vrothberg commented Apr 30, 2021

vrothberg commented May 4, 2021

vrothberg commented May 4, 2021

edsantiago May 4, 2021

vrothberg May 5, 2021

vrothberg May 5, 2021

rhatdan commented May 4, 2021

edsantiago commented May 4, 2021

vrothberg commented May 5, 2021

vrothberg commented May 5, 2021

vrothberg commented May 5, 2021

rhatdan May 5, 2021

vrothberg May 5, 2021

vrothberg May 5, 2021

rhatdan May 5, 2021

rhatdan left a comment

openshift-ci-robot commented May 5, 2021

rhatdan commented May 5, 2021

vrothberg commented May 5, 2021

mheon commented May 5, 2021

migrate Podman to containers/common/libimage #10147

migrate Podman to containers/common/libimage #10147

Conversation

vrothberg commented Apr 27, 2021 • edited Loading

vrothberg commented Apr 27, 2021

vrothberg commented Apr 27, 2021 • edited Loading

vrothberg commented Apr 30, 2021

vrothberg commented May 4, 2021

vrothberg commented May 4, 2021

edsantiago May 4, 2021

Choose a reason for hiding this comment

vrothberg May 5, 2021

Choose a reason for hiding this comment

vrothberg May 5, 2021

Choose a reason for hiding this comment

rhatdan commented May 4, 2021

edsantiago commented May 4, 2021

vrothberg commented May 5, 2021

vrothberg commented May 5, 2021

vrothberg commented May 5, 2021

rhatdan May 5, 2021

Choose a reason for hiding this comment

vrothberg May 5, 2021

Choose a reason for hiding this comment

vrothberg May 5, 2021

Choose a reason for hiding this comment

rhatdan May 5, 2021

Choose a reason for hiding this comment

rhatdan left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented May 5, 2021

rhatdan commented May 5, 2021

vrothberg commented May 5, 2021

mheon commented May 5, 2021

vrothberg commented Apr 27, 2021 •

edited

Loading

vrothberg commented Apr 27, 2021 •

edited

Loading