Add cgroup_memory_migrate metric #2796
Conversation
|
Hi @kwisniewski98. 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. |
|
/ok-to-test |
info/v1/container.go
Outdated
| ContainerData MemoryStatsMemoryData `json:"container_data,omitempty"` | ||
| HierarchicalData MemoryStatsMemoryData `json:"hierarchical_data,omitempty"` | ||
| } | ||
|
|
||
| type CPUSetStats struct { | ||
| Migrate uint64 `json:"memory_migrate"` |
There was a problem hiding this comment.
Could you use MemoryMigrate?
kwisniewski98
force-pushed
the
cgroup_memory_migrate
branch
from
92444de to
fa4a90a
Compare
info/v1/container.go
Outdated
| @@ -395,10 +395,16 @@ type MemoryStats struct { | |||
|
|
|||
| Failcnt uint64 `json:"failcnt"` | |||
|
|
|||
| CpuSet CPUSetStats `json:"cpuset,omitempty"` | |||
There was a problem hiding this comment.
curious to get your thoughts: does this make sense under MemoryStats, maybe it needs a top level field?
Since this is taken from the cpuset controller, maybe this makes sense to add a new CpuSets top level field under ContainerStats which will contain the relevant metrics from cpuset: https://man7.org/linux/man-pages/man7/cpuset.7.html
As I understand currently all the fields under MemoryStats actually come from the memory cgroup controller stats, and adding this field will now result some fields from MemoryStats coming from memory cgroup controller and others from cpuset controller.
Thoughts @kwisniewski98 @Creatone @iwankgb ?
There was a problem hiding this comment.
That will be a good place for other CPUSet metrics.
kwisniewski98
force-pushed
the
cgroup_memory_migrate
branch
from
fa4a90a to
bc8b918
Compare
metrics/prometheus.go
Outdated
| @@ -423,6 +423,14 @@ func NewPrometheusCollector(i infoProvider, f ContainerLabelsFunc, includedMetri | |||
| return metricValues{{value: float64(s.Memory.WorkingSet), timestamp: s.Timestamp}} | |||
| }, | |||
| }, | |||
| { | |||
| name: "container_memory_migrate", | |||
There was a problem hiding this comment.
this is still under if includedMetrics.Has(container.MemoryUsageMetrics).
I think since we're introducing new CPUSet category of metrics, makes sense to add a new metric kind for CpuSet metrics instead of putting this under existing container memory usage metrics.
kwisniewski98
force-pushed
the
cgroup_memory_migrate
branch
from
bc8b918 to
fd3c939
Compare
Signed-off-by: Wisniewski, Krzysztof2 <[email protected]>
kwisniewski98
force-pushed
the
cgroup_memory_migrate
branch
from
fd3c939 to
7d914b5
Compare
|
Thanks! LGTM |
Signed-off-by: Wisniewski, Krzysztof2 [email protected]