Implement --epoch-base-time #89
Conversation
src/state.rs
Outdated
| // Parse the epoch basetime if given. | ||
| let basetime: Option<time::OffsetDateTime> = config.into(); | ||
| // If no epoch basetime was specified, use the startup time. | ||
| let basetime = basetime.unwrap_or(starttime); |
There was a problem hiding this comment.
could we use unix epoch as the default start time?
There was a problem hiding this comment.
Why unix epoch in particular?
There was a problem hiding this comment.
just out of convenience. the time crate already defines a UNIX_EPOCH constant
There was a problem hiding this comment.
My objection is that all of these configuration switches are about tuning the config for testing and experimentation. Having a fixed base time by default means running with --first-epoch is surprising because it (usually) won't start with the given value. Doing the naive thing and requesting and evaluation with that epoch will fail!
Anything that's surprising is extra cognitive load for someone trying out the software for the first time. Maybe it actually makes sense for their application to have a fixed rotation schedule, like it does for ours, but they have to go track down why it works that way, update their own work to accommodate, etc. So it's not a good default.
On the other hand, if we're hard-coding a fixed base time for our application, we don't even need the switch; we could just always use UNIX_EPOCH or whatever, and have less code to maintain. The less-surprise issue justifies having the switch, but only if it's optional.
There was a problem hiding this comment.
good, simple documentation can address the above concerns. if we indicate that the first & last epoch switches are used for defining the full range of possible epochs, then i think that could mitigate any potential confusion.
Doing the naive thing and requesting and evaluation with that epoch will fail!
if they are querying the info endpoint, or they are paying attention to the logs, they should know the current epoch
On the other hand, if we're hard-coding a fixed base time for our application, we don't even need the switch; we could just always use UNIX_EPOCH or whatever, and have less code to maintain.
your previous points indicate that you're concerned about third-party users. i think keeping the switch will be good for such users who want to adhere to a different epoch schedule, as well as those who want to perform ad-hoc testing. i'm not sure if code maintenance is much of a concern, since clap would be doing most of the heavy lifting anyway
There was a problem hiding this comment.
It seems to me start.sh and Dockerfile are already somewhat separate from running just the star-randsrv application, so I think it's fine to have customization for our deployment there. If we're willing to publish the star-randsrv-ops repo where we actually define the deployed configuration, we could also keep customization like the epoch base time there. Otherwise it's better to have it in this repo so people can more easily reproduce our build.
There was a problem hiding this comment.
We could move those files to a enclave subdir if the top-level Dockerfile is confusing.
There was a problem hiding this comment.
we will have to expose some parameters in start.sh / Dockerfile for our own purposes soon, i.e. for different reporting cadences/epoch lengths. which probably means that we will need to keep our build config/parameter values in the github build config (which lives in this repo, luckily).
my main question is about where defaults should live. my rationale for having them live within clap was stated above:
it's more visible for others, especially those who are not running the server in a Docker image or via start.sh, i.e. developers that are compiling and running locally via cargo run.
There was a problem hiding this comment.
not really a showstopper, just something to think about
start.sh
Outdated
| star-randsrv --epoch-seconds 604800 | ||
| star-randsrv \ | ||
| --epoch-seconds 604800 \ | ||
| --epoch-basetime 2023-05-01T00:00:00Z |
There was a problem hiding this comment.
if we use unix epoch as the default base time, we may not need this
|
Rebased on recent dep changes so the PR merges cleanly. |
Option to fix the start epoch in absolution time, to keep the epoch tag metadata sequence aligned across restarts.
When the `--epoch-basetime` argument is present and parses to a time in the past, calculate which epoch would be current if the server had been running since that time. This helps align epoch numbers of rotation points between invocations which can be less disruptive. Calculate the delay until the next rotation point each time through the loop. Converting through seconds was the best path I could find to align `tokio::time` with calendar time.
Make startup in deployment more reproducible.
Address various review comments.
- Fix error in calculating the first `next_rotation`. - Remove debug statements - Make base time two words everywhere. - Move date parsing inline. NB this still has a bug where not setting a basetime advances to the next epoch immediately.
Instead of expliciting parsing the timestamp in the epoch loop initialization, pass a helper to `clap` so handles most of the Option-nesting and error reporting.
Verify we calculate an offset and align the rotation schedule.
Suggested in review.
Use the same integer for both the current epoch and next rotation calculations. This is simpler and behaves properly when the start time aligns exactly with a rotation point, as when base_time is start_time. Also clarify comment. Address review concern about having a separate integer binding to the truncated value.
|
Rebased over Cargo.toml conflicts. |
Align the epoch rotation schedule with a fixed point in absolute time, defaulting to the current time at startup. Skips ahead on the first iteration to the epoch (but not public key!) will be the same for separate instances started with the same base time.
Specify a fixed base time when building a container for more consistent behaviour.
Resolves #54