storage: improve error message on image mount from r/o store

[comick]

The PR follows an analysis available in podman repository at podman-container-tools/podman#29001

[mtrmac]

Thanks!

[mtrmac]

We already hold all of these locks; and due to the underlying implementation of sync.RWLock, it seems possible this might actually cause a deadlock.

[comick]

Read lock acquisition code removed.

For reference, existing lock was already acquired here

container-libs/storage/store.go

Line 2996 in 498ccd1

if err := s.startReading(); err != nil {

with deferred release such that we're covered.

[mtrmac]

Thank you!

[Luap99]

This seems to have caused a regression in buildah tests
https://github.com/podman-container-tools/buildah/actions/runs/28725370778/job/85181914369?pr=6940

not ok 533 bud with a previously-used graphroot with base image in it used as imagestore
# (from function `die' in file ./helpers.bash, line 455,
#  from function `run_buildah' in file ./helpers.bash, line 434,
#  in test file ./bud.bats, line 9339)
#   `run_buildah --root=${TEST_SCRATCH_DIR}/newroot --storage-opt=imagestore=${TEST_SCRATCH_DIR}/root build --pull=never --no-cache --layers=true ${contextdir}' failed
# /var/tmp/buildah/tests /var/tmp/buildah/tests
# # [checking for: quay.io/libpod/alpine:latest]
# # [copy docker://quay.io/libpod/alpine:latest dir:/tmp/bats-run-sLb8L9/suite/buildah-image-cache/quay.io-libpod-alpine-latest-]
# Getting image source signatures
# Copying blob sha256:9d16cba9fb961d1aafec9542f2bf7cb64acfc55245f9e4eb5abecd4cdc38d749
# Copying config sha256:961769676411f082461f9ef46626dd7a2d1e2b2a38e6a44364bcbecf51e66dd4
# Writing manifest to image destination
# # [copy dir:/tmp/bats-run-sLb8L9/suite/buildah-image-cache/quay.io-libpod-alpine-latest- containers-storage:quay.io/libpod/alpine:latest]
# Getting image source signatures
# Copying blob sha256:9d16cba9fb961d1aafec9542f2bf7cb64acfc55245f9e4eb5abecd4cdc38d749
# Copying config sha256:961769676411f082461f9ef46626dd7a2d1e2b2a38e6a44364bcbecf51e66dd4
# Writing manifest to image destination
# # /var/tmp/buildah/tests/./../bin/buildah --root=/tmp/buildah_tests.zvwvcx/newroot --storage-opt=imagestore=/tmp/buildah_tests.zvwvcx/root build --pull=never --no-cache --layers=true /tmp/buildah_tests.zvwvcx/context
# STEP 1/2: FROM quay.io/libpod/alpine:latest
# STEP 2/2: RUN mkdir hello
# COMMIT
# Error: committing container for step {Env:[PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin] Command:run Args:[mkdir hello] Flags:[] Attrs:map[] Message:RUN mkdir hello Heredocs:[] Original:RUN mkdir hello}: copying layers and metadata for container "c12adb764fef0aae9c4a9725a30d5694cfbbefc63a7226636cd86a82ca3d26b1": initializing source containers-storage:alpine-working-container: checking which exclusions are in base image "961769676411f082461f9ef46626dd7a2d1e2b2a38e6a44364bcbecf51e66dd4": mounting image "961769676411f082461f9ef46626dd7a2d1e2b2a38e6a44364bcbecf51e66dd4" to examine its contents: mounting read/only store images is not allowed: called a write method on a read-only store
# [ rc=125 (** EXPECTED 0 **) ]
# #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
# #| FAIL: exit code is 125; expected 0
# #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

podman-container-tools/buildah#6940

Since ErrLayerUnknown is no longer being returned buildah cannot catch that
podman-container-tools/buildah@a20e251

[Luap99]

I propose a revert for now #965

[comick]

If my understanding is correct, the changed error message slips away from the check at

container-libs/common/libimage/layer_tree.go

Line 45 in 7a91a6d

return errors.Is(err, storage.ErrImageUnknown) ||

The failing check ultimately makes buildah exit with non-zero status, unlike what expected by the test at https://github.com/comick/buildah/blob/d1bb55d0dafd73be07ceb05e1d439dc2223650d1/tests/bud.bats#L9392

Compared to the original one, a more sophisticated change is needed. I see a couple options:

• make buildah (also check podman) exit with status 0 also in presence of the new error
• change the test to account for the non-zero exit status in case of read only store

First option is certainly the easiest. What do you think?

[mtrmac]

@comick The layer_tree code has nothing obvious to do with mounting, and I don’t know why that one should be changed.

An earlier comment here points at podman-container-tools/buildah@a20e251 , and in that case recognizing the “trying to mount from a layer store” error instead of a misleading “image not found” would make sense.

Either way, I think we’ll need a coordinated set of PRs, one here, and one in each of Buildah + Podman exercising the full test suite with the proposed update (and proposing/demonstrating/testing any other adjustments in the callers that may be necessary).