feat: allow cpu limit to be removed

[jrRibeiro]

Currently, CPU limit can't be removed.

The getResources function, which creates the base for resources, sets the requests and limits for both memory and CPU.

If one defines the container's limits not to include a CPU limit, the reconciler merges the base resources with the overrides defined in the CR using a structural merge patch, which doesn't remove fields present in base but missing from the overrides. So, there's no way we can remove CPU limits.

Now, this should not be yet another discussion on whether we should or not specify CPU limits, but to allow those who need to remove CPU limits to do so.

Possible solutions:

• Remove the CPU limit from base: this is a breaking change, as users not defining the CPU limit would have to define it if needed. Also, having a CPU limit by default is a good thing if users don't want to explicitly remove it.

• Look for the grafana container defined in the CR and if the CR defines limits, but leaves out the CPU limit, don't add it to the base (ie. define the base with only a memory limit). This is not a breaking change because if users don't define limits, these will still be defined as they are now. The only change is, if the CR defines limits only containing a memory limit, then only the memory limit is set, not the CPU limit.

This is an implementation of the second solution, but I'm ok with implementing the first one.

[NissesSenap]

Hey @jrRibeiro, first, thanks for finding and fixing these kinds of things.
I'm leaning towards option 1.
Mostly due to that, I don't think you should use CPU limits at all in k8s, and I think this is a rather agreed on in the community.
You should only use cpu limits if you have very special needs, and I don't see how grafana got them.

I guess you could argue that it's a breaking change and I would agree but at the same time we are bringing better defaults.

Many of the maintainers are heading for PTO, so we won't have a meeting for some time so I can't give you a clear answer.
If you are up for it, please implement option 1 in a separate PR and we pick the one we agree on.

[jrRibeiro]

Hey @jrRibeiro, first, thanks for finding and fixing these kinds of things. I'm leaning towards option 1. Mostly due to that, I don't think you should use CPU limits at all in k8s, and I think this is a rather agreed on in the community. You should only use cpu limits if you have very special needs, and I don't see how grafana got them.

I guess you could argue that it's a breaking change and I would agree but at the same time we are bringing better defaults.

Many of the maintainers are heading for PTO, so we won't have a meeting for some time so I can't give you a clear answer. If you are up for it, please implement option 1 in a separate PR and we pick the one we agree on.

Hi @NissesSenap,

Thanks! Yes, I agree option 1 is a better and much simpler solution. I opened a PR for it: #1163

[NissesSenap]

We will go with #1163 instead