Enable caching imgpkg #178

joaopapereira · 2022-08-22T21:10:08Z

This PR includes the following changes:

Uses richgo, when present, to run tests to have an easier-to-read UX
Start using imgpkg as a library
- The only API available at this point is Pull so we still need the binary installed when the tags command is called
- Included temporary hack that needs to be removed when Using imgpkg as a library causes docker authentication failure imgpkg#397 is fixed
Allow the users to specify caching folder and the maximum size of caching they want to enable using the environment variables VENDIR_CACHE_DIR and VENDIR_MAX_CACHE_SIZE respectively.

Fixes #160

richgo provides a better UX while running tests in the command line Signed-off-by: Joao Pereira <[email protected]>

joe-kimmel-vmw

i have left you a sort of incomplete code review but i think that's all i can give you at the moment. thanks for letting me take a look.

joe-kimmel-vmw · 2022-08-23T17:55:31Z

hack/patch-k8s-pkg-credentialprovider.patch

+ // Enabled implements dockerConfigProvider
+ func (d *defaultDockerConfigProvider) Enabled() bool {
+-	return true
+	return false


this is like forking an open source project but sneakier. is this a long-term plan?

we tried to give provide a fix to the upstream repository but they told us that they wanted to keep the repository as a copy-paste from Kubernetes authentication libraries.
This is a temporary solution until we fix the issue carvel-dev/imgpkg#397

joe-kimmel-vmw · 2022-08-23T18:07:04Z

pkg/vendir/fetch/cache/cache.go

+
+// Hit checks if a particular entry in the cache is present
+// Returns the path to the entry and a flag information if the entry was found or not
+func (c FolderCache) Hit(id string) (string, bool) {


nit: but to my mind "Hit" sounds more like a command than a query. of course "Present" has the same issues and is a longer word. idk, for 3 letter words, ... "Has" ?

I was trying to use the caching more common terms like hit/miss to provide describe the behavior, but I think this can be an easy change

joe-kimmel-vmw · 2022-08-23T18:14:41Z

pkg/vendir/fetch/cache/resources/amount.go

@@ -0,0 +1,302 @@
+// Copyright 2022 VMware, Inc.


what does it mean to stick our copyright above something that has a different copyright?

we do add our copyright to all the files with cod in the project and we have been told that this is what we should be doing

joe-kimmel-vmw · 2022-08-23T18:23:30Z

pkg/vendir/fetch/image/imgpkg.go

@@ -135,3 +238,29 @@ func (t *Imgpkg) addDangerousArgs(args []string) []string {
 	}
 	return args
 }
+
+// Logger provided to the imgpkg API calls
+type Logger struct {


ok i'll admit, i'm confused why, in a PR about caching, there's a logger that looks like it has levels but actually they're hardcoded and you'll never know which level something was logged at, especially if it was a debug log bc those don't even log at all?

My guess is you needed an interface to pass to something else, but maybe some more elaborate hint in the comments would be helpful

i guess in general -- i like log messages. like being able to get debug output. i have some dreams w.r.t . being able to trigger all the tools in kapp-controller into debug at the same time, for the sake of figuring out what's going on.

The logger created in this particular instance is a bit peculiar because it needs to conform to the requirements from the imgpkg API.
Because of that, it is only present in the image package. In the future when we are using our tools and can provide directives to activate different logging levels we should definitely revisit all these instances to ensure that we are doing the correct thing.
In the meanwhile, I will update the logger struct comment to inform the user that not all the logs will be present

Starts using imgpkg as a library do pull images and bundles Signed-off-by: Joao Pereira <[email protected]>

Due to the inclusion of imgpkg as a library we need to ensure that the change made in carvel-dev/imgpkg#255 stays in effect. This will behavior can be removed when we complete the issue carvel-dev/imgpkg#397 Signed-off-by: Joao Pereira <[email protected]>

cppforlife · 2022-08-24T11:56:39Z

i just realized that by merging this, it would make vendir block in environments where ambient creds endpoints are present but blocked (same problem that imgpkg has). this is especially nasty if user is not even using imgpkg fetcher (previously this was not a problem because imgpkg's dependencies are not executed when vendir itself starts). id like us to see ambient cred blocking problem fixed before we release vendir to avoid this backwards compat breaking behaviour.

joaopapereira · 2022-08-24T14:34:22Z

@cppforlife what do you mean by ambient credentials blocking problem?

Signed-off-by: Joao Pereira <[email protected]>

joaopapereira · 2022-08-24T17:37:19Z

Are you talking about selectively choose the auth keychains?
From this issue carvel-dev/imgpkg#326
Also the fix that is coming on this PR carvel-dev/imgpkg#409 ?

cppforlife · 2022-09-07T23:10:54Z

Are you talking about selectively choose the auth keychains?

yup, im getting a little lost with one of the PRs closed and the other merged. looks like it the feature that we needed got merged. it seems that those changes were introduced in imgpkg v0.32.0. im ok merging this PR as is assuming we make another PR with bump to imgpkg v0.32.0 before we cut a next vendir release. (or feel free to revise this PR to do the bump).

Signed-off-by: Joao Pereira <[email protected]>

With imgpkg 0.32.0 there is not need for this hack script anymore This reverts commit 0b6d143.

Signed-off-by: Joao Pereira <[email protected]>

cppforlife

left one comment before merge

cppforlife · 2022-09-12T22:17:43Z

pkg/vendir/fetch/cache/cache.go

+}
+
+func (c FolderCache) idToFolder(artifactType string, id string) string {
+	normalizedID := strings.ReplaceAll(id, ":", "-")


should we turn id into a base64 or sha to avoid having to "try to sanitize" it?

Signed-off-by: Joao Pereira <[email protected]>

Uses richgo for testing when present

2695f10

richgo provides a better UX while running tests in the command line Signed-off-by: Joao Pereira <[email protected]>

vmwclabot added the cla-not-required label Aug 22, 2022

joaopapereira mentioned this pull request Aug 23, 2022

Add explanation on caching for vendir carvel-dev/carvel#527

Merged

joe-kimmel-vmw reviewed Aug 23, 2022

View reviewed changes

joaopapereira added 2 commits August 23, 2022 14:45

Include imgpkg as a library

b81737a

Starts using imgpkg as a library do pull images and bundles Signed-off-by: Joao Pereira <[email protected]>

joaopapereira force-pushed the enable-caching-imgpkg branch 2 times, most recently from 89c3df8 to 3714f5b Compare August 23, 2022 22:06

joaopapereira added 3 commits August 24, 2022 09:45

Add Caching for images and bundles

34e8c1d

Signed-off-by: Joao Pereira <[email protected]>

Copy resources package from apimachinary

960c480

Signed-off-by: Joao Pereira <[email protected]>

Add maximum size allowed for caching a folder

1c049fb

Signed-off-by: Joao Pereira <[email protected]>

joaopapereira force-pushed the enable-caching-imgpkg branch from 3714f5b to 1c049fb Compare August 24, 2022 16:00

joaopapereira added 3 commits September 12, 2022 15:23

Bump imgpkg to 0.32.0

d3631eb

Signed-off-by: Joao Pereira <[email protected]>

Revert "Errors out when changes are made to vdemeester library"

683a4bd

With imgpkg 0.32.0 there is not need for this hack script anymore This reverts commit 0b6d143.

fix spacing issue

48fc834

Signed-off-by: Joao Pereira <[email protected]>

cppforlife reviewed Sep 12, 2022

View reviewed changes

base64 encode the folder id to prevent strange characters

2be8324

Signed-off-by: Joao Pereira <[email protected]>

cppforlife merged commit d3f61c7 into develop Sep 13, 2022

joaopapereira deleted the enable-caching-imgpkg branch September 28, 2022 20:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable caching imgpkg #178

Enable caching imgpkg #178

joaopapereira commented Aug 22, 2022

joe-kimmel-vmw left a comment

joe-kimmel-vmw Aug 23, 2022

joaopapereira Aug 23, 2022

joe-kimmel-vmw Aug 23, 2022

joaopapereira Aug 23, 2022

joe-kimmel-vmw Aug 23, 2022

joaopapereira Aug 23, 2022

joe-kimmel-vmw Aug 23, 2022

joe-kimmel-vmw Aug 23, 2022

joaopapereira Aug 23, 2022

cppforlife commented Aug 24, 2022

joaopapereira commented Aug 24, 2022

joaopapereira commented Aug 24, 2022

cppforlife commented Sep 7, 2022

cppforlife left a comment

cppforlife Sep 12, 2022

Enable caching imgpkg #178

Enable caching imgpkg #178

Conversation

joaopapereira commented Aug 22, 2022

joe-kimmel-vmw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cppforlife commented Aug 24, 2022

joaopapereira commented Aug 24, 2022

joaopapereira commented Aug 24, 2022

cppforlife commented Sep 7, 2022

cppforlife left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment