[JENKINS-37862] Removing build symlink functionality #3982

jglick · 2019-04-12T20:08:59Z

See JENKINS-37862. Moving functionality to a new build-symlink plugin for anyone who needs it.

Run.createSymlink uses
PeepholePermalink uses

Proposed changelog entries

JENKINS-37862: Jenkins no longer creates symbolic links inside project or build directories. The Build Symlink plugin may be installed to restore this functionality if desired. URLs such as http://jenkins/job/upstream/lastStableBuild/ are not affected, only tools which directly access the $JENKINS_HOME filesystem. Existing symlinks are not automatically removed by the update; administrators wishing to clean up may run something like the following:

find $JENKINS_HOME/jobs -type l -name last\* -exec rm -v {} \;

Desired reviewers

@jenkinsci/code-reviewers

… fail in core.

…ather than symlinks.

jglick · 2019-04-13T16:38:43Z

TIL that a single build can be linked to from both lastSuccessfulBuild and lastUnsuccessfulBuild, if it is UNSTABLE. 🤷‍♂️

PierreBtz

lgtm, left a couple of remarks, non blocking though.

Played with it a bit and it seems to work as expected. Nice stuff :)

PierreBtz · 2019-04-13T17:54:37Z

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

+     * Inner maps are from permalink name tp build number.
+     * Synchronization is first on the outer map, then on the inner.
+     */
+    private static final Map<File, Map<String, Integer>> caches = new HashMap<>();


Minor styling, but as it is a constant I would suggest uppercase.

Well, it is a constant reference but not a constant value.

PierreBtz · 2019-04-13T18:02:19Z

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

