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

Cache rework Stage I #5088

Merged
merged 25 commits into from
Mar 6, 2020
Merged

Cache rework Stage I #5088

merged 25 commits into from
Mar 6, 2020

Conversation

dtrudg
Copy link
Contributor

@dtrudg dtrudg commented Mar 3, 2020

This is the 1st step toward reworking the cache / image source client code to reduce duplication and make operations that fetch things into the cache safe where multiple operations are run concurrently on a file system that supports atomic rename.

Fixes: #4015
Fixes: #5020

This PR does...

  • Move pkg/internal/client/cache to pkg/internal/cache as it's used by the clients, but also other code, and is not a client itself.
  • Move all the image source client code (oras, shub, net, etc.) to pkg/internal/client/*
  • Make downloads into the cache happen to a temporary file, which is then renamed, for safety with concurrent runs.
  • Remove duplication of image client & cache handling across pull, actions and in build/sources
  • Ensures all sources consistently use the cache across pull actions build - prior to this some things like net weren't caching in some of these.
  • Add a common ProgressBar callback in sylog, that respects --quiet and is used across the client code.

Future work

This PR is getting rather large, so instead of making it ever bigger I'd like to put up additional PRs over the next week to address the following:

  • The pkg/image/client/xxx code is just a collection of functions per client, mostly duplicated and named consistently, but with varying and long function sigs. Refactor so there is a common client.Config struct, and specific clients implement a common client.Puller and client.Pusher interfaces.

  • REGRESSION - CTRL-C handlers have been stripped out. They need to be added back in, but using contexts with cancellation so that actions to cleanup happen close to the code creating files.

  • The cache list and cache clean code needs to be properly refactored so that things directly acting on cache directories are in pkg/internal/cache and not throughout the CLI code.

@dtrudg dtrudg self-assigned this Mar 3, 2020
@dtrudg dtrudg added the ci:e2e label Mar 4, 2020
@dtrudg dtrudg changed the title WIP - Cache rework WIP - Cache rework Stage I Mar 5, 2020
@dtrudg dtrudg marked this pull request as ready for review March 5, 2020 21:06
@dtrudg dtrudg changed the title WIP - Cache rework Stage I Cache rework Stage I Mar 5, 2020
@dtrudg dtrudg requested review from ikaneshiro and cclerget March 5, 2020 21:06
@dtrudg dtrudg added this to the 3.6.0 milestone Mar 5, 2020
Copy link
Collaborator

@cclerget cclerget left a comment

Choose a reason for hiding this comment

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

Overall this is a nice cleanup 👍

}
if foundMatch {
matches++
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it something to remove or pending for the next PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to return this when I refactor the clean and list stuff so that the operations are in the cache code, not CLI, in a following cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make an issue so this is note missed - https://github.com/sylabs/singularity/issues/5095

internal/app/singularity/cache_list_linux.go Outdated Show resolved Hide resolved
internal/pkg/sylog/sylog.go Outdated Show resolved Hide resolved
@dtrudg dtrudg requested a review from cclerget March 6, 2020 15:08
Copy link
Collaborator

@cclerget cclerget left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Concurrent pullls fail (race condition?) Singularity pull doesn't respect cache
2 participants