KEP-2371: clarify plan for cAdvisor and target at Beta for 1.24 #3003

haircommander · 2021-10-04T17:48:59Z

Signed-off-by: Peter Hunt [email protected]

One-line PR description:
addressing comments in Add KEP for cAdvisor-less, CRI-full Container and Pod Stats #2364 (comment)

Issue link:

Other comments:

endocrimes · 2021-10-06T15:04:52Z

keps/sig-node/2371-cri-pod-container-stats/README.md

@@ -225,7 +225,7 @@ As such, this proposal will be broken up into what needs to be done for each of
 2. Add CRI Pod Level Stats message to CRI protobuf that includes all [Pod Level Stats](#summary-pod-stats-object) metrics from Summary API.
 3. Add support for the new CRI additions in supported container runtimes (CRI-O and containerd).
 4. Switch Kubelet's CRI stats provider from querying container and pod level stats from cAdvisor to newly added CRI pod and container level stats
-5. cAdvisor should stop collecting container and pod level stats. If any other components need container or pod level stats from cAdvisor, the CRI implementation should be queried instead.
+5. cAdvisor should be updated to support no longer collecting stats that are duplicated with CRI implementation. Any client that requires the metrics that are reported by the CRI should gather them from the CRI instead of cAdvisor.


Doesn't this somewhat go against the eventual goal of removing /metrics/cadvisor/externalizing metrics, at least unless we add a step 6 equivalent to the existing step 5?

hm yeah we're going to have to figure out how to merge the two. I believe there are metrics that cadvisor provides that are out of scope for CRI that should be provided by cadvisor, but there are a set that should no longer be provided by cadvisor.

thoughts @bobbypage

I agree, my thinking is that the high level goal is to provide ability for clients to obtain metrics from CRI (i.e. summary api) rather than cAdvisor.

k8s-triage-robot · 2022-01-05T13:49:54Z

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

This bot triages issues and 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 issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

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

/lifecycle stale

Signed-off-by: Peter Hunt <[email protected]>

Some pieces that were initially slated for Alpha have been pushed to Beta Signed-off-by: Peter Hunt <[email protected]>

haircommander · 2022-01-25T21:46:03Z

I've taken over my own PR and updated it to cover its status movign to beta in 1.24

Signed-off-by: Peter Hunt <[email protected]>

haircommander · 2022-01-26T13:55:32Z

/cc @bobbypage

bobbypage · 2022-01-27T18:41:57Z

keps/sig-node/2371-cri-pod-container-stats/README.md

- CRI implementations should report any difficulties collecting the stats, and by Beta the CRI stats implementation should perform better than they did with CRI+cAdvisor.
- CRI implementations should support their equivalent of `/metrics/cadvisor`, passing the performance and conformance suite created in Alpha.
+- Conduct research to find the set of metrics from `/metrics/cadvisor` that compliant CRI implementations must expose.
+- Conformance tests for the fields in `/metrics/cadvisor` should be created.


Full k8s conformance tests will be significant effort, perhaps instead we should just ensure that the fields we want to support have overlap?

how will we continually verify that without testing? especially as time goes on. If we consider these fields a sort of API of kube, then we should probably do something to verify they're there

bobbypage · 2022-01-27T18:42:23Z

keps/sig-node/2371-cri-pod-container-stats/README.md

- CRI implementations should support their equivalent of `/metrics/cadvisor`, passing the performance and conformance suite created in Alpha.
+- Conduct research to find the set of metrics from `/metrics/cadvisor` that compliant CRI implementations must expose.
+- Conformance tests for the fields in `/metrics/cadvisor` should be created.
+- Performance tests for CPU/memory usage between Kubelet/cAdvisor/CRI implementation should be added.


I think k8s already has OSS perf tests, perhaps we should rephrase this to say "Validate performance impact of this feature"

bobbypage · 2022-01-27T18:42:53Z

keps/sig-node/2371-cri-pod-container-stats/README.md

-  Kubelet should fail early if the CRI implementation won't report the expected metrics.
- For Beta/GA releases, components that rely on `/metrics/cadvisor` should take the decided action (use `/stats/summary`, or use the Kubelet provided `/metrics/cadvisor`).
+  Kubelet should fallback to use cAdvisor if the CRI implementation won't report the expected metrics.
+- For Beta/GA releases, components that rely on `/metrics/cadvisor` should take the decided action (use `/stats/summary`, or use the CRI provided `/metrics/cadvisor`).


maybe rename, CRI provided -> CRI provided endpoint

bobbypage · 2022-01-27T18:43:22Z

keps/sig-node/2371-cri-pod-container-stats/README.md

@@ -651,12 +668,17 @@ _This section must be completed when targeting beta graduation to a release._
  checking if there are objects with field X set) may be a last resort. Avoid
  logs or events for this purpose.

+The source of `/metrics/cadvisor` is the CRI implementation, not cAdvisor. Further, if the CRI implementation was using the


maybe avoid providing name for endpoint directly...

The source of the "new" prometheus endpoint will be CRI ...

I reworeded a bit

bobbypage · 2022-01-27T18:44:28Z

keps/sig-node/2371-cri-pod-container-stats/README.md

@@ -669,6 +691,9 @@ the health of the service?**
    job creation time) for cron job <= 10%
  - 99,9% of /health requests per day finish with 200 code

+- 100% coverage of metrics reported by cAdvisor reported by CRI


I think some of the metrics we said may not be needed; so promising 100% coverage may be a bit too optimistic :)

alright I dropped it, though I think we may need to provide close to it..

haircommander · 2022-01-27T20:45:03Z

