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

updog: commands for smooth coordination #301

Merged
merged 3 commits into from
Oct 1, 2019
Merged

updog: commands for smooth coordination #301

merged 3 commits into from
Oct 1, 2019

Conversation

sam-aws
Copy link
Contributor

@sam-aws sam-aws commented Sep 25, 2019

Issue #, if available:
Related to #184

Description of changes:
Flesh out the updog commands to facilitate being called by some update
coordinator.

  • update check-update to allow listing all applicable updates
  • add the --reboot flag to reboot into an update after updating flags
  • add prepare which will download migrations before an update
    (currently stubbed).

TODO

Signed-off-by: Samuel Mendoza-Jonas samjonas@amazon.com

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@sam-aws
Copy link
Contributor Author

sam-aws commented Sep 25, 2019

@jahkeup this will require some corresponding tweaks to the Updog commands in #239, but aside from jitter reporting anything else we might want to add in now?

@sam-aws
Copy link
Contributor Author

sam-aws commented Sep 26, 2019

Also @jahkeup for the purposes of coordination would you expect check-update --all to return all available upgrades or all applicable images based on flavor and architecture? I'm leaning towards the latter so that there's a way to view all images if a downgrade is required.

@sam-aws
Copy link
Contributor Author

sam-aws commented Sep 27, 2019

Taking this out of draft and including the jitter feature in #264 here as it makes a bit more sense.

workspaces/updater/updog/src/main.rs Outdated Show resolved Hide resolved
workspaces/updater/updog/src/main.rs Outdated Show resolved Hide resolved
Flesh out the updog commands to facilitate being called by some update
coordinator.
- update `check-update` to allow listing all applicable updates
- add the --reboot flag to reboot into an update after updating flags
- add `prepare` which will download migrations before an update
  (currently stubbed).

Signed-off-by: Samuel Mendoza-Jonas <samjonas@amazon.com>
Instead of calculating some jitter and waiting, Updog will return the
calculated update time to the caller so that it can be called again at a
later time. The caller should then supply the start time to Updog when
calling it again.
If an update exists but Updog is in a later wave, the start of that wave
is returned.

This also adds a helper function `output()` to clean up a few spots
where Updog's output depends on whether --json has been set.

Signed-off-by: Samuel Mendoza-Jonas <samjonas@amazon.com>
@sam-aws
Copy link
Contributor Author

sam-aws commented Sep 27, 2019

Rebased after #313

@jahkeup
Copy link
Member

jahkeup commented Oct 1, 2019

@sam-aws let me check out the operation given these changes, after that I think we can start to plan the next commands and flags to enrich its functionality. I'll review this PR here in a few! 👍

Copy link
Member

@jahkeup jahkeup left a comment

Choose a reason for hiding this comment

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

One change in logging, but otherwise LGTM.

workspaces/updater/updog/src/main.rs Show resolved Hide resolved
serde_json::to_string(&object).context(error::UpdateSerialize)?
);
} else {
println!("{}", string);
Copy link
Member

Choose a reason for hiding this comment

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

Nice! This is handy for the human v. machine usage.

workspaces/updater/updog/src/main.rs Outdated Show resolved Hide resolved
Co-Authored-By: Jake <jakeev@amazon.com>
Copy link
Member

@jahkeup jahkeup left a comment

Choose a reason for hiding this comment

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

🐶 🐕 🚢

Comment on lines 40 to 47
enum Command {
CheckUpdate,
Whats,
Prepare,
Update,
UpdateImage,
UpdateFlags,
UpdateApply,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not for this PR, but I think having doc comments above each of these variants that matches fn usage is going to be necessary pretty soon.

Comment on lines +79 to +82
if let Some((_, wave)) = self.waves.range((Included(0), Included(seed))).last() {
return Some(wave);
}
None
Copy link
Contributor

Choose a reason for hiding this comment

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

This can also be written as:

self.waves.range((Included(0), Included(seed))).last().map(|(_, wave)| wave)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}
}
Command::Prepare => {
// TODO unimplemented
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want this to silently do nothing for the time being, rather than use unimplemented!()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the former for dogswatch's sake - we don't do this yet but we don't want dogswatch to think something went wrong.

@sam-aws sam-aws merged commit 20b433d into develop Oct 1, 2019
@sam-aws sam-aws deleted the updog branch October 1, 2019 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants