Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add new D-Bus Updates interface #521

Closed
wants to merge 1 commit into from
Closed

[WIP] Add new D-Bus Updates interface #521

wants to merge 1 commit into from

Conversation

kelvinfan001
Copy link
Member

@kelvinfan001 kelvinfan001 commented Apr 7, 2021

Add a new Updates D-Bus interface. Currently, this has two APIs that allow programs to tell Zincati to check for an update immediately and attempt to finalize an update immediately (with the option to override the updates strategy).
This is basically implemented by having the D-Bus actor send a RefreshTick message to the update agent actor, thereby ushering the update agent to either check for updates or finalize an update, depending on its current state.

Copy link
Member Author

@kelvinfan001 kelvinfan001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some notes on parts I'm iffy about.

ctx.notify(RefreshTick {})
pub fn tick_now(&mut self, ctx: &mut Context<Self>) {
// Cancel scheduled ticks, if any.
self.cancel_scheduled_ticks(ctx);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here it is possible to have a race where a self tick msg and a command msg is sent at the same time, and the command msg arrives first.
If this happens, we can't really cancel the scheduled tick like this because the message had alrady been sent. Though I don't think it is too big of a deal to have an extra tick; in practice, this results in two continuous ticks without any delay in between when Zincati is in a steady state (NoNewUpdates and UpdateStaged) where it periodically sends itself refresh ticks.

}
}

impl UpdateAgent {
/// Cancel the scheduled refresh tick whose handle is stored in the update agent's
/// `tick_later_handle` field, if any.
fn cancel_scheduled_ticks(&mut self, ctx: &mut Context<Self>) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When transitioning "naturally" (no messages received from the D-Bus actor) from a "steady state" (e.g. NoNewUpdates or UpdateAvailable) to a new state, tick_later_handle is a handle to
a scheduled msg that has already been sent, so there is no need to cancel it. But it doesn't look like there is any harm in calling ctx.cancel_future() on a handle that is already sent, so for simplicity, always attempt to cancel.
But let me know if this doesn't make sense.

@@ -17,6 +17,7 @@ env_logger = "0.8"
envsubst = "0.2"
fail = "0.4"
failure = "0.1"
failure_derive = "0.1.8"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do #519 instead of going this way.

}
}

#[dbus_interface(name = "org.coreos.zincati.Updates")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep this under the existing Experimental interface for now, and iterate on the design there.

/// Error thrown when the command that attempted to initiate a refresh tick
/// is not permitted to do so in the current state of the update agent.
#[derive(Debug, Fail)]
struct TickPermissionError {}
Copy link
Contributor

@lucab lucab Apr 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Permission usually has a different semantic nuance, related to a lack of privileges. In this case this is mostly related to an unsatisfiable request or an unfulfilled precondition. I don't have a proper suggestion for this right now, but it would be better to rename this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InvalidStateError?

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants