Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update system tests to handle zstd:chunked images #24286

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Oct 15, 2024

  • Compare image configs, not IDs, when checking image identity from different way of pulling them
  • Try to make the additional image store test a bit more robust
  • Remove an image identity assumption from the “load by URL” test.

Working on test failures as discussed in containers/storage#2130 .

At this point absolutely untested, and I don’t really know what I’m doing.

Cc: @edsantiago — perhaps read this only later after this is at least minimally shown to be working.

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 15, 2024
Copy link
Contributor

openshift-ci bot commented Oct 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2024
Copy link

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

Copy link
Collaborator Author

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Yeah, this is nowhere close to working, ignore for now.


Also [255] podman-kube@.service template with rollback

#|     FAIL: global auto-update policy gets applied
   #| expected: '.*podman-kube@-tmp-podman_bats.roXH3U-test.yaml.service,.* (test_pod-a),quay.io/libpod/testimage:20241010,false,registry.*' (using expr)
   #|   actual: 'podman-kube@-tmp-podman_bats.roXH3U-test.yaml.service,c104ff383fd7 (test_pod-a),quay.io/libpod/testimage:20241010,pending,registry'
   #|         > 'podman-kube@-tmp-podman_bats.roXH3U-test.yaml.service,aa204347c263 (test_pod-b),localhost/image:Kl8NYi3jxL,false,local'

test/system/010-images.bats Show resolved Hide resolved
test/system/120-load.bats Show resolved Hide resolved
test/system/150-login.bats Show resolved Hide resolved
test/system/150-login.bats Show resolved Hide resolved
@mtrmac mtrmac force-pushed the compare-image-configs branch 8 times, most recently from 51edef6 to a644fe8 Compare October 17, 2024 21:21
@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 17, 2024

@edsantiago With this, make localsystem is passing for me in #24287 , and it should continue to work with non-chunked images.

Could you take a preliminary look to make sure I’m not missing/breaking the point of these tests, please?

See individual commit messages for details.

@mtrmac mtrmac changed the title FIXME Various zstd:chunked test changes Update system tests to handle zstd:chunked images Oct 17, 2024
@edsantiago
Copy link
Member

@mtrmac I've been peeking periodically and have seen no major cause for alarm. I will not be able to give this a thorough review until Monday. Apologies.

test/system/helpers.bash Outdated Show resolved Hide resolved
test/system/helpers.bash Outdated Show resolved Hide resolved
@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 17, 2024

@edsantiago Thanks! The peeks are reassuring already, and I need to make this pass first. There’s no rush at all.

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

I think the 120 tests need a complete revamp: my initial decision to use $IMAGE has backfired. Next week I will look into a different safer approach.

The test got the stores RW status backwards.

Before zstd:chunked, both image IDs should be the same, so this used
to make no difference.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
When looking up the current-store image ID, do that
from the same output where we verify that the ID is from the
current store, instead of listing images twice.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
… methods

Historically, non-schema1 images had a deterministic image ID == config digest.
With zstd:chunked, we don't want to deduplicate layers pulled by consuming the
full tarball and layers partially pulled based on TOC, because we can't cheaply
ensure equivalence; so, image IDs for images where a TOC was used differ.

To accommodate that, compare images using their configs digests, not using image IDs.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
The additional image store feature assumes that images / layers
in the additional store never go away, while we do remove it after
this test. Try to repair the store.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Don't assume that the loaded image will be deduplicated
with the server image.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Oct 18, 2024

Ready for review:
System tests are passing both here, with current images, and in #24287 , with zstd:chunked images. (Integration tests are failing with zstd:chunked images, I’ll work on that in a separate PR.)

@mtrmac mtrmac marked this pull request as ready for review October 18, 2024 22:58
@openshift-ci 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 Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants