TargetLocker.Refresh and .Unlock API changes, clean up tests

[rojer9-fb]

Change the semantics of TargetLocker.Refresh to require that owner holds locks for the requested targets (having an expired lock for the same job is also permitted).

This is necessary for job resumption: when resuming a paused job we need to be sure that targets have not in the mean time been assigned to other jobs and thus had their state interfered with.

Some drive-by improvements:

• Added TargetLocker.Close() to clean up locker instances. Used in tests.
• Parametrise lock lease length, expose as -targetLockDuration argument and increase the default to 10 minutes.
• Unify testing for TargetLocker implementations: use the same suite of tests for both inmemory and dblocked implementations, resolved subtle differences exposed by this.
• Passing empty target list to the locker API is now an error (used to be allowed by dblocked but not inmemory).
• Trying to unlock a target that was not locked by the specified job is an now an error.
• Wire clock.Clock through job manager and runner API, for use in tests. Actual use is a TODO (turned out to be non-trivial)
• Speed up job manager shutdown (speeds up tests)

[codecov[bot]]

Codecov Report

Merging #262 (a8e6c04) into master (058ccb5) will increase coverage by 0.30%. The diff coverage is 86.66%.

:exclamation: Current head a8e6c04 differs from pull request most recent head b984eb7. Consider uploading reports for the commit b984eb7 to get more accurate results

@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
+ Coverage   65.43%   65.74%   +0.30%     
==========================================
  Files         121      121              
  Lines        5379     5445      +66     
==========================================
+ Hits         3520     3580      +60     
- Misses       1463     1467       +4     
- Partials      396      398       +2     

Flag	Coverage Δ
integration	59.96% <87.24%> (+0.57%)	:arrow_up:
integration_storage	∅ <ø> (∅)
unittests	43.94% <0.00%> (-2.04%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
plugins/targetlocker/noop/noop.go	75.00% <0.00%> (-6.82%)	:arrow_down:
tests/plugins/teststeps/slowecho/slowecho.go	54.54% <50.00%> (-2.13%)	:arrow_down:
pkg/runner/job_runner.go	61.29% <60.71%> (-0.14%)	:arrow_down:
pkg/jobmanager/options.go	72.72% <83.33%> (+6.06%)	:arrow_up:
plugins/targetlocker/dblocker/dblocker.go	77.27% <89.13%> (+2.27%)	:arrow_up:
pkg/jobmanager/jobmanager.go	76.80% <100.00%> (+0.37%)	:arrow_up:
pkg/target/locker.go	100.00% <100.00%> (ø)
plugins/targetlocker/inmemory/inmemory.go	98.24% <100.00%> (+7.62%)	:arrow_up:
tests/integ/jobmanager/common.go	90.42% <100.00%> (+0.09%)	:arrow_up:
pkg/cerrors/cerrors.go	0.00% <0.00%> (-33.34%)	:arrow_down:
... and 2 more

---

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 058ccb5...b984eb7. Read the comment docs.

[rojer9-fb]

i doubt it because inmemory locker fails requests with no targets.

[tfg13]

Ha you are right (D25060297)

[rojer9-fb]

rebased

[rojer9-fb]

PTAL