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
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
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 Once the patch is verified, the new status will be reflected by the 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. |
/cc @eddiezane |
/ok-to-test |
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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},
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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"), "")) | |
} | |
} |
+1
from this PR and also from the printers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harjas27 please update the test assertion and add a release note to your PR description |
/label tide/merge-method-squash 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? |
[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:
Approvers can indicate their approval by writing |
…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
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 theEventSeries.Count
andEventSeries.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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: