fix: adapt kubernetes event by xjh1996 · Pull Request #29591 · elastic/beats

xjh1996 · 2021-12-23T06:53:54Z

What does this PR do?

adapt kubernetes event which lastTimestamp is empty or count is empty

apiVersion: v1
items:
- action: Scheduling
  apiVersion: v1
  eventTime: "2021-12-22T11:39:51.388829Z"
  firstTimestamp: null
  involvedObject:
    apiVersion: v1
    kind: Pod
    name: insufficent-697c868bd6-xvjxp
    namespace: sjh-ns
    resourceVersion: "390502"
    uid: 11f3fe9f-f918-482d-9302-09f7dc69f450
  kind: Event
  lastTimestamp: null
  message: '0/3 nodes are available: 3 Insufficient cpu.'
  metadata:
    creationTimestamp: "2021-12-22T11:39:51Z"
    name: insufficent-697c868bd6-xvjxp.16c310d3dada4017
    namespace: sjh-ns
    resourceVersion: "5095"
    uid: 956af82e-fb37-44a3-b23e-ab3e158d2e90
  reason: FailedScheduling
  reportingComponent: default-scheduler
  reportingInstance: default-scheduler-mc70ksqfqtofq1s4t4njg
  source: {}
  type: Warning
- action: Scheduling
  apiVersion: v1
  eventTime: "2021-12-22T11:39:51.434179Z"
  firstTimestamp: null
  involvedObject:
    apiVersion: v1
    kind: Pod
    name: insufficent-697c868bd6-xvjxp
    namespace: sjh-ns
    resourceVersion: "390503"
    uid: 11f3fe9f-f918-482d-9302-09f7dc69f450
  kind: Event
  lastTimestamp: null
  message: '0/3 nodes are available: 3 Insufficient cpu.'
  metadata:
    creationTimestamp: "2021-12-22T11:39:51Z"
    name: insufficent-697c868bd6-xvjxp.16c310d3dd8dc308
    namespace: sjh-ns
    resourceVersion: "5202"
    uid: 796cd76b-fc90-427f-a899-2587c181c180
  reason: FailedScheduling
  reportingComponent: default-scheduler
  reportingInstance: default-scheduler-mc70ksqfqtofq1s4t4njg
  series:
    count: 18
    lastObservedTime: "2021-12-22T12:06:01.048247Z"
  source: {}
  type: Warning
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""

Why is it important?

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

[ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

cla-checker-service · 2021-12-23T06:53:57Z

❌ Author of the following commits did not sign a Contributor Agreement:
, , , , , , , , , , , , , , , , , , , , 8d175b6

Please, read and sign the above mentioned agreement if you want to contribute to this project

elasticmachine · 2021-12-23T06:59:13Z

❕ Build Aborted

The PR is not allowed to run in the CI yet

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Reason: The PR is not allowed to run in the CI yet
Start Time: 2022-02-03T10:14:52.702+0000
Duration: 8 min 11 sec
Commit: 8d175b6

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

mtojek

Hi @xjh1996, you opened this PR against the old branch. Would you mind opening it against the latest main branch? Does it make sense?

xjh1996 · 2021-12-23T09:29:51Z

Hello @mtojek，I need to use this branch to avoid introducing other modifications in our environment.

mtojek · 2021-12-23T09:32:50Z

I'm afraid that you have to use your own fork in this case. We don't actively backport/support old branches. If you want to add a feature or code modification, feel free to start with the main branch and we can backport it to other places.

xjh1996 · 2021-12-23T09:55:54Z

I'm afraid that you have to use your own fork in this case. We don't actively backport/support old branches. If you want to add a feature or code modification, feel free to start with the main branch and we can backport it to other places.

@mtojek ok，I will rebase my code.

mergify · 2022-01-03T03:01:01Z

This pull request does not have a backport label. Could you fix it @xjh1996? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

xjh1996 · 2022-01-03T03:03:44Z

/Team:Integrations

xjh1996 · 2022-01-03T03:07:16Z

/test

elasticmachine · 2022-01-10T09:24:28Z

Pinging @elastic/integrations (Team:Integrations)

ChrsMark

Hey @xjh1996, thank you very much for your contribution!

I have some concerns about the implementation but the direction is in general sth that we can adopt so let me provide some feedback and we can discuss on it and iterate on the PR:

+1 on using eve.Series.Count as fallback when eve.Count is not present.
I would suggest using eve.EventTime as fallback when FirstTimestamp is not present
+1 on using eve.Series.LastObservedTime as fallback when eve.LastTimestamp is not present.

Let me know what you think :).

