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

Make cache pruning more flexible #1409

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

jay7x
Copy link

@jay7x jay7x commented Mar 8, 2023

This command deletes only those cached downloads which are not referenced by any existing VM configs.

@afbjorklund
Copy link
Member

afbjorklund commented Mar 8, 2023

Should we maybe keep also the images that are included in the current template versions, not only instances ?

@jay7x
Copy link
Author

jay7x commented Mar 8, 2023

Should we maybe keep also the images that are included in the current template versions, not only instances ?

I have no strong opinion on this.. my case is to cleanup images I've downloaded for some tests and which are not used (doesn't exists) anymore.. Though I'd like to keep the images I'm still using as internet in some countries (Indonesia e.g.) is not that great to redownload few gigabytes frequently.

@afbjorklund
Copy link
Member

afbjorklund commented Mar 8, 2023

I added an issue (#1410) for the discussion - there's also Slack, if we want to keep this PR for the implementation review

cmd/limactl/prune.go Outdated Show resolved Hide resolved
cmd/limactl/prune.go Outdated Show resolved Hide resolved
cmd/limactl/prune.go Outdated Show resolved Hide resolved
cmd/limactl/prune.go Outdated Show resolved Hide resolved
@jay7x jay7x marked this pull request as draft March 10, 2023 08:52
@jay7x jay7x changed the title Implement limactl prune downloads Make cache pruning more flexible Mar 19, 2023
@jay7x
Copy link
Author

jay7x commented Mar 19, 2023

I've simplified things and removed the prune subcommands. New CLI switches added instead:

  -F, --fallback       Only prune images without a digest specified (fallback images usually)
  -U, --unreferenced   Only prune downloads not referenced in any VM or template

cmd/limactl/prune.go Outdated Show resolved Hide resolved
cmd/limactl/prune.go Outdated Show resolved Hide resolved
@jay7x jay7x marked this pull request as ready for review March 28, 2023 18:48
@jay7x
Copy link
Author

jay7x commented May 13, 2023

Updates:

  1. Downloads referenced by a template are dropped from the PR scope
  2. Changes are rebased on the current master branch
  3. Commits are squashed
  4. Few small improvements (better logging, use digest instead of sprintf()'ed sha256)

@jay7x jay7x requested a review from AkihiroSuda May 13, 2023 16:44
@AkihiroSuda AkihiroSuda added this to the v0.16.0 milestone May 19, 2023
AkihiroSuda
AkihiroSuda previously approved these changes May 19, 2023
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@jandubois
Copy link
Member

I think this PR is good, but have run out of time right now (will finish tomorrow).

I found it useful to add a --dry-run option for testing, and I think it may be generally useful, so please consider adding it (but can also be done later in another PR):

--- cmd/limactl/prune.go
+++ cmd/limactl/prune.go
@@ -21,12 +21,17 @@ func newPruneCommand() *cobra.Command {
                ValidArgsFunction: cobra.NoFileCompletions,
        }

+       pruneCommand.Flags().Bool("dry-run", false, "Show which images would be deleted, but don't really delete them")
        pruneCommand.Flags().Bool("no-digest-only", false, "Only prune images without a digest specified (\"fallback\" images usually)")
        pruneCommand.Flags().Bool("unreferenced-only", false, "Only prune downloads not referenced in any VM")
        return pruneCommand
 }

 func pruneAction(cmd *cobra.Command, args []string) error {
+       dryRun, err := cmd.Flags().GetBool("dry-run")
+       if err != nil {
+               return err
+       }
        pruneWithoutDigest, err := cmd.Flags().GetBool("no-digest-only")
        if err != nil {
                return err
@@ -68,20 +73,24 @@ func pruneAction(cmd *cobra.Command, args []string) error {
                                if pruneWithoutDigest && refFile.Digest == "" {
                                        // delete the fallback image entry (entry w/o digest) even if referenced
                                        logrus.WithFields(entryFields).Infof("Deleting fallback entry")
-                                       if err := os.RemoveAll(entry.Path); err != nil {
-                                               logrus.WithError(err).WithFields(logrus.Fields{
-                                                       "path": entry.Path,
-                                               }).Errorf("Cannot delete directory. Skipping...")
+                                       if !dryRun {
+                                               if err := os.RemoveAll(entry.Path); err != nil {
+                                                       logrus.WithError(err).WithFields(logrus.Fields{
+                                                               "path": entry.Path,
+                                                       }).Errorf("Cannot delete directory. Skipping...")
+                                               }
                                        }
                                }
                        } else {
                                if pruneUnreferenced {
                                        // delete the unreferenced cached entry
                                        logrus.WithFields(entryFields).Infof("Deleting unreferenced entry")
-                                       if err := os.RemoveAll(entry.Path); err != nil {
-                                               logrus.WithError(err).WithFields(logrus.Fields{
-                                                       "path": entry.Path,
-                                               }).Errorf("Cannot delete directory. Skipping...")
+                                       if !dryRun {
+                                               if err := os.RemoveAll(entry.Path); err != nil {
+                                                       logrus.WithError(err).WithFields(logrus.Fields{
+                                                               "path": entry.Path,
+                                                       }).Errorf("Cannot delete directory. Skipping...")
+                                               }
                                        }
                                }
                        }
@@ -96,6 +105,9 @@ func pruneAction(cmd *cobra.Command, args []string) error {
        }
        cacheDir := filepath.Join(ucd, "lima")
        logrus.Infof("Pruning everything in %q", cacheDir)
+       if dryRun {
+               return nil
+       }
        return os.RemoveAll(cacheDir)
 }

@AkihiroSuda
Copy link
Member

Shall we merge this and discuss dry-run in another issue/PR?

@jay7x
Copy link
Author

jay7x commented May 25, 2023

If there is no release expected in 2 next days I'd like to add dry run because I like the idea. I'll add it tomorrow.

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

  • Downloads referenced by a template are dropped from the PR scope

I somehow missed this. What is the reason for that?

I just noticed that when I deleted all my current instances, then limactl prune --unreferenced-only will delete everything in the cache (including the current nerdctl-full archives).

That seems like a major breakage.

If this is the functionality you want, then you should add a different option name for it. Like --all-except-currently-in-use (but a better name).

@jandubois
Copy link
Member

Just for illustration, assuming my cache looks like this:

653M	https://cloud-images.ubuntu.com/releases/22.04/release-20230302/ubuntu-22.04-server-cloudimg-amd64.img
654M	https://cloud-images.ubuntu.com/releases/22.04/release-20230518/ubuntu-22.04-server-cloudimg-amd64.img
727M	https://cloud-images.ubuntu.com/releases/22.10/release-20230413/ubuntu-22.10-server-cloudimg-amd64.img
779M	https://cloud-images.ubuntu.com/releases/23.04/release-20230502/ubuntu-23.04-server-cloudimg-amd64.img
233M	https://github.com/containerd/nerdctl/releases/download/v1.3.0/nerdctl-full-1.3.0-linux-amd64.tar.gz
233M	https://github.com/containerd/nerdctl/releases/download/v1.3.1/nerdctl-full-1.3.1-linux-amd64.tar.gz
238M	https://github.com/containerd/nerdctl/releases/download/v1.4.0/nerdctl-full-1.4.0-linux-amd64.tar.gz
 66M	https://github.com/lima-vm/alpine-lima/releases/download/v0.2.28/alpine-lima-std-3.18.0-x86_64.iso
 66M	https://github.com/lima-vm/alpine-lima/releases/download/v0.2.30/alpine-lima-std-3.18.0-x86_64.iso

I would expect limactl prune --unreferenced-only to leave these files behind, even if I delete all my current instances:

727M	https://cloud-images.ubuntu.com/releases/22.10/release-20230413/ubuntu-22.10-server-cloudimg-amd64.img
238M	https://github.com/containerd/nerdctl/releases/download/v1.4.0/nerdctl-full-1.4.0-linux-amd64.tar.gz
 66M	https://github.com/lima-vm/alpine-lima/releases/download/v0.2.30/alpine-lima-std-3.18.0-x86_64.iso

I guess for backwards compatibility we need to keep the existing behaviour, but I feel like we should require an --all option to wipe the whole cache, and have something safer, like --unreferenced-only the default.

@jandubois
Copy link
Member

I'm running out of time again, but I also think the --no-digest-only implementation doesn't work:

$ ls -l ~/Library/Caches/lima/download/by-url-sha256/791040918a29bb23fd0bc4bd074e32891299ffcdc0d0d20cd76a4af9593b0fa4
total 1503112
-rw-r--r--  1 jan  staff  769589248 25 May 22:59 data
-rw-r--r--  1 jan  staff         93 25 May 22:57 url
$ limactl ls
WARN[0000] No instance found. Run `limactl start` to create an instance.
$ limactl prune --dry-run --no-digest-only
[no output at all]

Maybe I made a mistake, will need to take a look again on the weekend.

@AkihiroSuda
Copy link
Member

Let me move this to v0.17

@AkihiroSuda AkihiroSuda modified the milestones: v0.16.0, v0.17.0 May 26, 2023
This commit implements 2 more CLI flags:
* --no-digest-only
  Only prune images without a digest specified ("fallback" images usually)
* --unreferenced-only
  Only prune downloads not referenced in any VM

Signed-off-by: Yury Bushmelev <[email protected]>
@jay7x
Copy link
Author

jay7x commented May 26, 2023

  • Downloads referenced by a template are dropped from the PR scope

I somehow missed this. What is the reason for that?

The reason was to remove the point of the conflict and merge things which everyone is ok with (except @AkihiroSuda maybe 😄 )

I just noticed that when I deleted all my current instances, then limactl prune --unreferenced-only will delete everything in the cache (including the current nerdctl-full archives).

This is expected as there is nothing referencing the downloads. Which is the default behavior of lima prune before this PR.

@jay7x
Copy link
Author

jay7x commented May 26, 2023

I'm running out of time again, but I also think the --no-digest-only implementation doesn't work:

Well.. you're right :( I just found it's broken.. I've over-optimized things a bit.. let me fix it

@AkihiroSuda
Copy link
Member

I'm running out of time again, but I also think the --no-digest-only implementation doesn't work:

Well.. you're right :( I just found it's broken.. I've over-optimized things a bit.. let me fix it

Do you plan to update this PR?

@jay7x
Copy link
Author

jay7x commented Jun 23, 2023

I'm running out of time again, but I also think the --no-digest-only implementation doesn't work:

Well.. you're right :( I just found it's broken.. I've over-optimized things a bit.. let me fix it

Do you plan to update this PR?

Yes, I'll continue to work on this later. @jandubois created pretty good definition of the process in #1409. My plan is to follow it. Though this requires some thinking.. Let me change the PR to draft again.

@jay7x jay7x marked this pull request as draft June 23, 2023 06:27
@AkihiroSuda AkihiroSuda modified the milestones: v0.17.0, v0.18.0 Jul 30, 2023
@AkihiroSuda AkihiroSuda modified the milestones: v0.17.1, v0.18.0, v0.17.2 Aug 11, 2023
@AkihiroSuda AkihiroSuda removed this from the v0.18.0 milestone Sep 30, 2023
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.

4 participants