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

support local cache exporter and importer #615

Merged
merged 1 commit into from
Jan 18, 2019

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Sep 11, 2018

  • Local exporter/importer: Support index.json
  • CLI: Support specifying multiple cache exporters/importers
  • Solver: define build caps (do we need caps?)
  • *: add tests

Impact on CLI

  • Old (deprecated but still effective): --export-cache localhost:5000/myrepo:buildcache --export-cache-opt mode=max
  • New: --export-cache type=registry,ref=localhost:5000/myrepo:buildcache,mode=max

Impact on API

  • New fields are added to control.proto and gateway.proto. The daemon
    internally translates old API calls to the new ones.
  • While new API can be used for registry caches, the client continues to use the legacy API for registry caches to ensure compatibility with old daemons.
  • To import local caches with a frontend, the frontend needs to support a new frontend opt cache-imports.

Usage

To/From registry

buildctl build ... --export-cache type=registry,ref=localhost:5000/myrepo:buildcache
buildctl build ... --import-cache type=registry,ref=localhost:5000/myrepo:buildcache

To/From local filesystem

buildctl build ... --export-cache type=local,store=path/to/output-dir
buildctl build ... --import-cache type=local,store=path/to/input-dir

@tonistiigi
Copy link
Member

With the current solution if we don't use specific refs we need to pull all the cache metadata every time (a big json structure). I'm not sure if this could cause scaling issues here yet but if we would switch the backend from files to some highly available cache service that is exposed to multiple users we would probably need a solution for that. I'm ok with making cache.Query async for that in solver but we should figure out some protocol for how these requests are made to remote parties. Again, not that important for the file-backed sources unless we want to put this data in a boltdb/sqlite on the client side(and we probably don't want to do that).

@tonistiigi
Copy link
Member

@AkihiroSuda What are the next steps for this?

@AkihiroSuda
Copy link
Member Author

  • Convention for the name of lock file for index.json. Maybe just index.json.lock.

  • Syntax for specifying "type" ("registry" and "local". In future, we can add "containerd", "another-buildkitd-instance",...). My suggestion is to use "CSV" style.

  • Syntax for exposting without tag (even without "latest"). My suggestion is to use CSV styel for this syntax as well. e.g.
    --export-cache ref=localhost:5000/foo/bar,type=registry,no-index

@AkihiroSuda
Copy link
Member Author

Any thought about CSV?

@tonistiigi
Copy link
Member

Not strictly against csv but seems incompatible with the other flags of buildctl eg. frontend-, exporter-. Or are you suggesting to change these flags as well?

@AkihiroSuda
Copy link
Member Author

Can we deprecate --export-cache and introduce --cache-exporter for consistency with --exporter and --frontend?

e.g.
--cache-exporter registry --cache-exporter-opt name=localhost:5000/foo/bar (for consistency with --exporter, maybe s/registry/image/ ? not sure..)

--cache-exporter local --cache-exporter-opt output=path/to/output-dir

@tonistiigi
Copy link
Member

Can we deprecate --export-cache and introduce --cache-exporter for consistency with --exporter and --frontend?

Yes, shouldn't be a problem.

@AkihiroSuda
Copy link
Member Author

Sorry for delay, I'll try to work on next month

@AkihiroSuda AkihiroSuda force-pushed the export-cache-local branch 7 times, most recently from d84ea44 to 793fe1a Compare January 2, 2019 19:53
@AkihiroSuda AkihiroSuda added this to the v0.4.0 milestone Jan 2, 2019
@AkihiroSuda AkihiroSuda force-pushed the export-cache-local branch 5 times, most recently from e3d533f to f59455f Compare January 2, 2019 22:31
@AkihiroSuda
Copy link
Member Author

Implementation is WIP but CLI and API are ready to review.

@AkihiroSuda AkihiroSuda force-pushed the export-cache-local branch 2 times, most recently from 77c8143 to f1615d8 Compare January 2, 2019 23:54
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@AkihiroSuda AkihiroSuda force-pushed the export-cache-local branch 2 times, most recently from 1d1684b to 1b87099 Compare January 6, 2019 06:56
@AkihiroSuda AkihiroSuda force-pushed the export-cache-local branch 3 times, most recently from 51912eb to a49a1a5 Compare January 6, 2019 13:11
@AkihiroSuda AkihiroSuda changed the title [WIP/DNM/RFC] support local cache exporter and importer support local cache exporter and importer Jan 6, 2019
@AkihiroSuda
Copy link
Member Author

updated PR with support for index.json

@AkihiroSuda AkihiroSuda force-pushed the export-cache-local branch 2 times, most recently from c45cf8d to 1fea4ae Compare January 7, 2019 02:12
@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Jan 7, 2019

Updated: now session/content can aggregate map[string]content.Store with gRPC metadata string. i.e. --import-cache and --export-cache can refer to different directories

api/services/control/control.proto Outdated Show resolved Hide resolved
api/services/control/control.proto Outdated Show resolved Hide resolved
client/graph.go Outdated Show resolved Hide resolved
client/ociindex/ociindex.go Outdated Show resolved Hide resolved
client/solve.go Outdated Show resolved Hide resolved
frontend/gateway/pb/gateway.proto Outdated Show resolved Hide resolved
frontend/gateway/pb/gateway.proto Show resolved Hide resolved
@@ -1156,14 +1157,8 @@ func testBuildPushAndValidate(t *testing.T, sb integration.Sandbox) {
require.False(t, ok)
}

func testBasicCacheImportExport(t *testing.T, sb integration.Sandbox) {
func testBasicCacheImportExport(t *testing.T, sb integration.Sandbox, cacheOptionsEntry CacheOptionsEntry) {
Copy link
Member

Choose a reason for hiding this comment

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

possible follow-up: we need to update this test(or add a new one) to include local sources. Currently not testing a lot of the exporter/cachekeys code.

* `mode=min` (default): only export layers for the resulting image
* `mode=max`: export all the layers of all intermediate steps
* `ref=docker.io/user/image:tag`: reference for `registry` cache exporter
* `store=path/to/output-dir`: directory for `local` cache exporter
Copy link
Member

Choose a reason for hiding this comment

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

why isn't tag (or name) a supported import/export option for local?

Copy link
Member Author

Choose a reason for hiding this comment

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

can be follow-up?

README.md Outdated Show resolved Hide resolved
@AkihiroSuda AkihiroSuda force-pushed the export-cache-local branch 3 times, most recently from 32a3b1a to aad7b22 Compare January 15, 2019 11:51
@AkihiroSuda
Copy link
Member Author

updated

Export:

  $ buildctl build ... --export-cache type=local,store=/path/to/output-dir

Import:

  $ buildctl build ... --import-cache type=local,store=/path/to/input-dir

Impact on CLI:
* Old (deprecated but still effective): `--export-cache localhost:5000/myrepo:buildcache --export-cache-opt mode=max`
* New: `--export-cache type=registry,ref=localhost:5000/myrepo:buildcache,mode=max`

Impact on API:
* New fields are added to control.proto and gateway.proto. The daemon
internally translates old API calls to the new ones.
* While new API can be used for `registry` caches, the client continues
to use the legacy API for `registry` caches to ensure compatibility with
old daemons.
* To import `local` caches with a frontend, the frontend needs to support
a new frontend opt `cache-imports`.

Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda
Copy link
Member Author

updated

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