[bugfix]: Fix null pointer de-reference when parsing statistics from cAdvisor #83
Conversation
enp0s3
force-pushed
the
npd-fix-cadvisor-stat
branch
from
8f4e6a0 to
9f5ed67
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
enp0s3
force-pushed
the
npd-fix-cadvisor-stat
branch
from
9f5ed67 to
29c9986
Compare
there might be a situation where pod ranking is done on a freshly created pod whose containers stats are not yet ready in cadvisor. Signed-off-by: Igor Bezukh <[email protected]>
also fixed a bug in swap stats where swap usage stat was assigned with swap available bytes. Signed-off-by: Igor Bezukh <[email protected]>
enp0s3
force-pushed
the
npd-fix-cadvisor-stat
branch
from
29c9986 to
2de2b11
Compare
|
/cc @Barakmor1 |
| workingSet := *containerStats.MemoryWorkingSetBytes | ||
| swapUsage := *containerStats.MemorySwapCurrentBytes |
| workingSet := *containerStats.MemoryWorkingSetBytes | ||
| swapUsage := *containerStats.MemorySwapCurrentBytes | ||
| memoryAndSwapSumQuantity := memoryAndSwapUsage(workingSet, swapUsage) |
There was a problem hiding this comment.
Hey, what do you think about also checking if these are nil? Maybe we can log a message in that case for better debuggability.
| @@ -138,7 +138,7 @@ func (psc *PodStatsCollectorImpl) GetPodSummary(pod *v1.Pod) (PodSummary, error) | |||
| summary.Containers[containerName] = ContainerSummary{ | |||
| MemorySwapMaxBytes: cinfo.Spec.Memory.SwapLimit, | |||
| MemoryWorkingSetBytes: containerStats.Memory.WorkingSetBytes, | |||
| MemorySwapCurrentBytes: containerStats.Swap.SwapAvailableBytes, | |||
| MemorySwapCurrentBytes: containerStats.Swap.SwapUsageBytes, | |||
| UsageBytes: uint64Ptr(0), | ||
| WorkingSetBytes: uint64Ptr(0), | ||
| RSSBytes: uint64Ptr(0), |
There was a problem hiding this comment.
I think we never use UsageBytes and RSSBytes, i think we can delete them instead WDYT?
| SwapUsageBytes: uint64Ptr(0), | ||
| Time: metav1.NewTime(cstat.Timestamp), | ||
| SwapUsageBytes: uint64Ptr(0), | ||
| SwapAvailableBytes: uint64Ptr(0), |
There was a problem hiding this comment.
I guess we never use SwapAvailableBytes anymore, i think we can delete it instead WDYT?
Signed-off-by: Igor Bezukh [email protected]
Scenario 1: it can happen that API reports about a Pod, but the pod and its containers are not yet
fully created on the node cgroupfs, therefore there can be containers that are missing in the stats.
Scenario 2: Stats API has pointers to counters. Not all counters had default allocation of zero integer.