search

eclipse-tractusx / portal

Portal - Helm charts
Apache License 2.0
7 stars 12 forks source link

TRG 5.04 CPU / MEM resource limits not set #207

Closed FaGru3n closed 6 months ago

FaGru3n commented 7 months ago

Limits are not set within values.yaml for the portal chart https://eclipse-tractusx.github.io/docs/release/trg-5/trg-5-04

https://github.com/eclipse-tractusx/portal/blob/b1b2336f5a37af2dd0f86bf71300b2842905edaf/charts/portal/values.yaml#L326-L328

evegufy commented 7 months ago

@FaGru3n I'm really not convinced about setting limits in the default values.yaml, this should a conscious choice.

FaGru3n commented 7 months ago

Hmm just following our own rules.

And sure in case of memory ressource i agree a bit, because of MEM Limit

Memory is not a 'stretchable' resource and kubernetes will delete a pod if it starts using more memory than expected.

but on CPU level your chart will consume more CPU even when it is available.

To have the CPU Limit higher than the request allows the pod to use more CPU when available and utilizes a node resources better

And as far as i know it is a good practice to have ressource limits to our pods https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#requests-and-limits

SebastianBezold commented 7 months ago

@FaGru3n I'm really not convinced about setting limits in the default values.yaml, this should a conscious choice.

Hi @evegufy, we set this TRG in place, because we as teams potentially have the best insights in memory and CPU consumption based on our tests. That's why defining it in default values will give other users a glimpse in what to expect. Of course everyone else can overwrite it to match their needs. Setting defaults won't break anything.

Especially the consortia infra tooling should provide good insights historical values that we could use here

evegufy commented 7 months ago

@FaGru3n @SebastianBezold I'll look into it again for the next release. I did do some research on this before, in the course of that I even deployed goldilocks tools on the dev and int cluster (together with @almadigabor) to get some data.

SebastianBezold commented 7 months ago

Hi @evegufy,

does that mean, you won't do that for this release? I don't see any reason not doing that. As I said, it does not break anything, since it can always be overwritten for each deployment. This is definitely non-compliant with our release guidelines

evegufy commented 7 months ago

Hi @SebastianBezold, I won't do it for this release as I want to properly test it. Even if this change might not be breaking, I also don't see any reason why to force it into the release without proper testing. Please mark is as a finding to be addressed with the next release.

evegufy commented 7 months ago

Clarification: CPU and memory requests are set. CPU and memory limits are configured but commented out in the values.yaml but can be set at all times when using the chart. Setting the limits in the values.yaml will be addressed in the next release.

FaGru3n commented 6 months ago

Hi @evegufy,

thanks for info should we leave this issue open or will this taken over for the next release (24.05)?

Thanks in advance.

evegufy commented 6 months ago

Hi @evegufy,

thanks for info should we leave this issue open or will this taken over for the next release (24.05)?

Thanks in advance.

@FaGru3n it's part of the next release (24.05)