-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
master: Add an option to janitor to delete logs for specific builders. #6404
base: master
Are you sure you want to change the base?
Conversation
fa26fff
to
59cde30
Compare
@@ -39,43 +40,67 @@ class LogChunksJanitor(BuildStep): | |||
name = 'LogChunksJanitor' | |||
renderables = ["logHorizon"] | |||
|
|||
def __init__(self, logHorizon): | |||
def __init__(self, logHorizon=None, horizonPerBuilder=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buildbot wants to gradually move to snake_case
naming style like the rest of Python ecosystem. Replace all horizonPerBuilder
with horizon_per_builder
.
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: should fit in a single line I think (Buildbot uses 100 chars per line).
now() - self.logHorizon) | ||
deleted = yield self.master.db.logs.deleteOldLogChunks( | ||
older_than_timestamp=older_than_timestamp) | ||
self.descriptionDone = ["deleted", str(deleted), "logchunks"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, now I see that only either logHorizon or horizonPerBuilder is not None, not both.
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"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flake8 complains about my further solutions. Please show me how it should look like.
@@ -226,7 +289,7 @@ def setUp(self): | |||
|
|||
class TestRealDB(unittest.TestCase, | |||
connector_component.ConnectorComponentMixin, | |||
Tests): | |||
RealTests): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh :-( Thanks for noticing.
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 " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
per-builder horizon offers the possibility of using wildcards. Thus, I think global horizon can be reached by setting name to wildcard. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
4f034d0
to
4b6925c
Compare
d2e9e9f
to
9c482e6
Compare
9c482e6
to
846485d
Compare
846485d
to
ccacc52
Compare
Contributor Checklist:
newsfragments
directory (and read theREADME.txt
in that directory)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.