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

[fetch] add caching for pulling images #85

Closed
seemiller opened this issue Feb 1, 2021 · 12 comments
Closed

[fetch] add caching for pulling images #85

seemiller opened this issue Feb 1, 2021 · 12 comments
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution dk-think-more

Comments

@seemiller
Copy link
Contributor

I'm working on a project where we are using kapp-controller to deploy apps to a Kubernetes cluster. According to your readme, kapp-controller will "install, and continiously apply updates." It seems to be literally doing just that. I happened to look at Harbor, which is where I have my app images hosted, and there are thousands and thousands of image pulls. As far as I'm aware, there are only 2 clusters pulling against this repository, and I would not expect there to be 100K+ pulls of these apps. I've included a couple of screen shots from Harbor showing the frequency of the pulls.
Screen Shot 2021-02-01 at 9 01 35 AM
Screen Shot 2021-02-01 at 9 51 38 AM

I've also included the log from kapp-controller to show the frequency of reconciles and deploys.
kapp-controller.log

Is this expected behavior, or is this possibly a bug? Any help in tracking down what is causing this would be appreciated.

Thanks!

@seemiller seemiller added the carvel-triage This issue has not yet been reviewed for validity label Feb 1, 2021
@ewrenn8
Copy link
Contributor

ewrenn8 commented Feb 1, 2021

Hey @seemiller, what version of kapp-controller are you using and are any apps failing? The consistent pulling is expected since kapp-controller will sync an app every 30s by default, but this can be configured using the apps spec.syncPeriod configuration.

@joshrosso
Copy link

@ewrenn8 we certainly could increase spec.syncPeriod.

One thing that surprises me, when pulling container images (ie docker pull) if the layers pre-exist, it does not actually pull from the repository. For example, the Harbor "pulls" count does not increase.

Wouldn't we expect the same behavior from pulling imgpkg bundles? Where in a pull only occurs if the sha/layers don't match?

@ewrenn8
Copy link
Contributor

ewrenn8 commented Feb 1, 2021

Imgpkg uses ggcr to talk directly to the registry api, and it isn't currently storing pulled layers anywhere but in memory, so it isn't able to avoid re-pulling layers across invocations. Also, since we dont rely on any external tools, we aren't able to leverage any layer stores they're maintaining.

There are definitely a few of these scalability/efficiency improvements we would like to make in kapp-controller, and improvements around image pulling are in that list.

I am curious if adding some sort of caching to imgpkg is a feature we'd like to support over there. @cppforlife any thoughts?

@joshrosso
Copy link

on more thought: I wonder if we point at a bundle's digest sha rather than tag if it'd be appropriate to expect kapp-controller to only grab it once?

@ewrenn8
Copy link
Contributor

ewrenn8 commented Feb 1, 2021

I think thats definitely possible, but, in that case, we would probably still need to implement some kind of caching in kapp-controller so that we have the config from the bundle for future syncs without needing to pull. We need this so we can continue deploying the config in order to undo any potential changes someone made to the app's resources in the cluster.

@cppforlife cppforlife added discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution and removed carvel-triage This issue has not yet been reviewed for validity labels Feb 3, 2021
@cppforlife
Copy link
Contributor

cppforlife commented Feb 3, 2021

I am curious if adding some sort of caching to imgpkg is a feature we'd like to support over there. @cppforlife any thoughts?

this is something we need to think more about and decide if we really want to be caching things. caching/invalidation is complex.

in case of configuration images, keep in mind that they are very lightweight assets (just a few text files typically) so it should not be something that causes registry much problem. (caching probably would come handy when we are talking about large number of clusters though.)

As far as I'm aware, there are only 2 clusters pulling against this repository, and I would not expect there to be 100K+ pulls of these apps.

by default its 30s sync period, so that's 24 * 60 * 2 = 2880 fetches per day. we did fix some excessive retry behaviour in recent versions for failing apps so may be that's what was going on.

@cppforlife cppforlife changed the title Is kapp-controller pulling images non-stop? add caching for pulling images Feb 3, 2021
@cppforlife cppforlife changed the title add caching for pulling images [fetch] add caching for pulling images Feb 3, 2021
@joshrosso
Copy link

@cppforlife my primary worry would be triggering rate limits or denial of service as many clusters spin up?

What do you think about the model where, when referencing the imgpkg digest sha, you don't pull again unless that mutates? That way you can solve this case without complexity of caching imgpkg content?

@cppforlife
Copy link
Contributor

when referencing the imgpkg digest sha, you don't pull again unless that mutates?

that still involves implementing caching on our side since no content is persisted anywhere and we need the content to go through the sync steps. @aaronshurley @vibhas let's put this in the queue somewhere.

@seemiller
Copy link
Contributor Author

@joshrosso and I were discussing this and are willing to take a deeper look into what is going on and (assuming we have a solid handle on the problem) make a proposal to resolve the issue.

@voor
Copy link

voor commented Oct 24, 2021

We're interested in this only because of a peculiar use case where the registry that kapp-controller would pull from doesn't actually exist yet; we use kapp-controller to actually deploy the Kubernetes Cluster that hosts the Harbor instance kapp-controller would then pull from.

I'm not sure if this is a caching problem so much as a failover to something else problem. If kapp-controller could always talk to the registry, but if it is unable to reach the registry then do something else?

@neil-hickey
Copy link
Contributor

@joaopapereira did this get implemented in some form recently with caching? I think so right?

@joaopapereira
Copy link
Member

I think so. Going to close this issue. Please reopen if you think it needs more work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution dk-think-more
Projects
Archived in project
Development

No branches or pull requests

8 participants