Recommendations missing for some sidecars

[Pionerd]

Describe the bug
We do not get a recommendation for some containers if there are multiple of them in a pod, e.g. the Dapr sidecar. For other pods with multiple containers, e.g. the prometheus pod with the thanos-sidecar and config-reloader sidecars, we do get recommendations. The missing sidecars are not mentioned in any logs.

Expected behavior
Recommendations for all containers in a pod or at least a mention the there is no / not enough data to make a recommendation for that specific container.

[aantn]

Hi, do you see anything when running krr w/ --verbose

Any pattern to the sidecars? E.g. init containers vs regular contianers.

[Pionerd]

Hi @aantn, --verbose also does not give a mention of the missing containers.

They are regular containers in the sense that they are mentioned under containers in the spec of the pod. However, as they are 'injected' by Dapr (https://docs.dapr.io/concepts/dapr-services/sidecar/#kubernetes-with-dapr-sidecar-injector) they are not mentioned in the applicable deployment spec.

[aantn]

Got it. So they are present in kubectl get pod <name> -o yaml but not in kubectl get deployment <name> -o yaml.
Is that correct?

[Pionerd]

Yes, correct.

[aantn]

Does it work on #266 if you choose the Prometheus loader? That will load info about workloads from Prometheus where the data on actual containers in the pod should be present.

[Pionerd]

python krr.py simple -p http://localhost:9090 -m prometheus where localhost exposes the Prometheus service does indeed work!

Of course, we would prefer to have recommendations being based on the longer term Thanos data, but it looks like we can give the Thanos URL als Prometheus URL, is that correct?

[aantn]

Yes, exactly! Can you try and confirm that it works?

[Pionerd]

My first few tests gave identical recommendations using both Thanos and Prometheus, but I found another cluster where Prometheus didn't contain enough data to give recommendations but Thanos did :) So yes, it works. Looking forward to seeing this merged and available via brew.

[aantn]

Wonderful, thank you. To help get this merged, are you able to run a 3 way test:

1. Results on master (NOT the new branch) - this will obviously exclude the extra containers but I'd like to check if the other data matches up
2. Results on the new branch but WITHOUT the prometheus discovery flag. Just the standard API server discovery. Again, this will exclude the extra containers but we want to confirm that it is identical (or similar to) (1)
3. Results on the new branch with the extra prometheus discovery flag

We're doing testing to confirm that there aren't unexpected regressions in the new branch which includes some very large changes to the codebase. So getting another confirmation on this from real world data would be extremely valuable.

[Pionerd]

When performing the test, I actually observed that the current data without prometheus discovery is wrong for the pods that contain the multiple containers as discussed in this issue. This goes for both the master and the new branch:

According to the report, the both current request and memory limit for the main container are set at 1024Mi, while actually the request is 256 and the limit 512 (missing sidecar container 128/256). This error happens for multiple (but strangely enough not all) containers with this Dapr sidecar pattern. This makes this issue slightly more pressing than that there are just some containers missing.

Next to this, the 'discovery' option give me a lot of recommendations for resources that were not present in both non-discovery tests, but that is probably the point of this whole exercise :)

Apart from that, the recommendations given by all three are equal.

[aantn]

Thanks. Can you share the lines from the reports which are wrong? Sounds like the prom discovery is working but we should also get that bug fixed for the api server method. (We likely will continue to exclude injected containers from the report, but the data on other containers should be correct.)

…

On Wed, May 15, 2024, 19:19 Pieter ***@***.***> wrote: When performing the test, I actually observed that the current data without prometheus discovery is wrong for the pods that contain the multiple containers as discussed in this issue. This goes for both the master and the new branch: According to the report, the both current request and memory limit for the main container are set at 1024Mi, while actually the request is 256 and the limit 512 (missing sidecar container 128/256). This error happens for multiple (but strangely enough not all) containers with this Dapr sidecar pattern. This makes this issue slightly more pressing than that there are just some containers missing. Next to this, the 'discovery' option give me a lot of recommendations for resources that were not present in both non-discovery tests, but that is probably the point of this whole exercise :) Apart from that, the recommendations given by all three are equal. — Reply to this email directly, view it on GitHub <#276 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADYUB3QSYQ7GJNWLFRKJU3ZCODJNAVCNFSM6AAAAABHMWYZ4OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJSHE3DMMZZHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>