drupalwxt / helm-drupal

Helm Chart for deploying an enterprise-grade Drupal environment.
https://drupalwxt.github.io/helm-drupal/index.yaml
MIT License
31 stars 22 forks source link

Redis chart version update #119

Closed bernardmaltais closed 2 years ago

bernardmaltais commented 2 years ago

The current version of redis is VERY VERY VERY old and does not offer support for nodeSelector and other goodies. Updating to the latest version allow for better positioning of redis pods and improve site performance.

@sylus I am leaving the chart version update to you guys since this is my 3rd pull request and I have no idea which will be accepted and which will not.

sylus commented 2 years ago

Yeah ive been meaning to get around to this, with this jump did any of the sub params change or they all still the same?

bernardmaltais commented 2 years ago

I noticed only that the default replicaCount is now 3 and since it was not set in the example you provided I had to add the following to bring it back to two:

  replica:
   replicaCount: 2

But I think more will need to be updated in the yaml values file to make the best of the new version. I have to say I understand very little about redis and I have been fighting this issue where on sometime the site is fast (when master is on the same node as the drupal pod) and other it is very slow (if the redis master pod is not on the same node as drupal).

bernardmaltais commented 2 years ago

This is my current config for redis which appear to be fast at all time using the latest chart release:

redis:
  enabled: true
  password: ${var.redis_password_rsams}
  # image:
  #   registry: docker.io
  #   repository: bitnami/redis
  #   tag: 6.0.12-debian-10-r33
  #   clientInterface: PhpRedis
  # cluster:
  #   enabled: true
  #   slaveCount: 2
  # sentinel:
  #   enabled: false
  master:
    service:
      type: ClusterIP
    disableCommands: []
    affinity:
      nodeAffinity:
        requiredDuringSchedulingIgnoredDuringExecution:
          nodeSelectorTerms:
          - matchExpressions:
            - key: env
              operator: In
              values:
              - sand
    nodeSelector:
      env: sand
  replica:
    replicaCount: 2
    nodeSelector:
      env: sand

Let me know if I should tune a few things ;-)

zachomedia commented 2 years ago

This also looks good! I just want to confirm a couple of things in my local test and then it'll be good to merge :) Thank you!

zachomedia commented 2 years ago

Ok, I have tested this and it looks good. Only thing was the redis master statefulset needed to be manually removed: Error: UPGRADE FAILED: cannot patch "wxt-redis-master" with kind StatefulSet: StatefulSet.apps "wxt-redis-master" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden.

I'll make the appropriate version number bump to indicate this. Thanks!