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

Cleanup temp directories across image providers #132

Open
wagoodman opened this issue Jun 23, 2022 · 5 comments
Open

Cleanup temp directories across image providers #132

wagoodman opened this issue Jun 23, 2022 · 5 comments
Labels
bug Something isn't working

Comments

@wagoodman
Copy link
Contributor

What happened:
See docker/sbom-cli-plugin#22 for the original problem; invocations of docker sbom (which uses the docker daemon provider) are not cleaning up temp files.

What you expected to happen:
All temp directories should be cleaned up.

How to reproduce it (as minimally and precisely as possible):
docker sbom alpine:latest and ls the temp directory to see that there are sbom-cli-plugin-* directories still left (docker sbom uses stereoscope).

Anything else we need to know?:
Today the docker daemon provider uses the tarball provider, each create sibling temp directories under a common root directory. We should be cleaning up the image.tar as soon as we create and provide an image from the tarball provider (from within the docker daemon provider --downside is that multiple calls to provide() will fail then [which hints that another solution may be needed here] ).

@ltsonov-cb
Copy link

+1 on this - we noticed abnormal disk usage when utilizing syft and narrowed down the issue to this root cause as well.
The code to work around it is a bit clunky as users of our wrapper APIs expect that image.Cleanup() is enough - but that leaves .tar files hanging around.

@ghost
Copy link

ghost commented Feb 24, 2023

I think this is an interesting problem. There are a few options here:

Personally I am a fan of option 1... but would like a recommendations from the team - I am happy to open a PR with a little guidance ☺️

@ghost
Copy link

ghost commented Feb 24, 2023

I think this is an interesting problem. There are a few options here:

Personally I am a fan of option 1... but would like a recommendations from the team - I am happy to open a PR with a little guidance ☺️

nm... I think I was wrong.

Based on a further research - only thing really needed is to call client.go Cleanup(). which releases the root generator and all children generators....effectively cleaning up the directories.

The comment above cleanup is misleading and what leads to the leak of files I believe.

@G-Rath
Copy link

G-Rath commented Feb 25, 2023

I've just run into this myself - was using img.Cleanup per the basic example, only to find ~32gb of tmp files from stereoscope😅

Switching to stereoscope.Cleanup() looks like it's resolved the problem - it would be good to know the backstory on the comment @slimdevl you've indicated, since it does seem like that should be called instead of (or maybe in addition to?) img.Cleanup.

G-Rath added a commit to G-Rath/osv-detector that referenced this issue Feb 25, 2023
@ghost
Copy link

ghost commented Feb 27, 2023

I've just run into this myself - was using img.Cleanup per the basic example, only to find ~32gb of tmp files from stereoscope😅

Switching to stereoscope.Cleanup() looks like it's resolved the problem - it would be good to know the backstory on the comment @slimdevl you've indicated, since it does seem like that should be called instead (or maybe in addition to?) img.Cleanup.

I did a little digging this weekend and discovered the following:

  1. One cannot call stereoscope.Cleanup if the process uses the library for subsequent scan calls. This is because rootTempDirGenerator is the foundation of all allocated temp dirs, and the base directory attached to it is gone after first cleanup. This cascades into subsequent scan failures. So calling stereoscope.Cleanup is not a good option for long running processes.
  2. selectImageProvider() allocates a child tempDirGenerator. It passes that to the Provider (in my case DaemonProvider), which in turn passes only the directory to the image.NewImage() call. This passes the responsibility to the Image.Cleanup() method to cleanup directories. However this cannot work because *only the passed directory is cleaned up by the image, and the DaemonProvider directory is leaked at this point.

This feels like an incomplete chain of responsibility. Because the DaemonProvider allocates a separate directory for the image.tar, which must exist until after the client.go calls image.Read(). This is partially because of the way v1.Image lazy load feature behaves. Thus the relationships are broken during the Provide sequence.

IMHO - A better solution is to complete the chain of responsibility but having selectImageProvider() allocated a rootTempDirGenerator and pass it to the provider; which in turn passes the tempDirGenerator to the image.NewImage(), this completes the chain of responsibility.

If providers make a new generators, or directories, they are all passed to the image for cleanup after the image is no longer needed.

G-Rath added a commit to G-Rath/osv-detector that referenced this issue Mar 3, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Mar 4, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Mar 4, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Mar 4, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Mar 4, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Mar 4, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Mar 9, 2023
wagoodman added a commit that referenced this issue Mar 23, 2023
* fix tmpDirGenerator chain of responsibility associated with #132

Signed-off-by: Joseph Barnett <[email protected]>
Signed-off-by: [email protected] <[email protected]>

* reduce log message to debug
Signed-off-by: Joseph Barnett <[email protected]>

Signed-off-by: [email protected] <[email protected]>

* restore global cleanup function

Signed-off-by: Alex Goodman <[email protected]>

---------

Signed-off-by: Joseph Barnett <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Co-authored-by: Alex Goodman <[email protected]>
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Apr 27, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Apr 27, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Apr 27, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Apr 27, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Apr 27, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Apr 28, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Apr 29, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue May 3, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue May 3, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Jun 3, 2023
gnmahanth pushed a commit to deepfence/stereoscope that referenced this issue Jun 15, 2023
* fix tmpDirGenerator chain of responsibility associated with anchore#132

Signed-off-by: Joseph Barnett <[email protected]>
Signed-off-by: [email protected] <[email protected]>

* reduce log message to debug
Signed-off-by: Joseph Barnett <[email protected]>

Signed-off-by: [email protected] <[email protected]>

* restore global cleanup function

Signed-off-by: Alex Goodman <[email protected]>

---------

Signed-off-by: Joseph Barnett <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Co-authored-by: Alex Goodman <[email protected]>
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Jun 29, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Jul 11, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Jul 26, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Jul 26, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Aug 18, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Aug 19, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Aug 19, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Aug 25, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Aug 25, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Sep 14, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Sep 14, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Sep 14, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Sep 14, 2023
G-Rath added a commit to G-Rath/osv-detector that referenced this issue Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants