master: Add an option to janitor to delete logs for specific builders. #6404

rufinio · 2022-02-16T14:41:56Z

Contributor Checklist:

I have updated the unit tests
I have created a file in the newsfragments directory (and read the README.txt in that directory)
I have updated the appropriate documentation

There is a need for data from different builders to be kept for different lengths of time before it can be deleted.
This change adds the ability to specify the duration before data is deleted per buildername.

p12tic · 2022-03-13T10:52:04Z

master/buildbot/configurators/janitor.py

@@ -39,43 +40,67 @@ class LogChunksJanitor(BuildStep):
 name = 'LogChunksJanitor'
 renderables = ["logHorizon"]

- def __init__(self, logHorizon):
+ def __init__(self, logHorizon=None, horizonPerBuilder=None):


Buildbot wants to gradually move to snake_case naming style like the rest of Python ecosystem. Replace all horizonPerBuilder with horizon_per_builder.

p12tic · 2022-03-13T10:52:29Z

master/buildbot/configurators/janitor.py

- deleted = yield self.master.db.logs.deleteOldLogChunks(older_than_timestamp)
- self.descriptionDone = ["deleted", str(deleted), "logchunks"]
+ if self.logHorizon is not None:
+ older_than_timestamp = datetime2epoch(


Nit: should fit in a single line I think (Buildbot uses 100 chars per line).

p12tic · 2022-03-13T10:54:24Z

master/buildbot/configurators/janitor.py

+ now() - self.logHorizon)
+ deleted = yield self.master.db.logs.deleteOldLogChunks(
+ older_than_timestamp=older_than_timestamp)
+ self.descriptionDone = ["deleted", str(deleted), "logchunks"]


This should be just before return I think, because if both if conditions are true then we would set descriptionDone before step is actually finished.

OK, now I see that only either logHorizon or horizonPerBuilder is not None, not both.

p12tic · 2022-03-13T10:54:29Z

master/buildbot/configurators/janitor.py

+ older_than_timestamp = datetime2epoch(now() - self.build_data_horizon)
+ deleted = yield self.master.db.build_data.deleteOldBuildData(
+ older_than_timestamp=older_than_timestamp)
+ self.descriptionDone = ["deleted", str(deleted), "build data key-value pairs"]


This should be just before return I think, because if both if conditions are true then we would set descriptionDone before step is actually finished.

p12tic · 2022-03-13T10:54:42Z

master/buildbot/configurators/janitor.py

 return SUCCESS


 class JanitorConfigurator(ConfiguratorBase):
 """ Janitor is a configurator which create a Janitor Builder with all needed Janitor steps"""

- def __init__(self, logHorizon=None, hour=0, build_data_horizon=None, **kwargs):
+ def __init__(self, logHorizon=None, hour=0, build_data_horizon=None, horizonPerBuilder=None,
+ **kwargs):


weird indentation

def __init__(self, logHorizon=None, hour=0, build_data_horizon=None, horizonPerBuilder=None, **kwargs):

leads to flake8 error E125.

def __init__(self, logHorizon=None, hour=0, build_data_horizon=None, horizon_per_builder=None, **kwargs):

should it be like this?

p12tic · 2022-03-13T10:54:56Z

master/buildbot/configurators/janitor.py

 self.hour = hour
 self.kwargs = kwargs
+ if ((self.logHorizon is not None or self.build_data_horizon is not None)
+ and self.horizonPerBuilder is not None):


Weird indentation.

flake8 complains about my further solutions. Please show me how it should look like.

p12tic · 2022-03-13T10:56:22Z

master/buildbot/test/unit/db/test_build_data.py

@@ -226,7 +289,7 @@ def setUp(self):

 class TestRealDB(unittest.TestCase,
 connector_component.ConnectorComponentMixin,
- Tests):
+ RealTests):


