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

Image registry cache #669

Conversation

modulo11
Copy link
Contributor

This implements parts of #652 and additionally offers images to be used as caches. The current default behavior is not changed but some parts are renamed for better clarification.

Currently, it's an either volume or registry cache although supporting both (as depicted in #652) can be achieved easily. Using both caching arguments (-cache-dir and -cache-image) with the CNB lifecycle resulted in only utilizing the cache image (during my experiments).

@c0d1ngm0nk3y
@loewenstein

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2021

Codecov Report

Merging #669 (e1e91de) into main (33c4d25) will increase coverage by 0.11%.
The diff coverage is 66.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #669      +/-   ##
==========================================
+ Coverage   71.24%   71.35%   +0.11%     
==========================================
  Files         117      118       +1     
  Lines        5421     5439      +18     
==========================================
+ Hits         3862     3881      +19     
+ Misses       1191     1187       -4     
- Partials      368      371       +3     
Impacted Files Coverage Δ
pkg/apis/build/v1alpha2/build.go 35.71% <ø> (+7.30%) ⬆️
pkg/apis/build/v1alpha2/image_types.go 50.00% <ø> (ø)
pkg/cnb/cnb_metadata.go 46.29% <0.00%> (-14.68%) ⬇️
pkg/reconciler/build/build.go 72.80% <50.00%> (-0.76%) ⬇️
pkg/apis/build/v1alpha2/image_builds.go 60.62% <64.28%> (+1.30%) ⬆️
pkg/apis/build/v1alpha2/build_pod.go 97.92% <100.00%> (+<0.01%) ⬆️
pkg/apis/build/v1alpha2/build_types.go 100.00% <100.00%> (ø)
pkg/apis/build/v1alpha2/image_validation.go 97.84% <100.00%> (+0.09%) ⬆️
pkg/reconciler/image/image.go 50.99% <100.00%> (ø)
pkg/reconciler/image/reconcile_build.go 88.76% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33c4d25...e1e91de. Read the comment docs.

@@ -69,12 +69,17 @@ type BuildSpec struct {
Notary *NotaryConfig `json:"notary,omitempty"`
}

type BuildCacheConfig struct {
VolumeName string `json:"volumeName,omitempty"`
ImageTag string `json:"imageTag,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the spirit of reproducibility should we provide an image with a fully qualified digest here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We think no: If "someone" pushed an update to the cache we'd like to always use the most current version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The current image reference that is provided to the analyze step is the fully qualified built image reference from the last build. This allows deterministic builds that will not run into outside interference if something else is writing to the same tag. I think the same thing applies here, another image could write to the provided cache image tag and we would want to ensure we are using the exact cache digest from the last build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Should be implemented with 6b3e580 and 177e139. I'll also add some test for that. WDYT?

@matthewmcnew
Copy link
Collaborator

Thanks for submitting this @modulo11! It will be a breaking change to the image (and build) api so we need to provide it in a new api version. This PR for service bindings introduces the v1alpha2 api. I think it would make sense to include this api update alongside that api update.

@modulo11
Copy link
Contributor Author

Hi @matthewmcnew thanks for the review. We tried bumping the api from v1alpha1 to v1alpha2 for just Build and Image types analogous to PR #555. We gave up in the middle because we we're overwhelmed by subtle changes which are easily overlooked when mixing both versions in the code. For us it would be more feasible to bump all api objects to v1alpha2 and update all references.

We use a latest api package (referencing the current api object versions) in our own controllers, so bumping all versions can be done in one place. We can supply a (draft) PR if you're interested.

WDYT?

@matthewmcnew
Copy link
Collaborator

I'm not sure of the best path forward as implementing a new api version will be quite involved. Likely we should merge our PR for service bindings that will introduce a new api version and then allow you to provide your PR on top of that so you don't need to manage adding a new api version.

@matthewmcnew
Copy link
Collaborator

To help me better understand the desired timelines could you tell me a bit more about your desired usecase for this functionality?

@c0d1ngm0nk3y
Copy link
Contributor

I'm not sure of the best path forward as implementing a new api version will be quite involved.

But we have doubts that this is the best way forward. The #555 shows the problems with the imports:

´´´
"github.com/pivotal/kpack/pkg/apis/build/v1alpha1"
"github.com/pivotal/kpack/pkg/apis/build/v1alpha2"
´´´

So you have to be very careful which api object take from which import ( ´Build´ -> v1alpha2, but ´SourceResolver` -> v1alpha1). Those errors can be quite subtle.

