KEP-2831: adding beta graduation criteria #3714

sallyom · 2023-01-09T14:49:12Z

updates to KEP-2831 to track connected traces in kubelet and beta graduation
Issue link: Kubelet OpenTelemetry Tracing #2831

/cc @dashpole
/cc @saschagrunert
/cc @vrutkovs

k8s-ci-robot · 2023-01-09T14:49:17Z

@sallyom: GitHub didn't allow me to request PR reviews from the following users: vrutkovs.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

updates to KEP-2831 to track connected traces in kubelet and beta graduation

Issue link: Kubelet OpenTelemetry Tracing #2831

/cc @dashpole
/cc @saschagrunert
/cc @vrutkovs

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.

dashpole

/approve

(i'll leave lgtm to others involved, but this lgtm)

keps/sig-instrumentation/2831-kubelet-tracing/README.md

keps/prod-readiness/sig-instrumentation/2831.yaml

saschagrunert · 2023-01-23T11:39:49Z

@sallyom wanna give this one another update? :)

saschagrunert

/lgtm

wojtek-t · 2023-01-30T11:57:41Z

In my queue - just waiting for PRR shadow reviewer to take a look first :)

jeremyrickard · 2023-02-02T04:41:18Z

👋 I'm shadowing PRR review this time around and gave this a review. It doesn't look like this PR has updated the PRR questionare (which was pretty complete for the alpha stage). Since this is proposing to graduate to beta, have you given any additional thought to the metrics that might inform a rollback or monitoring requirements? With a bad TracingConfiguration, I expect users will things in the kubelet logs to help correct that configuration, but are there any other changes expected as part of this that might introduce better ways to monitor/troubleshoot this feature or understand if they are achieving the SLO (99% of spans delivered)? Not sure if this something that can be done (maybe ref: open-telemetry/opentelemetry-go#2547) , but I think it would be useful to add some clarity around this with an update.

Was also wondering if you added unit tests for the feature gate when this was implemented in alpha? If so, could you link those in an update to the PRR questionnaire?

keps/sig-instrumentation/2831-kubelet-tracing/README.md

wojtek-t · 2023-02-02T08:54:00Z

keps/sig-instrumentation/2831-kubelet-tracing/README.md

 - A test with kubelet-tracing & apiserver-tracing enabled to ensure no issues are introduced, regardless
 of whether a tracing backend is configured.

 ### Graduation Requirements

 Alpha

- [] Implement tracing of incoming and outgoing gRPC, HTTP requests in the kubelet
- [] Integration testing of tracing
+- [X] Implement tracing of incoming and outgoing gRPC, HTTP requests in the kubelet


Please also take a look a @jeremyrickard comment here:
#3714

Filling in the remaining part of monitoring sections is the main callout there (I would have the same) and it will be blocking PRR approval.

I added references to the opentelemetry-go issue and also added kubelet metrics that would signal issues

logicalhan · 2023-02-02T17:37:55Z

/assign @ehashman

logicalhan · 2023-02-02T17:38:17Z

/assign

sallyom · 2023-02-03T17:18:17Z

@jeremyrickard @wojtek-t thanks for your review & apologies for not getting to it right away - I've updated based on your feedback, thank you.

ehashman

This generally LGTM.

/approve
for SIG Instrumentation, waiting on PRR confirm for final lgtm

ehashman · 2023-02-03T22:48:49Z

keps/sig-instrumentation/2831-kubelet-tracing/kep.yaml

 approvers:
  - "@dashpole"
-  - "@ehashman"


We still need someone from instrumentation to approve this. Since @dashpole is a coauthor, please leave me in as an instrumentation (rather than PRR) approver.

will do, thank you

added, done

wojtek-t

@sallyom Few more small comments from me.

wojtek-t · 2023-02-06T11:00:18Z

keps/sig-instrumentation/2831-kubelet-tracing/README.md

- [] Integration testing of tracing
+- [X] Implement tracing of incoming and outgoing gRPC, HTTP requests in the kubelet
+- [X] Integration testing of tracing
+  - _component-base tracing/api/v1 integration test_ https://github.com/kubernetes/kubernetes/blob/master/test/integration/apiserver/tracing/tracing_test.go


This should be moved to the Testing section above (as part of integration).

moved to testing section

wojtek-t · 2023-02-06T11:03:27Z

keps/sig-instrumentation/2831-kubelet-tracing/README.md

@@ -328,7 +305,17 @@ _This section must be completed when targeting beta graduation to a release._
  No impact to running workloads, logs will indicate the problem.

 ###### What specific metrics should inform a rollback?
-  To be determined.
+


L296: Have those test been added?

FWIW - given that this isn't a persistent thing, rather in-memory switch, I would say that explicit enablement/disablement tests aren't really needed, as they won't provide visible value on top of the actual feature tests.

there are no tests for enablement/disablement - i'll add a note there

wojtek-t · 2023-02-06T11:05:23Z

keps/sig-instrumentation/2831-kubelet-tracing/README.md

+  ```
+
+  * Pod Lifecycle and Kubelet [SLOs](https://github.com/kubernetes/community/tree/master/sig-scalability/slos) are the signals that should guide a rollback.  In particular, the [`kubelet_pod_start_duration_seconds_count`, `kubelet_runtime_operations_errors_total`, and `kubelet_pleg_relist_interval_seconds_bucket`] would surface issues affecting kubelet performance.
+


L321: can you please ensure that those will happen (and you update the KEP to describe the scenarios tested and finding - please cc me on it) before the FG will be graduated to Beta ?

L346: I'm assuming this will be the OTLP metric that doesn't yet, exist, right? Or sth else?

(was)L321: I added under Graduation Requirements for Beta L245
(was)L346: yes, the OTLP metric tbd is what I am referring to & noted that L349

Signed-off-by: Sally O'Malley <somalley@redhat.com>

Co-authored-by: Sascha Grunert <sgrunert@redhat.com>

Signed-off-by: Sally O'Malley <somalley@redhat.com>

sallyom · 2023-02-09T01:31:49Z

@wojtek-t @ehashman updated based on the latest review/feedback - thank you again

wojtek-t · 2023-02-09T07:58:40Z

I'm fine with it from PRR perspective. You still need SIG-level approval though.

/approve PRR

k8s-ci-robot · 2023-02-09T07:58:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, ehashman, sallyom, saschagrunert, wojtek-t

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:

~~keps/prod-readiness/OWNERS~~ [wojtek-t]
~~keps/sig-instrumentation/OWNERS~~ [dashpole,ehashman]

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

wojtek-t · 2023-02-09T08:00:28Z

Actually - I see Elana already approved it above. So lgtm-ing to ensure it will make the release.

/lgtm

k8s-ci-robot requested review from dashpole and saschagrunert January 9, 2023 14:49

sallyom mentioned this pull request Jan 9, 2023

Kubelet OpenTelemetry Tracing #2831

Open

12 tasks

sallyom force-pushed the kep2831-update-1.27 branch 2 times, most recently from bfaa6dc to dd17862 Compare January 9, 2023 16:05

dashpole approved these changes Jan 9, 2023

View reviewed changes

saschagrunert reviewed Jan 10, 2023

View reviewed changes

sallyom force-pushed the kep2831-update-1.27 branch from 43cecb8 to da911e1 Compare January 23, 2023 14:57

wojtek-t self-assigned this Jan 24, 2023

saschagrunert approved these changes Jan 24, 2023

View reviewed changes

k8s-ci-robot assigned saschagrunert Jan 24, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 24, 2023

sallyom force-pushed the kep2831-update-1.27 branch from da911e1 to e5cd9df Compare January 26, 2023 16:49

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 26, 2023

wojtek-t reviewed Feb 2, 2023

View reviewed changes

k8s-ci-robot assigned ehashman Feb 2, 2023

k8s-ci-robot assigned logicalhan Feb 2, 2023

sallyom force-pushed the kep2831-update-1.27 branch from e5cd9df to 993ac97 Compare February 3, 2023 17:11

ehashman reviewed Feb 3, 2023

View reviewed changes

wojtek-t reviewed Feb 6, 2023

View reviewed changes

Atharva-Shinde mentioned this pull request Feb 8, 2023

VolumeGroupSnapshot #3476

Open

4 tasks

sallyom and others added 7 commits February 8, 2023 20:01

update KEP-2831

bbe94e3

Signed-off-by: Sally O'Malley <somalley@redhat.com>

capitalization fix

14d65cf

Signed-off-by: Sally O'Malley <somalley@redhat.com>

Update keps/sig-instrumentation/2831-kubelet-tracing/README.md

33a5b67

Co-authored-by: Sascha Grunert <sgrunert@redhat.com>

update KEP-2831

30e1830

Signed-off-by: Sally O'Malley <somalley@redhat.com>

update approver & connected spans section

2489afe

Signed-off-by: Sally O'Malley <somalley@redhat.com>

add links to tests and rollback signals

d1a701c

Signed-off-by: Sally O'Malley <somalley@redhat.com>

update rollback testing and add reviewers

a9c36f7

Signed-off-by: Sally O'Malley <somalley@redhat.com>

sallyom force-pushed the kep2831-update-1.27 branch from 993ac97 to a9c36f7 Compare February 9, 2023 01:29

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2023

k8s-ci-robot merged commit 33f7b95 into kubernetes:master Feb 9, 2023

k8s-ci-robot added this to the v1.27 milestone Feb 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2831: adding beta graduation criteria #3714

KEP-2831: adding beta graduation criteria #3714

sallyom commented Jan 9, 2023

k8s-ci-robot commented Jan 9, 2023

dashpole left a comment •

edited

saschagrunert commented Jan 23, 2023

saschagrunert left a comment

wojtek-t commented Jan 30, 2023

jeremyrickard commented Feb 2, 2023 •

edited

wojtek-t Feb 2, 2023

sallyom Feb 3, 2023

logicalhan commented Feb 2, 2023

logicalhan commented Feb 2, 2023

sallyom commented Feb 3, 2023

ehashman left a comment

ehashman Feb 3, 2023

sallyom Feb 9, 2023

sallyom Feb 9, 2023

wojtek-t left a comment

wojtek-t Feb 6, 2023

sallyom Feb 9, 2023

wojtek-t Feb 6, 2023

sallyom Feb 9, 2023

wojtek-t Feb 6, 2023

sallyom Feb 9, 2023

sallyom commented Feb 9, 2023

wojtek-t commented Feb 9, 2023

k8s-ci-robot commented Feb 9, 2023

wojtek-t commented Feb 9, 2023

		```

		* Pod Lifecycle and Kubelet [SLOs](https://github.com/kubernetes/community/tree/master/sig-scalability/slos) are the signals that should guide a rollback. In particular, the [`kubelet_pod_start_duration_seconds_count`, `kubelet_runtime_operations_errors_total`, and `kubelet_pleg_relist_interval_seconds_bucket`] would surface issues affecting kubelet performance.

KEP-2831: adding beta graduation criteria #3714

KEP-2831: adding beta graduation criteria #3714

Conversation

sallyom commented Jan 9, 2023

k8s-ci-robot commented Jan 9, 2023

dashpole left a comment • edited

Choose a reason for hiding this comment

saschagrunert commented Jan 23, 2023

saschagrunert left a comment

Choose a reason for hiding this comment

wojtek-t commented Jan 30, 2023

jeremyrickard commented Feb 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logicalhan commented Feb 2, 2023

logicalhan commented Feb 2, 2023

sallyom commented Feb 3, 2023

ehashman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sallyom commented Feb 9, 2023

wojtek-t commented Feb 9, 2023

k8s-ci-robot commented Feb 9, 2023

wojtek-t commented Feb 9, 2023

dashpole left a comment •

edited

jeremyrickard commented Feb 2, 2023 •

edited