Skip to content

Fix: Storage leak in Builder and Fetcher #2979

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

Merged
merged 5 commits into from
Jul 15, 2024
Merged

Conversation

soharab-ic
Copy link
Contributor

@soharab-ic soharab-ic commented Jul 10, 2024

Description

  • Builder pod contains builder and fetcher containers and they share a volume.
  • Builder build the packages and fetcher uploads the packages to storage service.
  • In case of build/upload failure, packages remains in the shared volume. These packages are of no use and needs to be cleaned up.
  • Added a Clean API in builder package. This API will be called by buildermgr for deleting src pkg after pkg is build and deployment is uploaded. I wanted to delete the src package from builder pod directly after failed/successful build but buildermgr uses retryablehttp client so in case of build failure buildermgr will retry build by sending Build request.
  • To delete deployment pkg, fetcher UploadHandler will delete the deployment pkg after successful/failed upload.

Which issue(s) this PR fixes:

Fixes #2821

Testing

  • Added a negative test for builder's Clean API.
  • Adding a positive test involves creating a /packages directory for which root permissions are required.Therefore, did not add any positive test.
{"level":"info","ts":"2024-07-10T10:45:48.563Z","logger":"builder","caller":"builder/builder.go:197","msg":"builder received clean request","request":{"srcPkgFilename":"hello-test-fdebb12e-60ff-4e95-b41b-e3531444a4a4-kfezxl"}}
{"level":"info","ts":"2024-07-10T10:45:48.564Z","logger":"builder","caller":"builder/builder.go:175","msg":"clean request complete","elapsed_time":0.001311227}

Checklist:

  • I ran tests as well as code linting locally to verify my changes.
  • I have done manual verification of my changes, changes working as expected.
  • I have added new tests to cover my changes.
  • My changes follow contributing guidelines of Fission.
  • I have signed all of my commits.

```
builder pod keeps old src and deployment packages irrespective of build status.
delete src package after every build request is completed.
delete deployment package after package is uploaded.
```

Signed-off-by: Md Soharab Ansari <soharab.ansari@infracloud.io>
Signed-off-by: Md Soharab Ansari <soharab.ansari@infracloud.io>
Signed-off-by: Md Soharab Ansari <soharab.ansari@infracloud.io>
Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 34.44444% with 59 lines in your changes missing coverage. Please review.

Project coverage is 44.68%. Comparing base (c6c811e) to head (9f9975b).
Report is 1 commits behind head on main.

Files Patch % Lines
pkg/builder/client/client.go 0.00% 23 Missing ⚠️
pkg/utils/utils.go 25.00% 15 Missing ⚠️
pkg/buildermgr/common.go 0.00% 13 Missing ⚠️
pkg/builder/builder.go 74.07% 6 Missing and 1 partial ⚠️
cmd/builder/app/server.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2979      +/-   ##
==========================================
- Coverage   44.70%   44.68%   -0.02%     
==========================================
  Files         236      236              
  Lines       24306    24396      +90     
==========================================
+ Hits        10865    10902      +37     
- Misses      12032    12085      +53     
  Partials     1409     1409              
Flag Coverage Δ
unittests 44.68% <34.44%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@soharab-ic soharab-ic requested a review from sanketsudake July 11, 2024 05:29
Signed-off-by: Md Soharab Ansari <soharab.ansari@infracloud.io>
Signed-off-by: Md Soharab Ansari <soharab.ansari@infracloud.io>
@sanketsudake sanketsudake merged commit 8de5a5b into main Jul 15, 2024
10 checks passed
@sanketsudake sanketsudake deleted the fix-builder-storage-leak branch September 26, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Storage Leak in Fetcher
2 participants