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

strategy/periodic: support configuring a time zone for reboot windows #524

Merged
merged 2 commits into from
Apr 29, 2021
Merged

strategy/periodic: support configuring a time zone for reboot windows #524

merged 2 commits into from
Apr 29, 2021

Conversation

kelvinfan001
Copy link
Member

@kelvinfan001 kelvinfan001 commented Apr 9, 2021

Support configuring reboot windows defined by a user-specified time zone, instead of only UTC.

Include a single time_zone field (that defaults to UTC if not
specified) in the periodic strategy's configuration. Whenever we
need to check whether we're currently in an allowed reboot window,
we convert the current naive time to the naive time under the
specified time zone, and check whether we are currently in a reboot
window.

Under this implementation, we keep invariant clock time for reboots,
but do NOT keep invariant reboot window length; thus, in some cases,
reboot windows can be lengthened, shortened, or skipped entirely.

@kelvinfan001
Copy link
Member Author

kelvinfan001 commented Apr 10, 2021

The current implementation uses Option 3 from #301 (comment).
This is still a work in progress as we need to properly address the third bullet point in #301 (comment).

Edit: outdated.

src/strategy/mod.rs Outdated Show resolved Hide resolved
src/config/fragments.rs Outdated Show resolved Hide resolved
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.

I added some comments in places I feel like needs some extra attention :)

@kelvinfan001 kelvinfan001 changed the title [WIP] Support local time for periodic strategy Support configuring a time zone for periodic strategy Apr 14, 2021
@lucab lucab added this to the vNext milestone Apr 14, 2021
docs/usage/updates-strategy.md Outdated Show resolved Hide resolved
docs/usage/updates-strategy.md Outdated Show resolved Hide resolved
docs/usage/updates-strategy.md Show resolved Hide resolved
docs/usage/updates-strategy.md Outdated Show resolved Hide resolved
docs/usage/updates-strategy.md Show resolved Hide resolved
p.human_remaining()
// If not using UTC, we cannot accurately calculate remaining time;
// display next window week day and time, instead.
if p.time_zone != tzfile::Tz::named("UTC").unwrap() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to place this into a p.is_calendar_utc() -> bool, caching the boolean upfront instead of re-computing it each time.

Copy link
Member Author

Choose a reason for hiding this comment

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

We now have a p.tz_name() that gets the "cached" name of the time zone. Using this over a boolean because we need to use this name in human_next_reboot_window() too.

src/strategy/mod.rs Outdated Show resolved Hide resolved
src/strategy/periodic.rs Show resolved Hide resolved
src/strategy/periodic.rs Outdated Show resolved Hide resolved
src/strategy/periodic.rs Outdated Show resolved Hide resolved
@kelvinfan001 kelvinfan001 linked an issue Apr 15, 2021 that may be closed by this pull request
Include a single `time_zone` field (that defaults to UTC if not
specified) in the `periodic` strategy's configuration. Whenever we
need to check whether we're currently in an allowed reboot window,
we convert the current naive time to the naive time under the
specified time zone, and check whether we are currently in a reboot
window.

Under this implementation, we keep invariant clock time for reboots,
but do NOT keep invariant reboot window length; thus, in some cases,
reboot windows can be lengthened, shortened, or skipped entirely.
@lucab lucab changed the title Support configuring a time zone for periodic strategy strategy/periodic: support configuring a time zone for reboot windows Apr 29, 2021
@lucab lucab enabled auto-merge April 29, 2021 12:23
@lucab lucab merged commit 541afbd into coreos:master Apr 29, 2021
@jlebon
Copy link
Member

jlebon commented Apr 29, 2021

Thanks, @kelvinfan001! Looking forward to using this on my FCOS server. :)

@travier
Copy link
Member

travier commented Apr 29, 2021

Same here! Thanks!

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.

updates/strategy: local timezone aware reboot windows
4 participants