Updog: nice JSON and better waves #539
Conversation
Signed-off-by: Samuel Mendoza-Jonas <[email protected]>
Change Update.update_wave() to return the wave bounds that encompass Updog's seed value instead of just the lower bound. This makes the actual update 'wave' relevant to a given Updog easier to conceptualise. With follow-on changes to update_ready() and jitter() this fixes an issue where if Updog was in the "0th" wave (ie. no lower bound) it would incorrectly assume it had to wait for the upper bound time, when it is actually free to update. Signed-off-by: Samuel Mendoza-Jonas <[email protected]>
| @@ -14,6 +14,29 @@ use std::ops::Bound::{Excluded, Included}; | |||
|
|
|||
| pub const MAX_SEED: u32 = 2048; | |||
|
|
|||
| #[derive(Debug, PartialEq, Eq)] | |||
| pub enum Wave { | |||
There was a problem hiding this comment.
With this name, should we rename Update.waves to Update.seed_start_times or something? Or, could go further and introduce knowledge of seed to Wave and remove the need for the map? (I lean toward a rename)
There was a problem hiding this comment.
We could, but it would be an internal-only change unless we want to add this to the list of reasons to have a manifest format change. I'll leave this out of this change and have a think about it.
Add a new test to check handling of max_version, update to reflect the changes to wave representation, and make some minor updates to other tests. Signed-off-by: Samuel Mendoza-Jonas <[email protected]>
Add the Wave enum which defines the wave for a given Updog seed as either the Initial wave, a General wave, or the Last wave. The associated functions has_started() and has_passed() use these types to simplify the logic in jitter() and update_ready(). Signed-off-by: Samuel Mendoza-Jonas <[email protected]>
|
Some minor formatting changes. |
| let update = update_required(&config, &manifest, &version, &flavor, None); | ||
|
|
||
| assert!(update.is_some(), "Updog ignored max version"); | ||
| assert!( |
There was a problem hiding this comment.
nit: You don't need to assert is_some if you unwrap - I was thinking of simplifying to this:
let update = update_required(...).unwrap();
assert!(update.version == ...);
We still see clearly that it was update_required that failed if the unwrap fails, so we're not losing clarity.
There was a problem hiding this comment.
Right it's not particularly required, I was erring on the side of clarity; in the event it is None the assert! message makes it clear why we care about it in this specific case.
There was a problem hiding this comment.
I think the unwrap failing on the update_required makes it clear that it returned None, which is a more common approach and I think equally clear with less code. Just a nit, though.
(Plain asserts aren't particularly clear, they just say that an assert failed and give a line number, and you get a line number with an unwrap too; assert_equal is much better, but not applicable here.)
Issue #, if available:
N/A
Description of changes:
Some small updates to Updog, the most significant of which is the internal change to wave representation:
Change Update.update_wave() to return the wave bounds that encompass
Updog's seed value instead of just the lower bound. This makes the
actual update 'wave' relevant to a given Updog easier to conceptualise.
With follow-on changes to update_ready() and jitter() this fixes an
issue where if Updog was in the "0th" wave (ie. no lower bound) it would
incorrectly assume it had to wait for the upper bound time, when it is
actually free to update.
Also updates Updog's JSON output to use
serde_json::to_string_prettyto make it easier to parse for human eyes, and updates/reorganises some tests.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tested by launching an AMI, checking for an update, and updating into it - checking that the 0th wave didn't wait for a later wave.