Fix secondary root rotation on metadata expiry #115
Conversation
The problem was that the first call to checkMetaOffline() throws an exception which results in image_repo_->checkMetaOffline() not being called. There are a bunch of root causes that should be fixed at some point: - The *Repository classes are not fully initialized in the ctor - The required initialization is called 'checkMetaOffline()'' - There is no documentation that says this is so - There are no asserts to enforce the above requirement at runtime.
There only one place that uses this default, so remove the default ctor and explicitly express it in RepositoryCommon. This means that no-one has to remember "What is the policy for a default-constructed Root object?"" any more.
This wasn't implicated in TOR-3452, but the new version is shorter and has simple control flow.
cajun-rat
force-pushed
the
fix/secondary-root-rotation
branch
from
f28bc0d to
bda0573
Compare
| image_repo_->checkMetaOffline(*storage_); | ||
| } catch (const std::exception &e) { | ||
| LOG_INFO << "No valid metadata found in storage."; | ||
| // See above ^ image_repo_ is OK |
There was a problem hiding this comment.
I think "OK" is a bit vague. Maybe should just say that it's stale? (Same with the comment above, really.)
There was a problem hiding this comment.
Yeah. 'OK' hints at a deeper problem with the types of state that DirectorRepository and ImageRepository and get into. After constructing they need someone to call checkMetaOffline() to get them into a state where they can accept updates. If that call throws and exception, then the thing is still in a state where it can receive updates, as long as someone starts by rotating the root metadata. We can/should tidy this up, but I didn't want to make sweeping changes when there was such a small fix available.
There was a problem hiding this comment.
I'm pretty out of the loop by this point but agree that this can definitely be done differently. Up to you but the comments wouldn't hurt.
There was a problem hiding this comment.
Agreed. I'll add some more description to the docs for checkMetaOffline(). I think I've described the bug a few times now, it is probably worth a definitive description
There was a problem hiding this comment.
This was reviewed earlier at Toradex so I'll just say that it looks good to me and I'll approve it since we already have this in Toradex' fork.
Minor question though: I see some commit message titles referencing a ticket number internal to Toradex (TOR-xxxx). Is that okay for the upstream project?
See discussion in #115 f# ../fail_state.tmpwb569uon/
|
I created #118 to improve the documentation. |
This fixes a bug that occurs when trying to update a secondary after the root metadata expires. There is a reproducer for the failure followed by a fix.
Thankfully, rebooting the system allows it to recover so we got lucky here.
The fix is already in the Toradex fork, bringing it over here too.