updated with the feedback!

bobbypage · 2022-01-28T03:21:27Z

Thank you for updating!

/lgtm

ehashman · 2022-01-31T21:27:02Z

/remove-lifecycle stale

keps/sig-node/2371-cri-pod-container-stats/README.md

sftim · 2022-01-31T22:51:49Z

/uncc

Signed-off-by: Peter Hunt <[email protected]>

ehashman

/approve
for PRR

mrunalp · 2022-02-02T21:18:22Z

/lgtm

derekwaynecarr

thanks for updates.

the <2% metric should be qualified a little further when we do testing for particular density of pods.

/approve
/lgtm

k8s-ci-robot · 2022-02-02T21:19:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, ehashman, haircommander

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~~ [ehashman]
~~keps/sig-node/OWNERS~~ [derekwaynecarr]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 4, 2021

haircommander mentioned this pull request Oct 4, 2021

Add KEP for cAdvisor-less, CRI-full Container and Pod Stats #2364

Merged

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 4, 2021

k8s-ci-robot requested review from dchen1107 and derekwaynecarr October 4, 2021 17:49

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. labels Oct 4, 2021

endocrimes reviewed Oct 6, 2021

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 5, 2022

haircommander added 2 commits January 25, 2022 16:32

2371: clarify plan for cAdvisor

3650547

Signed-off-by: Peter Hunt <[email protected]>

CRI stats: update based on work done during alpha

538d77d

Some pieces that were initially slated for Alpha have been pushed to Beta Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the stats-3 branch from 5ed8a28 to 1fee62c Compare January 25, 2022 21:45

haircommander changed the title ~~KEP-2371: clarify plan for cAdvisor~~ KEP-2371: clarify plan for cAdvisor and target at Beta for 1.24 Jan 25, 2022

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 25, 2022

haircommander mentioned this pull request Jan 25, 2022

cAdvisor-less, CRI-full Container and Pod Stats #2371

Open

17 tasks

haircommander force-pushed the stats-3 branch from 1fee62c to 36247ad Compare January 26, 2022 13:30

haircommander added 2 commits January 26, 2022 08:36

CRI stats: target beta at 1.24

f9b0442

Signed-off-by: Peter Hunt <[email protected]>

CRI stats: update history a bit

33d0afd

Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the stats-3 branch from 36247ad to 33d0afd Compare January 26, 2022 13:37

k8s-ci-robot requested a review from bobbypage January 26, 2022 13:55

bobbypage reviewed Jan 27, 2022

View reviewed changes

haircommander force-pushed the stats-3 branch from 011b3da to 2ae4a99 Compare January 27, 2022 20:44

k8s-ci-robot assigned bobbypage Jan 28, 2022

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

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 31, 2022

ehashman reviewed Jan 31, 2022

View reviewed changes

k8s-ci-robot requested a review from sftim January 31, 2022 21:31

k8s-ci-robot removed the request for review from sftim January 31, 2022 22:51

CRI stats: update PRR section for beta

ac9c25d

Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the stats-3 branch from 2ae4a99 to ac9c25d Compare February 1, 2022 16:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2022

ehashman reviewed Feb 1, 2022

View reviewed changes

k8s-ci-robot assigned mrunalp Feb 2, 2022

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

derekwaynecarr approved these changes Feb 2, 2022

View reviewed changes

k8s-ci-robot assigned derekwaynecarr Feb 2, 2022

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

k8s-ci-robot merged commit 4530b43 into kubernetes:master Feb 2, 2022

k8s-ci-robot added this to the v1.24 milestone Feb 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-2371: clarify plan for cAdvisor and target at Beta for 1.24 #3003

KEP-2371: clarify plan for cAdvisor and target at Beta for 1.24 #3003

haircommander commented Oct 4, 2021

endocrimes Oct 6, 2021

haircommander Oct 7, 2021

haircommander Oct 7, 2021

bobbypage Jan 27, 2022

k8s-triage-robot commented Jan 5, 2022

haircommander commented Jan 25, 2022

haircommander commented Jan 26, 2022

bobbypage Jan 27, 2022

haircommander Jan 27, 2022

bobbypage Jan 27, 2022

haircommander Jan 27, 2022

bobbypage Jan 27, 2022

haircommander Jan 27, 2022

bobbypage Jan 27, 2022 •

edited

Loading

haircommander Jan 27, 2022

bobbypage Jan 27, 2022 •

edited

Loading

haircommander Jan 27, 2022

haircommander commented Jan 27, 2022

bobbypage commented Jan 28, 2022

ehashman commented Jan 31, 2022

sftim commented Jan 31, 2022

ehashman left a comment

mrunalp commented Feb 2, 2022

derekwaynecarr left a comment

k8s-ci-robot commented Feb 2, 2022

KEP-2371: clarify plan for cAdvisor and target at Beta for 1.24 #3003

KEP-2371: clarify plan for cAdvisor and target at Beta for 1.24 #3003

Conversation

haircommander commented Oct 4, 2021

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

k8s-triage-robot commented Jan 5, 2022

haircommander commented Jan 25, 2022

haircommander commented Jan 26, 2022

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

bobbypage Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbypage Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented Jan 27, 2022

bobbypage commented Jan 28, 2022

ehashman commented Jan 31, 2022

sftim commented Jan 31, 2022

ehashman left a comment

Choose a reason for hiding this comment

mrunalp commented Feb 2, 2022

derekwaynecarr left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 2, 2022

bobbypage Jan 27, 2022 •

edited

Loading

bobbypage Jan 27, 2022 •

edited

Loading