Date: Accept all valid timezones from client, allow sub-second precis…

…ion (#5950) 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. - Dates (timestamps, really) without timezones do not contain a frame of reference, and hence placing them in the UTC timeline can't be done accurately in the general case. This means that comparing timestamps gives incorrect results. - XMLRPC enforces dates to be encoded in ISO8601format. This means timestamps can lack a timezone. - Xapi uses xmlrpc's dates, adding this unfortunate limitation. - While Date allows these timestamps, it assumes that these are in the UTC timezone. On top of that, it refuses to process any timestamps that are in any timezone other than UTC (`Z`) - The Date module tries really hard to hide the timezone of timestamps that lack it when printing them. This means that timezoneless timestamps can persist in the database, for no good reason, as they are treated as UTC timestamps. - Host.localtime is the only call that returns timezoneless timestamps, all the other calls correctly return timestamps in the UTC timezone. Because the call on purpose does not want to return the current time in UTC, changing this might break clients not ready to process any timezone, mainly SDK-built ones #5802 - Dates are stored as a tuple of date, hour, minutes, seconds. This has very limited precision, which might be unexpected. This PR does the following mitigation / fixes: - Document all calls with Datetimes as parameters that the timestamps will be interpreted as UTC ones if they miss the timezone. - Remove the limitation to process any valid timezone - Timezoneless timstamps are immediately processed as UTC timestamps, as refusing these timestamps can break clients. Issues that the PR does not fix - Host.localtime produces timestamps without timezones, this is needed as adding non-zero timestamps breaks the SDK clients. - The server does not reject timezoneless timestamp, this might break migrations, RPUs, or normal clients, so I've held back on this change. - Printed timestamp do _not_ retain sub-second precision Drafting as tests are ongoing
xapi-project · Sep 16, 2024 · 1d32682 · 1d32682
2 parents 85f9271 + 96195f7
commit 1d32682
Show file tree

Hide file tree

Showing 95 changed files with 526 additions and 384 deletions.
diff --git a/doc/content/xapi/storage/sxm.md b/doc/content/xapi/storage/sxm.md
@@ -450,8 +450,8 @@ but we've still got a bit of thinking to do: we sort the VDIs to copy based on a
     let compare_fun v1 v2 =
       let r = Int64.compare v1.size v2.size in
       if r = 0 then
-        let t1 = Date.to_float (Db.VDI.get_snapshot_time ~__context ~self:v1.vdi) in
-        let t2 = Date.to_float (Db.VDI.get_snapshot_time ~__context ~self:v2.vdi) in
+        let t1 = Date.to_unix_time (Db.VDI.get_snapshot_time ~__context ~self:v1.vdi) in
+        let t2 = Date.to_unix_time (Db.VDI.get_snapshot_time ~__context ~self:v2.vdi) in
         compare t1 t2
       else r in
     let all_vdis = all_vdis |> List.sort compare_fun in

diff --git a/ocaml/alerts/expiry_alert.ml b/ocaml/alerts/expiry_alert.ml
@@ -50,15 +50,15 @@ let all_messages rpc session_id =
 
 let message_body msg expiry =
   Printf.sprintf "<body><message>%s</message><date>%s</date></body>" msg
-    (Date.to_string expiry)
+    (Date.to_rfc3339 expiry)
 
 let expired_message obj = Printf.sprintf "%s has expired." obj
 
 let expiring_message obj = Printf.sprintf "%s is expiring soon." obj
 
 let maybe_generate_alert now obj_description alert_conditions expiry =
   let remaining_days =
-    days_until_expiry (Date.to_float now) (Date.to_float expiry)
+    days_until_expiry (Date.to_unix_time now) (Date.to_unix_time expiry)
   in
   alert_conditions
   |> List.sort (fun (a, _) (b, _) -> compare a b)

diff --git a/ocaml/idl/datamodel.ml b/ocaml/idl/datamodel.ml
@@ -217,9 +217,8 @@ module Session = struct
              session instance has is_local_superuser set, then the value of \
              this field is undefined."
         ; field ~in_product_since:rel_george ~qualifier:DynamicRO
-            ~default_value:(Some (VDateTime (Date.of_float 0.)))
-            ~ty:DateTime "validation_time"
-            "time when session was last validated"
+            ~default_value:(Some (VDateTime Date.epoch)) ~ty:DateTime
+            "validation_time" "time when session was last validated"
         ; field ~in_product_since:rel_george ~qualifier:DynamicRO
             ~default_value:(Some (VString "")) ~ty:String "auth_user_sid"
             "the subject identifier of the user that was externally \
@@ -3895,9 +3894,11 @@ module VDI = struct
     ; {
         param_type= DateTime
       ; param_name= "snapshot_time"
-      ; param_doc= "Storage-specific config"
+      ; param_doc=
+          "Storage-specific config. When the timezone is missing, UTC is \
+           assumed"
       ; param_release= tampa_release
-      ; param_default= Some (VDateTime Date.never)
+      ; param_default= Some (VDateTime Date.epoch)
       }
     ; {
         param_type= Ref _vdi
@@ -4089,7 +4090,11 @@ module VDI = struct
       ~params:
         [
           (Ref _vdi, "self", "The VDI to modify")
-        ; (DateTime, "value", "The snapshot time of this VDI.")
+        ; ( DateTime
+          , "value"
+          , "The snapshot time of this VDI. When the timezone is missing, UTC \
+             is assumed"
+          )
         ]
       ~flags:[`Session] ~doc:"Sets the snapshot time of this VDI."
       ~hide_from_docs:true ~allowed_roles:_R_LOCAL_ROOT_ONLY ()
@@ -4468,7 +4473,7 @@ module VDI = struct
               ~ty:(Set (Ref _vdi)) ~doc_tags:[Snapshots] "snapshots"
               "List pointing to all the VDIs snapshots."
           ; field ~in_product_since:rel_orlando
-              ~default_value:(Some (VDateTime Date.never)) ~qualifier:DynamicRO
+              ~default_value:(Some (VDateTime Date.epoch)) ~qualifier:DynamicRO
               ~ty:DateTime ~doc_tags:[Snapshots] "snapshot_time"
               "Date/time when this snapshot was created."
           ; field ~writer_roles:_R_VM_OP ~in_product_since:rel_orlando
@@ -4752,7 +4757,7 @@ module VBD_metrics = struct
           uid _vbd_metrics
         ; namespace ~name:"io" ~contents:iobandwidth ()
         ; field ~qualifier:DynamicRO ~ty:DateTime
-            ~default_value:(Some (VDateTime Date.never))
+            ~default_value:(Some (VDateTime Date.epoch))
             ~lifecycle:
               [
                 (Published, rel_rio, "")
@@ -5511,7 +5516,11 @@ module VMPP = struct
       ~params:
         [
           (Ref _vmpp, "self", "The protection policy")
-        ; (DateTime, "value", "the value to set")
+        ; ( DateTime
+          , "value"
+          , "When was the last backup was done. When the timezone is missing, \
+             UTC is assumed"
+          )
         ]
       ()
 
@@ -5521,7 +5530,11 @@ module VMPP = struct
       ~params:
         [
           (Ref _vmpp, "self", "The protection policy")
-        ; (DateTime, "value", "the value to set")
+        ; ( DateTime
+          , "value"
+          , "When was the last archive was done. When the timezone is missing, \
+             UTC is assumed"
+          )
         ]
       ()
 
@@ -5669,7 +5682,7 @@ module VMPP = struct
             "true if this protection policy's backup is running"
         ; field ~lifecycle:removed ~qualifier:DynamicRO ~ty:DateTime
             "backup_last_run_time" "time of the last backup"
-            ~default_value:(Some (VDateTime (Date.of_float 0.)))
+            ~default_value:(Some (VDateTime Date.epoch))
         ; field ~lifecycle:removed ~qualifier:StaticRO ~ty:archive_target_type
             "archive_target_type" "type of the archive target config"
             ~default_value:(Some (VEnum "none"))
@@ -5693,7 +5706,7 @@ module VMPP = struct
             "true if this protection policy's archive is running"
         ; field ~lifecycle:removed ~qualifier:DynamicRO ~ty:DateTime
             "archive_last_run_time" "time of the last archive"
-            ~default_value:(Some (VDateTime (Date.of_float 0.)))
+            ~default_value:(Some (VDateTime Date.epoch))
         ; field ~lifecycle:removed ~qualifier:DynamicRO ~ty:(Set (Ref _vm))
             "VMs" "all VMs attached to this protection policy"
         ; field ~lifecycle:removed ~qualifier:StaticRO ~ty:Bool
@@ -5782,7 +5795,11 @@ module VMSS = struct
       ~params:
         [
           (Ref _vmss, "self", "The snapshot schedule")
-        ; (DateTime, "value", "the value to set")
+        ; ( DateTime
+          , "value"
+          , "When was the schedule was last run. When a timezone is missing, \
+             UTC is assumed"
+          )
         ]
       ()
 
@@ -5856,7 +5873,7 @@ module VMSS = struct
             ~default_value:(Some (VMap []))
         ; field ~qualifier:DynamicRO ~ty:DateTime "last_run_time"
             "time of the last snapshot"
-            ~default_value:(Some (VDateTime (Date.of_float 0.)))
+            ~default_value:(Some (VDateTime Date.epoch))
         ; field ~qualifier:DynamicRO ~ty:(Set (Ref _vm)) "VMs"
             "all VMs attached to this snapshot schedule"
         ]
@@ -6311,15 +6328,24 @@ module Message = struct
         [
           (cls, "cls", "The class of object")
         ; (String, "obj_uuid", "The uuid of the object")
-        ; (DateTime, "since", "The cutoff time")
+        ; ( DateTime
+          , "since"
+          , "The cutoff time. When the timezone is missing, UTC is assumed"
+          )
         ]
       ~flags:[`Session]
       ~result:(Map (Ref _message, Record _message), "The relevant messages")
       ~allowed_roles:_R_READ_ONLY ()
 
   let get_since =
     call ~name:"get_since" ~in_product_since:rel_orlando
-      ~params:[(DateTime, "since", "The cutoff time")]
+      ~params:
+        [
+          ( DateTime
+          , "since"
+          , "The cutoff time. When the timezone is missing, UTC is assumed"
+          )
+        ]
       ~flags:[`Session]
       ~result:(Map (Ref _message, Record _message), "The relevant messages")
       ~allowed_roles:_R_READ_ONLY ()

diff --git a/ocaml/idl/datamodel_certificate.ml b/ocaml/idl/datamodel_certificate.ml
@@ -59,10 +59,10 @@ let t =
           ~default_value:(Some (VRef null_ref))
           "The host where the certificate is installed"
       ; field ~qualifier:StaticRO ~lifecycle ~ty:DateTime "not_before"
-          ~default_value:(Some (VDateTime Date.never))
+          ~default_value:(Some (VDateTime Date.epoch))
           "Date after which the certificate is valid"
       ; field ~qualifier:StaticRO ~lifecycle ~ty:DateTime "not_after"
-          ~default_value:(Some (VDateTime Date.never))
+          ~default_value:(Some (VDateTime Date.epoch))
           "Date before which the certificate is valid"
       ; field ~qualifier:StaticRO
           ~lifecycle:

diff --git a/ocaml/idl/datamodel_host.ml b/ocaml/idl/datamodel_host.ml
@@ -935,7 +935,9 @@ let create_params =
   ; {
       param_type= DateTime
     ; param_name= "last_software_update"
-    ; param_doc= "Date and time when the last software update was applied."
+    ; param_doc=
+        "Date and time when the last software update was applied. When the \
+         timezone is missing, UTC is assumed"
     ; param_release= dundee_release
     ; param_default= Some (VDateTime Date.epoch)
     }
@@ -2188,8 +2190,7 @@ let t =
             "tls_verification_enabled" ~default_value:(Some (VBool false))
             "True if this host has TLS verifcation enabled"
         ; field ~qualifier:DynamicRO ~lifecycle:[] ~ty:DateTime
-            "last_software_update"
-            ~default_value:(Some (VDateTime (Date.of_float 0.0)))
+            "last_software_update" ~default_value:(Some (VDateTime Date.epoch))
             "Date and time when the last software update was applied"
         ; field ~qualifier:DynamicRO ~lifecycle:[] ~ty:Bool
             ~default_value:(Some (VBool false)) "https_only"

diff --git a/ocaml/idl/datamodel_types.ml b/ocaml/idl/datamodel_types.ml
@@ -26,12 +26,12 @@
 
 *)
 module Date = struct
-  open Xapi_stdext_date
+  module Date = Xapi_stdext_date.Date
   include Date
 
-  let iso8601_of_rpc rpc = Date.of_string (Rpc.string_of_rpc rpc)
+  let t_of_rpc rpc = Date.of_iso8601 (Rpc.string_of_rpc rpc)
 
-  let rpc_of_iso8601 date = Rpc.rpc_of_string (Date.to_string date)
+  let rpc_of_t date = Rpc.rpc_of_string (Date.to_rfc3339 date)
 end
 
 (* useful constants for product vsn tracking *)
@@ -418,7 +418,7 @@ type api_value =
   | VInt of int64
   | VFloat of float
   | VBool of bool
-  | VDateTime of Date.iso8601
+  | VDateTime of Date.t
   | VEnum of string
   | VMap of (api_value * api_value) list
   | VSet of api_value list

diff --git a/ocaml/idl/datamodel_types.mli b/ocaml/idl/datamodel_types.mli
@@ -1,9 +1,9 @@
 module Date : sig
   include module type of Xapi_stdext_date.Date
 
-  val iso8601_of_rpc : Rpc.t -> Xapi_stdext_date.Date.iso8601
+  val t_of_rpc : Rpc.t -> Xapi_stdext_date.Date.t
 
-  val rpc_of_iso8601 : Xapi_stdext_date.Date.iso8601 -> Rpc.t
+  val rpc_of_t : Xapi_stdext_date.Date.t -> Rpc.t
 end
 
 val oss_since_303 : string option
@@ -115,7 +115,7 @@ type api_value =
   | VInt of int64
   | VFloat of float
   | VBool of bool
-  | VDateTime of Date.iso8601
+  | VDateTime of Date.t
   | VEnum of string
   | VMap of (api_value * api_value) list
   | VSet of api_value list

diff --git a/ocaml/idl/datamodel_values.ml b/ocaml/idl/datamodel_values.ml
@@ -40,7 +40,7 @@ let rec to_rpc v =
   | VBool b ->
       Rpc.Bool b
   | VDateTime d ->
-      Rpc.String (Date.to_string d)
+      Rpc.String (Date.to_rfc3339 d)
   | VEnum e ->
       Rpc.String e
   | VMap vvl ->
@@ -94,7 +94,7 @@ let to_db v =
   | VBool false ->
       String "false"
   | VDateTime d ->
-      String (Date.to_string d)
+      String (Date.to_rfc3339 d)
   | VEnum e ->
       String e
   | VMap vvl ->
@@ -117,7 +117,7 @@ let gen_empty_db_val t =
   | Bool ->
       Value.String "false"
   | DateTime ->
-      Value.String (Date.to_string Date.never)
+      Value.String Date.(to_rfc3339 epoch)
   | Enum (_, (enum_value, _) :: _) ->
       Value.String enum_value
   | Enum (_, []) ->

diff --git a/ocaml/idl/datamodel_vm.ml b/ocaml/idl/datamodel_vm.ml
@@ -339,7 +339,11 @@ let update_snapshot_metadata =
       [
         (Ref _vm, "vm", "The VM to update")
       ; (Ref _vm, "snapshot_of", "")
-      ; (DateTime, "snapshot_time", "")
+      ; ( DateTime
+        , "snapshot_time"
+        , "The timestamp the snapshot was taken. When a timezone is missing, \
+           UTC is assumed"
+        )
       ; (String, "transportable_snapshot_id", "")
       ]
     ~allowed_roles:_R_POOL_OP ()
@@ -2112,7 +2116,7 @@ let t =
             "List pointing to all the VM snapshots."
         ; field ~writer_roles:_R_VM_POWER_ADMIN ~qualifier:DynamicRO
             ~in_product_since:rel_orlando
-            ~default_value:(Some (VDateTime Date.never)) ~ty:DateTime
+            ~default_value:(Some (VDateTime Date.epoch)) ~ty:DateTime
             "snapshot_time" "Date/time when this snapshot was created."
         ; field ~writer_roles:_R_VM_POWER_ADMIN ~qualifier:DynamicRO
             ~in_product_since:rel_orlando ~default_value:(Some (VString ""))

diff --git a/ocaml/idl/dune b/ocaml/idl/dune
@@ -45,11 +45,12 @@
   (action (run %{x} -closed -markdown))
 )
 
-(test
-  (name schematest)
+(tests
+  (names schematest test_datetimes)
   (modes exe)
-  (modules schematest)
+  (modules schematest test_datetimes)
   (libraries
+    astring
     rpclib.core
     rpclib.json
     xapi_datamodel

diff --git a/ocaml/idl/json_backend/gen_json.ml b/ocaml/idl/json_backend/gen_json.ml
@@ -89,7 +89,7 @@ end = struct
     | VBool x ->
         string_of_bool x
     | VDateTime x ->
-        Date.to_string x
+        Date.to_rfc3339 x
     | VEnum x ->
         x
     | VMap x ->

diff --git a/ocaml/idl/ocaml_backend/gen_api.ml b/ocaml/idl/ocaml_backend/gen_api.ml
@@ -413,9 +413,9 @@ let gen_client_types highapi =
            "module Date = struct"
          ; "  open Xapi_stdext_date"
          ; "  include Date"
-         ; "  let rpc_of_iso8601 x = DateTime (Date.to_string x)"
-         ; "  let iso8601_of_rpc = function String x | DateTime x -> \
-            Date.of_string x | _ -> failwith \"Date.iso8601_of_rpc\""
+         ; "  let rpc_of_t x = DateTime (Date.to_rfc3339 x)"
+         ; "  let t_of_rpc = function String x | DateTime x -> Date.of_iso8601 \
+            x | _ -> failwith \"Date.t_of_rpc\""
          ; "end"
          ]
        ; [

diff --git a/ocaml/idl/ocaml_backend/gen_db_actions.ml b/ocaml/idl/ocaml_backend/gen_db_actions.ml
@@ -69,7 +69,7 @@ let dm_to_string tys : O.Module.t =
       | DT.Bool ->
           "string_of_bool"
       | DT.DateTime ->
-          "Date.to_string"
+          "Date.to_rfc3339"
       | DT.Enum (_name, cs) ->
           let aux (c, _) =
             Printf.sprintf {|| %s -> "%s"|} (OU.constructor_of c) c
@@ -119,7 +119,7 @@ let string_to_dm tys : O.Module.t =
       | DT.Bool ->
           "bool_of_string"
       | DT.DateTime ->
-          "fun x -> Date.of_string x"
+          "fun x -> Date.of_iso8601 x"
       | DT.Enum (name, cs) ->
           let aux (c, _) = "\"" ^ c ^ "\" -> " ^ OU.constructor_of c in
           "fun v -> match v with\n      "

diff --git a/ocaml/idl/ocaml_backend/gen_test.ml b/ocaml/idl/ocaml_backend/gen_test.ml
@@ -32,7 +32,7 @@ let rec gen_test_type highapi ty =
     | DT.Bool ->
         "true"
     | DT.DateTime ->
-        "(Date.of_string \"20120101T00:00:00Z\")"
+        "(Date.of_iso8601 \"20120101T00:00:00Z\")"
     | DT.Enum (_, (x, _) :: _) ->
         Printf.sprintf "(%s)" (OU.constructor_of x)
     | DT.Set (DT.Enum (_, y)) ->

diff --git a/ocaml/idl/ocaml_backend/ocaml_utils.ml b/ocaml/idl/ocaml_backend/ocaml_utils.ml
@@ -128,7 +128,7 @@ let rec ocaml_of_ty = function
   | Bool ->
       "bool"
   | DateTime ->
-      "Date.iso8601"
+      "Date.t"
   | Set (Record x) ->
       alias_of_ty (Record x) ^ " list"
   | Set x ->