Marshal MicroTime to json and proto at the same precision #111936

haoruan · 2022-08-19T14:59:50Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Truncate the nanosecond to microsecond when marshaling and unmarshaling proto, make sure the same precision for JSON and proto marshaling/unmarshaling.

Which issue(s) this PR fixes:

Fixes #111928

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Protobuf serialization of metav1.MicroTime timestamps (used in `Lease` and `Event` API objects) has been corrected to truncate to microsecond precision, to match the documented behavior and JSON/YAML serialization. Any existing persisted data is truncated to microsecond when read from etcd.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2022-08-19T14:59:53Z

Please note that we're already in Test Freeze for the release-1.25 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.25.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Fri Aug 19 13:42:37 UTC 2022.

k8s-ci-robot · 2022-08-19T14:59:58Z

Hi @haoruan. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-triage-robot · 2022-08-19T15:11:54Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

haoruan · 2022-08-19T22:13:24Z

/sig api-machinery

leilajal · 2022-08-23T20:07:14Z

/cc @liggitt
/triage accepted

liggitt · 2022-08-23T20:16:49Z

/ok-to-test

haoruan · 2022-08-23T22:37:56Z

/retest

haoruan · 2022-08-25T00:50:06Z

@liggitt PTAL :)

liggitt · 2022-08-25T14:14:44Z

/lgtm
/approve

I updated the release note with more details for users.

The fix looks correct to me, and because the truncation happens on both marshal and unmarshal, it handles already-persisted data that incorrectly kept nanosecond-level precision gracefully. I'd like a second set of eyes on this ahead of merge, just to double check my assertion about compatibility. I think we should probably backport this as well.

cc @kubernetes/api-reviewers
/assign @deads2k

liggitt · 2022-08-25T14:14:52Z

/hold for second ack from API reviewer

lavalamp · 2022-09-01T16:52:24Z

We should backport this as far as we can. I'm surprised it hasn't caused major problems.

liggitt · 2022-09-01T17:07:41Z

Thinking through all combinations of old/new proto clients, old/new servers, and good/bad existing data, I think we have one remaining gap:

Client	Server	Persisted Data	Result
Old	Old	Good	✅ client and server match
Old	Old	Bad	✅ client and server match and round-trip nanoseconds
Old	New	Good	✅ server no-ops, client round-trips with nanoseconds
Old	New	Bad	✅ server truncates to microseconds, client round-trips with nanoseconds
New	Old	Good	✅ server returns microseconds, client round-trips with microseconds
New	Old	Bad	🚫 server returns nanoseconds, client truncates on round-trip, server rejects update because it mutates the event time. Will be tolerated by servers that include #112183, being backported to 1.22+
New	New	Good	✅ client and server match and round-trip microseconds
New	New	Bad	✅ client and server match, server truncates to microseconds

Before changing serialization such that proto clients will drop nanoseconds, we need to make sure older servers will tolerate this.

liggitt · 2022-09-01T17:08:48Z

I'm surprised it hasn't caused major problems.

I think most components have not switched to the new Events API and are still using core/v1 Events, so are not hitting the stricter ValidateImmutableField(newEvent.EventTime, oldEvent.EventTime,... check yet

liggitt · 2022-09-01T17:17:22Z

I'm surprised it hasn't caused major problems.

I think most components have not switched to the new Events API and are still using core/v1 Events, so are not hitting the stricter ValidateImmutableField(newEvent.EventTime, oldEvent.EventTime,... check yet

cc @dgrisonnet for visibility to this issue that needs resolving prior to switching more components to the new Events API

liggitt · 2022-09-01T18:57:03Z

Before changing serialization such that proto clients will drop nanoseconds, we need to make sure older servers will tolerate this.

I opened #112183 to tolerate this, which we can backport independent of this serialization change.

deads2k · 2022-09-01T19:29:23Z

Other than the test gap noted, this looks correct to me. I prefer to merge and backport
#112183 before merging this.

haoruan · 2022-09-02T08:30:04Z

@liggitt @lavalamp @deads2k thanks for the review, one test case was added to test truncation on unmarshaling old nanosecond-precise microtime.

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/micro_time_test.go

liggitt · 2022-09-02T15:34:19Z

#112183 is merged and picked back to 1.22+

/lgtm
/approve
/hold cancel

After this merges to master, go ahead and open picks of this change to 1.23+ and we can figure out the timing we want to merge them to release branches

k8s-ci-robot · 2022-09-02T15:34:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haoruan, liggitt

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:

~~staging/src/k8s.io/apimachinery/pkg/apis/OWNERS~~ [liggitt]

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

haoruan · 2022-09-03T08:34:43Z

Thanks, opened picks to 1.23-1.25

…936-upstream-release-1.25 Automated cherry pick of #111936: Marshal MicroTime to json and proto at the same precision

…936-upstream-release-1.24 Automated cherry pick of #111936: Marshal MicroTime to json and proto at the same precision

…936-upstream-release-1.23 Automated cherry pick of #111936: Marshal MicroTime to json and proto at the same precision

Ref: kubernetes/kubernetes#111936

…`v0.14.1` (#7248) * Update `k8s.io/*` to `v0.26.0` * Update `sigs.k8s.io/controller-runtime` to `v0.14.1` * Set `RecoverPanic` globally for the manager of `controller-manager` controllers * Set `RecoverPanic` globally for the manager of `gardenlet` controllers * Set `RecoverPanic` globally for the manager of `operator` controllers * Set `RecoverPanic` globally for the manager of `resource-manager` controllers * Set `RecoverPanic` globally for the manager of `scheduler` controllers * Set `RecoverPanic` globally in manager options for extension controllers * Change RecoverPanic bool to bool pointer for other controllers * Run `make generate` * Set `AllowInvalidLabelValueInSelector` to true in LabelSelectorValidationOptions for backwards compatibility See kubernetes/kubernetes#113699 for more details * Drop `Not` predicate util function in favor of controller-runtime `predicate.Not` * Drop `NewClientWithFieldSelectorSupport` function in favor of controller-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025 * Use `Build()` function for all controllers * Use Subresource client for `shoots/binding` and drop generated clientset * Use Subresource client for `shoots/adminkubeconfig` and drop generated clientset * Use Status() client for Status Ref: kubernetes-sigs/controller-runtime#2072 * Adapt unit tests with mock StatusWriter * Add "ValidatingAdmissionPolicy" to default admission plugins * Adapt changes related to removed fields in kube-proxy config * Add unit test cases for "#IsAdmissionPluginSupported" function * Update envtest version * Return error from `informer.AddEventHandler` * Copy onsi/gomega/format package also to gomegacheck testdata * Address PR review feedback * Vendor current `etcd-druid` master * Use `builder.Watches()` wherever possible * Call `etcdOptions.Complete()` for gardener apiserver config etcd-encryption is supported out-of-the-box now. ref: kubernetes/kubernetes#112789 * Use Subresource client for `serviceaccounts/token` wherever possible * Update `ahmetb/gen-crd-api-reference-docs` to `0.3.0` * Update `sigs.k8s.io/controller-tools` to `v0.11.0` Update to `v0.11.1` once kubernetes/kubernetes/pull/114617 is released. In k8s v0.26.0 the CRD generation is broken. * Remove unneeded dependencies for `gardener-scheduler` from skaffold.yaml * Hardcode `RecoverPanic` to true for extensions controller * Use `k8s.io/apimachinery/pkg/util/sets` and drop copied packages * Drop ready check for garden informer sync for gardenlet Adding ready check to manager is not possible after it's started, since controller-runtime, `v0.11.0`. However the check was broken, and it is now fixed in kubernetes-sigs/controller-runtime#2090, so we have to drop this. * Adapt `provider-local` webhook * Address PR review feedback * Fix panic in ManagedSeed controller Contexts: from this PR, we're not setting RecoverPanic to true in tests * Vendor `etcd-druid` `v0.15.3` * Update `k8s.io/*` and `controller-runtime` k8s.io/* - 0.26.0=>0.26.1 controller-tools - 0.11.0=>0.11.1 * Run `make generate` * Rebase * Address PR review feedback * Fix failing test * Adapt `highavailabilityconfig` webhook integration test `autoscaling/v2beta2.HorizontalPodAutoscaler` is removed in v1.26. Ref: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-26 * Use `apiutil.NewDynamicRESTMapper` for Manager cache in tests Co-authored-by: Rafael Franzke <rafael.franzke@sap.com> * Don't set `RecoverPanic` for os extensions in AddToManager This is not required since we call `mgrOpts.Options()` here : https://github.com/gardener/gardener/blob/c9cb564d1adad0a1ecf9d44f23fb249bcf946a79/extensions/pkg/controller/operatingsystemconfig/oscommon/app/app.go#L88 * Truncate time in test to microsecond precision Ref: kubernetes/kubernetes#111936 * Rebase --------- Co-authored-by: Rafael Franzke <rafael.franzke@sap.com>

…`v0.14.1` (gardener#7248) * Update `k8s.io/*` to `v0.26.0` * Update `sigs.k8s.io/controller-runtime` to `v0.14.1` * Set `RecoverPanic` globally for the manager of `controller-manager` controllers * Set `RecoverPanic` globally for the manager of `gardenlet` controllers * Set `RecoverPanic` globally for the manager of `operator` controllers * Set `RecoverPanic` globally for the manager of `resource-manager` controllers * Set `RecoverPanic` globally for the manager of `scheduler` controllers * Set `RecoverPanic` globally in manager options for extension controllers * Change RecoverPanic bool to bool pointer for other controllers * Run `make generate` * Set `AllowInvalidLabelValueInSelector` to true in LabelSelectorValidationOptions for backwards compatibility See kubernetes/kubernetes#113699 for more details * Drop `Not` predicate util function in favor of controller-runtime `predicate.Not` * Drop `NewClientWithFieldSelectorSupport` function in favor of controller-runtime `WithIndex` function kubernetes-sigs/controller-runtime/pull/2025 * Use `Build()` function for all controllers * Use Subresource client for `shoots/binding` and drop generated clientset * Use Subresource client for `shoots/adminkubeconfig` and drop generated clientset * Use Status() client for Status Ref: kubernetes-sigs/controller-runtime#2072 * Adapt unit tests with mock StatusWriter * Add "ValidatingAdmissionPolicy" to default admission plugins * Adapt changes related to removed fields in kube-proxy config * Add unit test cases for "#IsAdmissionPluginSupported" function * Update envtest version * Return error from `informer.AddEventHandler` * Copy onsi/gomega/format package also to gomegacheck testdata * Address PR review feedback * Vendor current `etcd-druid` master * Use `builder.Watches()` wherever possible * Call `etcdOptions.Complete()` for gardener apiserver config etcd-encryption is supported out-of-the-box now. ref: kubernetes/kubernetes#112789 * Use Subresource client for `serviceaccounts/token` wherever possible * Update `ahmetb/gen-crd-api-reference-docs` to `0.3.0` * Update `sigs.k8s.io/controller-tools` to `v0.11.0` Update to `v0.11.1` once kubernetes/kubernetes/pull/114617 is released. In k8s v0.26.0 the CRD generation is broken. * Remove unneeded dependencies for `gardener-scheduler` from skaffold.yaml * Hardcode `RecoverPanic` to true for extensions controller * Use `k8s.io/apimachinery/pkg/util/sets` and drop copied packages * Drop ready check for garden informer sync for gardenlet Adding ready check to manager is not possible after it's started, since controller-runtime, `v0.11.0`. However the check was broken, and it is now fixed in kubernetes-sigs/controller-runtime#2090, so we have to drop this. * Adapt `provider-local` webhook * Address PR review feedback * Fix panic in ManagedSeed controller Contexts: from this PR, we're not setting RecoverPanic to true in tests * Vendor `etcd-druid` `v0.15.3` * Update `k8s.io/*` and `controller-runtime` k8s.io/* - 0.26.0=>0.26.1 controller-tools - 0.11.0=>0.11.1 * Run `make generate` * Rebase * Address PR review feedback * Fix failing test * Adapt `highavailabilityconfig` webhook integration test `autoscaling/v2beta2.HorizontalPodAutoscaler` is removed in v1.26. Ref: https://kubernetes.io/docs/reference/using-api/deprecation-guide/#v1-26 * Use `apiutil.NewDynamicRESTMapper` for Manager cache in tests Co-authored-by: Rafael Franzke <rafael.franzke@sap.com> * Don't set `RecoverPanic` for os extensions in AddToManager This is not required since we call `mgrOpts.Options()` here : https://github.com/gardener/gardener/blob/c9cb564d1adad0a1ecf9d44f23fb249bcf946a79/extensions/pkg/controller/operatingsystemconfig/oscommon/app/app.go#L88 * Truncate time in test to microsecond precision Ref: kubernetes/kubernetes#111936 * Rebase --------- Co-authored-by: Rafael Franzke <rafael.franzke@sap.com>

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. labels Aug 19, 2022

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Aug 19, 2022

k8s-ci-robot requested a review from deads2k August 19, 2022 15:00

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 19, 2022

k8s-ci-robot requested a review from soltysh August 19, 2022 15:00

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 19, 2022

k8s-ci-robot requested a review from liggitt August 23, 2022 20:07

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 23, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 23, 2022

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 25, 2022

liggitt moved this from In progress to Changes requested in API Reviews Sep 1, 2022

liggitt mentioned this pull request Sep 1, 2022

Tolerate sub-microsecond eventTime changes on update #112183

Merged

Marshal MicroTime to json and proto at the same precision

7aa80ad

haoruan force-pushed the bugfix-111928-microtime-marshal-precision branch from d81aa4e to 7aa80ad Compare September 2, 2022 08:21

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2022

liggitt reviewed Sep 2, 2022

View reviewed changes

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/micro_time_test.go Show resolved Hide resolved

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 2, 2022

liggitt moved this from Changes requested to API review completed, 1.26 in API Reviews Sep 2, 2022

k8s-ci-robot merged commit 673895d into kubernetes:master Sep 2, 2022

k8s-ci-robot added a commit that referenced this pull request Oct 24, 2022

Merge pull request #112218 from haoruan/automated-cherry-pick-of-#111…

aeddf85

…936-upstream-release-1.25 Automated cherry pick of #111936: Marshal MicroTime to json and proto at the same precision

k8s-ci-robot added a commit that referenced this pull request Oct 25, 2022

Merge pull request #112217 from haoruan/automated-cherry-pick-of-#111…

6705cfe

…936-upstream-release-1.24 Automated cherry pick of #111936: Marshal MicroTime to json and proto at the same precision

k8s-ci-robot added a commit that referenced this pull request Oct 25, 2022

Merge pull request #112216 from haoruan/automated-cherry-pick-of-#111…

d2f47dc

…936-upstream-release-1.23 Automated cherry pick of #111936: Marshal MicroTime to json and proto at the same precision

shafeeqes added a commit to shafeeqes/gardener that referenced this pull request Jan 29, 2023

Truncate time in test to microsecond precision

e1a11f5

Ref: kubernetes/kubernetes#111936

shafeeqes added a commit to shafeeqes/gardener that referenced this pull request Jan 30, 2023

Truncate time in test to microsecond precision

90fc37d

Ref: kubernetes/kubernetes#111936

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Marshal MicroTime to json and proto at the same precision #111936

Marshal MicroTime to json and proto at the same precision #111936

haoruan commented Aug 19, 2022 •

edited by liggitt

k8s-ci-robot commented Aug 19, 2022

k8s-ci-robot commented Aug 19, 2022

k8s-triage-robot commented Aug 19, 2022

haoruan commented Aug 19, 2022

leilajal commented Aug 23, 2022

liggitt commented Aug 23, 2022

haoruan commented Aug 23, 2022

haoruan commented Aug 25, 2022

liggitt commented Aug 25, 2022

liggitt commented Aug 25, 2022

lavalamp commented Sep 1, 2022

liggitt commented Sep 1, 2022 •

edited

liggitt commented Sep 1, 2022

liggitt commented Sep 1, 2022

liggitt commented Sep 1, 2022

deads2k commented Sep 1, 2022

haoruan commented Sep 2, 2022

liggitt commented Sep 2, 2022

k8s-ci-robot commented Sep 2, 2022

haoruan commented Sep 3, 2022

Marshal MicroTime to json and proto at the same precision #111936

Marshal MicroTime to json and proto at the same precision #111936

Conversation

haoruan commented Aug 19, 2022 • edited by liggitt

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Aug 19, 2022

k8s-ci-robot commented Aug 19, 2022

k8s-triage-robot commented Aug 19, 2022

haoruan commented Aug 19, 2022

leilajal commented Aug 23, 2022

liggitt commented Aug 23, 2022

haoruan commented Aug 23, 2022

haoruan commented Aug 25, 2022

liggitt commented Aug 25, 2022

liggitt commented Aug 25, 2022

lavalamp commented Sep 1, 2022

liggitt commented Sep 1, 2022 • edited

liggitt commented Sep 1, 2022

liggitt commented Sep 1, 2022

liggitt commented Sep 1, 2022

deads2k commented Sep 1, 2022

haoruan commented Sep 2, 2022

liggitt commented Sep 2, 2022

k8s-ci-robot commented Sep 2, 2022

haoruan commented Sep 3, 2022

haoruan commented Aug 19, 2022 •

edited by liggitt

liggitt commented Sep 1, 2022 •

edited