chore(helm): set Singlebinary as trigger for replicas

[nohant]

What this PR does / why we need it:
this will make the singlebinary chart option working again by setting to 0 the replicas number of backend, write,read

Special notes for your reviewer:
Thanks @DylanGuedes for the explanation on slack

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Documentation added
• Tests updated
• Title matches the required conventional commits format, see here
• Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
• For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
• If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

[CLAassistant]

All committers have signed the CLA.

[nohant]

i was disabling the various components by default by doing this:

{{- if ne .Values.deploymentMode "SingleBinary" }}
{{- include "loki.memcached.statefulSet" (dict "ctx" $ "valuesSection" "resultsCache" "component" "results-cache" ) }}
{{- end }}

but then i noticed that a better solution would be to put enabled: false as default for those components:

• resultsCache:
• chunksCache:

i don't know if it is correct that i set them to disabled by default, because i don't honestly know if i have to work on the documentation later.

[DylanGuedes]

lgtm. it is out-of-date with main though, could you update it?

[nohant]

@DylanGuedes Thanks, i cant merge so i will wait for someone to do that.
i force pushed the updated docs and changelog.

[DylanGuedes]

@DylanGuedes Thanks, i cant merge so i will wait for someone to do that. i force pushed the updated docs and changelog.

it is still conflicting, not sure if it was someone merging something earlier today or your rebase didn't catch everything but could you update it again?

[nohant]

@DylanGuedes Thanks, i cant merge so i will wait for someone to do that. i force pushed the updated docs and changelog.

it is still conflicting, not sure if it was someone merging something earlier today or your rebase didn't catch everything but could you update it again?

it got like 4 merges xD i will do that again, yeah

[danports]

If the intent here was to eliminate the verbose chart configuration required for SingleBinary mode:

backend:
  replicas: 0
read:
  replicas: 0
write:
  replicas: 0
ingester:
  replicas: 0
querier:
  replicas: 0
queryFrontend:
  replicas: 0
queryScheduler:
  replicas: 0
distributor:
  replicas: 0
compactor:
  replicas: 0
indexGateway:
  replicas: 0
bloomCompactor:
  replicas: 0
bloomGateway:
  replicas: 0

...then I don't think this works as intended: Helm upgrade failed for release monitoring/loki with chart loki@6.3.3: execution error at (loki/templates/validate.yaml:31:4): You have more than zero replicas configured for both the single binary and simple scalable targets. If this was intentional change the deploymentMode to the transitional 'SingleBinary<->SimpleScalable' mode

[nohant]

Hi @danports, in the version of the chart 6.2.0 which is where i started, this was enough to make the singlebinary work:

write:
  replicas: 0
read:
  replicas: 0
backend:
  replicas: 0

im gonna look into this asap, sorry if you are facing problems!

[DylanGuedes]

If the intent here was to eliminate the verbose chart configuration required for SingleBinary mode:

backend:
  replicas: 0
read:
  replicas: 0
write:
  replicas: 0
ingester:
  replicas: 0
querier:
  replicas: 0
queryFrontend:
  replicas: 0
queryScheduler:
  replicas: 0
distributor:
  replicas: 0
compactor:
  replicas: 0
indexGateway:
  replicas: 0
bloomCompactor:
  replicas: 0
bloomGateway:
  replicas: 0

...then I don't think this works as intended: Helm upgrade failed for release monitoring/loki with chart loki@6.3.3: execution error at (loki/templates/validate.yaml:31:4): You have more than zero replicas configured for both the single binary and simple scalable targets. If this was intentional change the deploymentMode to the transitional 'SingleBinary<->SimpleScalable' mode

could you share your full config?

[nohant]

@DylanGuedes im testing the same, effectively, there is something that overwrite my configuration but i still havent found where, do you have any idea?

[DylanGuedes]

@DylanGuedes im testing the same, effectively, there is something that overwrite my configuration but i still havent found where, do you have any idea?

Not sure, sorry 😢

Since this didn't have the intended effect, do you mind opening a PR reverting it?

[nohant]

yep, im thinking about how to do that. :V
its just a normal PR like this one?

[nohant]

Found why.
im gonna open a new pr. the problem is in the template:

{{- $isSimpleScalable := eq (include "loki.deployment.isScalable" .) "true" -}}
{{- if and $isSimpleScalable (not .Values.read.legacyReadTarget ) }}

it filters for those before ever going to check for replicas
@DylanGuedes gimme some time and i will fix it

[DylanGuedes]

Found why. im gonna open a new pr. the problem is in the template:

{{- $isSimpleScalable := eq (include "loki.deployment.isScalable" .) "true" -}}
{{- if and $isSimpleScalable (not .Values.read.legacyReadTarget ) }}

it filters for those before ever going to check for replicas @DylanGuedes gimme some time and i will fix it

let's first revert this PR to stop the bleed.

yep, im thinking about how to do that. :V its just a normal PR like this one?

git revert commithash should do it

[nohant]

@DylanGuedes technically this doesnt change anything from before, my edits gets ignored the same as before i made them :/ i reverted the in the new pr (i tagged you).
if you think we need to fix this fast for other reason i will do that, but i think it will be more useful to continue on the other side.

Thanks!
Greg

[danports]

To be clear, nothing is actually broken in my install due to this PR - it's just that the PR description gave me false hope that I could remove all of the redundant replicas: 0 bits from my configuration. I left those bits in for now and the chart is still working fine.

[DylanGuedes]

To be clear, nothing is actually broken in my install due to this PR - it's just that the PR description gave me false hope that I could remove all of the redundant replicas: 0 bits from my configuration. I left those bits in for now and the chart is still working fine.

Oh that's good to know. Yes, by your comment I thought that this could be breaking existing installations somehow 😅

@nohant don't worry about reverting it then.

[nohant]

no, @DylanGuedes it does not breaks anything because it gets ignored sadly :V, i missed the part i explained before but i should have fixed it in the next PR (sorry by the double) also @danports can you check that? a double check is better than one xD

[thorker]

Can you please fix the copy & paste issue referencing {{.Values.write.replicas}} in read & backend

[nohant]

iv resolved it here: https://github.com/grafana/loki/pull/12727/files, while also resolving the problem that it didnt counted for replicas at all.