[JENKINS-2111] Managed workspace indices #129

jglick · 2018-09-21T23:57:35Z

JENKINS-2111: the long-discussed overhaul of workspace selection at least for branch projects. Unlikely the previous technique from #50 (JENKINS-34564), the workspace path stays short (like JENKINS-30148) as we do not need to add any cryptographic hash to prevent collisions between projects: workspace subdirectories are allocated and tracked, so we can just add a simple uniquifying suffix as needed. That should allow JENKINS-38706 to be closed. Also handles renames (JENKINS-22240).

jglick · 2018-09-21T23:59:49Z

pom.xml

@@ -29,7 +29,7 @@
  <parent>
    <groupId>org.jenkins-ci.plugins</groupId>
    <artifactId>plugin</artifactId>
-    <version>2.37</version>
+    <version>3.22</version>


Just for fun.

jglick · 2018-09-22T00:00:22Z

pom.xml

@@ -65,7 +65,8 @@
  </scm>

  <properties>
-    <jenkins.version>1.642.3</jenkins.version>
+    <jenkins.version>2.73.3</jenkins.version>


Made various things more convenient, such as jenkinsci/jenkins#2914.

jglick · 2018-09-22T00:00:47Z

pom.xml

@@ -85,16 +86,6 @@
  </pluginRepositories>

  <dependencies>
-    <dependency>
-      <groupId>org.kohsuke</groupId>
-      <artifactId>access-modifier-annotation</artifactId>


These should not have been here.

jglick · 2018-09-22T00:00:59Z

src/main/java/jenkins/branch/BranchProjectFactory.java

