Update API #942
Update API #942
Conversation
|
Push above fixes some minor comment typos and documents the new API settings |
There was a problem hiding this comment.
Before reading the details, my first reaction to the testing is that the list of available_updates is already too big for the API output to be useful on the command line, and almost too big for it to be readable even after pretty-printing, and that's with only a few updates in the repo. It'll grow for a long time, and I think make the API output essentially unusable.
I think the easiest thing to do is splitting available_updates into its own GET, /updates/available. At least the main status will be usable then. I still worry about the available data being unwieldy, and I wonder if we should further split that into one call that returns a simple list (["0.3.2", "0.3.3", "0.3.4"]) and another call for details on one/all versions.
We could also discuss whether the simple version list could be included in the main status API, and /updates/available is the detail. (I'd still have some level of worry about any unbounded data source in the main status.) Perhaps the detail could have a filter to get details for a single version instead of all?
Boiling that down, I think this would be my proposal:
/updates/status- changeavailable_updatesto a simple version list./updates/available- returns the detail that's inavailable_updatesnow./updates/v0.3.4- a way to represent theavailable_updatesinformation about a single update
|
Push above addresses @tjkirch 's comment. Added new API paths:
Added functionality to thar-be-updates to support the above. Testing done: |
|
It won't let me respond to @bcressey's comments inline, so here are a couple responses.
Well, the apiserver and models are entirely specific to our use case, so I'm not sure a
I agree that reading synchronously is a potential problem, but let's please avoid bringing in tokio if we can help it. I'd rather move toward the fire-and-forget path you mentioned in another comment. t-b-u should be able to put any relevant output in the status file, so we shouldn't need stdout/stderr from it, and perhaps we can call it the same way as thar-be-settings then? |
We have
|
etungsten
force-pushed
the
update-api
branch
2 times, most recently
from
85bdcba
to
029ab58
Compare
|
Pushes above addresses @webern 's comments and the majority of @bcressey 's and @tjkirch 's comments.
I still need to add |
|
Push above fixes some issues with directory creation.
Also updated testing description in PR description. |
| match thar_be_updates::status::get_update_status() { | ||
| Ok(update_status) => { | ||
| let update_status = update_status.simplify_update_info(); | ||
| Ok(UpdateStatusResponse(update_status)) | ||
| } | ||
| Err(e) => match e { | ||
| thar_be_updates::error::Error::OpenStatusFile { .. } => { | ||
| return error::UninitializedUpdateStatus {}.fail() | ||
| } | ||
| _ => return error::UpdateError {}.fail(), | ||
| }, | ||
| } |
There was a problem hiding this comment.
This seems like its misusing the accessible functionality in tbu. I'm somewhat wary of embedding knowledge and error handling of the tool's internals here. That said, I'm sympathetic to its use and help here.
The only suggestion I can make is to specifically present an "API" to ourselves that namespaces off "safe" functions, types, and helpers to be used in these sort of contexts (eg: thar_be_updates::api or t_b_u::on_disk).
I'll defer to hear from some of the other rustic folks on this.
|
|
||
| #[allow(clippy::wrong_self_convention)] | ||
| /// Transition update state to Idle if the 'chosen' update version is not available | ||
| pub fn to_idle(&mut self) -> Result<()> { |
There was a problem hiding this comment.
I'm thinking that we might want to parcel out the legal transitions and operations themselves to handle them in a more focused fashion.
There have been a handful of discussions on blogs and such that use the type system of Rust to generate a "state machine" of types (even a macro-ize crate came of it!). It might be worth using richer types to enforce the allowable transitions internally.
Regardless, I would very much like to see the allowable state transition handling in its own type or Trait impl.
There was a problem hiding this comment.
I explored the trait option early on and realized (or at least it seemed to me from the reading I did) that serde does not play nice when de-serializing into generic types , since I can't tell the compiler the concrete type (state) the status should be de serialized as. For the sake of simplicity I opted for runtime checks here.
There was a problem hiding this comment.
Another thing is that since these states don't internally store any information nor do they do anything upon transitions, the use of types and traits to represent these states are somewhat unnecessary (aside from the compile time checks for transition validity, that's always nice).
There was a problem hiding this comment.
since I can't tell the compiler the concrete type (state) the status should be de serialized as
One way to do this actually is to have a separate enum in the status struct that indicates what type (state) the rest of the struct should be deserialized as. But then at the point I would have to have a enum to represent the state already and I don't really get much from the benefits of having the state as separate types.
There was a problem hiding this comment.
rust-lang/rfcs#2593
^ This would be really nice to have for this purpose
There was a problem hiding this comment.
One way to do this actually is to have a separate enum in the status struct that indicates what type (state) the rest of the struct should be deserialized as. But then at the point I would have to have a enum to represent the state already and I don't really get much from the benefits of having the state as separate types.
Yeah, you could lift in and out of a single enum with all states and use a get_state_object(&self) -> impl StateMachinery to read out the "smart transition". But as you said, you'd have to duplicate and that's no bueno. The types themselves would still hold the constraints so that'll still an improvement..
Serde does let you do tagged enums, but you're right about serde not lending a hand w.r.t. nicely deserializing into Trait objects (I wanted the same thing a while back). Which is too bad.
I wish this was nicer to have, we'll probably just have to punt on this until we get something more unwieldy. I still strongly recommend pulling the transition logic into its own type separate from the rest of the driving code. It could be embedded into the serialized data (and even flattened for other consumers if needed).
This comment has been minimized.
This comment has been minimized.
|
Push above addresses @tjkirch 's comments from #942 (review) and #942 (review) |
|
Rebased onto develop to pull in commit 3f3a26d that addresses #942 (comment) Addresses @bcressey 's comments. |
etungsten
force-pushed
the
update-api
branch
2 times, most recently
from
5ef0998
to
9653b5a
Compare
|
The pushes above addresses @tjkirch 's comments. Tested changes and things still work same as before as described in the PR description. |
|
Push above fixes the remnants of @tjkirch 's comments. |
Adds a new model type to represent versions that can optionally be prefixed with 'v' or represeted by "latest"
Adds a 'version-lock' setting for specifying the version to update to when updating. Adds a 'ignore-waves' setting for specifying whether to respect update waves when updating.
updog reads the 'version-lock' and 'ignore-wave' settings for determining default update behavior. Still allows overrides via the command line options.
Adds a new subcommand for reverting actions done by 'update-apply'. Used to "deactivate" an update.
Exposes 'next()', 'active()', 'inactive()' and consequently 'SetSelect' for external crate use.
Adds a new crate 'thar-be-updates' that serves as the interface for `apiserver` to dispatch updates.
|
Rebase onto develop and resolved some conflicts in updog |
Extends the Bottlerocket API with endpoints to do updates. New action endpoints: * /actions/refresh-update * /actions/prepare-update * /actions/activate-update * /actions/deactivate-update * /updates/status 'apiserver' uses 'thar-be-updates' to dispatch update commands.
|
Push above addresses #942 (comment). |
Issue number:
N/A
Adds a set of API endpoints to
apiserverthat exposes OS update information and manages OS updates.Adds a rust binary,
thar-be-updatesthatapiserveruses to dispatch update commands.Adds new updog changes to support the update API
thar-be-updatesmodels the Bottlerocket update process after a state machine:Description of changes:
Testing done:
Testing the updog changes:
Build an image with release version set to 0.3.2, launched instance where
version-lockis set to 0.3.3. Then tried the following:Updog configuration generated properly.
Initiate update with
updog update-imageand see that by default, updog updates to theversion-locked version (0.3.3 in this case).I could also update the boot flags and then revert it via
updog revert-update-applyTesting the update API
Built custom image with a 0.3.2 version tag. Launched instance, tried the following (
version-lockset to 'latest'):(I pretty-printed the json output for clarity)
Here you can see
apiservertrying to obtain the shareable lock to read the update status file. Caller gets 423 responses when the lock is held byt-b-u.Please let me know if there are any other scenarios you'd like me to run through.
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.