-          } finally {
-              cw.abort();
-          }
+    static @Nonnull File storageFor(@Nonnull File buildDir) {


Maybe this method could be private? Or is there a use case I'm missing?

Suggested change

static @Nonnull File storageFor(@Nonnull File buildDir) {

private static @Nonnull File storageFor(@Nonnull File buildDir) {

It is referenced from the test.

PierreBtz · 2019-04-13T18:51:21Z

core/src/main/java/hudson/model/Run.java

-        Util.createSymlink(rootDir, targetDir, name, listener);
-    }
+    @Deprecated
+    public final void updateSymlinks(@Nonnull TaskListener listener) throws InterruptedException {}


Shouldn't we log a WARNING message like a plugin called updateSymlinks which is now a NOOP, see JENKINS-37862 for details. This might help track down any plugin that rely on this feature.
We are only talking about maven plugin and workflow-job plugin for the community hosted plugin but I'm more thinking about custom/non hosted in jenkinsci ones.

We are only talking about maven plugin and workflow-job plugin for the community hosted plugin

Right, as per this search.

If there are others in private source, I think nothing special needs to be done—the deprecation warning should eventually lead to the calls being removed. The new impl does nothing, so it is harmless to leave calls alone for a while; and the new plugin does the same update automatically, so there is no loss of functionality when that plugin is installed, regardless of whether or not an exotic job type is still calling this method.

FWIW before we'd choose a warning I'd recommend collecting this via Telemetry, as admins can do nothing about it anyway. (But it looks like we need neither.)

Yeah I do not see anything to telemetrize here.

he new plugin does the same update automatically, so there is no loss of functionality when that plugin is installed

Yeah right, this is the part I missed when I suggested the warning.

Wadeck

🐝

As a Windows user I can only thank you 🎉

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

Co-Authored-By: jglick <jglick@cloudbees.com>

jglick · 2019-04-17T18:29:29Z

Feedback so far has been positive and I got hosting approved, so I will prepare for a plugin release but hold off unless and until this PR is actually merged.

jglick · 2019-04-19T14:04:08Z

One thing I should have mentioned: Jenkins core will not delete old symlinks that predated the upgrade. They will just be left to rot. If the new plugin is installed, it will update any old symlinks in addition to creating them for new jobs. If this feels like a problem, I suppose core could check for the existence of the plugin, and if not installed/enabled, actively attempt to delete the known symlinks where encountered.

daniel-beck · 2019-04-19T15:16:03Z

Jenkins core will not delete old symlinks that predated the upgrade. They will just be left to rot.

Should be fine. Definitely major rfe in the changelog, and deserves an entry in the upgrade guide, otherwise 🤷‍♂️

PierreBtz · 2019-04-19T15:29:29Z

Maybe just propose a simple bash script for administrators that might really want to clean those up (looks like an edge case to me, not worth developing migration code over). Something in the lines of what I propose for the shelve plugin:

find ${JENKINS_HOME}/jobs/ -type d \( -name lastFailedBuild -o -name lastSuccessfulBuild -o -name lastUnsuccessfulBuild -o -name lastStableBuild -o -name lastUnstableBuild -o -name lastFailed -o -name lastSuccessful -o -name lastUnsuccessful -o -name lastStable -o -name lastUnstable \) -exec rm -rfv "{}" \;

jglick · 2019-04-19T15:30:41Z

Maybe. If we are going to go to the trouble of publishing a migration script, I would rather just put in the little extra work to do it automatically inside core. Let me see if I can write something up today.

daniel-beck · 2019-04-19T15:42:30Z

I would rather just put in the little extra work to do it automatically inside core.

This will negatively impact people who upgrade first and then install the plugin; jobs will lose all existing symlinks and (I assume) only get them back on the next build (and perhaps then not even all of them).

jglick · 2019-04-19T16:06:22Z

This will negatively impact people who upgrade first and then install the plugin

Good point. In that case, I think it just makes sense to suggest a cleanup command right in the changelog / upgrade guide. The one by @PierreBtz is not right, I think, since -type d does not work without -L, and rm -r is unnecessary and dangerous here. I will try out some possibilities and post back here.

jglick · 2019-04-19T16:10:31Z

This seems to work fine on Linux:

find $JENKINS_HOME/jobs -type l -name last\* -exec rm -v {} \;

Doing the cleanup on Windows servers may be trickier, assuming the account running Jenkins was permitted to create symbolic links to begin with. Would need to figure out the right Powershell mantra.

daniel-beck · 2019-04-19T20:44:11Z

People who use them will install the plugin, people who don't, likely won't care that much. More something for the LTS upgrade guide, I think.

oleg-nenashev · 2019-04-26T07:59:58Z

If it is not urgent, I would like to review it before merging

jsoref · 2019-05-03T14:19:09Z

fwiw, my jenkins uses these links for all sorts of things (including protecting builds from deletion).

jglick · 2019-05-03T14:21:50Z

my jenkins uses these links for all sorts of things

I am not really sure what this is referring to—some custom plugin?—but I would urge you to use standard mechanisms instead, such as Run.keepLog.

jsoref · 2019-05-03T14:31:22Z

Custom jobs/scripts--mostly shell scripts. Some to retrieve archives for use in other stages in a psuedo-pipeline. Some to delete a portion of a run because it's a lot of disk space. But, this isn't about keeping the log, it's about keeping artifacts, so Run.keepLog is not going to help me.

jglick · 2019-05-03T14:46:21Z

Custom jobs/scripts--mostly shell scripts.

Then you would be one of the users who could run the new plugin until you have time to switch to supported techniques that do not rely on direct access to $JENKINS_HOME.

retrieve archives for use in other stages in a pseudo-pipeline

Normally done in actual Pipeline using stash, or in freestyle projects using the Copy Artifact plugin.

this isn't about keeping the log, it's about keeping artifacts, so Run.keepLog is not going to help me.

Despite the name, this field is about keeping the entire build, not merely the log file.

jsoref · 2019-05-03T17:53:46Z

That's a horrible name. I don't have the energy to file a bug w/ a PR to try to fix it, but someone should. The documentation says:

Marks this build to keep the log.

Same for Run.isKeepLog:

Returns true if this log file should be kept and not deleted. This is used as a signal to the BuildDiscarder.

If it means something else, it's doing a really horrible job of explaining itself. No one in their right mind would divine it means anything else.

jsoref · 2019-05-03T20:15:07Z

So, I looked, for one project, we have ~15 downstreams using custom code (we have at least 2 flavors of that project, so we're talking about at least 30 projects). We didn't have the copy-archive plugin installed, and the fact that it has bugs in its test coverage (which I'm about to fix) doesn't earn it any warm fuzzies from me.

I'm willing to offer help w/ this and the related PRs, but the cost for me to migrate 30+ projects (most of which are dead men walking, even if for 3 years) isn't worth it.

We don't rely on $JENKINS_HOME, we use https:// urls to get to the artifacts, those happen to include support for the symlinks.

jglick · 2019-05-03T20:24:39Z

We didn't have that plugin installed

Sorry, I have lost track of what you are talking about here.

We don't rely on $JENKINS_HOME, we use https:// urls to get to the artifacts, those happen to include support for the symlinks.

No, it does not. Stapler routing suffices for Job to be able to serve paths such as lastSuccessfulBuild, whose functionality is unchanged by this PR. The only people who would need to add the build-symlink plugin would be those who are running odd scripts on the master like

ftp /var/jenkins/jobs/upstream/builds/lastStable/artifacts/upstream.war www@something:/var/wars

that bypass the Jenkins server and directly rely on the existence of symbolic links to build directories.

jsoref · 2019-05-03T20:37:10Z

Err, sorry...

We have two things that from my perspective rely on the symbolic names:

downstream freestyle projects that retrieve files from upstream artifacts via https -- which in theory we could port to the copy-artifact plugin -- but where the time cost to do so is prohibitive.
build job cleanup (which tends to delete artifacts w/o deleting build logs) which run locally on the jenkins master file system ($JENKINS_HOME) and rely on the symlinks to decide which build artifacts to keep.

We'll install your plugin (the repo either isn't public yet or doesn't exist yet) in order to satisfy 2.

I've looked at some of the build cleanup plugins and their intelligence doesn't seem to do a good job of mapping to my understanding of what we're trying to do. It's possible that w/ the Run.keepLog stuff we could achieve it, but it's a lot easier to think and manage symlinks than to try to use some magical poorly documented java which has nothing to do w/ shell scripts which is where our old build system is at the time that it cares about such things.

From a documentation perspective, if the https:// ... symbolic names will still work, your LTS release notes should explain that so that people like me don't panic if that's all they rely on.

jglick · 2019-05-03T20:43:14Z

the repo either isn't public yet or doesn't exist yet

Oops, forgot to fix URL after it was forked to @jenkinsci.

I've looked at some of the build cleanup plugins

BTW I think I recall a GSoC proposal in this area.

your LTS release notes should explain that

Good point; edited proposed changelog entry accordingly.

oleg-nenashev · 2019-05-22T07:54:37Z

@jsoref What is your final opinion about the PR?

I've looked at some of the build cleanup plugins

BTW I think I recall a GSoC proposal in this area.

There was, but unfortunately it did not get to GSoC this year. Though @nisarg1499 is still interested in the area, so you could coordinate in https://gitter.im/jenkinsci/gsoc-build-discarder-project (which could be probably renamed now). JIRA tickets for specific issues would be also appreciated.

oleg-nenashev · 2019-05-31T05:16:21Z

@jsoref gentle ping. If you are not interested to continue reviewing this PR, I would like to go ahead with final review/merge

jsoref · 2019-05-31T09:01:26Z

I'm ok with it. Sorry didn't realize you were waiting on me. Is enough done for us to install that other plugin?

jglick · 2019-05-31T12:03:38Z

Is enough done for us to install that other plugin?

I am holding off on a release until this PR is merged.

oleg-nenashev · 2019-06-14T08:25:05Z

I had no time to review it, and my apologies for body-blocking the PR for a while.
Taking the feedback from others, I am happy to merge it. @jglick is it ready to go?

jglick · 2019-06-14T11:55:03Z

apologies for body-blocking the PR

Well we had symlinks for a decade, it is certainly no rush to remove them!

is it ready to go?

As far as I am concerned it is.

jglick · 2019-06-27T15:14:23Z

Latest test failure looks like a flake:

org.junit.ComparisonFailure: expected:<[hello]> but was:<[]>
	at org.junit.Assert.assertEquals(Assert.java:115)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at hudson.cli.PlainCLIProtocolTest.ignoreUnknownOperations(PlainCLIProtocolTest.java:126)

oleg-nenashev · 2019-07-11T14:56:45Z

@jglick taking the review approvals, please feel free to proceed with the merge and to coordinate the plugin release accordingly

…this plugin.

jglick added 2 commits April 12, 2019 15:50

Deprecating Run.updateSymlinks.

cb938ae

Removing some tests from AbstractProjectTest that are now expected to…

e638b67

… fail in core.

jglick mentioned this pull request Apr 12, 2019

[JENKINS-52781] Making a tar archive now keeps the symlinks #3569

Closed

3 tasks

jglick requested a review from PierreBtz April 12, 2019 20:12

jglick added 2 commits April 12, 2019 16:21

Moved a test out of JenkinsBuildsAndWorkspacesDirectoriesTest.

646e23e

Rewriting cache system in PeepholePermalink to use plain text files r…

83a465e

…ather than symlinks.

jglick marked this pull request as ready for review April 13, 2019 16:42

PierreBtz approved these changes Apr 13, 2019

View reviewed changes

Wadeck approved these changes Apr 15, 2019

View reviewed changes

core/src/main/java/jenkins/model/PeepholePermalink.java Outdated Show resolved Hide resolved

Typo in comment.

1dfe6ef

Co-Authored-By: jglick <jglick@cloudbees.com>

jglick added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 17, 2019

This was referenced Apr 17, 2019

[JENKINS-37862] Category for build-symlink jenkins-infra/update-center2#270

Merged

[JENKINS-37862] Added plugin-build-symlink.yml jenkins-infra/repository-permissions-updater#1100

Merged

batmat requested review from batmat and removed request for batmat April 18, 2019 10:41

jglick removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 19, 2019

jglick added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 22, 2019

oleg-nenashev added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Apr 26, 2019

oleg-nenashev self-requested a review April 26, 2019 07:59

oleg-nenashev removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 26, 2019

jglick mentioned this pull request May 3, 2019

[JENKINS-56643] Util.createSymlink: prefer Files.move(..., ATOMIC_MOVE) #3941

Merged

10 tasks

jglick mentioned this pull request May 3, 2019

Run.keepLog-related names were misleading #4012

Merged

oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed on-hold This pull request depends on another event/release, and it cannot be merged right now labels Jun 14, 2019

jglick mentioned this pull request Jun 17, 2019

[JENKINS-52781] Filter symlinks out when shelving a project jenkinsci/shelve-project-plugin#19

Merged

Merge branch 'master' into build-symlink-JENKINS-37862

6af78cb

jglick merged commit ece68cd into jenkinsci:master Jul 11, 2019

jglick deleted the build-symlink-JENKINS-37862 branch July 11, 2019 19:04

jglick added a commit to jenkinsci/build-symlink-plugin that referenced this pull request Jul 11, 2019

jenkinsci/jenkins#3982 merged, so preparing to do initial release of …

db16942

…this plugin.

jglick mentioned this pull request Sep 26, 2019

[JENKINS-56809] LAST_COMPLETED_BUILD should be a PeepholePermalink #4228

Merged

	static @Nonnull File storageFor(@Nonnull File buildDir) {
	private static @Nonnull File storageFor(@Nonnull File buildDir) {

[JENKINS-37862] Removing build symlink functionality #3982

[JENKINS-37862] Removing build symlink functionality #3982

Conversation

jglick commented Apr 12, 2019 • edited Loading

Proposed changelog entries

Desired reviewers

jglick commented Apr 13, 2019

PierreBtz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PierreBtz Apr 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wadeck left a comment

Choose a reason for hiding this comment

jglick commented Apr 17, 2019

jglick commented Apr 19, 2019

daniel-beck commented Apr 19, 2019

PierreBtz commented Apr 19, 2019

jglick commented Apr 19, 2019

daniel-beck commented Apr 19, 2019

jglick commented Apr 19, 2019

jglick commented Apr 19, 2019

daniel-beck commented Apr 19, 2019

oleg-nenashev commented Apr 26, 2019

jsoref commented May 3, 2019

jglick commented May 3, 2019

jsoref commented May 3, 2019

jglick commented May 3, 2019

jsoref commented May 3, 2019

jsoref commented May 3, 2019 • edited Loading

jglick commented May 3, 2019

jsoref commented May 3, 2019

jglick commented May 3, 2019

oleg-nenashev commented May 22, 2019

oleg-nenashev commented May 31, 2019

jsoref commented May 31, 2019

jglick commented May 31, 2019

oleg-nenashev commented Jun 14, 2019

jglick commented Jun 14, 2019

jglick commented Jun 27, 2019

oleg-nenashev commented Jul 11, 2019

jglick commented Apr 12, 2019 •

edited

Loading

PierreBtz Apr 13, 2019 •

edited

Loading

jsoref commented May 3, 2019 •

edited

Loading