feat(helm): allow external memcached setup

[abenbachir]

What this PR does / why we need it:
allow external memcached setup, with overriding addresses.

Which issue(s) this PR fixes:
Fixes #12559

Special notes for your reviewer:
the old config loki.memcached does't work anymore.

New way of setting up external memcache is with:

memcached:
    enabled: false # <- setting false here
resultsCache:
    addresses: 'my-resultsCache-memcached-address' # <- setting custom address here
chunksCache:
    addresses: 'my-chunksCache-memcached-address' # <- setting custom address here

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Documentation added
• Tests updated
• Title matches the required conventional commits format, see here

[Starefossen]

Looks good to me provided the suggestions are followed up and the tests pass.

[abenbachir]

Looks good to me provided the suggestions are followed up and the tests pass.

thanks for the feedback. Fixed it and rebased it.

[lina-semgrep]

Hi, waiting for the new version of the chart with this fix here as well :)

[abenbachir]

@Starefossen I don't know why tests are not running.

[Starefossen]

We need to ping the official maintainers to get them running.

[abenbachir]

We need to ping the official maintainers to get them running.

Can you share there usernames, I will ping them.

[lina-semgrep]

We need to ping the official maintainers to get them running.

Can you share there usernames, I will ping them.

loki/production/helm/loki/Chart.yaml

Lines 30 to 31 in 667701f

	- name: trevorwhitney
	- name: jeschkies

?

[JStickler]

We've started a new Loki maintainers group, and while I was triaging Helm charts PRs, I went ahead and ran the CI for you. I'll flag this for the team to look at.

[lina-semgrep]

@abenbachir it seems there are some helm linting issues - do you have cycles to look at? if not I can try take a look

[jkroepke]

Hey @abenbachir

I'm part of a new maintainers group.

I looked through your PR and ask myself, if this can be simplified.

Do we really need to expose host and clientService? addresses also should cover most use-cases and I feel that expose just addresses is enough. Is there a use-case where addresses can't be used?

And here is why: Your example

memcached:
    enabled: false # <- setting false here
resultsCache:
    host: 'my-resultsCache-memcached-host' # <- setting custom host here
    clientService: 'my-memcached-svc'
chunksCache:
    host: 'my-chunksCache-memcached-host' # <- setting custom host here
    clientService: 'my-memcached-svc'

Will not work. Because in that scenario, addresses is still set. If addresses and host is set, loki takes addresses over host. Ref:

loki/pkg/storage/chunk/cache/memcached_client.go

Lines 276 to 292 in 8fed2c9

	if len(c.addresses) > 0 {
	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
	defer cancel()
	if err := c.provider.Resolve(ctx, c.addresses); err != nil {
	return err
	}
	servers = c.provider.Addresses()
	} else {
	_, addrs, err := net.LookupSRV(c.service, "tcp", c.hostname)
	if err != nil {
	return err
	}
	for _, srv := range addrs {
	servers = append(servers, net.JoinHostPort(srv.Target, strconv.Itoa(int(srv.Port))))
	}
	}

Based on that, the user experience would be not great, because end-user have to set explicit addresses to an empty value to force loki pick addresses.

[abenbachir]

addresses

then I will only expose addresses, thanks for the feedback. below config will be the one to use.

memcached:
    enabled: false # <- setting false here
resultsCache:
    addresses: 'my-resultsCache-memcached-address' # <- setting custom address here
chunksCache:
    addresses: 'my-chunksCache-memcached-address' # <- setting custom address here

[jkroepke]

Thanks, please take now of the failing jobs.

[abenbachir]

@jkroepke thanks for the feedback, I have updated the PR with all suggestions.

[jkroepke]

Generally, this LGTM. However, I found this sections in value.yaml

loki/production/helm/loki/values.yaml

Lines 460 to 474 in 95095ba

	# -- Configure memcached as an external cache for chunk and results cache. Disabled by default
	# must enable and specify a host for each cache you would like to use.
	memcached:
	chunk_cache:
	enabled: false
	host: ""
	service: "memcached-client"
	batch_size: 256
	parallelism: 10
	results_cache:
	enabled: false
	host: ""
	service: "memcached-client"
	timeout: "500ms"
	default_validity: "12h"

This is a documented, but not implemented section in values.

@JStickler I'm not sure how we should proceed here. I recommend removing the section from the values file, as it confuses end users by presenting two configurations for the same functionality, even though only one of them actually works.

However, I'm not sure if this would count as a breaking change from your point of view, since it involves a documented (but non-functional) feature.

[abenbachir]

Configure memcached as an external

yeah, that section is an old config, I removed it.

[Jayclifford345]

Hey @jkroepke, @Starefossen
I am happy to merge this if we feel that removing the old config from the values file won't lead to breaking changes for other users. Especially since this changes how the configuration for default deployment detections of memcache that are deployed along side Loki

[jkroepke]

I scrape through the helm chart and to my best knowledge, the removed lines from the values.yaml are not used anywhere. They are untouched for 3 years as well. They doesn't have any effect. If customers blame for breaking changes, they should consult that the changes did not have any effect.