When bumping the ´build´ api as a whole, we could do something like

´´´
build "github.com/pivotal/kpack/pkg/apis/build/v1alpha2"
´´´

and then changing the api version is changing only the import in each relevant file.

@matthewmcnew
Copy link
Collaborator

matthewmcnew commented Apr 21, 2021

Yeah, we should introduce an entire new api version for all resources at v1alpha2

@modulo11 modulo11 force-pushed the image-registry-cache branch from bfb4704 to 177e139 Compare April 22, 2021 14:31
@modulo11 modulo11 requested a review from matthewmcnew April 22, 2021 14:31
@modulo11
Copy link
Contributor Author

Alright, then we'll put that PR on hold and continue after #555 is merged.

@c0d1ngm0nk3y
Copy link
Contributor

Yeah, we should introduce an entire new api version for all resources at v1alpha2

But then, there would be no real benefit waiting for #555, right? v1alpha2 can be changed again until it is released.

@matthewmcnew
Copy link
Collaborator

But then, there would be no real benefit waiting for #555, right? v1alpha2 can be changed again until it is released.

I'm not quite sure what you mean by this @c0d1ngm0nk3y. Are you suggesting that we create a different PR to bump to v1alpha?

@c0d1ngm0nk3y
Copy link
Contributor

I'm not quite sure what you mean by this @c0d1ngm0nk3y. Are you suggesting that we create a different PR to bump to v1alpha?

Sorry, yes. Our idea was to create a different PR bumping to v1alpha2. And then both PRs could rebase on that and focus only on the relevant parts. We have started with some documentation and renaming to make the bumping not so "involved".

@c0d1ngm0nk3y
Copy link
Contributor

@matthewmcnew Maybe you can have a look at #675 - we would propose to use this as a first step before actually bumping the api. We could then add another PR bumping the api to v1alpha2, but not changing anything. #555 and #669 could then base on that and only adapt what is really relevant. WDYT?

@matthewmcnew
Copy link
Collaborator

Hi, @modulo11 & @c0d1ngm0nk3y. Thanks for your patience on this! Our current goal is to provide our planned v0.3 release of kpack in the next few weeks with the underlying git library moved to use git2go libgit2 bindings as a fix to our long running azure devops woes. Migrating/building kpack images to use the libgit2 has been a major hurdle so our plan is to release this before releasing any other major kpack updates.

We would like to target the release of the new api version in a short follow-up v0.4 release. I think we are going to wait on merging this an other changes to the api until after the v0.3 release is released.

build-webhook.sh Outdated

set -e

pack build europe-west3-docker.pkg.dev/sap-se-gcp-istio-dev/public/kpack-667/webhook --builder gcr.io/cf-build-service-public/ci/tiny-builder -e BP_GO_TARGETS=./cmd/controller --publish --trust-builder
Copy link
Contributor

@tylerphelan tylerphelan Aug 17, 2021

Choose a reason for hiding this comment

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

could the the tag argument to pack be parameterized in this script? also the pack args for the controller have changed since this pr to

-B gcr.io/cf-build-service-public/ci/kpack-builder -e BP_GO_TARGETS=./cmd/controller -e BP_BUILD_LIBGIT2=true -e BP_GO_BUILD_FLAGS="-tags=\"static\""  --publish

You could also just remove the scripts if you don't want to update them.

docs/build.md Outdated
@@ -19,7 +19,8 @@ spec:
image: gcr.io/paketo-buildpacks/builder:base
imagePullSecrets:
- name: builder-secret
cacheName: persisent-volume-claim-name
cache:
volumeName: persisent-volume-claim-name
Copy link
Contributor

Choose a reason for hiding this comment

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

I think pvcName is more accurate here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should match the long name in VolumeSource and use the long name persistentVolumeClaimName?

@@ -641,14 +652,14 @@ func (b *Build) rebasePod(secrets []corev1.Secret, images BuildPodImages, config
}

