[JENKINS-72111] Add `Lifecycle#set` #8555

Vlatombe · 2023-10-03T10:35:04Z

This change is required for CloudBees CI as we implemented a new Lifecycle in a plugin. The rest is documented in the new method javadoc.

Testing done

Proposed changelog entries

Developer: JENKINS-72111, allow plugins to provide a custom lifecycle

Proposed upgrade guidelines

N/A

Submitter checklist

Give feedback

The Jira issue, if it exists, is well-described.

The Jira issue, if it exists, is well-described.
Options
Successfully updated the issue's project

There was an error updating the issue's project
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see [examples](https://github.com/jenkins-infra/jenkins.io/blob/master/content/_data/changelogs/weekly.yml)). Fill in the **Proposed upgrade guidelines** section only if there are breaking changes or changes that may require extra steps from users during upgrade.

The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
Options
Successfully updated the issue's project

There was an error updating the issue's project
There is automated testing or an explanation as to why this change has no tests.

There is automated testing or an explanation as to why this change has no tests.
Options
Successfully updated the issue's project

There was an error updating the issue's project
New public classes, fields, and methods are annotated with `@Restricted` or have `@since TODO` Javadocs, as appropriate.

New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
Options
Successfully updated the issue's project

There was an error updating the issue's project
New deprecations are annotated with `@Deprecated(since = "TODO")` or `@Deprecated(forRemoval = true, since = "TODO")`, if applicable.

New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
Options
Successfully updated the issue's project

There was an error updating the issue's project
New or substantially changed JavaScript is not defined inline and does not call `eval` to ease future introduction of Content Security Policy (CSP) directives (see [documentation](https://www.jenkins.io/doc/developer/security/csp/)).

New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
Options
Successfully updated the issue's project

There was an error updating the issue's project
For dependency updates, there are links to external changelogs and, if possible, full differentials.

For dependency updates, there are links to external changelogs and, if possible, full differentials.
Options
Successfully updated the issue's project

There was an error updating the issue's project
For new APIs and extension points, there is a link to at least one consumer.

For new APIs and extension points, there is a link to at least one consumer.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

Desired reviewers

@mention

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

Maintainer checklist

Give feedback

There are at least two (2) approvals for the pull request and no outstanding requests for change.

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.

Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Changelog entries in the pull request title and/or **Proposed changelog entries** are accurate, human-readable, and in the imperative mood.

Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Options
Successfully updated the issue's project

There was an error updating the issue's project
Proper changelog labels are set so that the changelog can be generated automatically.

Proper changelog labels are set so that the changelog can be generated automatically.
Options
Successfully updated the issue's project

There was an error updating the issue's project
If the change needs additional upgrade steps from users, the `upgrade-guide-needed` label is set and there is a **Proposed upgrade guidelines** section in the pull request title (see [example](https://github.com/jenkinsci/jenkins/pull/4387)).

If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
Options
Successfully updated the issue's project

There was an error updating the issue's project
If it would make sense to backport the change to LTS, a Jira issue must exist, be a _Bug_ or _Improvement_, and be labeled as `lts-candidate` to be considered (see [query](https://issues.jenkins.io/issues/?filter=12146)).

If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

jtnord · 2023-10-03T15:39:34Z

core/src/main/java/hudson/lifecycle/Lifecycle.java

+     * Appropriate for implementations defined in plugins,
+     * since {@link #get} may be called before plugins are initialized,
+     * and so it is not safe to pass a plugin-defined class to the system property {@code hudson.lifecycle}
+     * despite the use of {@link PluginManager#uberClassLoader}.


this comment is contradictory to me.

if this method is appropriate for plugins, but itself may be called before plugins are initialized, how can a plugin ensure that they can call set before get as any consumers of get can rightly assume that the lifecycle is fixed.
the answer is they can not - so it is not really "safe" to be called in a plugin.

once get is called - to all purposes INSTANCE should be final and never change.
You could probably demonstrate this by throwing an exception if INSTANCE is non null (which means get has been called)

so whilst setting the property is not safe to use with an implementation from a plugin equally this new method appears to not be safe as callers of get can get intermediate responses.

how can a plugin ensure that they can call set before get

It cannot, but it can call set in an @Initializer or otherwise during startup.

any consumers of get can rightly assume that the lifecycle is fixed

Why would a consumer make that assumption? I did a search of @jenkinsci sources and found no example of a consumer doing anything with the return value other than immediately calling some method on it.

It cannot, but it can call set in an @Initializer or otherwise during startup.

which means if startup fails the lifecycle will not be correctly replaced leading to "the wrong" lifecyle handling the issue?

Why would a consumer make that assumption

Because it was a single initialized only once in code.

I did a search of https://github.com/jenkinsci sources and found no example of a consumer doing anything with the return value other than immediately calling some method on it.

an example (but a bad one) is

jenkins/core/src/main/java/hudson/lifecycle/WindowsInstallerLink.java

Lines 267 to 288 in 7d2b484

public static WindowsInstallerLink registerIfApplicable() {

if (!Functions.isWindows())

return null; // this is a Windows only feature

if (Lifecycle.get() instanceof WindowsServiceLifecycle)

return null; // already installed as Windows service

// this system property is set by the launcher when we run "java -jar jenkins.war"

// and this is how we know where is jenkins.war.

String war = SystemProperties.getString("executable-war");

if (war != null && new File(war).exists()) {

WindowsInstallerLink link = new WindowsInstallerLink(new File(war));

// TODO possibly now unused (JNLP installation mode is long gone):

if (SystemProperties.getString(WindowsInstallerLink.class.getName() + ".prominent") != null)

Jenkins.get().getActions().add(link);

return link;

}

return null;

}

that changes behaviour once and once only for example at startup as it has assumed that the Lifecycle will not change.

I hope to rip that example out in #8523 so this becomes a moot point.

I think this just needs some clearer documentation that this is super advanced and you should not normally do this - and it you do that you will not be called back during early initialisation like a regular Lifecycle and that even changing it could cause other plugin code that is obtaining the lifecycle (say to check if you can restart once) may not observe any changes in the lifecycle. (or documewnt in the get that the lifecycle can change at runtime)
esp the class level comment is now incorrect about how the Lifecycle is found.

documentation that this is super advanced and you should not normally do this

Well, it is marked Beta, but yes documentation should be adjusted.

There are probably other ways to approach the problem. Currently

jenkins/core/src/main/java/hudson/model/RestartListener.java

Lines 24 to 27 in 7d2b484

/**

* Called immediately before the restart is actually triggered.

*/

public void onRestart() {}

offers no “veto” functionality, and so there is no other way to intercept

jenkins/core/src/main/java/jenkins/model/Jenkins.java

Lines 4649 to 4663 in 7d2b484

public void restart() throws RestartNotSupportedException {

final Lifecycle lifecycle = restartableLifecycle();

servletContext.setAttribute("app", new HudsonIsRestarting());

new Thread("restart thread") {

final String exitUser = getAuthentication2().getName();

@Override

public void run() {

try (ACLContext ctx = ACL.as2(ACL.SYSTEM2)) {

// give some time for the browser to load the "reloading" page

lifecycle.onStatusUpdate("Restart in 5 seconds");

Thread.sleep(TimeUnit.SECONDS.toMillis(5));

lifecycle.onStop(exitUser, null);

Listeners.notify(RestartListener.class, true, RestartListener::onRestart);

lifecycle.restart();

or

jenkins/core/src/main/java/jenkins/model/Jenkins.java

Lines 4689 to 4710 in 7d2b484

public void safeRestart(String message) throws RestartNotSupportedException {

final Lifecycle lifecycle = restartableLifecycle();

// Quiet down so that we won't launch new builds.

quietDownInfo = new QuietDownInfo(message, true);

new Thread("safe-restart thread") {

final String exitUser = getAuthentication2().getName();

@Override

public void run() {

try (ACLContext ctx = ACL.as2(ACL.SYSTEM2)) {

// Wait 'til we have no active executors.

doQuietDown(true, 0, message, true);

// Make sure isQuietingDown is still true.

if (isQuietingDown()) {

servletContext.setAttribute("app", new HudsonIsRestarting(true));

// give some time for the browser to load the "reloading" page

lifecycle.onStatusUpdate("Restart in 10 seconds");

Thread.sleep(TimeUnit.SECONDS.toMillis(10));

lifecycle.onStop(exitUser, null);

Listeners.notify(RestartListener.class, true, RestartListener::onRestart);

lifecycle.restart();

Alternately, it is possible (I suppose) to define a Lifecycle in a module in the same class loader as jenkins-core.jar, though this is logistically more complicated because then you need to package this module somewhere, and it cannot easily call other plugin methods, etc. The current patch is more expedient.

(for the avoidance of doubt my comments are non blocking)

jglick

A behavioral refinement could likely avoid the need for a setter.

jglick · 2023-10-07T11:43:26Z

core/src/main/java/hudson/lifecycle/Lifecycle.java

+     * since {@link #get} may be called before plugins are initialized,
+     * and so it is not safe to pass a plugin-defined class to the system property {@code hudson.lifecycle}
+     * despite the use of {@link PluginManager#uberClassLoader}.


While a veto-type method in RestartListener (as in #8555 (comment)) would probably be best, it occurs to me that we do not really need a set method. It would suffice for

jenkins/core/src/main/java/hudson/lifecycle/Lifecycle.java

Lines 83 to 86 in d1b8473

} catch (ClassNotFoundException e) {

NoClassDefFoundError x = new NoClassDefFoundError(e.getMessage());

x.initCause(e);

throw x;

to quietly fall back to the default, but then have an initializer run after plugins are loaded which tries again (and complains noisily if the class still cannot be found).

I do not think #8555 (comment) is a real issue; it just means that for example a plugin-provided lifecycle cannot override onStop for the corner case of a boot failure. (The CloudBees CI implementation only needs to override restart, which should never be called before startup is complete so far as I know.)

Vlatombe · 2023-10-10T16:04:34Z

Superceded by #8589

[JENKINS-72111] Add Lifecycle#set

352bc19

Vlatombe requested review from jglick and a team October 3, 2023 10:37

jglick approved these changes Oct 3, 2023

View reviewed changes

jtnord reviewed Oct 3, 2023

View reviewed changes

jglick requested changes Oct 7, 2023

View reviewed changes

jglick mentioned this pull request Oct 10, 2023

[JENKINS-72111] Allow Lifecycle to load implementations from plugins #8589

Merged

Vlatombe closed this Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-72111] Add `Lifecycle#set` #8555

[JENKINS-72111] Add `Lifecycle#set` #8555

Vlatombe commented Oct 3, 2023 •

edited

Submitter checklist

Maintainer checklist

jtnord Oct 3, 2023 •

edited

jglick Oct 3, 2023

jtnord Oct 3, 2023

jglick Oct 3, 2023

jtnord Oct 4, 2023

jglick left a comment

jglick Oct 7, 2023

Vlatombe commented Oct 10, 2023

	public static WindowsInstallerLink registerIfApplicable() {
	if (!Functions.isWindows())
	return null; // this is a Windows only feature

	if (Lifecycle.get() instanceof WindowsServiceLifecycle)
	return null; // already installed as Windows service

	// this system property is set by the launcher when we run "java -jar jenkins.war"
	// and this is how we know where is jenkins.war.
	String war = SystemProperties.getString("executable-war");
	if (war != null && new File(war).exists()) {
	WindowsInstallerLink link = new WindowsInstallerLink(new File(war));

	// TODO possibly now unused (JNLP installation mode is long gone):
	if (SystemProperties.getString(WindowsInstallerLink.class.getName() + ".prominent") != null)
	Jenkins.get().getActions().add(link);

	return link;
	}

	return null;
	}

	/**
	* Called immediately before the restart is actually triggered.
	*/
	public void onRestart() {}

	public void restart() throws RestartNotSupportedException {
	final Lifecycle lifecycle = restartableLifecycle();
	servletContext.setAttribute("app", new HudsonIsRestarting());

	new Thread("restart thread") {
	final String exitUser = getAuthentication2().getName();
	@Override
	public void run() {
	try (ACLContext ctx = ACL.as2(ACL.SYSTEM2)) {
	// give some time for the browser to load the "reloading" page
	lifecycle.onStatusUpdate("Restart in 5 seconds");
	Thread.sleep(TimeUnit.SECONDS.toMillis(5));
	lifecycle.onStop(exitUser, null);
	Listeners.notify(RestartListener.class, true, RestartListener::onRestart);
	lifecycle.restart();

	public void safeRestart(String message) throws RestartNotSupportedException {
	final Lifecycle lifecycle = restartableLifecycle();
	// Quiet down so that we won't launch new builds.
	quietDownInfo = new QuietDownInfo(message, true);

	new Thread("safe-restart thread") {
	final String exitUser = getAuthentication2().getName();
	@Override
	public void run() {
	try (ACLContext ctx = ACL.as2(ACL.SYSTEM2)) {

	// Wait 'til we have no active executors.
	doQuietDown(true, 0, message, true);
	// Make sure isQuietingDown is still true.
	if (isQuietingDown()) {
	servletContext.setAttribute("app", new HudsonIsRestarting(true));
	// give some time for the browser to load the "reloading" page
	lifecycle.onStatusUpdate("Restart in 10 seconds");
	Thread.sleep(TimeUnit.SECONDS.toMillis(10));
	lifecycle.onStop(exitUser, null);
	Listeners.notify(RestartListener.class, true, RestartListener::onRestart);
	lifecycle.restart();

	} catch (ClassNotFoundException e) {
	NoClassDefFoundError x = new NoClassDefFoundError(e.getMessage());
	x.initCause(e);
	throw x;

[JENKINS-72111] Add Lifecycle#set #8555

[JENKINS-72111] Add Lifecycle#set #8555

Conversation

Vlatombe commented Oct 3, 2023 • edited

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

jtnord Oct 3, 2023 • edited

Choose a reason for hiding this comment

jglick Oct 3, 2023

Choose a reason for hiding this comment

jtnord Oct 3, 2023

Choose a reason for hiding this comment

jglick Oct 3, 2023

Choose a reason for hiding this comment

jtnord Oct 4, 2023

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

jglick Oct 7, 2023

Choose a reason for hiding this comment

Vlatombe commented Oct 10, 2023

[JENKINS-72111] Add `Lifecycle#set` #8555

[JENKINS-72111] Add `Lifecycle#set` #8555

Vlatombe commented Oct 3, 2023 •

edited

jtnord Oct 3, 2023 •

edited