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

Apply pod name and namespace labels for pod cgroup for cadvisor metrics #63406

Merged
merged 1 commit into from May 10, 2018

Conversation

@derekwaynecarr
Member

derekwaynecarr commented May 3, 2018

What this PR does / why we need it:

  1. Enable Prometheus users to determine usage by pod name and namespace for pod cgroup sandbox.
  2. Label cAdvisor metrics for pod cgroups by pod name and namespace.
  3. Aligns with kubelet stats summary endpoint pod cpu and memory stats.

Special notes for your reviewer:
This provides parity with the summary API enhancements done here:
#55969

Release note:

Apply pod name and namespace labels to pod cgroup in cAdvisor metrics
@derekwaynecarr

This comment has been minimized.

Member

derekwaynecarr commented May 3, 2018

/hold

want to collect feedback, but i think this is useful to understand via prometheus usage of pod sandbox by pod name and namespace.

/cc @sjenning @smarterclayton @dashpole @brancz @DirectXMan12

@derekwaynecarr

This comment has been minimized.

Member

derekwaynecarr commented May 4, 2018

ok, this should be ready for review.

@@ -184,6 +184,27 @@ func (m *podContainerManagerImpl) ReduceCPULimits(podCgroup CgroupName) error {
return m.cgroupManager.ReduceCPULimits(podCgroup)
}
// IsPodCgroup returns true if the cgroup name corresponds to a pod
func (m *podContainerManagerImpl) IsPodCgroup(podCgroup CgroupName) (bool, types.UID) {

This comment has been minimized.

@dashpole

dashpole May 4, 2018

Contributor

I would love to see a unit test for this

@dashpole

This comment has been minimized.

Contributor

dashpole commented May 4, 2018

I like this change. I need to take a closer look, but it would be nice to make this and #55969 use the same logic for identifying pods so we dont get inconsistent results or bugs in one, but not the other.

@dims

This comment has been minimized.

Member

dims commented May 4, 2018

/test pull-kubernetes-local-e2e-containerized

metrics.LabelID: c.Name,
metrics.LabelName: name,
metrics.LabelImage: image,
"pod_name": podName,

This comment has been minimized.

@brancz

brancz May 6, 2018

Member

I've considered this a couple of times, but wasn't sure if it would be too breaking: technically this is the only place where the instrumentation guidelines are broken, these should be "pod", "namespace" and "container". There is actually common friction that this is not the case, but I as I said I wonder if it's too breaking and since we want to remove cAdvisor whether it's worth doing at this point. cc @DirectXMan12

This comment has been minimized.

@derekwaynecarr

derekwaynecarr May 7, 2018

Member

this just keeps the existing naming extended to pod cgroups. We can change or discuss label names in a follow-on

This comment has been minimized.

@DirectXMan12

DirectXMan12 May 9, 2018

Contributor

yeah, I'd like to change it at some point, but let's keep it as is in this PR.

This comment has been minimized.

@brancz

brancz May 9, 2018

Member

sounds reasonable to me

@derekwaynecarr

This comment has been minimized.

Member

derekwaynecarr commented May 7, 2018

@dashpole - added unit test. i am not seeing any room for common logic with #55969 given that this integrates with the label function logic in cAdvisor, and the later has container info it knows is associated with a pod and transforms to stats api. if i am missing something, let me know, but i think this should be all set. thanks for the first review!

@dashpole

This comment has been minimized.

Contributor

dashpole commented May 7, 2018

@derekwaynecarr agreed, #55969 takes the approach of iterating over all pods and doing Pod API Object -> Pod Cgroup Name -> Query Pod Cgroup Name in cAdvisor. This does the reverse, as we iterate over all cgroups, and determine if each one belongs to a pod by extracting the UID and querying for it in our known pods. So we can't share any of the logic.

@dashpole

one nit, otherwise LGTM. Waiting to add lgtm for question about prometheus labels to be resolved.

cgroupName := m.cgroupManager.CgroupName(cgroupfs)
qosContainersList := [3]CgroupName{m.qosContainersInfo.BestEffort, m.qosContainersInfo.Burstable, m.qosContainersInfo.Guaranteed}
basePath := ""
for _, qosContainersList := range qosContainersList {

This comment has been minimized.

@dashpole

dashpole May 7, 2018

Contributor

nit: rename either of the qosContainersList variables to avoid confusion.

@derekwaynecarr

This comment has been minimized.

Member

derekwaynecarr commented May 7, 2018

@dashpole updated, thx.

@DirectXMan12

This comment has been minimized.

Contributor

DirectXMan12 commented May 7, 2018

Let me respond to that one when I'm not still in post-KubeCon haze (hopefully tomorrow -- I think I caught some con crud over the weekend).

@derekwaynecarr

This comment has been minimized.

Member

derekwaynecarr commented May 7, 2018

/test pull-kubernetes-integration

@sjenning

This comment has been minimized.

Contributor

sjenning commented May 8, 2018

/retest

@DirectXMan12

couple nits inline, otherwise looks good to me

metrics.LabelID: c.Name,
metrics.LabelName: name,
metrics.LabelImage: image,
"pod_name": podName,

This comment has been minimized.

@DirectXMan12

DirectXMan12 May 9, 2018

Contributor

yeah, I'd like to change it at some point, but let's keep it as is in this PR.

func (m *podContainerManagerImpl) IsPodCgroup(cgroupfs string) (bool, types.UID) {
// convert the literal cgroupfs form to the driver specific value
cgroupName := m.cgroupManager.CgroupName(cgroupfs)
qosContainersList := [3]CgroupName{m.qosContainersInfo.BestEffort, m.qosContainersInfo.Burstable, m.qosContainersInfo.Guaranteed}

This comment has been minimized.

@DirectXMan12

DirectXMan12 May 9, 2018

Contributor

Is this list used elsewhere? If so, can we move it up to a constant so we have a single source of truth?

if !strings.HasPrefix(basePath, podCgroupNamePrefix) {
return false, types.UID("")
}
parts := strings.Split(basePath, podCgroupNamePrefix)

This comment has been minimized.

@DirectXMan12

DirectXMan12 May 9, 2018

Contributor

this looks weird. if it's just a prefix, basePath[len(podCgroupNamePrefix):] is clearer, IMO

@sjenning

This comment has been minimized.

Contributor

sjenning commented May 10, 2018

I'm going to go ahead and
/lgtm
nits can be fixed in follow on since there doesn't be any fundamental for functional objection
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm label May 10, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented May 10, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, sjenning

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:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented May 10, 2018

Automatic merge from submit-queue (batch tested with PRs 60200, 63623, 63406). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 321201f into kubernetes:master May 10, 2018

16 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation derekwaynecarr authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@serathius

This comment has been minimized.

Contributor

serathius commented May 14, 2018

Looks like this PR is cause of failing tests https://k8s-testgrid.appspot.com/sig-instrumentation#gce-prometheus
It introduced additional timeseries per pod causing cpu queries return doubled result.
Was this an intended behaviour? should we introduce breaking change in queries and require users to fix all their dashboard by adding additional filter (e. g. image!="" or pattern match on id)

@brancz

This comment has been minimized.

Member

brancz commented May 14, 2018

I'm not sure we are ready to provide any compatibility guarantees around metrics at this point, especially with cadvisor being an external project. In the past we have broken metrics several times simply because cadvisor had changes. I think we can only confidently provide stability around metrics with the new node-metrics proposal that's currently being discussed in sig-instrumentation.

@serathius

This comment has been minimized.

Contributor

serathius commented May 14, 2018

thanks for explaining.
I need to catch up with proposals

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment