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

Make ImageStatusByName race-free #7435

Merged
merged 5 commits into from
Dec 1, 2023

Conversation

mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Oct 25, 2023

What type of PR is this?

/kind bug

What this PR does / why we need it:

Currently, ImageStatusByName looks up the incoming name three times (once in GetStoreImage, once in NewImage, once in NewImageSource. If the name uses a tag and not a digest, that can result reading data from three different images, making the resulting ImageResult inconsistent.

A fix for this requires a new storage.ResolveReference function in c/image, being added in containers/image#2056 .

This PR is currently a proof-of-concept before merging that c/image feature, depending on an unmerged c/image PR.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

It seems possible to me that we don’t actually need this for signature verification (as long as we get one StorageID, we have something to work with, and the other fields of ImageResult might not matter), but it is just simpler for me fix ImageStatusByName than to worry about the details of that situation.

And I think CRI-O benefits form this fix either way.

Cc: @saschagrunert

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Oct 25, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. labels Oct 25, 2023
@mtrmac mtrmac force-pushed the consistent-inspect branch 3 times, most recently from 313886f to 49a3285 Compare October 25, 2023 17:56
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Merging #7435 (19cae30) into main (7499cc2) will decrease coverage by 0.01%.
Report is 6 commits behind head on main.
The diff coverage is 65.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7435      +/-   ##
==========================================
- Coverage   49.15%   49.14%   -0.01%     
==========================================
  Files         145      145              
  Lines       15808    15813       +5     
==========================================
+ Hits         7770     7771       +1     
- Misses       7105     7106       +1     
- Partials      933      936       +3     

@mtrmac
Copy link
Contributor Author

mtrmac commented Oct 25, 2023

Note to self:

[FAILED] failed to get image status: rpc error: code = Unknown desc = reference "[overlay@/tmp/tmp.BdnzWSMRdG/crio+/tmp/tmp.BdnzWSMRdG/crio-run]registry.k8s.io/e2e-test-images/busybox:1.29-2" does not resolve to an image ID: identifier is not an image

Those are real enough, ResolveReference returns a different error than GetStoreImage used to, and ImageStatus looks for that error.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2023
@mtrmac
Copy link
Contributor Author

mtrmac commented Oct 31, 2023

containers/image#2056 was merged, but this now depends on #7460 .

@openshift-ci openshift-ci bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Oct 31, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2023
@saschagrunert saschagrunert added this to the 1.29 milestone Nov 16, 2023
Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@openshift-ci openshift-ci bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Nov 30, 2023
@mtrmac mtrmac changed the title WIP: Make ImageStatusByName race-free Make ImageStatusByName race-free Nov 30, 2023
@mtrmac mtrmac marked this pull request as ready for review November 30, 2023 13:52
@mtrmac mtrmac requested a review from mrunalp as a code owner November 30, 2023 13:52
@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 Nov 30, 2023
@mtrmac
Copy link
Contributor Author

mtrmac commented Nov 30, 2023

Prerequisites are merged, this is now ready for review.

Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

Thanks!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2023
Copy link
Contributor

openshift-ci bot commented Nov 30, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, saschagrunert

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 Nov 30, 2023
@saschagrunert
Copy link
Member

/retest

2 similar comments
@saschagrunert
Copy link
Member

/retest

@mtrmac
Copy link
Contributor Author

mtrmac commented Nov 30, 2023

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit 798a86c into cri-o:main Dec 1, 2023
54 of 58 checks passed
@mtrmac mtrmac deleted the consistent-inspect branch December 4, 2023 12:59
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. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants