Avoid saving nextBuildNumber while loading Job
#9778
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jglick
commented
Sep 23, 2024
Comment on lines
-224
to
-226
| // starting 1.28, we store nextBuildNumber in a separate file. | ||
| // but old Hudson didn't do it, so if the file doesn't exist, | ||
| // assume that nextBuildNumber was read from config.xml |
There was a problem hiding this comment.
A comment about Hudson 1.27 did not seem particularly helpful at this point!
jglick
commented
Sep 23, 2024
| try { | ||
| synchronized (this) { | ||
| this.nextBuildNumber = Integer.parseInt(f.readTrim()); | ||
| } | ||
| } catch (NumberFormatException e) { | ||
| LOGGER.log(Level.WARNING, "Corruption in {0}: {1}", new Object[] {f, e}); | ||
| //noinspection StatementWithEmptyBody | ||
| if (this instanceof LazyBuildMixIn.LazyLoadingJob) { | ||
| // allow LazyBuildMixIn.onLoad to fix it |
There was a problem hiding this comment.
It no longer does so, so simplify and use the same logic for all Job types. (In practice all jobs in common usage use LazyBuildMixIn.)
jglick
commented
Sep 23, 2024
| } | ||
| } else { | ||
| // From the old Hudson, or doCreateItem. Create this file now. |
There was a problem hiding this comment.
In fact doCreateItem calls onCreatedFromScratch, which did not create this file, so that was misleading.
jglick
commented
Sep 23, 2024
Comment on lines
+236
to
+237
| } else if (nextBuildNumber == 0) { | ||
| nextBuildNumber = 1; |
There was a problem hiding this comment.
Can happen if the file was absent when a job which had never been built was loaded from disk. The field initializer
is not called by XStream.
jglick
commented
Sep 23, 2024
| @@ -109,8 +109,8 @@ public void onLoad(ItemGroup<? extends Item> parent, String name) throws IOExcep | |||
| int max = _builds.maxNumberOnDisk(); | |||
| int next = asJob().getNextBuildNumber(); | |||
| if (next <= max) { | |||
| LOGGER.log(Level.WARNING, "JENKINS-27530: improper nextBuildNumber {0} detected in {1} with highest build number {2}; adjusting", new Object[] {next, asJob(), max}); | |||
| asJob().updateNextBuildNumber(max + 1); | |||
| LOGGER.log(Level.FINE, "nextBuildNumber {0} detected in {1} with highest build number {2}; adjusting", new Object[] {next, asJob(), max}); | |||
jglick
commented
Sep 23, 2024
| LOGGER.log(Level.WARNING, "JENKINS-27530: improper nextBuildNumber {0} detected in {1} with highest build number {2}; adjusting", new Object[] {next, asJob(), max}); | ||
| asJob().updateNextBuildNumber(max + 1); | ||
| LOGGER.log(Level.FINE, "nextBuildNumber {0} detected in {1} with highest build number {2}; adjusting", new Object[] {next, asJob(), max}); | ||
| asJob().fastUpdateNextBuildNumber(max + 1); |
Vlatombe
approved these changes
Sep 24, 2024
timja
approved these changes
Nov 1, 2024
comment-ops-bot
bot
added
the
ready-for-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I spent a while looking into a semi-reproducible test failure affecting CloudBees CI running in high availability mode. The scenario involves a job being created and then a build immediately being scheduled;
config.xmlis created and thennextBuildNumberis created shortly thereafter. If another process is loading the job in between, it can attempt to write outnextBuildNumberitself, despite not actually running any build of the job. The details are probably not relevant to OSS Jenkins users but the essential guideline is that you should not be writing files to disk just because you are loading some configuration from disk, at least not without a good reason. The file should be written only if and when a build is scheduled; its absence should be treated as an implicit value of 1. (lastBuildNumberwith an implicit value of 0 would perhaps have been more intuitive.)I did keep the clause which recomputes and writes out
nextBuildNumberin case the file existed but was corrupt (not parsable as an integer), since otherwise you might be printing an error on every Jenkins startup, if the job is never built again.Testing done
Seems to resolve a timing-dependent test failure observed in CloudBees CI proprietary code.
Proposed changelog entries
N/A, likely not interesting.
Before the changes are marked as
ready-for-merge:Maintainer checklist