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

Fix date representations for CH Center #4721

Merged
merged 2 commits into from
May 25, 2022

Conversation

lindig
Copy link
Contributor

@lindig lindig commented May 24, 2022

Citrix Hypervisor center receives dates via XMLRPC and JSON RPC. Recently introduced dates don't conform to the XMLRPC date format - so fix these. The format itself is a somewhat questionable interpretation for ISO8601 because it mixes no punctuation for the the date with punctuation for the time but this is the format provided in examples by http://xmlrpc.com/spec.md. See the commit messages for details.

@lindig lindig requested review from edwintorok and psafont May 24, 2022 09:45
Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

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

The change from using localtime to of_float will indeed place the Z at the end of the string as needed. I wanted to note the dislike of baking in the representation at the time of creation of the date value instead of when it needs to be serialized to string.

@@ -950,7 +954,8 @@ let create ~__context ~uuid ~name_label ~name_description:_ ~hostname ~address
)
~control_domain:Ref.null ~updates_requiring_reboot:[] ~iscsi_iqn:""
~multipathing:false ~uefi_certificates:"" ~editions:[] ~pending_guidances:[]
~tls_verification_enabled ~last_software_update:(Date.localtime ()) ;
~tls_verification_enabled
~last_software_update:(get_servertime ~__context ~host:ref) ;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that setting this at host creation time is useful: it'd imply that updates got installed at the time the host was created, but that could've been just an outdated base ISO. Can the field be made optional and only set when we installed any updates?
This PR should fix the CHC protocol issue, so lets worry about fixing the semantics in a followup PR

Copy link
Member

Choose a reason for hiding this comment

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

I think Date.never could be used here. It's defined as Date.of_float 0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good.

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've changed the code to use Date.never and will merge it once the CI has passed.

We want to use Xapi_host.get_servertime instead of Date.localtime
because it impacts the representation used for XMLRPC. This is obviously
a design deficiency as the representation of dates in XMLRPC should be
decided during serialisation and not at creation.

Signed-off-by: Christian Lindig <christian.lindig@citrix.com>

fixup! CP-38583 add Host.last_software_update field with data/time

Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
Change the date format for the build as it is recorded in the
Host.software_version.date field to match the XMLRPC date format for
consistency and expectations by Citrix Hypervisor Center.

The format used by XMLRPC http://xmlrpc.com/spec.md
is claimed to be ISO8601:

  19980717T14:08:55

This is somewhat doubtful becuase it mixes no punctuation for the date
with punctuation for the time.

https://stackoverflow.com/a/36775198

Signed-off-by: Christian Lindig <christian.lindig@citrix.com>
@lindig lindig force-pushed the private/christianlin/CP-38583 branch from 2b1b06b to 34124f1 Compare May 25, 2022 09:57
@lindig lindig merged commit 98239da into xapi-project:master May 25, 2022
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.

3 participants