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

Use detached plugins as a cache for the Update Center #9476

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

basil
Copy link
Member

@basil basil commented Jul 17, 2024

While doing manual testing recently I was frustrated by how long it took to get through the setup wizard. This change makes the setup wizard a little faster by using the detached plugins directory as a cache for the update center. This is a best-effort optimization that only kicks in if we happen to have the file we are about to download in the WEB-INF/detached-plugins directory and if its checksum happens to match. If so we simply save ourselves a trip to the download site by using what we already have. If anything goes wrong here we ignore the error and proceed along the normal download path. This brings the number of plugins that need to be downloaded during the setup wizard from 89 to 50 and (on my slow internet connection) drops the setup wizard time from 1 minute 52 seconds to 1 minute 9 seconds.

Testing done

Manual testing:

  • Install ASM library plugin via UI; verify that cached copy was used.
  • Install Text Finder plugin via UI; verify that cached copy was not used.
  • Install ASM library plugin via UI after corrupting the copy in WAR; verify that checksum failed and cached copy was not used.
  • Ran Setup Wizard before and after these changes; confirmed the same set of plugins was installed but the installation took about a minute less time.

Automated testing:

mvn clean verify -Dtest=hudson.ClassicPluginStrategyTest,hudson.cli.DisablePluginCommandTest,hudson.cli.EnablePluginCommandTest,hudson.cli.InstallPluginCommandTest,hudson.cli.ListPluginsCommandTest,hudson.core.PluginManagerOverrideTest,hudson.CustomPluginManagerTest,hudson.ExtensionListListenerTest,hudson.lifecycle.LifecycleTest,hudson.model.DownloadServiceTest,hudson.model.UpdateCenter2Test,hudson.model.UpdateCenterConnectionStatusTest,hudson.model.UpdateCenterCustomTest,hudson.model.UpdateCenterPluginInstallTest,hudson.model.UpdateCenterTest,hudson.model.UpdateSiteTest,hudson.PluginManagerCheckUpdateCenterTest,hudson.PluginManagerInstalledGUITest,hudson.PluginManagerTest,hudson.PluginManagerUtil,hudson.PluginManagerWhichTest,hudson.PluginTest,hudson.PluginWrapperTest,hudson.slaves.JNLPLauncherRealTest,jenkins.install.InstallUtilTest,jenkins.install.LoadDetachedPluginsTest,jenkins.install.SetupWizardTest,jenkins.model.JenkinsManagePermissionTest,jenkins.plugins.DetachedPluginsUtilTest,jenkins.security.CustomClassFilterTest,jenkins.security.SecurityContextExecutorServiceTest,jenkins.security.stapler.Security914Test

Proposed changelog entries

  • Avoid unnecessary download of bundled plugins during the setup wizard.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

@basil basil added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted needs-security-review Awaiting review by a security team member labels Jul 17, 2024
@basil
Copy link
Member Author

basil commented Jul 17, 2024

@jtnord I don't think this should affect FIPS support since this is a best-effort optimization and since CloudBees doesn't ship a WEB-INF/detached-plugins directory, but you may be interested nonetheless.

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

Thanks @basil, I think this should be fine as plugin installation is limited to a specific set of plugins and this does not appear to interfere with that.
@olamy as you wrote the code can you check please?

* Could make PluginManager#getDetachedLocation public and consume it here, but this method is
* best-effort anyway.
*/
src = Jenkins.get().servletContext.getResource(String.format("/WEB-INF/detached-plugins/%s.hpi", plugin.name));

This comment was marked as off-topic.

This comment was marked as off-topic.

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.

Thanks!

@daniel-beck daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Jul 29, 2024
@MarkEWaite
Copy link
Contributor

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

/label ready-for-merge

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jul 29, 2024
@MarkEWaite MarkEWaite merged commit ca241b4 into jenkinsci:master Jul 30, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants