Sort trigger-arch build queue to push previously failed jobs to the end #23

yosifkit · 2024-02-16T19:50:33Z

This will give unbuilt items a chance to build before others get a retry.

(did a few successful run tests via Jenkins trigger-arm32v6 job)

whalelines

Looks good, a few questions

whalelines · 2024-02-16T20:28:52Z

Jenkinsfile.trigger

+					jq '.' <<<"$newFailedJobsJson" | tee pastFailedJobs.json
+				'''
+				archiveArtifacts(
+					artifacts: '*.json',


Why all .json files?

🤷 we know the directory is definitely 100% empty (deleteDir() above), so I'm not too worried about it either way

whalelines · 2024-02-16T20:29:21Z

Jenkinsfile.trigger

+				archiveArtifacts(
+					artifacts: '*.json',
+					fingerprint: true,
+					onlyIfSuccessful: true,


Does this mean something other than es.result == 'SUCCESS'? I'm wondering why the files are only archived on success when it seems these changes are focused on failures.

This confusingly refers to the success of this job, not the success of all the jobs this job is triggering (nor all the stages of this job, which each have their own status). This is a nice way to give us a safety against losing our data since we're doing a very poor-man's linked list here and one hiccup means we lose our intermediate data. 😄

(Hence pulling the previous version of this file from the lastSuccessful instead of just the most recent job.)

whalelines · 2024-02-16T20:34:20Z

Jenkinsfile.trigger

+				sh '''#!/usr/bin/env bash
+					set -Eeuo pipefail -x
+
+					jq '.' <<<"$newFailedJobsJson" | tee pastFailedJobs.json


Should the string pastFailedJobs.json be abstracted into a string since it is used several places?

Yeah, that could help cut down on typos, but it does get a little annoying to inject properly (we'd probably need to do something like env.PAST_FAILED_JOBS_FILENAME = ... or something so that it can be accessed from all the shell invocations without dealing with quoting hell).

tianon

A few really minor nits (none of which actually change the functionality so shouldn't need rigorous re-testing), but overall really solid and seems like it'll work just fine 👍

tianon · 2024-02-16T19:51:40Z

Jenkinsfile.trigger

@@ -13,6 +13,10 @@ env.BASHBREW_ARCH = env.JOB_NAME.split('/')[-1].minus('trigger-') // "windows-am
 def queue = []
 def breakEarly = false // thanks Jenkins...

+// this Json includes the number of attempts per failing buildId


Suggested change

// this Json includes the number of attempts per failing buildId

// this JSON includes the number of attempts per failing buildId

or even

Suggested change

// this Json includes the number of attempts per failing buildId

// this includes the number of attempts per failing buildId

tianon · 2024-02-16T20:35:56Z

Jenkinsfile.trigger

+						(env.pastFailedJobsJson | fromjson) as $pastFailedJobs |
+						[


Suggested change

(env.pastFailedJobsJson | fromjson) as $pastFailedJobs |

[

(env.pastFailedJobsJson | fromjson) as $pastFailedJobs

| [

tianon · 2024-02-16T20:37:07Z

Jenkinsfile.trigger

-					]
-				' builds.json
-			''').trim()
+						] | sort_by($pastFailedJobs[.buildId].count // 0)


Just to make sure:

$ jq -n '{} | .foo.bar // "baz"' "baz"

❤️

Oh, but maybe we should include some bit of my old TODO comment here, to remind us what this is doing / for?

Suggested change

] | sort_by($pastFailedJobs[.buildId].count // 0)

]

# this Jenkins job exports a JSON file that includes the number of attempts so far per failing buildId so that this can sort by attempts which means failing builds always live at the bottom of the queue (sorted by the number of times they have failed, so the most failing is always last)

| sort_by($pastFailedJobs[.buildId].count // 0)

tianon · 2024-02-16T20:40:50Z

Jenkinsfile.trigger

+					count: c,
+					identifier: identifier,
+					url: res.absoluteUrl,
+					endtime: (res.startTimeInMillis + res.duration) / 1000.0, // convert to seconds


Suggested change

endtime: (res.startTimeInMillis + res.duration) / 1000.0, // convert to seconds

endTime: (res.startTimeInMillis + res.duration) / 1000.0, // convert to seconds

tianon · 2024-02-16T20:43:10Z

Jenkinsfile.trigger

+				if (pastFailedJobs[buildObj.buildId]) {
+					c += pastFailedJobs[buildObj.buildId].count
+				}
+				// TODO maybe implement some amount of backoff? keep first url/endtime?


Suggested change

// TODO maybe implement some amount of backoff? keep first url/endtime?

// TODO maybe implement some amount of backoff? keep first url/endTime?

tianon · 2024-02-16T20:44:24Z

Jenkinsfile.trigger

+				sh '''#!/usr/bin/env bash
+					set -Eeuo pipefail -x
+
+					jq '.' <<<"$newFailedJobsJson" | tee pastFailedJobs.json


Suggested change

jq '.' <<<"$newFailedJobsJson" | tee pastFailedJobs.json

jq <<<"$newFailedJobsJson" '.' | tee pastFailedJobs.json

tianon · 2024-02-16T20:54:24Z

Jenkinsfile.trigger

 			echo(res.result)
 			if (res.result != 'SUCCESS') {
+				def c = 1
+				if (pastFailedJobs[buildObj.buildId]) {
+					c += pastFailedJobs[buildObj.buildId].count


Something to think about in the future / next round: if count isn't set for some reason (but the object exists), this will be null and we'll get an execption from java.lang.Integer#plus (shouldn't matter for this because we're the only ones creating this file and it should always be well-formed, but this will be a good reminder for if/when something goes wrong that this line might need to be more defensive against the inputs).

tianon · 2024-02-16T21:59:41Z

CI failure is #25, fixed in #26, and this file isn't tested by CI anyhow 👍

yosifkit requested a review from tianon as a code owner February 16, 2024 19:50

whalelines reviewed Feb 16, 2024

View reviewed changes

tianon reviewed Feb 16, 2024

View reviewed changes

yosifkit force-pushed the lower-retry branch from f01bbf4 to 3eae8a9 Compare February 16, 2024 21:25

Sort trigger-arch build queue to push previously failed jobs to the end

7d61be7

yosifkit force-pushed the lower-retry branch from 3eae8a9 to 7d61be7 Compare February 16, 2024 21:30

tianon approved these changes Feb 16, 2024

View reviewed changes

tianon merged commit 88a9541 into docker-library:main Feb 16, 2024

tianon deleted the lower-retry branch February 16, 2024 21:59

	// this Json includes the number of attempts per failing buildId
	// this JSON includes the number of attempts per failing buildId

	// this Json includes the number of attempts per failing buildId
	// this includes the number of attempts per failing buildId

	endtime: (res.startTimeInMillis + res.duration) / 1000.0, // convert to seconds
	endTime: (res.startTimeInMillis + res.duration) / 1000.0, // convert to seconds

	// TODO maybe implement some amount of backoff? keep first url/endtime?
	// TODO maybe implement some amount of backoff? keep first url/endTime?

	jq '.' <<<"$newFailedJobsJson" \| tee pastFailedJobs.json
	jq <<<"$newFailedJobsJson" '.' \| tee pastFailedJobs.json

Sort trigger-arch build queue to push previously failed jobs to the end #23

Sort trigger-arch build queue to push previously failed jobs to the end #23

Uh oh!

Conversation

yosifkit commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whalelines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tianon commented Feb 16, 2024

Uh oh!

Uh oh!

yosifkit commented Feb 16, 2024 •

edited

Loading