feat(update-agent): publishing update-agent progress to dbus #349
Conversation
| #[derive(Default, Debug, Serialize, Deserialize, Type, Clone, Value, OwnedValue)] | ||
| pub struct UpdateStatus { | ||
| pub components: Vec<ComponentStatus>, | ||
| pub error: String, |
There was a problem hiding this comment.
check out zbus::fdo::Result, this might let you get rid of this error field and use a result type instead
| pub fn signal_error(error: &str, conn: &Connection) { | ||
| if let Ok(iface) = get_iface_ref(conn) { | ||
| iface.get_mut().0.error = error.to_string(); | ||
| emit_signal(&iface); | ||
| } else { | ||
| warn!("failed to get interface reference"); | ||
| } | ||
| } | ||
|
|
||
| pub fn init_components(components: &[ManifestComponent], conn: &Connection) { | ||
| if let Ok(iface) = get_iface_ref(conn) { | ||
| iface.get_mut().0.components = components | ||
| .iter() | ||
| .map(|c| ComponentStatus { | ||
| name: c.name.clone(), | ||
| state: ComponentState::None, | ||
| }) | ||
| .collect(); | ||
| } else { | ||
| warn!("failed to get interface reference"); | ||
| } | ||
| } | ||
|
|
||
| pub fn update_component_state(name: &str, state: ComponentState, conn: &Connection) { | ||
| if let Ok(iface) = get_iface_ref(conn) { | ||
| if let Some(component) = iface | ||
| .get_mut() | ||
| .0 | ||
| .components | ||
| .iter_mut() | ||
| .find(|c| c.name == name) | ||
| { | ||
| component.state = state; | ||
| } | ||
| emit_signal(&iface); | ||
| } else { | ||
| warn!("failed to get interface reference"); | ||
| } | ||
| } | ||
|
|
||
| fn emit_signal(iface: &InterfaceRef<UpdateProgress<UpdateStatus>>) { | ||
| if let Err(err) = | ||
| async_io::block_on(iface.get_mut().status_changed(iface.signal_context())) | ||
| { | ||
| warn!("failed to emit signal: {}", err); | ||
| } | ||
| } | ||
|
|
||
| fn get_iface_ref( | ||
| conn: &Connection, | ||
| ) -> Result<InterfaceRef<UpdateProgress<UpdateStatus>>, zbus::Error> { | ||
| let object_server = conn.object_server(); | ||
| object_server | ||
| .interface::<_, UpdateProgress<UpdateStatus>>("org.worldcoin.UpdateProgress1") | ||
| } |
There was a problem hiding this comment.
I would probably delete the get_iface_ref function and the emit_signal functions
| } else { | ||
| match zbus::blocking::Connection::session() | ||
| .wrap_err("failed establishing a `session` dbus connection") | ||
| let supervisor_proxy = zbus::blocking::Connection::session() |
There was a problem hiding this comment.
you create a connection twice: once here and once in create_dbus_connection. lets instead create the connection only once, then you can clone that connection or give references out to any proxies or interfaces that need it.
Doing this will also eliminate the need for the tuple
| /// A wrapper struct for types implementing [`UpdateProgressT`]. | ||
| pub struct UpdateProgress<T: UpdateProgressT>(pub T); | ||
|
|
||
| #[derive(Debug, Serialize, Deserialize, Type, Clone, Value, OwnedValue)] |
There was a problem hiding this comment.
Might as well make it Copy, PartialEq, Eq
| #[derive(Debug, Serialize, Deserialize, Type, Clone, Value, OwnedValue)] | |
| #[derive(Debug, Serialize, Deserialize, Type, Clone, Copy, Eq, PartialEq, Value, OwnedValue)] |
| interfaces::update_component_state( | ||
| component.name(), | ||
| ComponentState::Installed, | ||
| conn, |
There was a problem hiding this comment.
try passing around the InterfaceRef instead of the connection itself. It makes it a bit more clear what this function is doing.
| } | ||
| } | ||
|
|
||
| pub fn init_components(components: &[ManifestComponent], conn: &Connection) { |
There was a problem hiding this comment.
the name could be more informative, maybe init_dbus_properties
| } | ||
| } | ||
|
|
||
| pub fn update_component_state(name: &str, state: ComponentState, conn: &Connection) { |
There was a problem hiding this comment.
name could be more informative, maybe set_dbus_properties
| //! org.freedesktop.DBus.Properties.Get org.worldcoin.UpdateProgress1 Status | ||
| //! ``` | ||
| //! | ||
| //! Montor for signals: |
| pub enum ComponentState { | ||
| None = 1, | ||
| Fetched = 2, | ||
| Processed = 3, |
There was a problem hiding this comment.
What is "Processed"? In update-agent I think we have 2 steps: fetch and update(install)
| #[derive(Debug, Serialize, Deserialize, Type, Clone, Value, OwnedValue)] | ||
| pub struct ComponentStatus { | ||
| pub name: String, | ||
| pub state: ComponentState, |
There was a problem hiding this comment.
i like it, also easily extensible for progress bar for each component, but then should we switch component status instead of past tense to current? "Installed" -> "Installing" etc.
There was a problem hiding this comment.
actually even now I don't see a reason why we should use past tense, it might be misleading?
| @@ -252,6 +278,15 @@ fn run(args: &Args) -> eyre::Result<()> { | |||
| info!("running update for component `{}`", component.name()); | |||
| component | |||
| .run_update(target_slot, &claim, settings.recovery) | |||
| .inspect(|_| { | |||
There was a problem hiding this comment.
What if for any reason update_component_state panics? do we want to be resilient and this case and continue the update?
WIP