-
Notifications
You must be signed in to change notification settings - Fork 285
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
Date: Accept all valid timezones from client, allow sub-second precision #5950
Conversation
2a8daf6
to
e41943e
Compare
@@ -26,12 +26,12 @@ | |||
|
|||
*) | |||
module Date = struct | |||
open Xapi_stdext_date | |||
module Date = Xapi_stdext_date.Date | |||
include Date |
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.
Would it be worth it removing this include
to only be able to access Date explicitly?
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.
include Date
add the whole signature of Date
to the current module, so I believe it's needed, unless I missed something.
; param_release= tampa_release | ||
; param_default= Some (VDateTime Date.never) | ||
; param_default= Some (VDateTime Date.epoch) |
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.
Is this just a different name but the same underlying date? Epoch is in the past but never could have been in the future.
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.
It might be better to capture the intended meaning using never
as an alias.
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.
Date.never is 0. I don't know why None isn't used for many of these, currently epoch has the same semantic meaning as None in many of these. I couldn't find any instance where the 0 / never value means something special, it's for users to understand that when they see 1970 it means never, which is awkward.
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.
None
for the default value means that the parameter will not have a default value. But sometimes it is necessary for a parameter to have a default value (for backwards compatability). But yeah, I think 1970 is kind of awkward as a sign of "never", hopefully this value will actually gets filled when it is quried
433de8a
to
1d8874d
Compare
; param_release= tampa_release | ||
; param_default= Some (VDateTime Date.never) | ||
; param_default= Some (VDateTime Date.epoch) |
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.
None
for the default value means that the parameter will not have a default value. But sometimes it is necessary for a parameter to have a default value (for backwards compatability). But yeah, I think 1970 is kind of awkward as a sign of "never", hopefully this value will actually gets filled when it is quried
@@ -152,19 +162,19 @@ let is_later ~than t = Ptime.is_later ~than:(to_ptime than) (to_ptime t) | |||
|
|||
let diff a b = Ptime.diff (to_ptime a) (to_ptime b) | |||
|
|||
let compare_print_tz a b = | |||
let compare_tz a b = |
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.
so this function makes input with timezones always bigger than those that does not? Is it possible to for a
to contain a negative number such as Some -1
?
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.
That's correct, remember that not having a timezone is not the same as being in the UTC. The order is arbitrary.
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.
emm, I am a bit confused, so in the doc of the parameter you said when no timezones are provided, UTC is assumed. So do we assume UTC when there are no time zones?
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.
In the dates that are read from strings, but those do have a timezone now. The ones that are produced with localtime do not. (we should be able to change that soon, when the SDK accepts any valid timezone)
BST is now passing with the regression fixed around the dates in xapi_xenopsd. This happened because now dates can be more precise than seconds, but dates stored in the database are clamped to full second, making the comparison unstable. I tried finding other comparisons like this one by looking into how the Datetime fields are used, but couldn't find any other like this one. In particular, Task has a lot of Datetime fields. |
645f423
to
648be29
Compare
905f8e6
to
27cc91c
Compare
The Java SDK failure happens on master as well, it's unrelated to this PR. |
And replace its users Signed-off-by: Pau Ruiz Safont <[email protected]>
Previously the comments and code talked about "timezone printing", but that is only a consequence of a more fundamental problem: datetimes without timezone do not share a frame of reference with datetimes that have a timezone, nor among themselves. This means that timezoneless shouldn't be compared with other timezones as assuming a timezone can be incorrect. Datetimes with timezone all share a frame of reference: the unix epoch. Now the code uses the exact offset in seconds, which also enables it to accept datetimes that have an offset different than 0. Comparisons with timezoneless datetimes are not forbidden for the time being. Instead UTC is assumed, like before, even if it might be the wrong guess. Signed-off-by: Pau Ruiz Safont <[email protected]>
27cc91c
to
9d76583
Compare
This allows to maintain sub-second precision, and only convert to datetimes when converting to strings. Now timezone conversion is more explicit. Datetimes without timezone are assumed to be UTC. While rare in practice, this is not safe in general, so print a warning to stdout whenever this happens. Signed-off-by: Pau Ruiz Safont <[email protected]>
Datetimes received as call parameters can lack timezone. In that case the host assumed the datetime is in UTC. This is not always safe to do, but returning an error will break clients. Instead keep doing the assumption and enforce proper documentation of the parameters in the datamodel. Signed-off-by: Pau Ruiz Safont <[email protected]>
Signed-off-by: Pau Ruiz Safont <[email protected]>
Previously the dates without timezone were assumed to use UTC when being converted to unix time, but the datatype maintained the lack of timezone when being printed. This means that the problematic timezoneless timestamps were kept and could be produced by the hosts. Since the dates are assumed to be UTC, add the timezone when they are parsed for the first time. Now their timezone is displayed at all times. The output of Localtime is kept without a timezone because clients are not prepared to accept arbitrary timezones in dates. (Both the SDK and XO(Lite)) Signed-off-by: Pau Ruiz Safont <[email protected]>
d68f9f3
to
96195f7
Compare
The Date module has a very convoluted way to deal with timezones because of its historic clients. While we can't remove all the issues, we can remove most of them.
Z
)This PR does the following mitigation / fixes:
Issues that the PR does not fix
Drafting as tests are ongoing