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

[JENKINS-38992] Allow caching library versions #50

Merged
merged 16 commits into from Jun 21, 2021

Conversation

julienduchesne
Copy link
Contributor

We have a jenkins instance that need to pull a lot of libraries. When many jobs are running simultaneously it can sometimes take up to 30 seconds to load a single library.

Probably fixes https://issues.jenkins-ci.org/browse/JENKINS-38992 and https://issues.jenkins-ci.org/browse/JENKINS-48210 in some measure

abayer
abayer previously requested changes Aug 22, 2018
Copy link
Member

@abayer abayer left a comment

Choose a reason for hiding this comment

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

Definitely needs tests for the actual caching behavior.

@julienduchesne
Copy link
Contributor Author

Alright, and I still have to fix failing tests. FYI, that's how it looks in the UI
image

@julienduchesne
Copy link
Contributor Author

Added tests, the build failure seems unrelated:
java.lang.RuntimeException: Error grabbing Grapes -- [download failed: org.apache.commons#commons-math3;3.4.1!commons-math3.jar]

@julienduchesne
Copy link
Contributor Author

Any update on this?

Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

Idea looks good to me. It would be nice to refactor LibraryCacheConfiguration to implement Describable so that we have CasC support, and it seem like the caching logic might be better suited to be part of of SCMSourceRetriver so that the cache can use take the SCM into account.

The only high-level thing I am concerned about is some way of automatically clearing out long-unused caches. Maybe we could the time each cache was used most recently and have an AsyncPeriodicWork instance that deletes caches that haven't been used in some amount of time (could be configurable, but a week also seems like a decent default).

}
}

if(versionCacheDir.exists()) {
Copy link
Member

Choose a reason for hiding this comment

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

If we change the SCM configuration (i.e. switch to a new repository), but not the library name, then the cache is still valid. It seems like it might be better to move the caching logic inside of SCMSourceRetriver (and SCMRetriever if possible) so that we can use SCM#getKey as part of the cache name so that SCM changes automatically invalidate the cache.

@julienduchesne
Copy link
Contributor Author

Hey @dwnusbaum. I've fixed every issue except putting the config in SCMSourceRetriever. I put the config in LibraryConfiguration on purpose, that way it is independant from the SCM. The same way that other configs such as the default version and default version override are. I also don't think that it's a bug if I change the repository but the version stays the same. Let me know what you think.

@jglick jglick dismissed abayer’s stale review October 13, 2018 14:29

Looks obsolete.

@jglick jglick changed the title Allow caching library versions [JENKINS-38992] Allow caching library versions Oct 13, 2018
@@ -0,0 +1,3 @@
<div>
Space separated list of versions excluded from caching. Ex: "master release10 release9"
Copy link
Member

Choose a reason for hiding this comment

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

But there is no possibility here to specify a regular expression.

FWIW my proposal in JENKINS-38992 was to make this automatic—zero configuration. The SCM should know whether or not a given version was deterministic. The current SCMSource.retrieve(String, TaskListener) does not suffice since at least in the case of AbstractGitSCMSource the resulting SCMRevision.isDeterministic will always be true—the method resolves the actual current head of a branch. Thus we would need to extend the SCMHead API in scm-api to let the SCM source indicate whether the head is deterministic (like a hash, or arguably a tag) vs. nondeterministic (like a branch). This would be easy to implement in git at least.

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 agree that it should support regex and maybe add an include field as well (only include specific versions or regexes) but is it really user friendly to cache some versions but not others. In our use case, it would be annoying to cache tags by default because we use semantic versioning and publish minor versions as well as patch versions (ex: release v4.2.1 also releases v4.2 and v4).

All I'm saying is I'm not a fan of hidden behavior and the current implementation is very simple. Is it really necessary to complicate it?

@d3rp3tt3
Copy link

d3rp3tt3 commented Oct 16, 2018

Based on input here, this might not be the best implementation.

@julienduchesne
Copy link
Contributor Author

julienduchesne commented Oct 16, 2018

It works very well though. We are using it on a jenkins instance running about 20 very short jobs (15 sec to 5 minutes) concurrently at all times during the day. Each of those jobs pull two shared libraries. We used to only be able to do a full cycle of our workflow in over an hour before this. It now takes under 20 minutes.

@criley-eagle
Copy link

"Based on input here, this might not be the best implementation."
@jennbriden Is this pr still viable, or is there a better implementation in the works, as we are definitely hitting up against the same issue this pr was attempting to resolve?

@julienduchesne
Copy link
Contributor Author

👋 I haven't received any "pings" until now. What needs to be done to have this merged?

@msrb
Copy link
Member

msrb commented Aug 28, 2020

@oleg-nenashev LGTM? :)

@oleg-nenashev
Copy link
Member

👋 I haven't received any "pings" until now. What needs to be done to have this merged?

Looks like somebody needs to take ownership of the plugin or to get the current maintainers to review/merge the pull request. Unfortunately I cannot do much more to push it forward at the moment.

@atmcarmo
Copy link

Thanks for this PR! Hope this gets merged. It's really an issue for us the time it gets to clone and load a library when many pipelines are starting to execute at the same time.

MarkEWaite added a commit to MarkEWaite/docker-lfs that referenced this pull request Nov 2, 2020
@robodenitro
Copy link

any updates here?

@MarkEWaite
Copy link
Contributor

@robodenitro I've been running this pull request since early November 2020 without detecting any issue. You could do the same by installing the file from MarkEWaite/docker-lfs@408684e . Once you do that, your specific need should be resolved and you can report your test results

It is more persuasive to report successful use of a pull request than to ask for a status update.

@nnadams
Copy link

nnadams commented Feb 19, 2021

Following Mark's example, I have been using this pull request since June 2020 and seen no issues. We run thousands of jobs weekly that all make use of this caching. Our CI has been faster and much more robust. I hope this can be merged. Either way thank you @julienduchesne!

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Remove trailing white space in diffs

sampleRepo.git("add", "src", "resources");
sampleRepo.git("commit", "--message=init");
initFixedContentLibrary();

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove trailing white space

Suggested change

@Test public void cachingExcludedLibrary() throws Exception {
clearCache("stuff");
initFixedContentLibrary();

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove trailing white space

Suggested change

@Test public void caching() throws Exception {
clearCache("stuff");
initFixedContentLibrary();

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove trailing white space

Suggested change

@Test public void cachingRefresh() throws Exception {
clearCache("stuff");
initFixedContentLibrary();

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove trailing white space

Suggested change

MarkEWaite added a commit to MarkEWaite/docker-lfs that referenced this pull request Feb 19, 2021
@ababushk
Copy link

We use version of workflow-cps-global-lib with changes from this PR on production instance with huge amount of jobs and builds per day. No issues so far!
Moreover, this allowed us to run multiple concurrent builds (>1000 simultaneously) without waiting for a lock on git repo cache directory for our global library on Jenkins server, which has speeded up them drastically. Many thanks to @julienduchesne !

@rfvmonteiro
Copy link

Thanks for this contribution. We are testing it too and no issues found so far.
I believe this is a solid solution and should be merged soon 🙏

@atmcarmo
Copy link

This solution seems to be rock solid, really improving performance even with a very big amount of builds happening at the same time.

What needs to be done to merge to be included in a new release?

Thank you very much for your work 👏

@michelgasser
Copy link

Why is the PR not being merged? Why is it taking so long?

@timja
Copy link
Member

timja commented Jun 21, 2021

@dwnusbaum @car-roll @bitwiseman Would it be possible for someone to take a look at this?

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Jun 21, 2021 via email

Copy link
Contributor

@car-roll car-roll left a comment

Choose a reason for hiding this comment

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

So, I'm just going over the comments here and I think this summarizes what went on here:

It does look like most issues were addressed, but there was a preference by both @dwnusbaum and @jglick to implement things slightly different, which is why this PR stalled. But because there was no significant effort to address the alternatives proposed by them, and since this PR was the ONLY solution available, it became widely adopted. Thus the wide adoption of this solution is now causing pressure for this PR to be merged.

Given that this PR has been actively used in the wild now for quite some time, I'm going ahead to merge it.

edit: The UI seems to be fine after resolving the latest merge

@car-roll car-roll merged commit b367de6 into jenkinsci:master Jun 21, 2021
@michelgasser
Copy link

Cool, thanks a lot for the merge! :-)

@MaximeAnsquer
Copy link

How can I disable caching please?

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Oct 7, 2021

How can I disable caching please?

Caching is disabled by default. If you have enabled caching in the "Manage Jenkins" -> "Configure System" page, you would disable it by unchecking the "Cache fetched versions on master for quick retrieval"

Screenshot 2021-10-07 163520

@MaximeAnsquer
Copy link

Thank you @MarkEWaite!

@aaronr93
Copy link

aaronr93 commented Feb 13, 2022

Is there a way to programmatically clear the cache from a pipeline?

Here are my ideal use cases:

  1. Cache all versions for a long time, then clear the cache when new revisions are available (for example, called directly from a job called by an SCM webhook or some other trigger). This would follow an industry-standard concept of "cache busting".
  2. Clean the caches of old/deleted branches via a regularly-scheduled maintenance job, rather than remembering to manually click "Clear cache" every now and then.
  3. Programmatically configure certain pipelines to not use caching (perhaps a parameter passed to library).

Regardless, this feature should be added to the "Extending with Shared Libraries" documentation.

Let me know if I should start a new issue for this.

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Feb 13, 2022

Regardless, this feature should be added to the "Extending with Shared Libraries" documentation.

Let me know if I should start a new issue for this.

Best would be to submit the pull request proposing the changes to the existing documentation. See the "Improve this page" link at the bottom of that page.

If not ready to submit the documentation pull request, then submit the issue using the "Report a problem" at the bottom of that page.

@aaronr93
Copy link

aaronr93 commented May 2, 2022

Hi @MarkEWaite, just following up on my first question above. Is there a way to programmatically clear the cache from a pipeline?

@MarkEWaite
Copy link
Contributor

Hi @MarkEWaite, just following up on my first question above. Is there a way to programmatically clear the cache from a pipeline?

No method that I know to clear cache from a Pipeline.

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