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

fix: New cleanup when using --cleanup flag (#1406) #2783

Conversation

dadurex
Copy link

@dadurex dadurex commented Oct 9, 2023

Fixes #1406

Description

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

** Notes**

Flag cleanup does not clean kaniko root directory (default /kaniko). As a result, when building more than one image on same kaniko container we can face the problem of leftovers directories like /kaniko/0/... which are stage-dependent files (https://github.com/GoogleContainerTools/kaniko/blob/v1.16.0/pkg/executor/build.go#L807) saved by kaniko.

New function CleanupFilesystem() first calls DeleteFilesystem() and then removes all files and directories from kaniko root directory with names are digits. Those files are directories that store stage-dependent files and snapshots.

image

This fix can be also achieved in different way. All snapshot can be placed in kanikoDir/snapshots and layers dependent files in kanikoDir/layers and then CleanupFilesystem() could simply call os.RemoveAll() on those directories instead of this logic with regex.

Described also in issue #1406

I'm open to discussion.

@google-cla
Copy link

google-cla bot commented Oct 9, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@aaron-prindle aaron-prindle self-requested a review October 24, 2023 20:25
@aaron-prindle aaron-prindle self-assigned this Oct 24, 2023
@aaron-prindle
Copy link
Collaborator

aaron-prindle commented Nov 10, 2023

@dadurex thanks for the PR here! Not sure why the CI tests didn't run for your submission, can you resubmit your PR to kickoff the CI tests? The PR itself looks good, need the CI to be green and then we should be able to merge

@dadurex
Copy link
Author

dadurex commented Nov 13, 2023

@dadurex thanks for the PR here! Not sure why the CI tests didn't run for your submission, can you resubmit your PR to kickoff the CI tests? The PR itself looks good, need the CI to be green and then we should be able to merge

@aaron-prindle Do you mean close and create new PR?

@aaron-prindle
Copy link
Collaborator

aaron-prindle commented Nov 16, 2023

@dadurex you should be able to re-push the PR here as is (resubmitting is also fine)

Something like the following should work:

git commit --amend --no-edit
git push -f origin HEAD

That will resubmit the PR here and kick-off the CI tests

@cmorty
Copy link

cmorty commented Nov 23, 2023

Your alternative solution sounds much better. But I'm happy if this works at all. ;)

@aaron-prindle
Copy link
Collaborator

@dadurex can you re-push the PR here? For some reason our CI/CD tests weren't run for your PR submission initially

@aaron-prindle
Copy link
Collaborator

Closing, changes made are now tracked in #3130

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.

multistage build in same container fails because cross stage deps are not cleaned up ( symlinks )
3 participants