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.

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

Emit an event when the result of a probe for a container changes #115963

Conversation

intUnderflow
Copy link
Member

@intUnderflow intUnderflow commented Feb 22, 2023

/kind feature
/kind api-change
/sig node
/hold

What this PR does / why we need it:

This PR introduces a new event that is emitted by the probe worker when the result of the probe changes. End-users can watch this new event to learn when their probes entered the Sucess or Failure state and in the case of Failure they gain some contextual information in the form of the output from the last probe.

Fixes #115823 by dealing with the lack of information on when a probe fails (as users can now just look at this event)

Does this PR introduce a user-facing change?

A new event is emitted on containers when their readiness, liveness or startup probes change result from Success to Failure or vice-versa. In the case of Failure information on why the probe failed may be included.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 22, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @intUnderflow. 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-ci-robot k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet labels Feb 22, 2023
@intUnderflow
Copy link
Member Author

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 22, 2023
@RobertKielty
Copy link
Member

/ok-to-test

@k8s-ci-robot 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 Feb 22, 2023
@intUnderflow
Copy link
Member Author

Had some thoughts:

  • If this does require some work on the API serialisation side its probably best to break that up into another PR anyway
  • This is still delivering something useful, the events are now annotated with belowFailureThreshold

Given that I think this is ready to go
/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2023
@dgrisonnet
Copy link
Member

/cc

@k8s-triage-robot
Copy link

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.

@intUnderflow
Copy link
Member Author

/retest

@dgrisonnet
Copy link
Member

dgrisonnet commented Feb 22, 2023

As far as I can tell this will not work as you expect. I believe that what you expect to see is the Event going from below threshold to above threshold after some failures right?

If that is the case, this is not what you will see on the apiserver side because of how we aggregate Events in the client. When merging similar Events together the client disregards the annotations and it is completely intended because annotations are not part of what we considered to be the structure of an Event. What will happen in practice is that the client will merge the existing emissions of below threshold Events for your Pod with the newer above threshold Event and emit a below threshold Event instead of an above threshold one.

That said, have you perhaps considered emitting a new Event instead of reusing an existing one?

@intUnderflow
Copy link
Member Author

intUnderflow commented Feb 22, 2023

That said, have you perhaps considered emitting a new Event instead of reusing an existing one?

I have very little context in this area, but if you have any ideas as to how we consider the design around new events / etc I can start to put together a KEP and refactor this PR around that?

@SergeyKanzhelev
Copy link
Member

when I looked at it I imagined two events - one for failed probe under the threshold and another - after. Those should be different events so the first will not make the second to be throttled.

As for tracking specific failure trends - it is indeed the best done via metrics.

@intUnderflow
Copy link
Member Author

gentle ping @mrunalp @derekwaynecarr @dchen1107 @klueska

@intUnderflow
Copy link
Member Author

intUnderflow commented Apr 28, 2023

@SergeyKanzhelev propose to move to priority/important-longterm, are you happy with this given you initially triaged the underlying issue?

Basing off https://www.kubernetes.dev/docs/guide/issue-triage/

@intUnderflow
Copy link
Member Author

/priority important-longterm

@k8s-ci-robot k8s-ci-robot added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label May 1, 2023
@intUnderflow
Copy link
Member Author

/remove-priority backlog

@k8s-ci-robot k8s-ci-robot removed the priority/backlog Higher priority than priority/awaiting-more-evidence. label May 1, 2023
@intUnderflow intUnderflow force-pushed the annotate-container-events-emitted-by-probes-with-failure-thresholds branch from d94a7fb to 4aa51f5 Compare May 2, 2023 17:11
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2023
@intUnderflow
Copy link
Member Author

(merge conflicts)

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a29a3294e7e615092f744c3d79c4faaef410f1cc

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: intUnderflow, SergeyKanzhelev
Once this PR has been reviewed and has the lgtm label, please ask for approval from dchen1107. For more information see the Kubernetes Code Review Process.

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

@intUnderflow
Copy link
Member Author

/retest

@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2023
@pacoxu pacoxu moved this from Needs Approver to Waiting on Author in SIG Node PR Triage Sep 7, 2023
@dims dims added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Oct 24, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

SIG Node PR Triage automation moved this from Waiting on Author to Done Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project