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

feat: unpin images from unknown oci bundles #4500

Merged

Conversation

ricardomaraschini
Copy link
Contributor

@ricardomaraschini ricardomaraschini commented May 27, 2024

Description

after importing all bundles from the oci directory we go through all containerd images and unpin all images that were imported from bundles that no longer exist on the system.

we start now to add the following label when importing:

  • k0sproject.ocibundle.paths

on this label we keep a list of all oci bundles that contain such image and we only unpin it if all the oci bundles were removed from the disk.

as now we are unpinning images based on their presence on disk we start to operate also on rename and remove fs events.

this commit also address an issue where we could leave a containerd connection opened.

Fixes #4433

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Copy link
Contributor

This pull request has merge conflicts that need to be resolved.

@ricardomaraschini ricardomaraschini force-pushed the unpin-images-from-unknown-oci-bundles branch from 9ba4baf to 3d4f4e4 Compare May 28, 2024 08:50
@jnummelin jnummelin added this to the 1.31 milestone May 29, 2024
Copy link
Member

@twz123 twz123 left a comment

Choose a reason for hiding this comment

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

It could be easier to collect all the loaded images and return them as an array from loadAll(). Then use this array of loaded images in unpinAll() to skip the ones that are still on disk. That way, it'd be enough to have a simple marker label that's indicating that an image was originally imported by the OCI bundle importer, whose value is unused and could default to the empty string.

pkg/component/worker/ocibundle.go Outdated Show resolved Hide resolved
pkg/component/worker/ocibundle.go Outdated Show resolved Hide resolved
pkg/component/worker/ocibundle.go Outdated Show resolved Hide resolved
pkg/component/worker/ocibundle.go Outdated Show resolved Hide resolved
pkg/component/worker/ocibundle.go Outdated Show resolved Hide resolved
@ricardomaraschini ricardomaraschini force-pushed the unpin-images-from-unknown-oci-bundles branch from ebb7234 to 4245c28 Compare July 2, 2024 18:51
@ricardomaraschini
Copy link
Contributor Author

It could be easier to collect all the loaded images and return them as an array from loadAll(). Then use this array of loaded images in unpinAll() to skip the ones that are still on disk. That way, it'd be enough to have a simple marker label that's indicating that an image was originally imported by the OCI bundle importer, whose value is unused and could default to the empty string.

Oh yeah, loadAll() should have been called loadAllAsLongAsTheyHaventBeenLoadedYet() :-) . We do not go through every file in the directory every time, we skip already imported files. In other words: I am not sure how this would make things easier or cleaner but I may be not understanding you.

@ricardomaraschini ricardomaraschini force-pushed the unpin-images-from-unknown-oci-bundles branch 2 times, most recently from d8ffdbb to fbe8692 Compare July 8, 2024 11:14
Copy link
Contributor

github-actions bot commented Aug 7, 2024

The PR is marked as stale since no activity has been recorded in 30 days

@github-actions github-actions bot added Stale and removed Stale labels Aug 7, 2024
@ricardomaraschini ricardomaraschini force-pushed the unpin-images-from-unknown-oci-bundles branch from fbe8692 to 26e22df Compare August 27, 2024 12:28
after importing all bundles from the oci directory we go through all
containerd images and unpin all images that were imported from bundles
that no longer exist on the system.

we start now to add the following label when importing:

- k0sproject.ocibundle.paths

on this label we keep a list of all oci bundles that contain such image
and we only unpin it if all the oci bundles were removed from the disk.

as now we are unpinning images based on their presence on disk we start
to operate also on rename and remove fs events.

this commit also address an issue where we could leave a containerd
connection opened.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
- fixed wrong "copyright year".
- add error wrap to a function return.
- change label name.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
the previous version of this was way too much optimistic, we better
handle other errors that are not of "does not exist" class.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
if we fail to unpin one image we can keep moving and unpining the
others.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
just checks dates are equal.

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
windows don't seem to be happy if we remove a file without closing it.
if you are seeing this commit then it means "yes, windows is not happy
about that". if you aren't seeing this comment then the answer is "no,
windows does not care about that" but you won't know (except if you
already knew).

Signed-off-by: Ricardo Maraschini <ricardo.maraschini@gmail.com>
@ricardomaraschini ricardomaraschini force-pushed the unpin-images-from-unknown-oci-bundles branch from c311817 to e016733 Compare September 5, 2024 15:36
@twz123 twz123 merged commit 9067e7d into k0sproject:main Sep 16, 2024
86 checks passed
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.

Remove pinning from outdated k0s-imported OCI images
3 participants