Ugh :-( Thanks for noticing.

p12tic · 2022-03-13T11:06:25Z

master/buildbot/configurators/janitor.py

 self.hour = hour
 self.kwargs = kwargs
+ if ((self.logHorizon is not None or self.build_data_horizon is not None)
+ and self.horizonPerBuilder is not None):
+ bbconfig.error("JanitorConfigurator: horizonPerBuilder only " +


I think it makes sense to support all of logHorizon, build_data_horizon, log_horizon_per_builder and build_data_horizon_per_builder at the same time.

Having both global and per-builder log horizons makes sense because it's then possible to have a longer global horizon and shorter per-builder horizons. It also doesn't increase the complexity of the code.

p12tic

Looks great, thanks! I had several comments, most of them very simple to address.

I think it makes sense to support global horizon and per-builder horizons at the same time (see my comment with longer justification).

rufinio · 2022-04-01T07:38:22Z

per-builder horizon offers the possibility of using wildcards. Thus, I think global horizon can be reached by setting name to wildcard.

codecov · 2022-06-24T07:10:10Z

Codecov Report

Merging #6404 (120ae5b) into master (3ef08ef) will decrease coverage by 0.09%.
The diff coverage is 62.69%.

❗ Current head 120ae5b differs from pull request most recent head 7425235. Consider uploading reports for the commit 7425235 to get more accurate results

@@            Coverage Diff             @@
##           master    #6404      +/-   ##
==========================================
- Coverage   92.05%   91.95%   -0.10%     
==========================================
  Files         343      343              
  Lines       38744    38841      +97     
==========================================
+ Hits        35665    35718      +53     
- Misses       3079     3123      +44

Impacted Files	Coverage Δ
master/buildbot/db/builds.py	`85.20% <11.53%> (-13.40%)`	⬇️
master/buildbot/db/builders.py	`80.19% <13.63%> (-18.54%)`	⬇️
master/buildbot/db/build_data.py	`94.28% <80.00%> (-1.27%)`	⬇️
master/buildbot/configurators/janitor.py	`97.53% <100.00%> (+0.60%)`	⬆️
master/buildbot/db/logs.py	`96.19% <100.00%> (+0.27%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ef08ef...7425235. Read the comment docs.

rufinio force-pushed the janitorPerBuilder branch 14 times, most recently from fa26fff to 59cde30 Compare February 18, 2022 11:17

p12tic reviewed Mar 13, 2022

View reviewed changes

p12tic requested changes Mar 13, 2022

View reviewed changes

rufinio force-pushed the janitorPerBuilder branch 2 times, most recently from 4f034d0 to 4b6925c Compare June 27, 2022 07:39

rufinio force-pushed the janitorPerBuilder branch 3 times, most recently from d2e9e9f to 9c482e6 Compare July 4, 2022 11:59

rufinio requested a review from p12tic July 4, 2022 13:02

p12tic force-pushed the janitorPerBuilder branch from 9c482e6 to 846485d Compare August 10, 2022 14:19

master: Add an option to janitor to delete logs for specific builders.

ccacc52

p12tic force-pushed the janitorPerBuilder branch from 846485d to ccacc52 Compare September 16, 2023 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

master: Add an option to janitor to delete logs for specific builders. #6404

master: Add an option to janitor to delete logs for specific builders. #6404

rufinio commented Feb 16, 2022

p12tic Mar 13, 2022

p12tic Mar 13, 2022

p12tic Mar 13, 2022

p12tic Mar 13, 2022

p12tic Mar 13, 2022

p12tic Mar 13, 2022

rufinio Apr 6, 2022

p12tic Mar 13, 2022

rufinio Apr 6, 2022

p12tic Mar 13, 2022

p12tic Mar 13, 2022

p12tic left a comment

rufinio commented Apr 1, 2022

codecov bot commented Jun 24, 2022 •

edited

master: Add an option to janitor to delete logs for specific builders. #6404

Are you sure you want to change the base?

master: Add an option to janitor to delete logs for specific builders. #6404

Conversation

rufinio commented Feb 16, 2022

Contributor Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p12tic left a comment

Choose a reason for hiding this comment

rufinio commented Apr 1, 2022

codecov bot commented Jun 24, 2022 • edited

Codecov Report

codecov bot commented Jun 24, 2022 •

edited