Handle overprovisioned flag based on CPU values

[RafalKorepta]

Overprovisioned till Redpanda chart version 5.8.8 should be
set to true when Statefulset CPU request value is bellow 1000 mili cores.

Function redpanda-smp, that should overwrite overprovisioned flag,
was not called before setting overprovisioned flag.

Reference

redpanda-smp template -

helm-charts/charts/redpanda/templates/_helpers.tpl

Line 187 in 5f287d4

{{- $_ := set $.Values.resources.cpu "overprovisioned" true -}}

redpanda-smp template invocation -

helm-charts/charts/redpanda/templates/_configmap.tpl

Line 610 in 5f287d4

{{- get ((include "redpanda.RedpandaAdditionalStartFlags" (dict "a" (list . (include "redpanda-smp" .) ))) | fromJson) "r" | toYaml | nindent 4 }}

overprovisioned flag -

helm-charts/charts/redpanda/templates/_configmap.tpl

Line 607 in 5f287d4

overprovisioned: {{ dig "cpu" "overprovisioned" false .Values.resources }}

[chrisseto]

Could you explain the original issue a bit more? Either in the commit message or as a comment somewhere. I don't think I fully understand why this is a fix?

[chrisseto]

nit: This is kinda hidden right now and using regex to change up JSON always feels a bit sketchy. What about moving this up to line 205 and making it something like:

// If this path is deeper, use the `unstructured` package from k8s to access it
// Explain the old bug here. Now that it's considered fixed, we'll ignore this diff by pushing the correct value into the old conf
oldConf.redpanda["overprovisioned"] = newConf.redpanda["overprovisioned"]

Placing this right before the assertion will help group up all the logic that we start to accumulate like this.

You could also compute a JSON diff and remove the overprovisioned field from it in a similar way.

[RafalKorepta]

What about moving this up to line 205

If this path is deeper, use the unstructured package from k8s to access it

I can not use on manifests as --- (YAML delimiter) is unrecognized by the unstructured unmarshall function.

Received unexpected error:
invalid character '-' in numeric literal

Placing this right before the assertion will help group up all the logic that we start to accumulate like this.

I agree.

You could also compute a JSON diff and remove the overprovisioned field from it in a similar way.

Which JSON diff function/library?

[chrisseto]

I can not use on manifests as --- (YAML delimiter) is unrecognized by the unstructured unmarshall function.

Wouldn't that be a non issue around line 205 as you won't be working with manifests (multi-document YAMLs) but with the parsed out configmap values?

[RafalKorepta]

newConf.redpanda holds a raw redpanda config not K8S ConfigMap. When I'm unmarshall raw redpanda config I get the error:

Received unexpected error:
Object 'Kind' is missing in ...

[chrisseto]

There's a really interesting issue from @StephanDollberg on what the correct way to set this is. Curious if he (currently) has any suggestions on what we should be doing here.

[StephanDollberg]

I am not sure I understand the full PR but this change only seems to be concerned with setups where we have less than one 1 core/1000 millicpus assigned to Redpanda?

In that case using overprovisioned unconditionally is the right thing to do.

[chrisseto]

That's exactly what it is, I just didn't understand at the time 😓

Thank you for confirming! I was going to seek further guidance on the --overprovisioned flag but we'll do that at a later time.