Conversation
7be21b5 to
2f4d18c
Compare
You make a great point. If you look at a standard kube-state-metrics deployment, you are right that the restartPolicy is not included in the labels for kube_pod_init_container_resource_requests. Instead, it is exposed on the kube_pod_init_container_info metric. This is documented in the kube-state-metrics pod workload metrics. Because of this, we can actually separate sidecars from ordinary init containers at the metric level by joining these two metrics. The PromQL query would look something like this: |
|
Thanks @subpathdev, that's a useful correction. I only checked the labels on I'd still keep it out of this PR. This one is Happy to open a follow-up with your query as the starting point if that suits. |
…ulations Signed-off-by: somaz <genius5711@gmail.com>
2f4d18c to
f36d04a
Compare
Description
TransformPodSpeconly copiesspec.containersinto the cluster cache, so native sidecars (init containers withrestartPolicy: Always, such as an injectedistio-proxy) never reach the cost model and are costed at zero.InitContainerdoes not appear anywhere in the codebase today.This adds init containers with
restartPolicy: Alwaysto the cached container list, and deliberately leaves ordinary init containers out.That split is the one Kubernetes itself makes. In
k8s.io/component-helpers/resource.PodRequests, a restartable init container's requests are added to the pod's cumulative requests exactly like a regular container, while an ordinary init container only reaches the total through amax(). See KEP-753. Including every init container would over-charge any pod that runs a plain one for the pod's whole lifetime; including none under-charges every pod with a sidecar.One change at the cache level covers all three readers of
pod.Spec.Containers:costmodel.ComputeCostData, the request figures behind allocationcostmodel.NewContainerMetricsFromPod, the per-container allocation keysmetrics.KubePodCollector, thekube_pod_container_resource_requestsseries OpenCost emits itself. That emitter is on by default (EmitKsmV1Metricsdefaults totrue), so the Prometheus-sourced path picks the sidecars up too.Sidecars are appended after the regular containers so that index 0 stays a regular container, which
ComputeCostDatarelies on when it assigns PV claims to the first container only.Not covered here: deployments that switch OpenCost's own emitter off and scrape a real kube-state-metrics instead. There a sidecar arrives as
kube_pod_init_container_resource_requests, which carries norestartPolicylabel, so it cannot be separated from an ordinary init container at the metric level. That needs its own design, so I left it out rather than guess.Related Issues
Fixes #3956
User Impact
Pods that run native sidecars will report higher, and now correct, costs. The size of the change depends on the sidecar: for a pod whose sidecar requests are comparable to its app container, the reported pod cost roughly doubles.
Anyone who has been reading OpenCost numbers for a mesh-injected workload has been seeing the app container only, so this will look like a cost increase in dashboards even though nothing about the workload changed. Pods without native sidecars are unaffected, and pods with ordinary init containers are unaffected.
No configuration change, no API change, no new dependency.
Testing
Added
core/pkg/clustercache/clustercache_test.go:TestTransformPodSpecIncludesNativeSidecars, table driven over five cases: regular containers only, ordinary init container excluded, native sidecar included and ordered after the regular containers,restartPolicy: OnFailurenot treated as a sidecar, and a mix of ordinary init containers and two sidecars.TestTransformPodSpecKeepsSidecarRequests, so a sidecar cannot be carried into the cache and then still costed at zero.Both were confirmed to fail without the change (removing the
restartPolicybranch makes them fail, restoring it makes them pass).justis not installed here, so I ran the justfile recipes by hand as CONTRIBUTING suggests:go test ./...plusgo vet ./...incore,modules/prometheus-source,modules/collector-sourceand the root module. 88 packages pass, vet is clean everywhere.golangci-lint run ./pkg/clustercache/...reports 0 issues.One note:
format-checkcurrently fails ondevelopformodules/pricing/public/httpclient/httpclient.go, which is unrelated to this PR and already unformatted upstream (same area as #3942).gofmt -lis clean for both files touched here, and I left that file alone.