KEP-2371: update kep to reflect current state of enhancement #2812
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -183,9 +183,9 @@ We want to avoid using cAdvisor for container & pod level stats and move metric | |
| * cAdvisor and metric dependency: CRI mission is not fully fulfilled - container runtime is not fully plugable. | ||
| * Break the monolithic design of cAdvisor, which needs to be aware of the underlying container runtime. | ||
| * Duplicate stats are collected by both cAdvisor and the CRI runtime, which can lead to: | ||
| * Different information from different sources | ||
| * Confusion from unclear origin of a given metric | ||
| * Performance degradations (increased CPU / Memory / etc) [xref][perf-issue] | ||
| * Stats should be reported by the container runtime which knows behavior of the container/pod the best. | ||
| * cAdvisor only supports runtimes that run processes on the host, not e.g. VM based runtime like Kata Containers. | ||
| * cAdvisor only supports linux containers, not Windows ones. | ||
|
|
@@ -304,7 +304,8 @@ These correspond to some fields of the [ContainerStats](#summary-container-stats | |
| + // Corresponds to Stats Summary API CPUStats UsageCoreNanoSeconds | ||
| = UInt64Value usage_core_nano_seconds = 2; | ||
| + // Total CPU usage (sum of all cores) averaged over the sample window. | ||
| + // The "core" unit can be interpreted as CPU core-nanoseconds per second. | ||
| + UInt64Value usage_nano_cores = 3; | ||
| =} | ||
|
|
||
| =// MemoryUsage provides the memory usage information. | ||
|
|
@@ -315,74 +316,19 @@ These correspond to some fields of the [ContainerStats](#summary-container-stats | |
| = // The amount of working set memory in bytes. | ||
| + // Corresponds to Stats Summary API MemoryStats WorkingSetBytes field | ||
| = UInt64Value working_set_bytes = 2; | ||
| + // Available memory for use. This is defined as the memory limit - workingSetBytes. | ||
| + UInt64Value available_bytes = 3; | ||
| + // Total memory in use. This includes all memory regardless of when it was accessed. | ||
| + UInt64Value usage_bytes = 4; | ||
| + // The amount of anonymous and swap cache memory (includes transparent hugepages). | ||
| + UInt64Value rss_bytes = 5; | ||
| + // Cumulative number of minor page faults. | ||
| + UInt64Value page_faults = 6; | ||
| + // Cumulative number of major page faults. | ||
|
Comment on lines
-353
to
-361
There was a problem hiding this comment. these are whole disk metrics, not specific to the writable layer or image fs size. they can be populated by cadvsior who is responsible for monitoring node-level information (whole disk) |
||
| + UInt64Value major_page_faults = 7; | ||
| =} | ||
| ``` | ||
|
|
||
|
There was a problem hiding this comment. logs are the responsibility of the kubelet to watch and collect information for. |
||
|
Comment on lines
-367
to
-384
There was a problem hiding this comment. no changes needed here, just the extension of MemoryUsage and CpuUsage (covered above) |
||
| Notes: | ||
| - In Stats Summary API ContainerStats object, there's a timestamp field. We do not need such a field, as each struct in the ContainerStats object | ||
| has its own timestamp, allowing CRI implementations flexibility when they collect which metrics. | ||
|
|
@@ -402,49 +348,45 @@ They will be defined as follows: | |
| // Runtime service defines the public APIs for remote pod runtimes | ||
| service RuntimeService { | ||
| ... | ||
| // PodSandboxStats returns stats of the pod. If the pod sandbox does not | ||
| // exist, the call returns an error. | ||
| rpc PodSandboxStats(PodSandboxStatsRequest) returns (PodSandboxStatsResponse) {} | ||
| // ListPodSandboxStats returns stats of the pods matching a filter. | ||
| rpc ListPodSandboxStats(ListPodSandboxStatsRequest) returns (ListPodSandboxStatsResponse) {} | ||
| ... | ||
| } | ||
| ... | ||
| message PodSandboxStatsRequest { | ||
| // ID of the pod sandbox for which to retrieve stats. | ||
| string pod_sandbox_id = 1; | ||
| } | ||
|
|
||
| message PodSandboxStatsResponse { | ||
| PodSandboxStats stats = 1; | ||
| } | ||
|
|
||
| // PodSandboxStatsFilter is used to filter the list of pod sandboxes to retrieve stats for. | ||
| // All those fields are combined with 'AND'. | ||
| message PodSandboxStatsFilter { | ||
| // ID of the pod sandbox. | ||
| string id = 1; | ||
| // LabelSelector to select matches. | ||
| // Only api.MatchLabels is supported for now and the requirements | ||
| // are ANDed. MatchExpressions is not supported yet. | ||
| map<string, string> label_selector = 2; | ||
| } | ||
|
|
||
| message ListPodSandboxStatsRequest { | ||
| // Filter for the list request. | ||
| PodSandboxStatsFilter filter = 1; | ||
| } | ||
|
|
||
| message ListPodSandboxStatsResponse { | ||
| // Stats of the pod sandbox. | ||
| repeated PodSandboxStats stats = 1; | ||
| } | ||
|
|
||
| // PodSandboxAttributes provides basic information of the pod sandbox. | ||
| message PodSandboxAttributes { | ||
| // ID of the pod. | ||
| string id = 1; | ||
|
|
@@ -460,58 +402,65 @@ message PodSandboxAttributes { | |
| } | ||
|
|
||
| // PodSandboxStats provides the resource usage statistics for a pod. | ||
| // The linux or windows field will be populated depending on the platform. | ||
| message PodSandboxStats { | ||
| // Information of the pod. | ||
| PodSandboxAttributes attributes = 1; | ||
| // Stats from linux. | ||
| LinuxPodSandboxStats linux = 2; | ||
| // Stats from windows. | ||
| WindowsPodSandboxStats windows = 3; | ||
|
Comment on lines
+409
to
+412
There was a problem hiding this comment. added two sections for windows and linux as discussed but not added There was a problem hiding this comment. just leaving note: This message design differs a bit from existing I prefer to have it this way though since it has more flexibility of allowing to extend stats for windows/linux separately. |
||
| } | ||
|
|
||
| // LinuxPodSandboxStats provides the resource usage statistics for a pod sandbox on linux. | ||
| message LinuxPodSandboxStats { | ||
| // CPU usage gathered for the pod sandbox. | ||
| CpuUsage cpu = 1; | ||
| // Memory usage gathered for the pod sandbox. | ||
| MemoryUsage memory = 2; | ||
| // Network usage gathered for the pod sandbox | ||
| NetworkUsage network = 3; | ||
|
There was a problem hiding this comment. memory and cpu have the suffix Usage, make this more consistent |
||
| // Stats pertaining to processes in the pod sandbox. | ||
| ProcessUsage process = 4; | ||
| // Stats of containers in the measured pod sandbox. | ||
| repeated ContainerStats containers = 5; | ||
| } | ||
|
|
||
| // WindowsPodSandboxStats provides the resource usage statistics for a pod sandbox on windows | ||
| message WindowsPodSandboxStats { | ||
| // TODO: Add stats relevant to windows. | ||
| } | ||
|
|
||
| // NetworkUsage contains data about network resources. | ||
| message NetworkUsage { | ||
|
There was a problem hiding this comment. Should this be named There was a problem hiding this comment. it is mirroring the Cpu and Memory structs: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto#L611-L621 |
||
| // The time at which these stats were updated. | ||
| int64 timestamp = 1; | ||
| // Stats for the default network interface. | ||
| NetworkInterfaceUsage default_interface = 2; | ||
|
There was a problem hiding this comment. |
||
| // Stats for all found network interfaces, excluding the default. | ||
| repeated NetworkInterfaceUsage interfaces = 3; | ||
| } | ||
|
|
||
| // NetworkInterfaceUsage contains resource value data about a network interface. | ||
| message NetworkInterfaceUsage { | ||
|
There was a problem hiding this comment. How about There was a problem hiding this comment. same as above :) |
||
| // The name of the network interface. | ||
| string name = 1; | ||
| // Cumulative count of bytes received. | ||
| UInt64Value rx_bytes = 2; | ||
| // Cumulative count of receive errors encountered. | ||
| UInt64Value rx_errors = 3; | ||
| // Cumulative count of bytes transmitted. | ||
| UInt64Value tx_bytes = 4; | ||
| // Cumulative count of transmit errors encountered. | ||
| UInt64Value tx_errors = 5; | ||
| } | ||
|
|
||
| // ProcessUsage are stats pertaining to processes. | ||
| message ProcessUsage { | ||
| // The time at which these stats were updated. | ||
| int64 timestamp = 1; | ||
| // Number of processes. | ||
| UInt64Value process_count = 2; | ||
| } | ||
| ``` | ||
|
|
||
|
|
@@ -561,18 +510,18 @@ The table above describes the various metrics that are in this endpoint. | |
| Each compliant CRI implementation must: | ||
| - Have a location broadcasted about where these metrics can be gathered from. The endpoint name must not necessarily be `/metrics/cadvisor`, nor be gathererd from the same port as it was from cAdvisor | ||
| - Implement *all* metrics within the set of metrics that are decided on. | ||
| - **TODO** How will we decide this set? We could support all, or take polls from the community and come up with a set of sufficiently useful metrics. | ||
| - Pass a set of tests in the critest suite that verify they report the correct values for *all* supported metrics labels (to ensure continued conformance and standardization). | ||
|
|
||
| Below is the proposed strategy for doing so: | ||
|
|
||
| 1. The Alpha release will strictly cover research, performance testing and the creation of conformance tests. | ||
| - Initial research on the set of metrics required should be done. This will, possibly, allow the community to declare metrics that are not required to be moved to the CRI implementations. | ||
| - Testing on how performant cAdvisor+Kubelet are today should be done, to find a target, acceptable threshold of performance for the CRI implementations | ||
| - Creation of tests verifying the metrics are reported correctly should be created and verified with the existing cAdvisor implementation. | ||
| 2. For the Beta release, add initial support for CRI implementations to report these metrics | ||
| - This set of metrics will be based on the research done in alpha | ||
| - Each will be validated against the conformance and performance tests created in alpha. | ||
| 3. For the GA release, the CRI implementation should be the source of truth for all pod and container level metrics that external parties rely on (no matter how many endpoints the Kubelet advertises). | ||
|
|
||
| #### cAdvisor | ||
|
|
@@ -618,7 +567,7 @@ As a requirement for the Beta stage, cAdvisor must support optionally collecting | |
| ### Version Skew Strategy | ||
|
|
||
| - Breaking changes between versions will be mitigated by the FeatureGate. | ||
| - By the time the FeatureGate is deprecated, it is expected the transition between CRI and cAdvisor is complete, and CRI has had at least one release to expose the required metrics (to allow for `n-1` CRI skew). | ||
| - In general, CRI should be updated in tandem with or before the Kubelet. | ||
|
|
||
| ## Production Readiness Review Questionnaire | ||
|
|
@@ -775,13 +724,13 @@ operations covered by [existing SLIs/SLOs]?** | |
| Think about adding additional work or introducing new steps in between | ||
| (e.g. need to do X to start a container), etc. Please describe the details. | ||
| - The process of collecting and reporting the metrics should not differ too much between cAdvisor and the CRI implementation: | ||
| - At a high level, both need to watch the changes to the stats (from cgroups, disk and network stats) | ||
| - Once collected, the CRI implementation will need to report them (both through the CRI and eventually through the prometheus endpoint). | ||
| - Both of these steps are already done by cAdvisor, so the work is changing hands, but not fundamentally changing. | ||
| - It is possible the Alpha iteration of this KEP may affect CPU/memory usage on the node: | ||
| - This may come because cAdvisor's performance has been fine-tuned, and changing the location of work may loose some optimizations. | ||
| - However, it is explicitly stated that a requirement for transition from Alpha->Beta is little to no performance degradation. | ||
| - The existence of the feature gate will allow users to mitigate this potential blip in performance (by not opting-in). | ||
| * **Will enabling / using this feature result in non-negligible increase of | ||
| resource usage (CPU, RAM, disk, IO, ...) in any components?** | ||
| - It most likely will reduce resource utilization. Right now, there is duplicate work being done between CRI and cAdvisor. | ||
|
|
@@ -818,6 +767,6 @@ Note: This is by design as this will enable to decouple runtime implementation d | |
| ## Alternatives | ||
|
|
||
| - Instead of teaching CRI how to do *everything* cAdvisor does, we could instead have cAdvisor not do the work the CRI stats end up doing (specifically when reporting disk stats, which are the most expensive operation to report). | ||
| - However, this doesn't address the anti-pattern of having multiple parties confusingly responsible for a wide array of metrics and other issues described. | ||
| - Have cAdvisor implement the summary API. A cAdvisor daemonset could be a drop-in replacement for the summary API. | ||
| - Don't keep supporting the summary API. Replace it with a "better" format, like prometheus. Or help users migrate to equivalent APIs that container runtimes already expose for monitoring. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
this is the actual variable name
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.
and matches summary API naming:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubelet/pkg/apis/stats/v1alpha1/types.go#L205-L208
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.
The "core" unit can be interpreted as CPU core-nanoseconds per secondmaybe add ^ that sentence...