Skip to content

Conversation

Honny1
Copy link
Member

@Honny1 Honny1 commented Aug 15, 2024

This PR changes runtime.layerTree method to use consistent values for the creation of layerTree. Before images and layers could vary between calls that obtain images and layers now using storage.MultiList to obtain consistent images and layers. Also, this PR adapts callers of runtime.layerTree to use consistent images and layers and vendor pre-release of c/storage with storage.MultiList method.

Fixes: containers/podman#23331

Depends on: #2131

@Honny1 Honny1 force-pushed the image-list branch 6 times, most recently from c1113a8 to 599eb71 Compare August 21, 2024 11:30
@Honny1 Honny1 changed the title Image list libimage.runtime.layerTree: Use consistent layers and images Aug 21, 2024
@Honny1 Honny1 mentioned this pull request Aug 21, 2024
@Honny1 Honny1 marked this pull request as ready for review August 21, 2024 14:28
Copy link
Contributor

@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.

Note particularly the comment about racy lookups; I don’t immediately know what to do there.

Maybe we can change the callers instead — I see only Podman’s Mount and Unmount, maybe those could get a ListImagesByNames with one set of features, and everyone else could get an ListAllImages with the names parameter removed entirely.

We need to resolve this somehow, I didn’t analyze the callers enough to have an opinion yet.


As a general principle, a commit should go from a buildable (ideally, working) codebase to a buildable codebase; don’t split a change of a function parameter from updates of callers into separate commits. (The reason is to make git bisect work well, and to generally make it possible to do backports without having to think too much about the commit sequence.)

@Honny1 Honny1 force-pushed the image-list branch 6 times, most recently from fc5b51a to cc7ded3 Compare August 22, 2024 13:11
Copy link
Contributor

@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.

.

@rhatdan
Copy link
Member

rhatdan commented Aug 22, 2024

@mtrmac Did you add a bogus comment?

@mtrmac
Copy link
Contributor

mtrmac commented Aug 22, 2024

Yes, I’m sorry — I think all the pending review comments got published in the meantime.

@Honny1 Honny1 requested a review from mtrmac August 26, 2024 15:33
@Honny1 Honny1 force-pushed the image-list branch 3 times, most recently from 5d8807d to e3ae831 Compare August 26, 2024 18:03
Copy link
Contributor

@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.

Great, that solves that problem.

Now, can we fix the other one as well, and do the filtering based on the same layerTree? I think this should work:

  • Have compileImageFilters also return a “needs a layer tree” boolean
  • Add a *layerTree parameter to filterFunc; move tree creation from compileImageFilters to filterImages (based on the boolean from the previous step)
  • Then move compileImageFilters directly into ListImages, and call it before the MultiList
  • MultiList will now know whether a layerTree is necessary, and always create only one.

This is getting a bit large — can this be split into at least 3 commits?

  • Add ListImagesByNames, change the ListImages API, without changing the implementation
  • Refactor filterImages + filterFunc to create the tree in filterFunc
  • Use MultiList, and all the other stuff.

Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
@Honny1 Honny1 force-pushed the image-list branch 2 times, most recently from 5c42955 to 0177418 Compare August 28, 2024 13:34
Copy link
Contributor

@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.

Thanks! ACK overall, just some nits to help future maintainers.

Honny1 added 2 commits August 28, 2024 15:59
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Copy link
Contributor

@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.

LGTM.

Great work!

@rhatdan
Copy link
Member

rhatdan commented Aug 28, 2024

@vrothberg PTAL

@vrothberg
Copy link
Member

@vrothberg PTAL

No time to review, sorry. I trust the PR has been thoroughly reviewed :)

@rhatdan
Copy link
Member

rhatdan commented Aug 29, 2024

/approve
/lgtm

Copy link
Contributor

openshift-ci bot commented Aug 29, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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-merge-bot openshift-merge-bot bot merged commit 8483ef6 into containers:main Aug 29, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

race: completion: Top layer ... not found ... storage may be corrupted
4 participants