Add a config key `cache_revs` to specify how many revs to keep
ClosedPublic

Authored by mmodell on Nov 27 2017, 10:04 PM.

Details

Maniphest Tasks
T181176: Need to make the number of cached revisions configurable
Reviewers
thcipriani
demon
Group Reviewers
Release-Engineering-Team
Commits
rMSCA70c0e1a5074a: Add a config key `cache_revs` to specify how many revs to keep
Patch without arc
git checkout -b D900 && curl -L https://phabricator.wikimedia.org/D900?download=true | git apply
Summary

This should be less necessary after T137124: Scap3 submodule space issues is resolved but why not.

Fixes T181176

We <3 scap.cfg

Test Plan

didn't test it

Diff Detail

Repository
rMSCA Scap
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mmodell created this revision.Nov 27 2017, 10:04 PM
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptNov 27 2017, 10:04 PM
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
mmodell requested review of this revision.Nov 27 2017, 10:05 PM
demon accepted this revision.Nov 28 2017, 9:20 PM
This revision is now accepted and ready to land.Nov 28 2017, 9:20 PM
thcipriani requested changes to this revision.Nov 28 2017, 9:59 PM

cache_revs needs to be added to DEPLOY_CONF for the scap deploy subcommand so that it gets added to .git/DEPLOY_HEAD, otherwise deploy-local will not have access to that var.

This revision now requires changes to proceed.Nov 28 2017, 9:59 PM
mmodell updated this revision to Diff 2371.Nov 29 2017, 6:54 AM

Add cache_revs to the DEPLOY_CONF list

This is needed so that the flag will be included in DEPLOY_HEAD

Also: sort the DEPLOY_CONF list almost alphabetically.

thcipriani accepted this revision.Nov 29 2017, 4:57 PM

Seems to work as expected, although I made a note about some old logic that looks like it would result in keeping more rev_dirs than expected (not a big deal, but something to keep in mind).

scap/context.py
184

Now that I'm looking at this again, this logic is probably not correct. We should probably remove the off_limits directories from the set before removing the cache_revs oldest directories.

The current logic doesn't work as expected in the case of a manual deploy of an old revision and a subsequent manual deploy of another old revision that then fails. This would mean that we have 2 old offlimits directories, then another n cache_revs. So we skip removing n old cache_revs (let's say 2), then we have 2 off_limits revs which would leave 4 directories. Anyway, I think this is all solved by doing something like (untested):

final_rev_dirs = [dir for dir in rev_dirs_by_ctime if dir not in off_limits]
cache_revs = max(cache_revs - len(off_limits), 0)
for rev_dir in final_rev_dirs[cache_revs:]:
...
This revision is now accepted and ready to land.Nov 29 2017, 4:57 PM
This revision was automatically updated to reflect the committed changes.