func (b *Build) cacheVolume(os string) []corev1.Volume {
if b.Spec.CacheName == "" || os == "windows" {
if b.Spec.Cache.VolumeName == "" || os == "windows" {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to protect from .Cache being nil here?

@@ -262,9 +279,9 @@ func testImageValidation(t *testing.T, when spec.G, it spec.S) {
it("image.cacheSize has not decreased", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

might be nice to update the text here

Copy link
Contributor

Choose a reason for hiding this comment

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

@tylerphelan Which text? 😕 Sorry...

Copy link
Contributor

Choose a reason for hiding this comment

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

We got it :) Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually meant the test text: it("image.cacheSize has not decreased", func() { -> it("image.cache.volume.size has not decreased", func() { 😀

docs/image.md Outdated
@@ -9,7 +9,9 @@ The following defines the relevant fields of the `image` resource spec in more d
- `builder`: Configuration of the `builder` resource the image builds will use. See more info [Builder Configuration](builders.md).
- `serviceAccount`: The Service Account name that will be used for credential lookup.
- `source`: The source code that will be monitored/built into images. See the [Source Configuration](#source-config) section below.
- `cacheSize`: The size of the Volume Claim that will be used by the build cache.
- `cache`: Caching configuration, two variants are available:
- `volume.request`: Creates a Volume Claim of the given size
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be volume.size ? request seems less user-friendly. anyone else have thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. I like how request matches the k8s terminology for PVCs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this actually be requests and be a RequestsList?

Copy link
Contributor

@dumez-k dumez-k Aug 23, 2021

Choose a reason for hiding this comment

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

Latest iteration:

- `cache`: Caching configuration, two variants are available:
  - `volume.size`: Creates a Volume Claim of the given size
  - `registry.imageTag`: Creates an image with cached contents

Copy link
Contributor

@tylerphelan tylerphelan left a comment

Choose a reason for hiding this comment

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

very nice pr, I had just a few notes

@c0d1ngm0nk3y
Copy link
Contributor

very nice pr, I had just a few notes

Thanks, we will have a look. Probably after #670 is merged because they are in conflict anyway.

Image string `json:"image,omitempty"`
StackId string `json:"stackId,omitempty"`
Image string `json:"image,omitempty"`
CacheImage string `json:"cacheImage,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

To this point we have tried to keep every reference in the api to an image to use the image key. Could we move this to cache.image?

build-webhook.sh Outdated
@@ -0,0 +1,9 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this file really be part of this PR? That is just our local "hack" to not build everything, right? If, it should be done for all components I guess. Maybe even in the Makefile?

build.sh Outdated
@@ -0,0 +1,9 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also some local "hack" and should not be part of this PR I guess.

@@ -19,7 +19,8 @@ spec:
image: gcr.io/paketo-buildpacks/builder:base
imagePullSecrets:
- name: builder-secret
cacheName: persisent-volume-claim-name
cache:
Copy link
Contributor

Choose a reason for hiding this comment

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

  • This text above should also reflect the new cache option
  • v1alpha1 is outdated (line 10)

Is this some kind of example? Should we mention the other cache possibility?

@@ -164,17 +163,28 @@ func (b *Build) BuildPod(images BuildPodImages, secrets []corev1.Secret, taints
}

var cacheArgs []string
Copy link
Contributor

Choose a reason for hiding this comment

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

The cacheArgs are not needed in this scope.

} else {
cacheArgs = []string{"-cache-dir=/cache"}
exportCacheArgs = cacheArgs
Copy link
Contributor

Choose a reason for hiding this comment

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

If we fix the scope, this is not needed anymore.

@@ -45,8 +45,12 @@ func (i *Image) SetDefaults(ctx context.Context) {
i.Spec.SuccessBuildHistoryLimit = &defaultSuccessfulBuildHistoryLimit
}

if i.Spec.CacheSize == nil && ctx.Value(HasDefaultStorageClass) != nil {
i.Spec.CacheSize = &defaultCacheSize
if i.Spec.Cache == nil && ctx.Value(HasDefaultStorageClass) != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? Should we check for i.Spec.Cache.Request != nil as well? What if it uses registryCache?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is right!

if is.CacheSize != nil && ctx.Value(HasDefaultStorageClass) == nil {
return apis.ErrGeneric("spec.cacheSize cannot be set with no default StorageClass")
func (is *ImageSpec) validateVolumeCache(ctx context.Context) *apis.FieldError {
if is.Cache.Volume != nil && ctx.Value(HasDefaultStorageClass) == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is with is.Cache != nil?

Copy link
Contributor

Choose a reason for hiding this comment

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

with the current implementation it will get defaulted, so I think it's ok

@@ -38,7 +38,11 @@ func testImageValidation(t *testing.T, when spec.G, it spec.S) {
Revision: "master",
},
},
CacheSize: &cacheSize,
Cache: &CacheConfig{
Copy link
Contributor

Choose a reason for hiding this comment

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

What is with an empty volumeCache?

@@ -262,9 +279,9 @@ func testImageValidation(t *testing.T, when spec.G, it spec.S) {
it("image.cacheSize has not decreased", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@tylerphelan Which text? 😕 Sorry...

@@ -713,6 +717,7 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) {
Image: builder.Status.LatestImage,
},
ServiceAccount: image.Spec.ServiceAccount,
Cache: &buildapi.BuildCacheConfig{},
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't nil be valid as well?


imageName := fmt.Sprintf("%s-%s", "test-git-image", "cluster-builder")
imageName := fmt.Sprintf("%s-%s", "test-git-image", "cluster-builder")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is worth to use different imageNames? Since this generateRebuild is called twice (for each cache).

@c0d1ngm0nk3y
Copy link
Contributor

@tylerphelan @matthewmcnew We think we have addressed all feedback. Except the part with "cache image lost on failure" which we didn't understand 100% to be honest. Could you have another look?

@tylerphelan
Copy link
Contributor

Nice, all of my comments have been addressed. I had one more thought, what do others think about adding validation ensuring only one of image.spec.cache.volume and image.spec.cache.registry is set? Right now the registry will be used by the lifecycle and in the future multiple cache types may be possible, but it might be nice to validate this for now.

@c0d1ngm0nk3y
Copy link
Contributor

I had one more thought, what do others think about adding validation ensuring only one of image.spec.cache.volume and image.spec.cache.registry is set?

Good point. We have added validation to Build and Image that only one type of cache can be used.

@c0d1ngm0nk3y
Copy link
Contributor

@matthewmcnew @tylerphelan We have rebased and squashed this pr and from our point of view we are good to go. Please check and let us know if there is still something else to do.

}

type RegistryCache struct {
ImageTag string `json:"imageTag"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make this tag to match the other instance of tags in the api?

@matthewmcnew
Copy link
Collaborator

I think we are essentially ready to Merge! (Thank you so much for your patience on this)

One last request, could we update spec.cache.registry.imageTag to spec.cache.registry.tag?

@c0d1ngm0nk3y
Copy link
Contributor

I think we are essentially ready to Merge

Sounds great :)

One last request, could we update spec.cache.registry.imageTag to spec.cache.registry.tag?

Ah. It was called spec.cache.registry.tag in an earlier state of the pr. We have changed it to spec.cache.registry.imageTag because we thought that is a bit clearer. tag sounded like it is only the tag that will be applied to the cache image. But it is the whole image name.

But I have no strong opinion on that. We can change it back to spec.cache.registry.tag.

Copy link
Collaborator

@tomkennedy513 tomkennedy513 left a comment

Choose a reason for hiding this comment

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

Great work!

@matthewmcnew
Copy link
Collaborator

Ah. It was called spec.cache.registry.tag in an earlier state of the pr. We have changed it to spec.cache.registry.imageTag because we thought that is a bit clearer. tag sounded like it is only the tag that will be applied to the cache image. But it is the whole image name.

Yeah, the docker terminology is a bit cumbersome, especially with the tag field. I think tag makes sense in this case because it matches spec.tag on images,builders, and builds.

* Split up image cache configuration
* Split up build cache configuration
* Add registy/image based cache
* Remove unused buildCacheName arg
* Update documentation
* Add basic registry-cache e2e test
* Fix duplicate import
* Fix unmatched quotes
* Add home volume to restore step
* Allow build.cache to be nil
* Persist cache image in build
* Use cache image digest if present
* Move changes to buildapi from v1alpha1 to v1alpha2
* Add test cache for Spec.Cache = nil
* Add validation that only one type of cache is used for build and image
* Use `spec.cache.registry.tag` to be more consistent

Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
Co-authored-by: Sumit Kulhadia <sumit.kulhadia@sap.com>
Co-authored-by: Pavel Busko <pavel.busko@sap.com>
@c0d1ngm0nk3y
Copy link
Contributor

Yeah, the docker terminology is a bit cumbersome, especially with the tag field. I think tag makes sense in this case because it matches spec.tag on images,builders, and builds.

Done. I think we are good to go...

@tomkennedy513 tomkennedy513 merged commit beec5d1 into buildpacks-community:main Aug 26, 2021
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.

7 participants