@@ -204,7 +204,7 @@ public P decorate(P project) {
            List<BranchProperty> properties = new ArrayList<BranchProperty>(branch.getProperties());
            Collections.sort(properties, DescriptorOrder.reverse(BranchProperty.class));
            for (BranchProperty property : properties) {
-                JobDecorator<P, R> decorator = property.jobDecorator(project.getClass());
+                JobDecorator<P, R> decorator = property.jobDecorator((Class) project.getClass());


Incomprehensible compiler errors in -source 8.

jglick · 2018-09-22T00:01:18Z

src/main/java/jenkins/branch/NameMangler.java

@@ -41,7 +41,7 @@
 */
 public final class NameMangler {

-    private static final int MAX_SAFE_LENGTH = 32;
+    static final int MAX_SAFE_LENGTH = 32;


This is now the default being applied to workspace lengths.

jglick · 2018-09-22T00:06:20Z

src/main/java/jenkins/branch/WorkspaceLocatorImpl.java

    static /* not final */ int PATH_MAX = Integer.getInteger(WorkspaceLocatorImpl.class.getName() + ".PATH_MAX", PATH_MAX_DEFAULT);

+    static /* not final */ int MAX_LENGTH = Integer.getInteger(WorkspaceLocatorImpl.class.getName() + ".MAX_LENGTH", NameMangler.MAX_SAFE_LENGTH);
+
+    static /* not final */ boolean MULTIBRANCH_ONLY = Boolean.parseBoolean(System.getProperty(WorkspaceLocatorImpl.class.getName() + ".MULTIBRANCH_ONLY", /* TBD */ "true"));


Could be set to false to turn on the smarter behavior for all projects, including standalone Pipeline and even freestyle, matrix, etc. On the one hand I do not like having installation of what is otherwise a library plugin affect behavior of core functionality. On the other hand, the name shortening is very useful for projects in deep folder hierarchies; the character sanitization is useful for all sorts of project generation systems other than multibranch; and the automatic cleanup is useful for pretty much everyone, since WorkspaceCleanupThread is unreliable.

jglick · 2018-09-22T00:07:44Z

src/main/java/jenkins/branch/WorkspaceLocatorImpl.java

+    }
+
+    private static FilePath locate(TopLevelItem item, Node node, boolean create) {
+        if (MULTIBRANCH_ONLY && !(item.getParent() instanceof MultiBranchProject)) {


Maybe could be a three-way flag to disable the feature altogether?

jglick · 2018-09-22T01:07:43Z

Test failure is due to a Windows FS problem which I am tracking down.

jglick · 2018-09-24T15:09:08Z

Seems to be another Windows test failure I cannot reproduce locally and which looks unrelated to my changes. Will see if it goes away after planned test expansion commits.

… example.

…ms to be very flaky.

WindowsPathParser.reservedChars = "<>:\"|?*"

…oyed on ci.jenkins.io and/or tests depend on that version.

svanoort · 2018-10-05T13:34:50Z

@joseblas or @dwnusbaum Could one of you give this a peek please? If you think it's solid, happy to see it released.

jglick · 2018-10-05T16:30:06Z

If you are reviewing, then you should also check an update to the integration test in jenkinsci/workflow-multibranch-plugin#82, plus the two other PRs that depends on.

dwnusbaum · 2018-10-05T13:39:09Z

src/main/java/jenkins/branch/WorkspaceLocatorImpl.java

-            if (!workspaceDir.contains("ITEM_FULL")) {
-                LOGGER.log(Level.WARNING, "JENKINS-34564 path sanitization ineffective when using legacy Workspace Root Directory ‘{0}’; switch to $'{'JENKINS_HOME'}'/workspace/$'{'ITEM_FULLNAME'}' as in JENKINS-8446 / JENKINS-21942", workspaceDir);
+        if (fullName.contains("\n") || fullName.equals(INDEX_FILE_NAME)) {
+            throw new IllegalArgumentException(); // better not to mess around


Can we add a message and the invalid name to the exception?

Yes, will do.

dwnusbaum · 2018-10-05T18:39:18Z

src/main/java/jenkins/branch/WorkspaceLocatorImpl.java

-                "ITEM_FULLNAME", itemFullName,   // legacy, deprecated
-                "ITEM_FULL_NAME", itemFullName.replace(':','$'))); // safe, see JENKINS-12251
+    private static void save(Map<String, String> index, FilePath workspace) throws IOException, InterruptedException {
+        // FilePath.renameTo does not support REPLACE_EXISTING, and AtomicFileWriter does not work remotely.


FilePath.renameTo not using REPLACE_EXISTING might just have been something we missed when reviewing jenkinsci/jenkins#3173, although based on the File#renameTo documentation it looks like the behavior when a file existed was previously platform dependent so it could have gone either way. Do you think it would be helpful to add the flag? (Doesn't matter for this PR, but better to clean up the default flags sooner than later if they aren't useful)

the behavior when a file existed was previously platform dependent so it could have gone either way

Exactly. It seems that the original behavior on Windows did replace an existing file and the new behavior does not. The renameTo Javadoc does not go into any details about behavior so it is a matter of interpretation whether this is a core bug or not.

src/main/java/jenkins/branch/WorkspaceLocatorImpl.java

dwnusbaum · 2018-10-05T20:35:15Z

src/main/java/jenkins/branch/WorkspaceLocatorImpl.java

-                "ITEM_FULL_NAME", itemFullName.replace(':','$'))); // safe, see JENKINS-12251
+    private static void save(Map<String, String> index, FilePath workspace) throws IOException, InterruptedException {
+        // FilePath.renameTo does not support REPLACE_EXISTING, and AtomicFileWriter does not work remotely.
+        // Anyway we are synchronizing access to this file so the only potential problem is half-written content.


Maybe you could add something like the expected number of pairs to the first line of the file to help diagnose a corrupted index in #load, although with os/disk caches I'm not sure how likely it would be for that data to be written correctly if there is a problem with the rest of the file.

I'm not sure how likely it would be for that data to be written correctly if there is a problem with the rest of the file

My feelings as well.

Probably best to bite the bullet and copy-paste some code from AtomicFileWriter.

…spaces.txt.

vivek · 2018-11-08T03:18:55Z

@dwnusbaum good to merge this one?

stephenc · 2018-11-08T07:31:51Z

As I understand, one of the biggest issues for Windows systems is too long a path, for some people even the 32 character mangled names may prove too long.

It would be awesome - now we have tracking - to allow a per-node configurable strategy (default is to use the name mangler, and only default provided here)

That way a Windows User could install a mangler that just picks an available one or two character name (failing up to 3+ characters) where they are willing to trade knowing which workspace is which from the name for shortest possible paths and having to look up in the index file

dwnusbaum · 2018-11-08T14:10:00Z

@vivek It looks fine to me, but I am not very familiar with this API, so it would probably be best for @rsandell or @stephenc to review it.

jglick · 2018-11-08T20:31:57Z

allow a per-node configurable strategy […so] a Windows User could install a mangler that just picks an available one or two character name (failing up to 3+ characters)

Might be overkill. I think the new default maximum should be small enough for most users (assuming they use a reasonably concise remoteFS); if not, they can still run with

-Djenkins.branch.WorkspaceLocatorImpl.MAX_LENGTH=10

or whatever.

Note that the max length includes any applied uniquification suffix, so it is really a hard limit¹ and you cannot set the max so small that an exponentially large number of workspaces would overflow. You would of course have to have an absurdly large number of Jenkins projects for that to be an issue.

¹This does not apply to suffixes added by WorkspaceList, such as @2, or the @tmp added by various build steps.

jglick · 2018-11-08T20:33:05Z

Of course we could also just set a smaller default max value on Windows. I do not have a strong opinion either way. Can always be tweaked later.

daniel-beck · 2019-10-04T14:40:52Z

The logging here is really annoying with masters that don't have executors (so their master workspace path shouldn't matter). From ci.j.io:

Oct 04, 2019 12:05:02 PM WARNING jenkins.branch.WorkspaceLocatorImpl getWorkspaceRoot
JENKINS-2111 path sanitization ineffective when using legacy Workspace Root Directory ‘${ITEM_ROOTDIR}/workspace’; switch to ‘${JENKINS_HOME}/workspace/${ITEM_FULL_NAME}’ as in JENKINS-8446 / JENKINS-21942
Oct 04, 2019 12:05:02 PM FINE jenkins.branch.WorkspaceLocatorImpl locate
no available workspace root for hudson.model.Hudson@2ad98027 so skipping org.jenkinsci.plugins.workflow.job.WorkflowJob@615a99f0[Infra/infra-reports/PR-22]
Oct 04, 2019 12:07:01 PM WARNING jenkins.branch.WorkspaceLocatorImpl getWorkspaceRoot
JENKINS-2111 path sanitization ineffective when using legacy Workspace Root Directory ‘${ITEM_ROOTDIR}/workspace’; switch to ‘${JENKINS_HOME}/workspace/${ITEM_FULL_NAME}’ as in JENKINS-8446 / JENKINS-21942
Oct 04, 2019 12:07:01 PM FINE jenkins.branch.WorkspaceLocatorImpl locate
no available workspace root for hudson.model.Hudson@2ad98027 so skipping org.jenkinsci.plugins.workflow.job.WorkflowJob@723417aa[Infra/charts/master]
Oct 04, 2019 12:07:01 PM WARNING jenkins.branch.WorkspaceLocatorImpl getWorkspaceRoot
JENKINS-2111 path sanitization ineffective when using legacy Workspace Root Directory ‘${ITEM_ROOTDIR}/workspace’; switch to ‘${JENKINS_HOME}/workspace/${ITEM_FULL_NAME}’ as in JENKINS-8446 / JENKINS-21942
Oct 04, 2019 12:07:01 PM FINE jenkins.branch.WorkspaceLocatorImpl locate
no available workspace root for hudson.model.Hudson@2ad98027 so skipping org.jenkinsci.plugins.workflow.job.WorkflowJob@7b8287f7[Infra/acceptance-tests/install-lts-debian-package]

Since https://jenkins.io/changelog/#v2.119 this is more cumbersome to implement since it got caught in JENKINS-50164.

jglick · 2019-12-10T14:01:55Z

@daniel-beck

The logging here is really annoying with masters that don't have executors (so their master workspace path shouldn't matter).

No such log message should be printed for a system with no master executor, because this code path is only run when looking for a workspace on the master node. I cannot speculate as to the cause of such messages on ci.jenkins.io; would need to have some sort of debug access, or steps to reproduce from scratch.

[JENKINS-2111] Managed workspace indices.

481e485

jglick added the work-in-progress label Sep 21, 2018

jglick requested review from abayer, stephenc, rsandell, dwnusbaum and svanoort September 21, 2018 23:57

jglick commented Sep 22, 2018

View reviewed changes

jglick added 3 commits September 21, 2018 18:26

Easier to use a newer baseline. For one, FilePath methods use NIO.

21a6c3d

Index save did not work on Windows.

e7512a2

TimeUnit2

06705b4

jglick mentioned this pull request Sep 26, 2018

Update to latest parent pom and incrementals-tools jenkinsci/git-plugin#621

Closed

12 tasks

dwnusbaum mentioned this pull request Oct 2, 2018

[JENKINS-40109] Make compilation errors serializable jenkinsci/workflow-cps-global-lib-plugin#54

Merged

jglick added 8 commits October 2, 2018 14:36

Updated parent.

b92f528

Three-way mode, permitting this to be disabled altogether.

2ebbf63

Test coverage for Collector.

e71d269

Test coverage for name uniquification.

58307e1

CI test failure.

a08b992

Windows does not seem to like colons in filenames, so use a different…

74cfed1

… example.

Rename online workspaces when a project is moved or renamed.

260cdd1

Verify that the workspace was moved, not simply recreated.

848c996

jglick removed the work-in-progress label Oct 2, 2018

jglick added 3 commits October 2, 2018 17:24

given_multibranch_when_oneEventBlocking_then_otherEventsProcessed see…

184b4e3

…ms to be very flaky.

FindBugs

bdfb334

Still trying to find a job name Windows will accept.

33a99bf

WindowsPathParser.reservedChars = "<>:\"|?*"

jglick mentioned this pull request Oct 5, 2018

[JENKINS-26143] [JENKINS-53917] Integration testing of ChoiceParameterDefinition jenkinsci/workflow-multibranch-plugin#81

Merged

jglick added a commit to jglick/workflow-multibranch-plugin that referenced this pull request Oct 5, 2018

Disabling Windows tests until jenkinsci/branch-api-plugin#129 is depl…

af95c21

…oyed on ci.jenkins.io and/or tests depend on that version.

abayer approved these changes Oct 5, 2018

View reviewed changes

jglick requested a review from joseblas October 5, 2018 13:39

Incrementalified.

81000cf

jglick mentioned this pull request Oct 5, 2018

[JENKINS-2111] Integration test of new workspace naming strategy jenkinsci/workflow-multibranch-plugin#82

Merged

dwnusbaum mentioned this pull request Oct 5, 2018

Pick up various updates to shorten file names on Windows jenkinsci/workflow-cps-global-lib-plugin#56

Merged

dwnusbaum added a commit to dwnusbaum/workflow-cps-global-lib-plugin that referenced this pull request Oct 5, 2018

Pick up jenkinsci/branch-api-plugin#129 to try to shorten paths

09803ac

dwnusbaum reviewed Oct 5, 2018

View reviewed changes

jglick added 2 commits October 8, 2018 12:24

Add an exception message.

259fed5

Use a remoted TextFile to better assure an atomic file write for work…

1afb188

…spaces.txt.

dwnusbaum approved these changes Oct 23, 2018

View reviewed changes

Merge branch 'master' into WorkspaceLocatorImpl-JENKINS-2111

d938886

rsandell approved these changes Nov 9, 2018

View reviewed changes

rsandell merged commit 67c6d3d into jenkinsci:master Nov 9, 2018

rsandell mentioned this pull request Nov 9, 2018

[FIXED JENKINS-38706] Add some tweaks to make the fix usable in the real world #128

Closed

jglick deleted the WorkspaceLocatorImpl-JENKINS-2111 branch November 9, 2018 18:53

jglick added a commit to jglick/workflow-multibranch-plugin that referenced this pull request Nov 9, 2018

jenkinsci/branch-api-plugin#129 was released.

8546dc0

This was referenced Nov 9, 2018

More logging for JENKINS-2111 #133

Merged

PCT fixes #134

Merged

jglick added a commit to jglick/branch-api-plugin that referenced this pull request Nov 14, 2018

Backporting another test fix from jenkinsci#129.

b3d1130

jglick mentioned this pull request Nov 19, 2018

[JENKINS-54640] Index collision check was not working #135

Merged

jglick mentioned this pull request Mar 4, 2025

Windows agents failing to clone Git (LongPathsEnabled disabled) jenkins-infra/helpdesk#4574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-2111] Managed workspace indices #129

[JENKINS-2111] Managed workspace indices #129

jglick commented Sep 21, 2018 •

edited

Loading

jglick Sep 21, 2018

jglick Sep 22, 2018

jglick Sep 22, 2018

jglick Sep 22, 2018

jglick Sep 22, 2018

jglick Sep 22, 2018

jglick Sep 22, 2018

jglick commented Sep 22, 2018

jglick commented Sep 24, 2018

svanoort commented Oct 5, 2018

jglick commented Oct 5, 2018

dwnusbaum Oct 5, 2018

jglick Oct 8, 2018

dwnusbaum Oct 5, 2018

jglick Oct 8, 2018

dwnusbaum Oct 5, 2018

jglick Oct 8, 2018

vivek commented Nov 8, 2018

stephenc commented Nov 8, 2018

dwnusbaum commented Nov 8, 2018

jglick commented Nov 8, 2018

jglick commented Nov 8, 2018

daniel-beck commented Oct 4, 2019 •

edited

Loading

jglick commented Dec 10, 2019

[JENKINS-2111] Managed workspace indices #129

[JENKINS-2111] Managed workspace indices #129

Conversation

jglick commented Sep 21, 2018 • 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

jglick commented Sep 22, 2018

jglick commented Sep 24, 2018

svanoort commented Oct 5, 2018

jglick commented Oct 5, 2018

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

vivek commented Nov 8, 2018

stephenc commented Nov 8, 2018

dwnusbaum commented Nov 8, 2018

jglick commented Nov 8, 2018

jglick commented Nov 8, 2018

daniel-beck commented Oct 4, 2019 • edited Loading

jglick commented Dec 10, 2019

jglick commented Sep 21, 2018 •

edited

Loading

daniel-beck commented Oct 4, 2019 •

edited

Loading