Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Add support for client-side scheduling #2

Merged
merged 14 commits into from
Oct 20, 2023
Merged

Add support for client-side scheduling #2

merged 14 commits into from
Oct 20, 2023

Conversation

richfitz
Copy link
Member

@richfitz richfitz commented Oct 19, 2023

This PR adds a scheduler to run on the clients. I've not tried this out yet in anger, but will do so on Friday.

With this PR we can run a long-running process on production/production2 that will ship data to annex2, running daily.

Some things here:

  • I've used the timezone argument to yacron to make the cron commands interpretable against the local time even when the container is running in utc (the default).
  • we can expose the yacron web service out of the container, which will be nice for getting metrics out from backups, and better than doing something weird ourselves I think
  • I've run the generated yaml through the yacron validator before writing it out, which makes up for the fact that I am not using a yaml emitter here but just writing it out by hand like a savage
  • the scheduler has the same start/status/stop pair as the server - I took the liberty of a little light refactoring in c876866 which I snuck in just before Prototype #1 was merged
  • note also I did a really boring bit of code moving around in Break configure and check into their own modules #3, so the keys.py module is a little less sprawling than before, and had to hotfix some bugs in Fix restore bugs #4 this morning as I got backup running from production and production2

Don't be afraid about the diff - this is 168 lines of code changed within src/, the rest is tests and config

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b74e039) 100.00% compared to head (868737f) 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##              main        #2    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           24        28     +4     
  Lines         1520      1810   +290     
  Branches       177       211    +34     
==========================================
+ Hits          1520      1810   +290     
Files Coverage Δ
src/privateer2/backup.py 100.00% <100.00%> (ø)
src/privateer2/cli.py 100.00% <100.00%> (ø)
src/privateer2/config.py 100.00% <100.00%> (ø)
src/privateer2/configure.py 100.00% <100.00%> (ø)
src/privateer2/restore.py 100.00% <100.00%> (ø)
src/privateer2/schedule.py 100.00% <100.00%> (ø)
src/privateer2/service.py 100.00% <100.00%> (ø)
src/privateer2/util.py 100.00% <100.00%> (ø)
src/privateer2/yacron.py 100.00% <100.00%> (ø)
tests/test_backup.py 100.00% <ø> (ø)
... and 8 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@richfitz richfitz force-pushed the reside-352 branch 2 times, most recently from 40e17ed to c897e32 Compare October 19, 2023 16:25
@richfitz richfitz marked this pull request as ready for review October 19, 2023 17:38
Copy link
Member

@r-ash r-ash left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link

@weshinsley weshinsley left a comment

Choose a reason for hiding this comment

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

Looks good and clear - I had one thought which might save about 5 lines of code! (But might change the order - pehaps that's why you've written it that way...)

seen.add(el)
return ret


Choose a reason for hiding this comment

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

Is this equiv to:

def unique(x):
    return list(set(x))

Copy link
Member Author

Choose a reason for hiding this comment

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

that does not preserve ordering, otherwise yes that's the way to do it

Copy link
Contributor

@M-Kusumgar M-Kusumgar left a comment

Choose a reason for hiding this comment

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

LGTM! not familiar with yachron, couldnt find the @daily and @weekly options for schedule tag but im sure you know what youre doing with that!

@richfitz
Copy link
Member Author

https://github.com/josiahcarlson/parse-crontab - the cron parsing comes from this module, and is interpreted the same way as: https://crontab.guru/#@daily

I'll have a quick go firing this up in anger and see if it works...

@richfitz
Copy link
Member Author

Pretty close in the end, but the jobs failed because I was mounting things inconsistently between the backup and the schedule modules - fixed that in 04fbe5a and it works

@richfitz richfitz merged commit ae10fb0 into main Oct 20, 2023
6 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants