[opentelemetry-operator] remove default limits for cpu and memory

[Pionerd]

Setting default limits prevents users to use this chart as a dependency and leave the limits empty (which is desirable in the case of cpu limits). Removing the defaults gives more flexibility.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Pionerd / name: Pieter (b27e3f1, 4caeaf5, 039df6a, 5dfc22d)

[TylerHelmuth]

@Pionerd if you're using the latest version of helm you should be able to remove the cpu resource using null.

opentelemetry-operator:
  manager:
    resources:
      limits:
        cpu: null

[Pionerd]

@TylerHelmuth great suggestion that I was not yet aware of, however I don't get it working on latest Helm v3.13.2.

values.yaml

opentelemetry-operator:
  manager:
    resources:
      limits:
        cpu: null

templated deployment:

[...]
          resources:
            limits:
              cpu: 100m
              memory: 128Mi
            requests:
              cpu: 100m
              memory: 64Mi
[...]

I understand that this is not the helm support forum, but curious how others look at this.

[Pionerd]

I found the issue (we were having multiple layers of dependencies where the limits.cpu: null apparently got lost, but the schema of this chart does not allow not setting CPU limit:

➜  grafana-tempo git:(master) ✗ helm template . -f values.yaml -f values/dev.yaml -f values/common.yaml > /tmp/tmpltmpl
Error: values don't meet the specifications of the schema(s) in the following chart(s):
opentelemetry-operator:
- manager.resources.limits: cpu is required

Which means my PR is still desired.

[TylerHelmuth]

We've seen this issue come up in other helm repos and removed default resources from the collector chart already so I'm inclined to do it here as well. @open-telemetry/helm-approvers do you agree?

[povilasv]

I agree, let's add some notes in upgrading.md about this change, so users upgrading can know about this

[Allex1]

@Pionerd Can you validate the null propagates into the dependency chart after removing the schema requirements ? I'm reasonably sure it does with helm 3.13.2. If so , should we just keep the default limits and just add instructions on how to remove them?

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[Pionerd]

@Allex1 Yes, that works. Can you guys let me know if you want to keep the default limits, or that I should update the upgrading.md as @povilasv suggests?

[Allex1]

Can you guys let me know if you want to keep the default limits, or that I should update the upgrading.md

I'd say keep the default limits and update the main readme with a section on sub-charting where you detail how to remove default keys (limits in this case) and what helm version is needed...etc

Thanks

[Pionerd]

@Allex1 done as suggested, please review.