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.

Sign up for GitHub

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

Jump to bottom

Events corev1: adding eventTime initialization #80222

Closed
wants to merge 1 commit into from
Closed

Events corev1: adding eventTime initialization #80222

wants to merge 1 commit into from

Conversation

krzysied
Copy link
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:
Initialized deprecated fields for the correctness purpose.

Which issue(s) this PR fixes:

Fixes #80219

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 16, 2019
@k8s-ci-robot k8s-ci-robot requested review from wojtek-t and yastij July 16, 2019 15:17
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 16, 2019
@krzysied
Copy link
Contributor Author

/assign @wojtek-t
/cc @yastij

@krzysied
Copy link
Contributor Author

Corev1 event creation for a reference:

kubernetes/staging/src/k8s.io/client-go/tools/record/event.go

Lines 334 to 347 in 0791f5b

return &v1.Event{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("%v.%x", ref.Name, t.UnixNano()),
Namespace: namespace,
Annotations: annotations,
},
InvolvedObject: *ref,
Reason: reason,
Message: message,
FirstTimestamp: t,
LastTimestamp: t,
Count: 1,
Type: eventtype,
}

wojtek-t
wojtek-t reviewed Jul 16, 2019
View reviewed changes
staging/src/k8s.io/client-go/tools/events/event_recorder.go Outdated
Type: eventtype,
DeprecatedFirstTimestamp: t,
DeprecatedLastTimestamp: t,
DeprecatedCount: 1,
Copy link
Member

Choose a reason for hiding this comment

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

I'm far from being convinced this is exactly what we want.

  1. We should probably change conversion to convert between deprecated_last_time <-> eventTime
    ahh - we can't do that easily because of different types, I need to think about it a bit more..
  2. DeprecatedCount - we shouldn't populate it, because it will get outdated later
  3. FirstTimestamp - maybe, but I need to think about it more...

Copy link
Member

Choose a reason for hiding this comment

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

I agree mostly agree. we need to change conversion

  1. we should do it anyway I think
  2. maybe should be converted from EventSeries count
  3. either from creationTimestamp or maybe set it until we drop these fields

is this change related to #80212 ?

Copy link
Member

Choose a reason for hiding this comment

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

s this change related to #80212 ?

It's not.

After thinking a bit more, I'm not even sure the conversions are broken.
@krzysied - to fix your problem, we should simply start propagating EventTime field in the old events library too:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/record/event.go#L343
and switch to using EventTime field instead of FirstEventTimestamp.

Re DeprecatedLasttimestamp and DeprecatedCount, I wouldn't touch it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I added EventTime initialization to corev1 and removed previous changed.

@krzysied krzysied force-pushed the event_deprecated_timestamp branch from 47e4898 to 59ba15c Compare July 17, 2019 10:00
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 17, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: krzysied
To complete the pull request process, please assign wojtek-t
You can assign the PR to them by writing /assign @wojtek-t in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@krzysied krzysied changed the title Events v1beta1: adding deprecated fields initialization Events corev1: adding eventTime initialization Jul 17, 2019
wojtek-t
wojtek-t reviewed Jul 17, 2019
View reviewed changes
staging/src/k8s.io/client-go/tools/record/event.go
@@ -340,6 +340,7 @@ func (recorder *recorderImpl) makeEvent(ref *v1.ObjectReference, annotations map
InvolvedObject: *ref,
Reason: reason,
Message: message,
EventTime: (metav1.MicroTime)(t),
Copy link
Member

Choose a reason for hiding this comment

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

Requires changing unit tests too.

@krzysied krzysied force-pushed the event_deprecated_timestamp branch 2 times, most recently from 43919fe to 05588dd Compare July 17, 2019 11:26
@krzysied krzysied force-pushed the event_deprecated_timestamp branch from 05588dd to 67e1974 Compare July 17, 2019 12:43
@krzysied
Copy link
Contributor Author

/test pull-kubernetes-integration

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 17, 2019 •
edited
Loading

@krzysied: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce 67e1974 link /test pull-kubernetes-e2e-gce
pull-kubernetes-integration 67e1974 link /test pull-kubernetes-integration

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@krzysied
Copy link
Contributor Author

After digging into k8s code I found out that EventTime being set or not is a differentiation between old and new events. My assumption that EventTime shoud be always set was incorrect then...

kubernetes/pkg/apis/core/validation/events.go

Lines 42 to 43 in b5dfcf3

// "New" Events need to have EventTime set, so it's validating old object.
if event.EventTime.Time == zeroTime {

I'm closing the PR

@krzysied krzysied closed this Jul 18, 2019
@krzysied krzysied mentioned this pull request Jul 18, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wojtek-t wojtek-t wojtek-t left review comments

@yastij yastij Awaiting requested review from yastij

Assignees

@wojtek-t wojtek-t

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Empty FirstTimestamp field in scheduler events
4 participants
@krzysied @k8s-ci-robot @yastij @wojtek-t