evryfs / helm-charts

OpenSourced Helm charts
Apache License 2.0
49 stars 76 forks source link

[mcrouter] Wrong memcached service name in configmap #251

Open dragoangel opened 1 year ago

dragoangel commented 1 year ago

Describe the bug Proper naming should be:

{
  "pools": {
    "A": {
      "servers": [
        // hosts of replicated pool, https://github.com/facebook/mcrouter/wiki/Replicated-pools-setup e.g.:
        "mcrouter-memcached-0.mcrouter-memcached.mcrouter.svc.cluster.local:11211",
        "mcrouter-memcached-1.mcrouter-memcached.mcrouter.svc.cluster.local:11211",
        "mcrouter-memcached-2.mcrouter-memcached.mcrouter.svc.cluster.local:11211",
      ]
    }
  },

but by default service names generated as mcrouter-0.mcrouter.mcrouter.svc.cluster.local:11211 resulting in non-working mcrouter, template missing -memcached twice, for pod and for service names.

Version of Helm and Kubernetes:

helm v3.12.3
kubernetes v1.24.15

**Which chart**:
mcrouter

What happened: wrong configuration applied by default

What you expected to happen: default config have proper settings to work

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know:

dependencies:
  - name: memcached
    version: ~6.6
    repository: https://charts.bitnami.com/bitnami
    condition: mcrouter.memcached.enabled

condition mcrouter.memcached.enabled should be memcached.enabled?

dragoangel commented 1 year ago

It would be also good to bump Bitnami Memcached chart version to ~6.6 and allow templating route via helm values, as default section could not fit well for everyone, for example "get": "LatestRoute|Pool|A" use just same memcached pod from a pool as described here: https://github.com/facebook/mcrouter/issues/127#issuecomment-226425514