[JENKINS-73824][JENKINS-73835] Remove redundant log rotation after changes in core and add regression tests related to deleting Pipeline jobs and builds #470

dwnusbaum · 2024-10-01T21:12:48Z

See JENKINS-73835 and jenkinsci/jenkins#9810. Thematically related to #468 and jenkinsci/jenkins#9790.

The upstream changes allow us to remove the direct call to getParent().logRotate() from WorkflowRun.finish() in this PR, and we can add a regression test for log rotation potentially cleaning up Pipeline builds too early.

Testing done

See new automated test.

Submitter checklist

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

…ng only on a OneOffExecutor

…nd add a regression test for LogRotator deleting incomplete builds

dwnusbaum · 2024-10-01T21:18:10Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

-            Timer.get().submit(() -> {
-                try {
-                    getParent().logRotate();
-                } catch (Exception x) {
-                    LOGGER.log(Level.WARNING, "failed to perform log rotation after " + this, x);
-                }
-            });


Originally made asynchronous in 63fdbe8. Note though that since jenkinsci/jenkins#4368, Jenkins core was also calling Job.logRotate synchronously from Run.onEndBuilding -> RunListener.onFinalized (at least with the default global build discarder configuration), so one of the calls was redundant.

With this PR only Jenkins core calls Job.logRotate, and it does so synchronously. We could make that call asynchronous if there are any concerns, but given that default configurations have been calling it synchronously for years now, it didn't seem necessary to preserve the asynchronicity.

dwnusbaum · 2024-10-01T21:18:31Z

src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java

+    @SafeVarargs
+    private static void assertPollingBaselines(List<WorkflowRun.SCMCheckout> checkouts, Matcher<Object>... indexedMatchers) {
+        assertThat("Number of checkouts should match number of matchers", checkouts.size(), equalTo(indexedMatchers.length));
+        for (int i = 0; i < checkouts.size(); i++) {
+            assertThat("Unexpected baseline for checkout at index " + i, checkouts.get(i).pollingBaseline, indexedMatchers[i]);
+        }
+    }
+
+    private static String checkoutString(GitSampleRepoRule repo, boolean changelog, boolean polling) {
+        return "    checkout(changelog:" + changelog +", poll:" + polling +
+                ", scm: [$class: 'GitSCM', branches: [[name: '*/master']], " +
+                ", userRemoteConfigs: [[url: $/" + repo.fileUrl() + "/$]]])\n";
+    }
+


Just moving these next to the relevant test for clarity.

dwnusbaum · 2024-10-01T21:19:31Z

src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java

@@ -520,7 +524,7 @@ private void assertCulprits(WorkflowRun b, String... expectedIds) throws IOExcep
        assertThat(await().until(() -> ExtensionList.lookupSingleton(CheckCompletedFlag.class).buildXml.get(b.getExternalizableId()), notNullValue()),
            containsString("<completed>true</completed>"));
    }