ChrsMark · 2022-03-17T10:19:05Z

+	lastTimestamp := kubernetes.EventLastTimestamp(eve)
+	tsMap["first_occurrence"] = func() time.Time {
+		if eve.FirstTimestamp.IsZero() {
+			return kubernetes.Time(&lastTimestamp).UTC()


I'm not sure if this is correct. If an event has no FirstTimestamp attached then we fallback to lastTimestamp so this means that each time the event occurs we emit data with different first_occurrence which is always equal to the current lastTimestamp. Is this something that is intentional, and if so why?

Maybe it's better here to use EventTime as a fallback?

ChrsMark · 2022-03-17T10:24:39Z

+		} else if !eve.EventTime.IsZero() {
+			lastTimestamp = metav1.Time(eve.EventTime)
+		} else {
+			lastTimestamp = eve.CreationTimestamp


Does this field actually exist? I cannot find on the version of the client library we use (v1.21): https://github.com/kubernetes/api/blob/release-1.21/core/v1/types.go#L5463

ChrsMark · 2022-03-17T10:34:25Z

+		if eve.Series != nil && !eve.Series.LastObservedTime.IsZero() {
+			lastTimestamp = metav1.Time(eve.Series.LastObservedTime)
+		} else if !eve.EventTime.IsZero() {
+			lastTimestamp = metav1.Time(eve.EventTime)


Here we fallback to a value that is described as Time when this Event was first observed, is this something that is actually correct? EventTime is mostly a good fallback if FirstTimestamp is not present.

ChrsMark · 2022-04-04T11:50:55Z

@xjh1996 I'm going to close this PR for now and keep the discussion going at #31126. Let us know there your opinions and if you still want to contribute to this you can open the PR after we decide on the changes approach at #31126.

botelastic Bot added the needs_team Indicates that the issue/PR needs a Team:* label label Dec 23, 2021

mtojek self-requested a review December 23, 2021 09:25

mtojek reviewed Dec 23, 2021

View reviewed changes

fix: adapt kubernetes event

8d175b6

xjh1996 force-pushed the fix-kubernetes-event branch from b0309d6 to 8d175b6 Compare January 3, 2022 02:59

xjh1996 requested review from a team as code owners January 3, 2022 02:59

xjh1996 changed the base branch from 7.12 to master January 3, 2022 03:00

mergify Bot added the backport-skip Skip notification from the automated backport with mergify label Jan 3, 2022

ChrsMark added the Team:Integrations Label for the Integrations team label Jan 10, 2022

botelastic Bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 10, 2022

ChrsMark added the needs_reviewer PR needs to be assigned a reviewer label Jan 10, 2022

mergify Bot assigned xjh1996 Feb 8, 2022

ChrsMark self-assigned this Mar 16, 2022

ChrsMark reviewed Mar 17, 2022

View reviewed changes

gsantoro mentioned this pull request Apr 4, 2022

Handle K8s events with invalid count or invalid Timestamps #31126

Open

ChrsMark closed this Apr 4, 2022

Conversation

xjh1996 commented Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Uh oh!

cla-checker-service Bot commented Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 23, 2021 • edited by jenkins-beats-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❕ Build Aborted

Build stats

🤖 GitHub comments

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

xjh1996 commented Dec 23, 2021

Uh oh!

mtojek commented Dec 23, 2021

Uh oh!

xjh1996 commented Dec 23, 2021

Uh oh!

mergify Bot commented Jan 3, 2022

Uh oh!

xjh1996 commented Jan 3, 2022

Uh oh!

xjh1996 commented Jan 3, 2022

Uh oh!

elasticmachine commented Jan 10, 2022

Uh oh!

ChrsMark left a comment

Choose a reason for hiding this comment

Uh oh!

ChrsMark Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

ChrsMark Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

ChrsMark Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

ChrsMark commented Apr 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xjh1996 commented Dec 23, 2021 •

edited

Loading

cla-checker-service Bot commented Dec 23, 2021 •

edited

Loading

elasticmachine commented Dec 23, 2021 •

edited by jenkins-beats-ci Bot

Loading