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

Support rfc3339 micro-sec (6 digit) timestamp #1477

Conversation

yue9944882
Copy link
Member

@yue9944882 yue9944882 commented Jan 6, 2021

@brendandburns @dsyer

it's a follow-up of #1418.

this pull will force the timestamp types to be serialized including 6 digit of fraction in order to support the precision of micro-second.

the original kubernetes go types basically accepts two kinds of timestamp in RFC3339:

  1. RFC3339 = "2006-01-02T15:04:05Z07:00" for the go type metav1.Time
  2. RFC3339Micro = "2006-01-02T15:04:05.000000Z07:00" for the go type metav1.MicroTime

at the kubernetes server side, (2) can be implicitly casted to (1) if extra precision is provided. ideally java model should discriminate (1) and (2) w/ separate java types so that we can switch the precision dynamically upon serialization. however, that's infeasible for now b/c the published openapi spec from kubernetes server is marking (1) and (2) as the same openapi schema types.

the bright side is that discriminating time types in different precision w/ different java types can be odd aesthetically..

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 6, 2021
@yue9944882 yue9944882 changed the title Support rfc3339 micro 6 digit timestamp Support rfc3339 micro-sec (6 digit) timestamp Jan 6, 2021
.optionalEnd()
.appendLiteral("Z")
.toFormatter();

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is necessary. Why do we need an explicit formatter for a format that already works by default with OffsetDateTime (I believe)?

Copy link
Member Author

@yue9944882 yue9944882 Jan 6, 2021

Choose a reason for hiding this comment

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

kubernetes server-side requires 6-digit fraction for micro timestamp types (e.g. LeaseSpec#accquireTime) where "0" should be padded in the end if necessary. however, the default DateTimeFormatter.ISO_OFFSET_DATE_TIME doesn't support right-padding for micro-sec timestamps. e.g. by the default formatter2018-04-03T11:32:26.1234Z is a valid timestamp, but kubernetes server will be rejecting b/c lacking right-padded "0"s, it's expected to be 2018-04-03T11:32:26.123400Z

spec =
spec.acquireTime(
OffsetDateTime.ofInstant(
Instant.ofEpochMilli(record.getAcquireTime().getTime()), ZoneOffset.UTC));
Copy link
Contributor

Choose a reason for hiding this comment

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

This drops the milliseconds, right? Is that the intention? It also assumes that the timestamps are all UTC (but I think that is guaranteed by the k8s spec if they come from the server).

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, the leader-elector manages resource-locks w/ millis-sec timestamp, so it doesn't make a difference whether LeaseLock provides extra precision.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 6, 2021
@brendandburns
Copy link
Contributor

This LGTM but I will wait for an ack from @dsyer

@dsyer
Copy link
Contributor

dsyer commented Jan 7, 2021

I see what this does now, and while I hate to add even more manual overrides to the JSON utility, I guess this is the right thing to do. I think I would prefer RFC3339MICRO_FORMATTER to be private though (it's an implementation detail, used once in a test, but you could just as easily call JSON.serialize() there instead of formatter.format()).

@dsyer
Copy link
Contributor

dsyer commented Jan 7, 2021

I also think there might be a problem with using java.util.Date in the LeaderElectionRecord because it might use the system default timezone whereas the incoming timestamps from the API server are all in UTC. Seems like it would be simpler just to pass the OffsetDateTime through from the API to the leader elector.

@yue9944882
Copy link
Member Author

I see what this does now, and while I hate to add even more manual overrides to the JSON utility, I guess this is the right thing to do. I think I would prefer RFC3339MICRO_FORMATTER to be private though (it's an implementation detail, used once in a test, but you could just as easily call JSON.serialize() there instead of formatter.format()).

@dsyer i made the formatter private in the latest commit

I also think there might be a problem with using java.util.Date in the LeaderElectionRecord because it might use the system default timezone whereas the incoming timestamps from the API server are all in UTC. Seems like it would be simpler just to pass the OffsetDateTime through from the API to the leader elector.

refactoring the leader-elector to use OffsetDateTime to track the lease record sounds good to me. but i think we should defer that in a separate follow-up to unwound the e2e failure ASAP.

@dsyer
Copy link
Contributor

dsyer commented Jan 7, 2021

LGTM then.

@brendandburns
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, yue9944882

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [brendandburns,yue9944882]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit a8c89e0 into kubernetes-client:master Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants