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-48365] Install detached plugins when upgrading Jenkins past the version the plugins were detached #3193

Merged
merged 5 commits into from Dec 17, 2017

Conversation

5 participants
@dwnusbaum
Copy link
Member

commented Dec 12, 2017

See JENKINS-48365.

PluginManager checks for InstallState.UPGRADE when deciding what logic to use. However, in Jenkins' constructor, the call to Jenkins#executeReactor, which will execute the 'Loading bundled plugins' task that calls PluginManager#loadDetachedPlugins, happens a few lines before the call to InstallUtil#proceedToNext, which will update InstallState to UPGRADE if it detects that an upgraded occurred, so the upgrade branch is never taken in PluginManager.

This PR instead checks the version numbers of the last running version and this version to decide if an upgrade occurred.

Proposed changelog entries

  • Make sure detached plugins (Plugins which used to be part of Jenkins itself) are installed when upgrading Jenkins past the version at which the plugin was detached.

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees

@jglick jglick self-requested a review Dec 12, 2017

assertTrue("Detached plugins should not be installed on a new instance",
getInstalledDetachedPlugins(r, detachedPlugins).isEmpty());
// Trick PluginManager into thinking an upgrade happened when Jenkins restarts.
InstallUtil.saveLastExecVersion(since.toString());

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Dec 12, 2017

Author Member

This is a hack, but I don't think there is a way to test an actual upgrade with JTH (or ATH). Is the test worth keeping?

This comment has been minimized.

Copy link
@jglick

jglick Dec 12, 2017

Member

Sure you can do it. Use @LocalData. The old version would be in config.xml. Not necessarily any better than what you have.

This comment has been minimized.

Copy link
@dwnusbaum

dwnusbaum Dec 12, 2017

Author Member

Ah, that makes sense. I'll take a look at switching to local data because that should make it possible to test the edge cases in InstallUtil#getLastExecVersions by doing a pre 2.0 upgrade as well as a post 2.0 upgrade.

@jglick

jglick approved these changes Dec 12, 2017

assertTrue("Detached plugins should not be installed on a new instance",
getInstalledDetachedPlugins(r, detachedPlugins).isEmpty());
// Trick PluginManager into thinking an upgrade happened when Jenkins restarts.
InstallUtil.saveLastExecVersion(since.toString());

This comment has been minimized.

Copy link
@jglick

jglick Dec 12, 2017

Member

Sure you can do it. Use @LocalData. The old version would be in config.xml. Not necessarily any better than what you have.

VersionNumber since = new VersionNumber("1.1");
rr.then(r -> {
List<DetachedPlugin> detachedPlugins = ClassicPluginStrategy.getDetachedPlugins(since);
assertFalse("Many plugins have been detached since the given version", detachedPlugins.isEmpty());

This comment has been minimized.

Copy link
@jglick

jglick Dec 12, 2017

Member

or

assertThat("", detachedPlugins, not(empty()));
@daniel-beck

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

Can we assert that all installed plugins loaded successfully to preempt e.g. dependency problems?

@dwnusbaum

This comment has been minimized.

Copy link
Member Author

commented Dec 12, 2017

@daniel-beck That sounds like a good idea to me. Right now for detached plugins I assert PluginWrapper#isActive is true, and I will add an assertion that PluginWrapper#getDependencyErrors is empty, and an assertion that PluginManager#getFailedPlugins is empty. Is that what you had in mind?

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

@dwnusbaum

Is that what you had in mind?

Exactly 👍

@recampbell

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

@dwnusbaum dwnusbaum requested a review from jglick Dec 13, 2017

@jglick

jglick approved these changes Dec 15, 2017

@oleg-nenashev
Copy link
Member

left a comment

All recent issues in the installation logic just show how bad was our test suite for Jenkins 2.0 whet the Setup Wizard was introduced

I do not like that a constant is being made public without restrictions. It would be better to fix it before the merge

@@ -70,7 +70,7 @@
private static final Logger LOGGER = Logger.getLogger(InstallUtil.class.getName());

// tests need this to be 1.0
private static final VersionNumber NEW_INSTALL_VERSION = new VersionNumber("1.0");
public static final VersionNumber NEW_INSTALL_VERSION = new VersionNumber("1.0");

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Dec 16, 2017

Member

Make it restricted?

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Dec 16, 2017

Manual test looks reasonable.

Dec 16, 2017 10:13:45 PM hudson.PluginManager loadDetachedPlugins
INFO: Upgrading Jenkins. The last running version was 2.60.1. This Jenkins is version 2.94-SNAPSHOT (private-12/12/2017 22:40 GMT-win2012-deb6b0$).
Dec 16, 2017 10:13:45 PM hudson.PluginManager loadDetachedPlugins
INFO: Upgraded Jenkins from version 2.60.1 to version 2.94-SNAPSHOT (private-12/12/2017 22:40 GMT-win2012-deb6b0$). Loaded detached plugins (and dependencies): [command-launcher.hpi, script-security.hpi]
@daniel-beck

This comment has been minimized.

Copy link
Member

commented Dec 16, 2017

@oleg-nenashev I pushed a commit to this PR that restricts the newly public field.

Will merge after PR build succeeds towards this weekly unless someone vetoes.

@dwnusbaum

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2017

@oleg-nenashev @daniel-beck The InstallUtil class is already @Restricted(NoExternalUse.class), is it necessary to restrict the field as well?

@oleg-nenashev
Copy link
Member

left a comment

whatever works. This issue is pretty bad, so we need to integrate the change. 🐝
@reviewbybees done

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Dec 17, 2017

@dwnusbaum Oops. I missed that. Good point. Oh well. We're going to merge today, so there will now be a pointless commit 🤷‍♂️

@oleg-nenashev oleg-nenashev merged commit 968b6ad into jenkinsci:master Dec 17, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
@daniel-beck

This comment has been minimized.

Copy link
Member

commented Dec 18, 2017

Believed to have introduced JENKINS-48604 in 2.96.

olivergondza added a commit that referenced this pull request Jan 3, 2018

Merge pull request #3193 from dwnusbaum/JENKINS-48365
[JENKINS-48365] Install detached plugins when upgrading Jenkins past the version the plugins were detached

(cherry picked from commit 968b6ad)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.