kubectl: Use fields from event series when computing describe events for a object #104482

harjas27 · 2021-08-20T16:32:39Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

When using kubectl describe <resource> <name> , the events returned for the resource do not use the EventSeries.Count and EventSeries.LastObservedTime
This PR adds the logic to compute the age of the event using these fields

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#1095

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Changed kubectl describe to compute Age of an event using the count and lastObservedTime fields available in the event series

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

k8s-ci-robot · 2021-08-20T16:32:47Z

Hi @harjas27. 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.

harjas27 · 2021-08-20T16:36:46Z

/cc @eddiezane

lauchokyip · 2021-08-22T18:50:52Z

/ok-to-test
/triage accepted
/priority backlog

KnVerey · 2021-09-01T15:08:58Z

staging/src/k8s.io/kubectl/pkg/describe/describe.go

+			firstTimestampSince = translateTimestampSince(e.FirstTimestamp)
+		}
+		if e.Series != nil {
+			interval = fmt.Sprintf("%s (x%d over %s)", translateMicroTimestampSince(e.Series.LastObservedTime), e.Series.Count+1, firstTimestampSince)


Why e.Series.Count+1? Strictly speaking e.Series should be nil when only one event has occurred:

// Data about the Event series this event represents or nil if it's a singleton Event. // +optional Series *EventSeries `json:"series,omitempty" protobuf:"bytes,11,opt,name=series"`

I can see the argument that since we have an event, e.Series.Count should logically be at least 1. However, if the field does contain a number, I don't think we should increment it.

Take a look at this example from the KEP:

# After second occurrence, Event looks like: { regarding: A, action: B, reportingController: C, ..., series: {count: 2}, }

I had added the +1 following a comment in the code:

kubernetes/pkg/printers/internalversion/printers.go

Lines 1789 to 1793 in 7a0638d

// When a series is created for the first time, its count is set to 1.

// However, for a series to be created, there needs to be an isomorphic

// singleton event created beforehand. This singleton event is not

// counted in the series count which is why one is added here.

count = obj.Series.Count + 1

Also referred this. When a series is created for the first time, its count is set to 1 and for the series to be created, there needs to be a singleton event created.

Hm, that's a good find, but I'm still unsure what is correct. It strikes me as very strange to expect clients (including humans reading the -o yaml output directly) to know the implementation details of the reporter, i.e. that it considers the series to exclude the first occurrence. The description in the KEP (that count should be 2 on the second occurrence) makes a lot more sense to me.

I also found the following validation, which disallows event counts less than two:

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

Lines 110 to 117 in c224ca7

if event.Series != nil {

if event.Series.Count < 2 {

allErrs = append(allErrs, field.Invalid(field.NewPath("series.count"), "", "should be at least 2"))

}

if event.Series.LastObservedTime.Time == zeroTime {

allErrs = append(allErrs, field.Required(field.NewPath("series.lastObservedTime"), ""))

}

}

And the following comment thread, from the implementation PR:

cc @soltysh since you were approver on the printers PR

@KnVerey good catch, it looks like at the time the printer PR merged the validation wasn't fully complete. If you check the current code

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

Lines 110 to 117 in 758ad07

if event.Series != nil {

if event.Series.Count < 2 {

allErrs = append(allErrs, field.Invalid(field.NewPath("series.count"), "", "should be at least 2"))

}

if event.Series.LastObservedTime.Time == zeroTime {

allErrs = append(allErrs, field.Required(field.NewPath("series.lastObservedTime"), ""))

}

}

you'll notice that the minimum required value for counter is 2 and not one like from the above script. Which means that the series represent values exactly like describe in the KEP and not in client-go. So we need to drop +1 from this PR and also from the printers.

soltysh

/approve
/hold

You still need to address @KnVerey comment dropping +1 from counter and fix the release notes. @KnVerey has the final approval on the PR

KnVerey · 2021-09-10T18:52:46Z

@harjas27 please update the test assertion and add a release note to your PR description

KnVerey · 2021-09-13T19:54:29Z

/label tide/merge-method-squash
/unhold
/lgtm
/approve

Thank you @harjas27 ! Would you be interested in contributing the fix to remove the extra +1 from the printer code you found in a follow up PR?

k8s-ci-robot · 2021-09-13T19:55:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harjas27, KnVerey, soltysh

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/kubectl/OWNERS~~ [soltysh]

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

…for a object (kubernetes#104482) * take into account new fields for event * add event with old event fields for test * fix: remove unnecessary "+1" from event series count * fix: update the assertion for failing test case

harjas27 added 2 commits August 20, 2021 20:49

take into account new fields for event

2947ac8

add event with old event fields for test

b4db3bd

k8s-ci-robot requested review from brianpursley and KnVerey August 20, 2021 16:33

k8s-ci-robot requested a review from eddiezane August 20, 2021 16:36

pacoxu added this to Needs review in SIG CLI Aug 24, 2021

KnVerey reviewed Sep 1, 2021

View reviewed changes

soltysh approved these changes Sep 10, 2021

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 10, 2021

fix: remove unnecessary "+1" from event series count

5680c7f

fix: update the assertion for failing test case

8c3e156

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 10, 2021

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 13, 2021

k8s-ci-robot assigned KnVerey Sep 13, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2021

k8s-ci-robot merged commit c6935ad into kubernetes:master Sep 13, 2021

SIG CLI automation moved this from Needs review to Done Sep 13, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 13, 2021

harjas27 mentioned this pull request Sep 14, 2021

kubectl: remove extra +1 for printing event count #105009

Merged

ccampo133 mentioned this pull request Sep 16, 2021

elbv2 multicert ccampo133/kubernetes#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubectl: Use fields from event series when computing describe events for a object #104482

kubectl: Use fields from event series when computing describe events for a object #104482

harjas27 commented Aug 20, 2021 •

edited

k8s-ci-robot commented Aug 20, 2021

harjas27 commented Aug 20, 2021

lauchokyip commented Aug 22, 2021

KnVerey Sep 1, 2021

harjas27 Sep 1, 2021

KnVerey Sep 1, 2021

soltysh Sep 10, 2021

soltysh left a comment

KnVerey commented Sep 10, 2021

KnVerey commented Sep 13, 2021

k8s-ci-robot commented Sep 13, 2021

	// When a series is created for the first time, its count is set to 1.
	// However, for a series to be created, there needs to be an isomorphic
	// singleton event created beforehand. This singleton event is not
	// counted in the series count which is why one is added here.
	count = obj.Series.Count + 1

	if event.Series != nil {
	if event.Series.Count < 2 {
	allErrs = append(allErrs, field.Invalid(field.NewPath("series.count"), "", "should be at least 2"))
	}
	if event.Series.LastObservedTime.Time == zeroTime {
	allErrs = append(allErrs, field.Required(field.NewPath("series.lastObservedTime"), ""))
	}
	}

kubectl: Use fields from event series when computing describe events for a object #104482

kubectl: Use fields from event series when computing describe events for a object #104482

Conversation

harjas27 commented Aug 20, 2021 • edited

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 20, 2021

harjas27 commented Aug 20, 2021

lauchokyip commented Aug 22, 2021

KnVerey Sep 1, 2021

Choose a reason for hiding this comment

harjas27 Sep 1, 2021

Choose a reason for hiding this comment

KnVerey Sep 1, 2021

Choose a reason for hiding this comment

soltysh Sep 10, 2021

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

KnVerey commented Sep 10, 2021

KnVerey commented Sep 13, 2021

k8s-ci-robot commented Sep 13, 2021

harjas27 commented Aug 20, 2021 •

edited