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-27329 Less aggressive WorkspaceCleanupThread #3444

Merged
merged 4 commits into from May 26, 2018

Conversation

3 participants
@reinholdfuereder
Copy link
Contributor

commented May 15, 2018

See JENKINS-27329 (https://issues.jenkins-ci.org/browse/JENKINS-27329).

I dare to claim that the default behaviour of WorkspaceCleanupThread is too aggressive => this little change is by no means perfect (or admittedly even far from perfect), but IMHO a saner or slightly more defensive default behaviour.

Mind that according to

public boolean isBuilding() {
this "only" checks whether or not the last build of a job is in progress, while the JavaDoc says "Returns true if a build of this project is in progress." (cf. http://javadoc.jenkins-ci.org/hudson/model/Job.html#isBuilding--)

Proposed changelog entries

  • Internal: Less aggressive WorkspaceCleanupThread

reinholdfuereder added some commits May 15, 2018

JENKINS-27329 Less aggressive WorkspaceCleanupThread
I dare to claim that the default behaviour of WorkspaceCleanupThread is too aggressive => this little change is by no means perfect (or admittedly even far from perfect), but IMHO a saner or slightly more defensive default behaviour.

Mind that according to https://github.com/jenkinsci/jenkins/blob/9e64bcdcb4a2cf12d59dfa334e09ffb448d361e9/core/src/main/java/hudson/model/Job.java#L301 this "only" checks whether or not the last build of a job is in progress, while the JavaDoc says "Returns true if a build of this project is in progress." (cf. http://javadoc.jenkins-ci.org/hudson/model/Job.html#isBuilding--)
Dummy commit to trigger pipeline
Previous pipeline execution (https://ci.jenkins.io/blue/organizations/jenkins/Core%2Fjenkins/detail/PR-3444/2/tests) failed with one failing test that at first glance appears to be unrelated with my change(s) and looks like a flaky test?
@oleg-nenashev
Copy link
Member

left a comment

It makes the situation better IMHO, so +1.
The CI failure seems to be an ATH glitch, cc @raul-arabaolaza

if (item instanceof Job<?,?>) {
Job<?,?> j = (Job<?,?>) item;
if (j.isBuilding()) {
return false;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 20, 2018

Member

Fine logging message here would be useful

This comment has been minimized.

Copy link
@reinholdfuereder

reinholdfuereder May 22, 2018

Author Contributor

(Added fine logging message in 4th commit)

@@ -139,6 +139,14 @@ private boolean shouldBeDeleted(@Nonnull TopLevelItem item, FilePath dir, @Nonnu
}
}

// TODO this may only check the last build in fact:

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 20, 2018

Member

This is right. AbstractByuid#isBuilding() API checks only the last job, so it won't behave reliably for parallel AbstractBuild launches.

@reinholdfuereder

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2018

@raul-arabaolaza

This comment has been minimized.

Copy link
Contributor

commented May 22, 2018

Yeah, that failure seems a lot like a race condition, not sure why it happens now, and not in master, maybe is due to load in the CI system.

I would suggest ignoring in this case, thanks @reinholdfuereder for creating the ticket

@oleg-nenashev oleg-nenashev merged commit f258aff into jenkinsci:master May 26, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@reinholdfuereder

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2018

@oleg-nenashev Great, thanks a lot!

@reinholdfuereder reinholdfuereder deleted the reinholdfuereder:patch-1 branch May 27, 2018

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

@reinholdfuereder thank you too! I will see whether I can get it backported to 2.121.x

olivergondza added a commit that referenced this pull request Jun 7, 2018

[JENKINS-27329] Less aggressive WorkspaceCleanupThread (#3444)
* JENKINS-27329 Less aggressive WorkspaceCleanupThread

I dare to claim that the default behaviour of WorkspaceCleanupThread is too aggressive => this little change is by no means perfect (or admittedly even far from perfect), but IMHO a saner or slightly more defensive default behaviour.

Mind that according to https://github.com/jenkinsci/jenkins/blob/9e64bcdcb4a2cf12d59dfa334e09ffb448d361e9/core/src/main/java/hudson/model/Job.java#L301 this "only" checks whether or not the last build of a job is in progress, while the JavaDoc says "Returns true if a build of this project is in progress." (cf. http://javadoc.jenkins-ci.org/hudson/model/Job.html#isBuilding--)

* Fix compilation

* Dummy commit to trigger pipeline

Previous pipeline execution (https://ci.jenkins.io/blue/organizations/jenkins/Core%2Fjenkins/detail/PR-3444/2/tests) failed with one failing test that at first glance appears to be unrelated with my change(s) and looks like a flaky test?

* Add fine logging message

(cherry picked from commit f258aff)
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.