Skip to content

Conversation

@rhatdan
Copy link
Member

@rhatdan rhatdan commented Jul 17, 2023

Does this PR introduce a user-facing change?

Temporary files created when dealing with images in /var/tmp will automatically be cleaned up on reboot.

[NO NEW TESTS NEEDED]

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 17, 2023
@rhatdan
Copy link
Member Author

rhatdan commented Jul 17, 2023

Replaces: #19201

Comment on lines 9 to 10
Copy link
Member

Choose a reason for hiding this comment

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

Path in the comment must change as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Waiting on repush, to make sure tests pass. Not sure how important this change would be.

Copy link
Member

Choose a reason for hiding this comment

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

Why only at boot? I feel like if you have a long running server we can clean these up much earlier. I would even say set this to 1 day.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well then you would have a risk of someone pulling an image and then systemd kills it.
These should only leak when something is killed, so it should not be that common.

Copy link
Member

Choose a reason for hiding this comment

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

No that is not what tmpfiles does. It will only removes files that are not touched in the last X time.

The date field, when set, is used to decide what files to delete when cleaning. If a file or directory is older than the current time minus the age field, it is deleted.

So I don't see how systemd would ever delete files that are still in use by us.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see how you can use a timer with a glob to remove files?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah not 100% sure either, I don't have time to look into it but this change works as well so fine for me.

@rhatdan rhatdan force-pushed the tmpfs branch 3 times, most recently from cc71e6e to 66b82ee Compare July 22, 2023 12:10
rhatdan added 2 commits July 24, 2023 10:34
 containers

We need to remove /var/tmp/container_images_* and
/var/tmp/container_images_* which are podman temporary directories on each
boot which are created when creating containers from oci-archive tarballs
or other pull operations.

Signed-off-by: Joe Doss <joe@solidadmin.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Member Author

rhatdan commented Jul 24, 2023

Use this PR rather then #19305

@rhatdan
Copy link
Member Author

rhatdan commented Jul 24, 2023

@containers/podman-maintainers PTAL
@edsantiago @Luap99 @giuseppe @vrothberg @TomSweeneyRedHat @mheon @flouthoc PTAL

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
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM
/lgtm
/approve
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 24, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 24, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, 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

@edsantiago
Copy link
Member

Why is this revendoring the world?

@rhatdan
Copy link
Member Author

rhatdan commented Jul 24, 2023

For some reason, containers/image would not update until I vendored the universe.

@rhatdan rhatdan removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 24, 2023
@rhatdan rhatdan merged commit b9383f4 into containers:main Jul 24, 2023
@github-actions github-actions 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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2023
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. release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants