wait to be able to compute the resource usage of all the containers of a pod before exposing its PodMetrics.

[yangjunmyfm192085]

Signed-off-by: JunYang [email protected]

What this PR does / why we need it:
There are many ci failuers recently
Based on previous research, Within two cycles of resource reporting, a container of the pod reported duplicate data, we skip the containers for which we can't compute the resource usage instead of skipping the whole PodMetrics.
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[yangjunmyfm192085]

/cc @serathius @dgrisonnet

[yangjunmyfm192085]

/hold
Let me do more tests

[yangjunmyfm192085]

Through more tests, I found when we get PodMetricses, we can get only information of one container.

For example:
&PodMetrics{ObjectMeta:{sidecarpod-consumer default 0 2021-08-05 17:06:01 +0800 CST <nil> <nil> map[] map[] [] [] []},Timestamp:2021-08-05 17:05:53 +0800 CST,Window:{10.393s},Containers:[]ContainerMetrics{ContainerMetrics{Name:sidecarpod-consumer,Usage:k8s_io_api_core_v1.ResourceList{cpu: {{49530274 -9} {<nil>} 49530274n DecimalSI},memory: {{8704000 0} {<nil>} 8500Ki BinarySI},},},},}

Now I fixed up, we need check the length of ms.Containers here if (err == nil && len(ms.Containers) == 2) || time.Now().After(deadline) {

the correct result, for example:
&PodMetrics{ObjectMeta:{sidecarpod-consumer default 0 2021-08-05 17:09:57 +0800 CST <nil> <nil> map[] map[] [] [] []},Timestamp:2021-08-05 17:09:50 +0800 CST,Window:{25.523s},Containers:[]ContainerMetrics{ContainerMetrics{Name:sidecar-container,Usage:k8s_io_api_core_v1.ResourceList{cpu: {{49919787 -9} {<nil>} 49919787n DecimalSI},memory: {{8429568 0} {<nil>} BinarySI},},},ContainerMetrics{Name:sidecarpod-consumer,Usage:k8s_io_api_core_v1.ResourceList{cpu: {{49619887 -9} {<nil>} 49619887n DecimalSI},memory: {{9089024 0} {<nil>} 8876Ki BinarySI},},},},}

[yangjunmyfm192085]

/unhold

[dgrisonnet]

I wonder if we should really check for len(ms.Containers) == 2 in the tests, in my opinion, MS should always return complete PodMetrics containing metrics about all the containers of a pod. The fact that we only get metrics from one container out of the two from the pods sounds like an issue to me.
In your recent changes, you added a check to only return PodMetrics whenever we have metrics for all the containers of the pods: https://github.com/kubernetes-sigs/metrics-server/blob/master/pkg/scraper/client/resource/decode.go#L148-L151. So it means that kubelet is only exposing metrics about one container of the Pod. At the very least, I would expect to have metrics about both containers initialized and exposed by kubelet.

[dgrisonnet]

this shouldn't be needed since the test is only running for 60 seconds: https://github.com/kubernetes-sigs/metrics-server/blob/master/test/e2e_test.go#L173

[dgrisonnet]

ditto

[yangjunmyfm192085]

I wonder if we should really check for len(ms.Containers) == 2 in the tests, in my opinion, MS should always return complete PodMetrics containing metrics about all the containers of a pod. The fact that we only get metrics from one container out of the two from the pods sounds like an issue to me.
In your recent changes, you added a check to only return PodMetrics whenever we have metrics for all the containers of the pods: https://github.com/kubernetes-sigs/metrics-server/blob/master/pkg/scraper/client/resource/decode.go#L148-L151. So it means that kubelet is only exposing metrics about one container of the Pod. At the very least, I would expect to have metrics about both containers initialized and exposed by kubelet.

This seems to be a bug, before storing podmetrcis, we should check the number of containers from both PodMetrics and apiserver.
I will do some research recently and Let us modify it

[yangjunmyfm192085]

I wonder if we should really check for len(ms.Containers) == 2 in the tests, in my opinion, MS should always return complete PodMetrics containing metrics about all the containers of a pod. The fact that we only get metrics from one container out of the two from the pods sounds like an issue to me.
In your recent changes, you added a check to only return PodMetrics whenever we have metrics for all the containers of the pods: https://github.com/kubernetes-sigs/metrics-server/blob/master/pkg/scraper/client/resource/decode.go#L148-L151. So it means that kubelet is only exposing metrics about one container of the Pod. At the very least, I would expect to have metrics about both containers initialized and exposed by kubelet.

This seems to be a bug, before storing podmetrcis, we should check the number of containers from both PodMetrics and apiserver.
I will do some research recently and Let us modify it

I have researched this issue recently, the result:

• Within two cycles of resource reporting, a container of the pod reported duplicate data
• This causes only one cycle of data to be retained for this container and both cycles of data to be retained for another container
• so we only get metrics from one container out of the two from the pod at the begining
• this seems not a issue of metrics-server

[yangjunmyfm192085]

/cc @serathius @dgrisonnet ,Do we need to deal with this scenario specially?

[dgrisonnet]

Great investigation @yangjunmyfm192085! I think you found a bug in metrics-server. IMO we should handle this scenario since decoding metrics batch and getting metrics from the storage have different behaviors.
When decoding, we are preventing MS from storing PodMetrics until we can get the metrics for all the containers of the pod, whereas when we get the metrics from the storage, we skip the containers for which we can't compute the resource usage instead of skipping the whole PodMetrics:

metrics-server/pkg/storage/pod.go

Lines 66 to 69 in d4def03

	prevContainer, found := prevPod.Containers[container]
	if !found {
	continue
	}

I think we should update this part of the code and wait to be able to compute the resource usage of all the containers of a pod before exposing its PodMetrics.

[yangjunmyfm192085]

Great investigation @yangjunmyfm192085! I think you found a bug in metrics-server. IMO we should handle this scenario since decoding metrics batch and getting metrics from the storage have different behaviors.
When decoding, we are preventing MS from storing PodMetrics until we can get the metrics for all the containers of the pod, whereas when we get the metrics from the storage, we skip the containers for which we can't compute the resource usage instead of skipping the whole PodMetrics:

metrics-server/pkg/storage/pod.go

Lines 66 to 69 in d4def03

	prevContainer, found := prevPod.Containers[container]
	if !found {
	continue
	}

I think we should update this part of the code and wait to be able to compute the resource usage of all the containers of a pod before exposing its PodMetrics.

Yeah, you are right. Let me update this part of the code

[yangjunmyfm192085]

/retitle wait to be able to compute the resource usage of all the containers of a pod before exposing its PodMetrics.

[dgrisonnet]

Looks good from my side, but do we really need the changes made to test/e2e_test.go?

[yangjunmyfm192085]

Looks good from my side, but do we really need the changes made to test/e2e_test.go?

I am not sure if it nessary to set ResourceCPU 100m and ResourceMemory 100Mi for each container, so I optimized it, but it is also ok to make no changes.

[dgrisonnet]

/lgtm

[yangjunmyfm192085]

/retest

[yangjunmyfm192085]

/retest

[yangjunmyfm192085]

/cc @dgrisonnet pull-metrics-server-test-e2e-ha is failed.

no matches for kind \"PodDisruptionBudget\" in version \"policy/v1\"\n" , Use kindest/node:v1.20.7 does not match?

[dgrisonnet]

Indeed, it seems that I missed a couple of errors with Kuberneetes v1.19 and v1,20, I'll have a look.

[dgrisonnet]

/lgtm

[dgrisonnet]

/retest

[yangjunmyfm192085]

I think we should merge this pr now.
/assign @serathius
/cc @serathius @dgrisonnet

[serathius]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, serathius, yangjunmyfm192085

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:

• OWNERS [serathius]

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