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

Refactor groovy script to include batching. #1008

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nuclearsandwich
Copy link
Contributor

The last several years, we've added batching to the this template script when executing it on the official build farms but never actually updated it here.

This change includes what is essentially, a rewrite of the batch process example script to more closely match the one I've been using the last year or so.

I someday hope to get even more meta about this and create a management job on Jenkins that runs variations of this script on templated job types in order to remove the need for batching and the chance for a typo when copy-pasting and modifying the example script.

The last several years, we've added batching to the this template script
when executing it on the official build farms but never actually updated
it here.

This change includes what is essentially, a rewrite of the batch process example script to more closely match the one I've been using the last year or so.

I someday hope to get even more meta about this and create a management
job on Jenkins that runs variations of this script on templated job types in order to remove the need for batching and the chance for a typo when copy-pasting and modifying the example script.
@nuclearsandwich nuclearsandwich requested a review from cottsay June 26, 2023 20:45
@nuclearsandwich nuclearsandwich self-assigned this Jun 26, 2023

The following Groovy script is a good starting point for various actions:

.. code-block:: groovy

import hudson.model.Cause
DISTRO_LETTER = "F"
PREFIXES = ["ci_", "dev_", "pr_", "rel", "src_", "bin_"].collect({pre -> DISTRO_LETTER + pre})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be overkill to add a DISTRO_LETTER constant one line before its only use but I like making the bits that are meant to be adjusted very obvious rather than just hard-coding the letter into the map/collect that generates all of the prefixes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as an FYI, if we wanted to get just a bit fancy, we could do this at the top:

DISTRO_NAME = "foxy"
DISTRO_LETTER = DISTRO_NAME[0..0].toUpperCase()
PREFIXES = [DISTRO_NAME]
PREFIXES += ["ci_", "dev_", "pr_", "rel", "src_", "bin_"].collect({pre -> DISTRO_LETTER + pre})   

That way we'll pick up the jobs that are prefixed with the DISTRO_NAME as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this is also missing the doc jobs, so it should also be added to the list above.

Comment on lines +110 to +116
/* Some operations take time and an http timeout
* creates performance issues with dangling script
* runs.
* To prevent them use a batch size that returns
* fairly instant results and run the script multiple
* times
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hoisted this comment up into the documentation above as well. I am torn between leaving it in the script and cutting it since it's documented above.

{
if (count >= BATCH_SIZE)
{
println("Reached ${BATCH_SIZE} limit before processing ${job.name}.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see someone omitting the print of processed job names, which is otherwise the only feedback when processing batches that you aren't processing the same 100 jobs over again (such as if there's a mismatch between the predicate and operation) so I added this print as an indicator of where we stopped.

If I was more bored I might be tempted to implement some kind of cursor pattern.


if (count <= BATCH_SIZE)
{
println("Completed execution of the last batch.")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've always previously just run the script until it returns 0 results but this way the script positively reports that it's processed the last batch (based on the fact that the batch is smaller than the max batch size).

It is possible if the number of jobs to process is precisely divisible by the batch size that this will print only after an empty batch, but it should still print.

}

This script will print only the matched job names.
You can uncomment any of the actions to disable, enable, trigger or delete these projects.
You can uncomment any of the actions to disable, enable, or delete these projects.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the instructions for triggering builds using this script because the _trigger-jobs jobs should cover those use cases now and there is not an entirely reasonable way to track that state using the batch setup.

Copy link

@quarkytale quarkytale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, but wouldn't mind waiting for Scott's comments.

Comment on lines +88 to +90
Some operations take time and an HTTP timeout creates performance issues with dangling script runs.
To prevent them the example script uses a ``BATCH_SIZE`` constant to control how many jobs to change before returning.
You can leave the default, fairly conservative batch size or increase that constant steadily until results are no longer instantaneous and then adjusting back down.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, does it process jobs even if they're processed/disabled in the previous iteration?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fair to say that in this context, "processing" means changing. That's what seems to take significant time and causes the script invocation to time out.

Simply inspecting all of the jobs can happen in every invocation. The continue in the loop for each should avoid incrementing the counter if the operation would have been a no-op, so "processing" be 1-to-1 with "changing" here.

Copy link
Contributor Author

@nuclearsandwich nuclearsandwich Jun 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! @cottsay is correct here that iterating over jobs does not take much time at all but performing operations on them (enable, disable, delete) does.

So each subsequent run of the script would iterate over a growing number of (previously processed jobs) but checking the predicate is not very time intensive compared to actually performing an operation, so it does not have much of an observable effect on the return time of the script.

I resisted the urge to do even more programming and create predicate,operation pairs and setting another top-level variable so that we could actually add the operation-predicate to the filter closure that's passed to getItems and thus only jobs that meet the predicate would be iterated over.

Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Comment on lines +88 to +90
Some operations take time and an HTTP timeout creates performance issues with dangling script runs.
To prevent them the example script uses a ``BATCH_SIZE`` constant to control how many jobs to change before returning.
You can leave the default, fairly conservative batch size or increase that constant steadily until results are no longer instantaneous and then adjusting back down.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fair to say that in this context, "processing" means changing. That's what seems to take significant time and causes the script invocation to time out.

Simply inspecting all of the jobs can happen in every invocation. The continue in the loop for each should avoid incrementing the counter if the operation would have been a no-op, so "processing" be 1-to-1 with "changing" here.

Copy link

@quarkytale quarkytale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edits from the recent run

doc/ongoing_operations.rst Outdated Show resolved Hide resolved
doc/ongoing_operations.rst Show resolved Hide resolved
nuclearsandwich and others added 3 commits July 5, 2023 13:41
Groovy does not allow a closure variable to shadow another one.

Co-authored-by: Dharini Dutia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants