Parallelise smaps #2715
Parallelise smaps #2715
Conversation
|
Hi @wacuuu. Thanks for your PR. I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
wacuuu
force-pushed
the
jwalecki/parallel_smaps_2
branch
from
32a1e4b to
f51dd03
Compare
|
/ok-to-test |
docs/runtime_options.md
Outdated
| --pid=host \ | ||
| --privileged \ | ||
| --name=cadvisor \ | ||
| cadvisor:$CADVISOR_TAG |
There was a problem hiding this comment.
According to docs/deploy.md it shouldn't be google/cadvisor:$CADVISOR_TAG?
docs/runtime_options.md
Outdated
| --privileged \ | ||
| --name=cadvisor \ | ||
| cadvisor:$CADVISOR_TAG | ||
| --disable_metrics="cpu_topology,resctrl,udp,sched,hugetlb,node_vmstat,memory_numa,tcp,advtcp,percpu,process" \ |
There was a problem hiding this comment.
Got: invalid value "cpu_topology,resctrl,udp,sched,hugetlb,node_vmstat,memory_numa,tcp,advtcp,percpu,process" for flag -disable_metrics: unsupported metric "node_vmstat" specified in disable_metrics
docs/runtime_options.md
Outdated
| - `--referenced_read_interval` duration Read interval for referenced bytes (container_referenced_bytes metric), number of seconds after which referenced bytes are read, if set to 0 referenced bytes are never read (default: 0s) | ||
| - `--referenced_reset_interval` duration Reset interval for referenced bytes (container_referenced_bytes metric), number of seconds after which referenced bytes are cleared, if set to 0 referenced bytes are never cleared (default: 0s) | ||
|
|
||
| The referenced memory value is based on one of two files in `/sys/<PID>`: smaps or smaps_rollup. If the latter exists it reports the same value as the first one, but in aggregated form. This is only implementation flavoring and there is no difference whether smaps_rollup exists or not. |
There was a problem hiding this comment.
You mean /proc/<PID> instead of /sys/<PID> ?
| if err != nil { | ||
| klog.V(4).Infof("Could not get PIDs for container %d: %v", h.pid, err) | ||
| } else { | ||
| stats.ReferencedMemory, err = referencedBytesStat(pids, h.cycles, *referencedResetInterval) | ||
| stats.ReferencedMemory = h.referencedMemory * 1024 |
There was a problem hiding this comment.
Can you add a comment about converting ReferencedMemory to bytes?
| smapsFilePath := fmt.Sprintf(smapsFilePathPattern, pid) | ||
| smapsFilePath = fmt.Sprintf(smapsRollupFilePattern, pid) | ||
| if _, err := os.Stat(smapsFilePath); err == nil { | ||
| klog.V(6).Infof("Using smaps_rollup for pid %d instead of smaps", pid) |
There was a problem hiding this comment.
Can you provide a reason why this verbosity level is set to 6?
There was a problem hiding this comment.
Because this is an information useful in process of debugging and understanding the code, yet it would trash the logs if reported on lower level
| return | ||
| } | ||
| castResetInterval := *referencedResetInterval | ||
| time.Sleep(time.Duration(rand.Intn(int(castResetInterval.Seconds())))) |
There was a problem hiding this comment.
Could you explain this sleep?
| return | ||
| } | ||
| castReadInterval := *referencedReadInterval | ||
| time.Sleep(time.Duration(rand.Intn(int(castReadInterval.Seconds())))) |
There was a problem hiding this comment.
Could you explain this sleep?
There was a problem hiding this comment.
Both of those sleeps are responsible for preventing cpu usage spikes. Imagine a situation that cadvisor is started on a machine populated by a lot of containers, or a situation in which the containers are created 'in a batch' that is a lot of containers in a short period of time. In any of those cases, with every container creation a new handler starts, and so does the thread responsible for reading smaps file(and if requestested, another one for reseting it).
As the interval is common for all the threads(it comes from parameter) now you are in situation, where all of those reading threads call their respective smaps files at almost the same moment. As smaps is not a regular file but an interface to a kernel function, this will trigger kernel execution and eat up cpu. Have a lot of not-so-multiprocess containers and you'll find yourself having cpu usage spikes every referenced_read_interval
Signed-off-by: Jakub Walecki <[email protected]>
Signed-off-by: Jakub Walecki <[email protected]>
Signed-off-by: Jakub Walecki <[email protected]>
wacuuu
force-pushed
the
jwalecki/parallel_smaps_2
branch
from
f51dd03 to
1625fe6
Compare
|
Feel free to reopen this PR if you think it's important. |
In relation to #2622 I introduced a change that should lower the cost of gathering referenced bytes information. As said in the issue, using this feature with big containers in current implementation will stop cAdvisor from refreshing metrics, as it waits for referenced bytes to be read, To mitigate the problem, I implemented reading referenced bytes independently from main execution, as go routines. Moreover, to avoid peaks in CPU usage by cAdvisor, reading is distributed over the operation interval.
Also it is important to mention, that for sake of precise estimation of wss it is good to reset independently from reading, hence there is a new parameter.
Signed-off-by: Jakub Walecki [email protected]