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

libpod: cleanup default cache on system reset #22830

Merged
merged 2 commits into from
May 29, 2024

Conversation

giuseppe
Copy link
Member

Closes: #22825

None

giuseppe added 2 commits May 29, 2024 11:06
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Closes: containers#22825

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 29, 2024
@Luap99
Copy link
Member

Luap99 commented May 29, 2024

Do you know if there a reason that the blob cache is not located on the actual graphroot but rather hard coded to ~/.local/share/containers/cache? I am asking because this means that running e2e system reset tests (which should run isolated) will now nuke my local blob info cache.
I guess it will not be big deal in practice as it is just a cache but it still seems rather unexpected to me.

@giuseppe giuseppe added the No New Tests Allow PR to proceed without adding regression tests label May 29, 2024
@giuseppe
Copy link
Member Author

Do you know if there a reason that the blob cache is not located on the actual graphroot but rather hard coded to ~/.local/share/containers/cache? I am asking because this means that running e2e system reset tests (which should run isolated) will now nuke my local blob info cache.
I guess it will not be big deal in practice as it is just a cache but it still seems rather unexpected to me.

no idea why we picked that directory.

@mtrmac do you think we should change it to be under storage?

@mtrmac
Copy link
Collaborator

mtrmac commented May 29, 2024

The reason is that it has no direct relationship with the c/storage graph root. It is used, and beneficial, e.g. also for skopeo copy docker://… docker://…

@Luap99
Copy link
Member

Luap99 commented May 29, 2024

The reason is that it has no direct relationship with the c/storage graph root. It is used, and beneficial, e.g. also for skopeo copy docker://… docker://…

Should podman system reset then delete it or not in your opinion?

@mtrmac
Copy link
Collaborator

mtrmac commented May 29, 2024

I don’t have a strong opinion.

You’re right to point out that the relationship is not that tight.

OTOH, from time to time, the blob info cache is key in behavior decisions, and users who are unaware of it find it very hard to reproduce the unwanted behavior they are trying to report. In that sense, having podman system reset reset the cache’s state might help those users.

@Luap99
Copy link
Member

Luap99 commented May 29, 2024

Ack thanks, as it is just a cache and it is not like many user running podman system reset with non default location anyway I think it is fine then

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented May 29, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, Luap99

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

@mtrmac
Copy link
Collaborator

mtrmac commented May 29, 2024

I do agree it’s not ideal if Podman tests affect things outside of the test perimeter. I’m tempted to say that should be changed in the tests, maybe by changing XDG_DATA_HOME. But that feels like a big and risky hammer.

@mheon
Copy link
Member

mheon commented May 29, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 7eb1ceb into containers:main May 29, 2024
89 checks passed
@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat added a commit to TomSweeneyRedHat/podman that referenced this pull request Aug 6, 2024
Bump c/images to v5.29.5 as part of the a fix for https://issues.redhat.com/browse/ACCELFIX-267
"Podman system resets" containers#22830

Signed-off-by: tomsweeneyredhat <tsweeney@redhat.com>
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Oct 31, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Oct 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. No New Tests Allow PR to proceed without adding regression tests release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman system reset doesn't remove blob-info-cache-v1.sqlite
5 participants