-    @TestExtension public static final class CheckCompletedFlag extends RunListener<WorkflowRun> {
+    @TestExtension("completedFlag") public static final class CheckCompletedFlag extends RunListener<WorkflowRun> {


Only need by this specific test and causes annoying log spam in the new test since it tries to look at build directories for deleted builds.

dwnusbaum · 2024-10-01T21:19:56Z

src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java

+            if (filesInBuildDir == null) {
+                filesInBuildDir = new String[0];
+            }
+            assertThat("Expected " + buildDirs[i] + " to be empty but saw: " + Arrays.toString(filesInBuildDir), filesInBuildDir, emptyArray());


Prior to the fix, this fails intermittently because some directory contains build.xml.

dwnusbaum · 2024-10-01T21:24:33Z

src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java

+        p.setDefinition(new CpsFlowDefinition(
+                "echo params.FOO; semaphore 'wait'", true));
+        p.addProperty(new ParametersDefinitionProperty(List.of(new StringParameterDefinition("FOO"))));
+        // Keep 0 builds, i.e. delete all builds immediately.


I am keeping 0 builds rather than 1 build to simplify test assertions. Mainly I am trying to avoid complicated issues involving the exact timing of calls to RunList.size and RunList.sublist in LogRotator.perform compared to other builds completing, which can cause builds other than the one with the largest build number to be preserved by LogRotator.

jglick · 2024-10-02T18:09:20Z

pom.xml

-        <jenkins.version>2.454</jenkins.version>
+        <!-- TODO: Waiting for https://github.com/jenkinsci/jenkins/pull/9810 to make it to an LTS line. -->
+        <jenkins.version>2.479-rc35393.97fb_6a_a_25d9b_</jenkins.version>
+        <!-- Waiting for a release of https://github.com/jenkinsci/plugin-pom/pull/1004 -->


jenkinsci/plugin-pom#1004

src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java

Co-authored-by: Jesse Glick <[email protected]>

dwnusbaum · 2024-10-14T18:50:13Z

Core PR has been merged but not yet released. I think in this case we should go ahead and release the cleanup here once the next weekly is out, even if it means we need to use a weekly as a baseline temporarily. I think the core fix is a reasonable backporting candidate as well, so I will mark it as lts-candidate, and then we could swap to the LTS line here if it does get backported.

dwnusbaum · 2024-10-15T17:40:08Z

pom.xml

@@ -63,7 +63,8 @@
    </pluginRepositories>
    <properties>
        <changelist>999999-SNAPSHOT</changelist>
-        <jenkins.version>2.479</jenkins.version>
+        <!-- TODO: Waiting for JENKINS-73824 and JENKINS-73835 to make it into an LTS line -->
+        <jenkins.version>2.481</jenkins.version>


Up to reviewers what we want to do here. The removal of the call to logRotate in WorkflowRun.finish eliminates a redundancy, but things should be fine for 2.481+ users even with the redundant log rotation. We can either merge this as-is or wait for the fixes to make it into an LTS line.

pom.xml

jglick

I see no problem with merging as is (just bump the BOM). Can always drop the core dep back down to 2.479.x later if the requisite core fixes are backported.

dwnusbaum added 3 commits September 26, 2024 13:47

Add test for deleting a Pipeline job while one of its builds is runni…

cd986f1

…ng only on a OneOffExecutor

[JENKINS-73824] Pick up incremental version of core and simplify test

3426a6a

[JENKINS-73835] Remove redundant log rotation after changes in core a…

bf16a19

…nd add a regression test for LogRotator deleting incomplete builds

dwnusbaum added the bug label Oct 1, 2024

dwnusbaum commented Oct 1, 2024

View reviewed changes

dwnusbaum mentioned this pull request Oct 1, 2024

[JENKINS-73835] Do not allow builds to be deleted while they are still running and ensure build discarders run after builds are fully complete jenkinsci/jenkins#9810

Merged

jglick reviewed Oct 2, 2024

View reviewed changes

jglick approved these changes Oct 2, 2024

View reviewed changes

src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java Outdated Show resolved Hide resolved

dwnusbaum and others added 6 commits October 3, 2024 14:37

[JENKINS-73835] Update to latest incremental version of core PR

3ac9611

Remove trailing space in log message

727f35b

Co-authored-by: Jesse Glick <[email protected]>

Merge branch 'master' into test-deletion

fe9e2fa

Update pom.xml

e42df03

Merge branch 'master' into test-deletion

fdc5a32

Merge remote-tracking branch 'origin/test-deletion' into test-deletion

f87c54e

Merge branch 'test-deletion' into log-rotation-cleanup

ff71764

dwnusbaum mentioned this pull request Oct 15, 2024

[JENKINS-73824] Add test for deleting a Pipeline job while one of its builds is running only on a OneOffExecutor #468

Closed

6 tasks

dwnusbaum commented Oct 15, 2024

View reviewed changes

dwnusbaum marked this pull request as ready for review October 15, 2024 17:40

dwnusbaum requested a review from a team as a code owner October 15, 2024 17:40

jglick reviewed Oct 15, 2024

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

jglick approved these changes Oct 15, 2024

View reviewed changes

Swap to 2.479.x BOM

1232095

dwnusbaum enabled auto-merge October 15, 2024 19:57

dwnusbaum merged commit 6c53109 into jenkinsci:master Oct 15, 2024
17 checks passed

basil mentioned this pull request Oct 30, 2024

Drop core dependency from 2.481 to 2.479.1 #483

Merged

jglick mentioned this pull request Jan 14, 2025

Flaky WorkflowRunTest.logRotationOnlyProcessesCompletedBuilds on Windows #502

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73824][JENKINS-73835] Remove redundant log rotation after changes in core and add regression tests related to deleting Pipeline jobs and builds #470

[JENKINS-73824][JENKINS-73835] Remove redundant log rotation after changes in core and add regression tests related to deleting Pipeline jobs and builds #470

dwnusbaum commented Oct 1, 2024

dwnusbaum Oct 1, 2024 •

edited

Loading

jglick Oct 2, 2024

dwnusbaum Oct 1, 2024

dwnusbaum Oct 1, 2024

dwnusbaum Oct 1, 2024

dwnusbaum Oct 1, 2024

jglick Oct 2, 2024

dwnusbaum commented Oct 14, 2024 •

edited

Loading

dwnusbaum Oct 15, 2024 •

edited

Loading

jglick left a comment

[JENKINS-73824][JENKINS-73835] Remove redundant log rotation after changes in core and add regression tests related to deleting Pipeline jobs and builds #470

[JENKINS-73824][JENKINS-73835] Remove redundant log rotation after changes in core and add regression tests related to deleting Pipeline jobs and builds #470

Conversation

dwnusbaum commented Oct 1, 2024

Testing done

Submitter checklist

dwnusbaum Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

jglick Oct 2, 2024

Choose a reason for hiding this comment

dwnusbaum Oct 1, 2024

Choose a reason for hiding this comment

dwnusbaum Oct 1, 2024

Choose a reason for hiding this comment

dwnusbaum Oct 1, 2024

Choose a reason for hiding this comment

dwnusbaum Oct 1, 2024

Choose a reason for hiding this comment

jglick Oct 2, 2024

Choose a reason for hiding this comment

dwnusbaum commented Oct 14, 2024 • edited Loading

dwnusbaum Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

dwnusbaum Oct 1, 2024 •

edited

Loading

dwnusbaum commented Oct 14, 2024 •

edited

Loading

dwnusbaum Oct 15, 2024 •

edited

Loading