Honor RESTIC_CACHE_DIR environment variable

[kitone]

What does this PR change? What problem does it solve?

CacheDir option doesn't honor the RESTIC_CACHE_DIR environment variable by default, this is done by default now.
This avoid checking for gopts.CacheDir and RESTIC_CACHE_DIR environment variable.

Was the change previously discussed in an issue or on the forum?

Fix #3382: restic check doesn't obey the RESTIC_CACHE_DIR environment variable

Checklist

• I have read the contribution guidelines.
• I have enabled maintainer edits.
• I have added tests for all code changes.
• I have added documentation for relevant changes (in the manual).
• There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
• I have run gofmt on the code in all commits.
• All commit messages are formatted in the same style as the other commits in the repo.
• I'm done! This pull request is ready for review.

[MichaelEischer]

When placing the temporary check directories inside the regular restic cache directory, then no there's no guarantee that a cleanup of these directories will occur.

The problem is that the current cache cleanup mechanism explicitly ignores all directories which don't look like a repository id. Changing that would require a change in the following location or somewhere nearby:

restic/internal/cache/cache.go

Line 174 in 553ea36

func validCacheDirName(s string) bool {

[kitone]

@MichaelEischer Thank you for your quick feedback and guidance. I have updated the pull request.

If cache.DefaultDir return an error, should we only continue to fall back with TempDir or disable the cache with warning ?

When placing the temporary check directories inside the regular restic cache directory, then no there's no guarantee that a cleanup of these directories will occur.

The problem is that the current cache cleanup mechanism explicitly ignores all directories which don't look like a repository id. Changing that would require a change in the following location or somewhere nearby:

restic/internal/cache/cache.go

Line 174 in 553ea36

func validCacheDirName(s string) bool {

The temp directory restic-check-cache-* create a new subfolder cache dir with 64 character folder name and CACHEDIR.TAG file, changing the regex to accept this folder name will list the folder but not the content.
cache.OlderThan do not list recursively.

What should be the behaviour of cache --cleanup ? Always delete this folder ?

Best regards,

[MichaelEischer]

If cache.DefaultDir return an error, should we only continue to fall back with TempDir or disable the cache with warning ?

My reference to DefaultDir was only intended to illustrate what the other parts of restic, but wasn't meant to refer to the chagnes necessary here, sorry for the confusion. I'd like to have a new helper function next to DefaultDir which only queries the environment variable and does not fall back to os.UserCacheDir(). With the current change in the PR, the temporary cache directory would always be placed in the regular cache directory unless the environment variable was changed. But the PR should only change the behavior when the environment variable is specified and keep the old behavior in all other cases.

The temp directory restic-check-cache-* create a new subfolder cache dir with 64 character folder name and CACHEDIR.TAG file, changing the regex to accept this folder name will list the folder but not the content.
cache.OlderThan do not list recursively.

What should be the behaviour of cache --cleanup ? Always delete this folder ?

I don't think that there's a reason to not just delete the whole restic-check-cache-* folder. Each check run should use its own temporary cache folder.

[MichaelEischer]

@kitone Did you make any progress on this PR?

[kitone]

@MichaelEischer: Sorry for the delay, I was busy with work and forget to commit my change.

So, the previous behaviour is now respected, we only change the cache directory if --cache-dir is setup or RESTIC_CACHE_DIR.

For feedbacks, I added another commit that change the behaviour of cache --cleanup to manage restic-check-cache-*. in case restic check doesn't remove the tempdir.
(this can be easly done by removing the code in this function)

cleanup = func() {
  err := fs.RemoveAll(tempdir)
  if err != nil {
    Warnf("error removing temporary cache directory: %v\n", err)
  }
}

Changing validCacheDirName with allow to list the restic-check-cache-*. but cleanup will remove the wrong caches and keep the last.

Repo ID     Last Used   Old  Size
----------------------------------------
restic-che  0 days ago       157.792 KiB
360f0d7b0d  0 days ago       162.315 KiB
restic-che  0 days ago       162.358 KiB
----------------------------------------

For now, restic cache will not show restic-check-cache-* caches, but with --cleanup, it will remove regular cache dirs, and then search for restic-check-cache-* names and remove them too.

Thanks for the feedbacks

Best regards,

[MichaelEischer]

Changing validCacheDirName with allow to list the restic-check-cache-*. but cleanup will remove the wrong caches and keep the last.

I'm not sure I understand. Which caches would cleanup remove which it shouldn't have?

I'd still prefer to just extend the cache name regexp and maybe filter the check caches from the output. That would also have the benefit of allowing automatic cleanups by other restic commands using --cleanup-cache.

For now, restic cache will not show restic-check-cache-* caches, but with --cleanup, it will remove regular cache dirs, and then search for restic-check-cache-* names and remove them too.

The code currently deletes every check cache it encounters even if it was created recently. That way it could actually delete a cache currently used by running check operation. Is there are reason not to use the same "x days old" logic as for normal cache directories?

[kitone]

Changing validCacheDirName with allow to list the restic-check-cache-*. but cleanup will remove the wrong caches and keep the last.

I'm not sure I understand. Which caches would cleanup remove which it shouldn't have?

I'd still prefer to just extend the cache name regexp and maybe filter the check caches from the output. That would also have the benefit of allowing automatic cleanups by other restic commands using --cleanup-cache.

For now, restic cache will not show restic-check-cache-* caches, but with --cleanup, it will remove regular cache dirs, and then search for restic-check-cache-* names and remove them too.

The code currently deletes every check cache it encounters even if it was created recently. That way it could actually delete a cache currently used by running check operation. Is there are reason not to use the same "x days old" logic as for normal cache directories?

I'm really sorry, I didn't fully understand the cache cleaning behavior, I just re-read the code and do some test and indeed this behavior is the correct one. I have updated the pr.

I chose to display all the caches, in order to allow the user to have the information and thus choose if he should run a cleanup.

Maybe we can filter the name to have a more explicit listing ?

Repo ID     Last Used    Old  Size
-----------------------------------------
restic-che  90 days ago  yes   95.066 KiB
d82a4e2897  0 days ago         95.023 KiB
-----------------------------------------

by

Repo ID                       Last Used    Old  Size
-----------------------------------------------------------
restic-check-cache-037381024  90 days ago  yes   95.066 KiB
d82a4e2897                    0 days ago         95.023 KiB
-----------------------------------------------------------

or

Repo ID                       Last Used    Old  Size
-----------------------------------------------------------
037381024 (check)  90 days ago  yes   95.066 KiB
d82a4e2897         0 days ago         95.023 KiB
-----------------------------------------------------------

Thanks again for your feedbacks,

[MichaelEischer]

Repo ID                       Last Used    Old  Size
-----------------------------------------------------------
restic-check-cache-037381024  90 days ago  yes   95.066 KiB
d82a4e2897                    0 days ago         95.023 KiB
-----------------------------------------------------------

I like that variant. The number after restic-check-cache is not a repository id, so just displaying that number would be misleading.

restic/cmd/restic/cmd_cache.go

Line 144 in 6c84ea1

entry.Name()[:10],

It should be sufficient to not shorten the repo ID when it starts with restic-check-cache-.

[kitone]

Change added in the last commit.

Best regards,

[MichaelEischer]

LGTM. Thanks!