-
Notifications
You must be signed in to change notification settings - Fork 293
Add host.get_ntp_synchronized and host.set_server_localtime #6743
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
base: feature/config-ntp-timezone-maxcstate
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2627,6 +2627,34 @@ let get_ntp_servers_status = | |
| ) | ||
| ~allowed_roles:_R_READ_ONLY () | ||
|
|
||
| let get_ntp_synchronized = | ||
| call ~name:"get_ntp_synchronized" ~lifecycle:[] | ||
| ~doc: | ||
| "Returns true if the system clock on the host is synchronized with the \ | ||
| NTP servers." | ||
| ~params:[(Ref _host, "self", "The host")] | ||
| ~result: | ||
| ( Bool | ||
| , "true if the system clock on the host is synchronized with the NTP \ | ||
| servers." | ||
| ) | ||
| ~allowed_roles:_R_READ_ONLY () | ||
|
|
||
| let set_server_localtime = | ||
| call ~name:"set_server_localtime" ~lifecycle:[] | ||
| ~doc: | ||
| "Set the host's system clock in its local timezone when NTP is disabled." | ||
| ~params: | ||
| [ | ||
| (Ref _host, "self", "The host") | ||
| ; ( DateTime | ||
| , "value" | ||
| , "A datetime without timezone information. If UTC is specified, the \ | ||
| timezone will be ignored." | ||
| ) | ||
|
Comment on lines
+2652
to
+2654
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's not enough information in the commit message to understand the requirements of set_server_localtime. Please explain your decisions in there.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In particular the function is dangerous because it does not show the timezone, a correct function must return the time in this format: I raise the the issue about lack of requirements is there because we need to know what the user needs to do, exactly. For example:
Note that the second one means that the timezone data needs to be packaged and updated periodically, which I don't think xenserver nor xcp-ng do. My recommendation would be to avoid letting users setting the timezone for the server. UTC is more than enough and avoid complexity. Fortunately the time can be set always correctly, even if the user provides a timestamp with a timezone difference like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm afraid this is a valid request and use case which can't be avoided.
Generally I agree to set the time + timezone together always. But the existing two APIs:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. I think I can change the implementation of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @psafont May I please know your thoughts on above proposal?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is what I would prefer as well. I probably would also add two other timezone-related functions:
I have to repeat that I would prefer if the server was always in UTC, it can disrupts timestamps in logs and other debug information, which can be confusing. I have not found a real need to set a concrete timezone, this is because API clients can convert times to the timezone of the machine the user is using.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks.
There are real user's requests for setting timezone on servers. And a user who doesn't want to set timezone can ignore the new setting APIs.
IMO, this is the unavoidable consequence of the user's action of setting timezone. |
||
| ] | ||
| ~allowed_roles:_R_POOL_OP () | ||
|
|
||
| (** Hosts *) | ||
| let t = | ||
| create_obj ~in_db:true | ||
|
|
@@ -2779,6 +2807,8 @@ let t = | |
| ; disable_ntp | ||
| ; enable_ntp | ||
| ; get_ntp_servers_status | ||
| ; get_ntp_synchronized | ||
| ; set_server_localtime | ||
| ] | ||
| ~contents: | ||
| ([ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3555,3 +3555,28 @@ let get_ntp_servers_status ~__context ~self:_ = | |
| Xapi_host_ntp.get_servers_status () | ||
| else | ||
| [] | ||
|
|
||
| let get_ntp_synchronized ~__context ~self:_ = | ||
| match Xapi_host_ntp.is_synchronized () with | ||
| | Ok r -> | ||
| r | ||
| | Error msg -> | ||
| Helpers.internal_error "%s" msg | ||
|
|
||
| let set_server_localtime ~__context ~self:_ ~value = | ||
| let param = | ||
| match value with | ||
| | Date.{t; tz= Some 0} | Date.{t; tz= None} -> | ||
| (* Ideally it should be of a new type like NaiveDateTime. For | ||
| simplicity, reuse DateTime here. But it can't tell if the UTC is | ||
| specified explicitly or not. Just ignore it in that case. *) | ||
| let (y, mon, d), ((h, min, s), _) = Ptime.to_date_time t in | ||
| Printf.sprintf "%04i-%02i-%02i %02i:%02i:%02i" y mon d h min s | ||
| | _ -> | ||
| raise | ||
| (Api_errors.Server_error (Api_errors.not_allowed_tz_in_localtime, [])) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't quite understand the timezone handlling here. I expected the time format "YYYY-MM-DD HH:MM:SS" which is localtime. It should have no business with timezone. Where does the timezone come from? The DateTime type?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. The DateTime contains timezone.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then why can't distinguish the local time and UTC.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The API uses
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see
Comment on lines
+3567
to
+3577
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is all this code needed? Reading the manual of timedatectl, it should be able to accept any date in RFC3339 format, see Additionally, datetimes without a timezone reference must be refused, and not assumed to be UTC. We've had this issue in xapi for a very long long time, I don't want to perpetuate it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But we hope to accept a local time without timezone as there is another API to set the timezone.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That API is old and outright dangerous, please treat it as deprecated. A new, sane API function should be exposed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe better not reuse the datetime type but just string that fit |
||
| in | ||
| try | ||
| Helpers.call_script !Xapi_globs.timedatectl ["set-time"; param] |> ignore ; | ||
| () | ||
| with e -> Helpers.internal_error "%s" (ExnHelper.string_of_exn e) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,3 +175,12 @@ let promote_legacy_default_servers () = | |
| set_servers_in_conf defaults ; | ||
| restart_ntp_service () | ||
| ) | ||
|
|
||
| let is_synchronized () = | ||
| let patterns = ["System clock synchronized: yes"; "NTP synchronized: yes"] in | ||
| try | ||
| Helpers.call_script !Xapi_globs.timedatectl ["status"] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another option is to use c function ntp_gettime to get the maxerror of the system. That can avoid the timedatectl output changes in different versions. Although the current method is easy enough too. I'm OK for both. |
||
| |> String.split_on_char '\n' | ||
| |> List.exists ((Fun.flip List.mem) patterns) | ||
| |> Result.ok | ||
| with e -> Error (ExnHelper.string_of_exn e) | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using localtime is confusing because it's used for a very specific thing in UNIX: time + timezone, but this call is _not_setting the timezone. Please use something that doesn't use the term localtime, for example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be timezone management in the following PR, see the feature branch name.
The ntp can be disabled, so offer an api to set host local time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly the
writerregarding the existingreader:host.get_server_localtime, which returning onlytimewithout timezone.I would think it should be used together with
host.set_timezone.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a feature and it's missing a design a requirements, we've talked about this before :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I will push the feature user cases and design later.