-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Ignore any .mount cgroup in docker #1573
Ignore any .mount cgroup in docker #1573
Conversation
} | ||
for _, test := range tests { | ||
if actual := isContainerName(test.name); actual != test.expected { | ||
t.Fatalf("%s: expected: %v, actual: %v", test.name, test.expected, actual) |
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.
minor nit: use t.Errorf
to go through all test cases.
@@ -142,6 +142,10 @@ func ContainerNameToDockerId(name string) string { | |||
} | |||
|
|||
func isContainerName(name string) bool { | |||
// always ignore .mount cgroup even if associated with docker and delegate to systemd |
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.
cadvisor will log Error trying to work out if we can handle containerName: error
, which seems misleading. How about changing this so that CanHandleAndAccept
return (true, false, nil)
or (false, false, nil)` instead?
I also think the caller of CanHandleAndAccept
logs way too many errors periodically...but that's another issue.
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.
Just checked and systemd/factory.go decides this is "can handle but not accept", which produces equal amount of noise though
0435634
to
14fdb89
Compare
14fdb89
to
4ad5c58
Compare
@yujuhong -- updated pr so for .mount cgroup, docker will not handle or accept. in addition, just updated the handler to no longer return an error when a container is not a valid docker name since it caused excessive noise. at log-level 5, you now see:
this should be good to go, PTAL |
Jenkins GCE e2e failed for commit 14fdb89. Full PR test history. cc @derekwaynecarr The magic incantation to run this job again is 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. I understand the commands that are listed here. |
@k8s-bot test this |
LGTM |
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.
LGTM
merging... |
@derekwaynecarr just curious (I'm hitting this), will this get to a 1.5.x or 1.6 kubernetes release? (as far as I know, cadvisor is included in the kubelet, so there is no easy way to fix this). IIUC, k8s 1.5 and 1.6 use these cadvisor versions: https://github.com/kubernetes/kubernetes/blob/release-1.5/Godeps/Godeps.json#L1141 and 1.6 https://github.com/kubernetes/kubernetes/blob/release-1.6/Godeps/Godeps.json#L1239 But 1.6 "revision" in json, Any change to fix it in 1.5 too? Thanks! :) |
Automatic merge from submit-queue [Release 1.5] Update cadvisor godeps to v0.25.0 This PR updates the cAdvisor Godeps for the 1.5 branch. This includes a number of critical bugfixes including: [cadvisor#1558](google/cadvisor#1558), [cadvisor#1573](google/cadvisor#1573) This is a large change on account of the aws dependency update. However, many affected users have requested that this be cherrypicked into 1.5 and 1.4 (coming soon) ```release-note - Disable thin_ls due to excessive iops - Ignore .mount cgroups, fixing dissappearing stats - Fix wc goroutine leak - Update aws-sdk-go dependency to 1.6.10 ``` cc @calebamiles [remember this?](#40095 (comment)) @jessfraz @timstclair
This is in 1.5.6 |
@dashpole thanks! |
Fixes #1572
Fixes #1510
/cc @timstclair @ncdc @smarterclayton