Upgrade redis chart version

[cabrinha]

The redis chart is pretty out of date and it seems it won't let me set tolerations or nodeSelector on the replicas.

[sagor999]

@cabrinha will this upgrade already existing sentry installation without any issues?
redis chart has changed a lot between version 9 and 15 (6 major version changes). Including statefulset naming has also changed. I am wondering if upgrade might get borked because of that?
Did you try upgrading redis chart on already existing sentry deployment? Did it work without issues?

[cabrinha]

@cabrinha will this upgrade already existing sentry installation without any issues?
redis chart has changed a lot between version 9 and 15 (6 major version changes). Including statefulset naming has also changed. I am wondering if upgrade might get borked because of that?
Did you try upgrading redis chart on already existing sentry deployment? Did it work without issues?

I just deployed it to my cluster and it looks like the biggest change is that redis-slave is being renamed to redis-replicas.

The redis-master STS did get updated with new scripts, but after syncing the app, it's all back up and running.

@sagor999 please test on your end or allow anyone else to try it out.

[sagor999]

Ah, good to hear that it upgrades without any issues.

[Mokto]

As @sagor999 said, this PR is risky. I would like a few people to try this change to be more confident about it.

[cabrinha]

As @sagor999 said, this PR is risky. I would like a few people to try this change to be more confident about it.

Please, go ahead and try it out yourself, use kind or minikube, try it on your testing clusters, etc.

It'd be great if there were a test environment for this chart to make risky but important PRs easier to test publicly.

[sagor999]

I just tested it, but for me upgrade failed (as I kind of thought it would because of statefulset use by redis):

Error: UPGRADE FAILED: cannot patch "sentry-staging-sentry-redis-master" with kind StatefulSet: StatefulSet.apps "sentry-staging-sentry-redis-master" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden

Also if you are specifying storage class for your redis replica (used to be named slave in chart version 9 of redis) then you need to update your values.yaml to:

  replica: # used to be named slave
    persistence:
      storageClass: standard-ssd

So to be able to actually upgrade the chart I had to delete statefulset for redis, both master and slave.
Then roll out chart upgrade, and it rolled out new statefulsets for redis (named master and replica).
Your data is preserved on master, since PVC has not changed. Your slave redis has now become redis replica, and has created new PVC.

Right now it seems to be running fine. I will let it soak for at least 24 hours to confirm I don't see any other issues with upgrade.
But this change will definitely require a major version change, as it will require to do manual steps before upgrading (outlined above).

[cabrinha]

I just tested it, but for me upgrade failed (as I kind of thought it would because of statefulset use by redis):

Error: UPGRADE FAILED: cannot patch "sentry-staging-sentry-redis-master" with kind StatefulSet: StatefulSet.apps "sentry-staging-sentry-redis-master" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', and 'updateStrategy' are forbidden

Also if you are specifying storage class for your redis replica (used to be named slave in chart version 9 of redis) then you need to update your values.yaml to:

  replica: # used to be named slave
    persistence:
      storageClass: standard-ssd

So to be able to actually upgrade the chart I had to delete statefulset for redis, both master and slave.
Then roll out chart upgrade, and it rolled out new statefulsets for redis (named master and replica).
Your data is preserved on master, since PVC has not changed. Your slave redis has now become redis replica, and has created new PVC.

Right now it seems to be running fine. I will let it soak for at least 24 hours to confirm I don't see any other issues with upgrade.
But this change will definitely require a major version change, as it will require to do manual steps before upgrading (outlined above).

Yeah, statefulsets need a force apply or delete/recreate. Let me know if there is anything else in the PR you'd like me to change.

[sagor999]

My suggestion would be to do a major version bump to the chart.yaml.
And in readme, add a note as to why major version was bumped, and how to upgrade (either force apply or manual delete of statefulset for redis).

Besides that I think it is looking good. My staging sentry is working well after upgrade as well (at least I did not notice any issues or errors after redis upgrade).

Edit: Oh, and maybe add a note about if you are specifying storage class for redis slave, to update it to replica.

[cabrinha]

My suggestion would be to do a major version bump to the chart.yaml.
And in readme, add a note as to why major version was bumped, and how to upgrade (either force apply or manual delete of statefulset for redis).

Besides that I think it is looking good. My staging sentry is working well after upgrade as well (at least I did not notice any issues or errors after redis upgrade).

Edit: Oh, and maybe add a note about if you are specifying storage class for redis slave, to update it to replica.

Can someone help with the README / notes stuff? I'm not sure where to put it or what to say. I've bumped the major chart version.

[sagor999]

@cabrinha in README.md you can add something like that I think:

## Upgrading from 11.x.x version of this Chart to 12.0.0

Redis chart was upgraded to newer version. 
If you are using external redis, you don't need to do anything.
Otherwise, when upgrading to chart version 12.x.x from 11.x.x you need to either run helm upgrade with --force flag, or prior to upgrade delete statefulsets for redis master and redis slave. Then run upgrade and it will roll out new statefulsets. Your master redis data will not be lost (PVC is not deleted when you delete statefulset). Your redis slave will now be named redis replica and you can delete PVCs that were used by redis slave after the upgrade.

## Upgrading from 10.x.x version of this Chart to 11.0.0

If you were using clickhouse tabix externally, we disabled it per default.

@Mokto if you have suggestions ^

[cabrinha]

@cabrinha in README.md you can add something like that I think:

## Upgrading from 11.x.x version of this Chart to 12.0.0

Redis chart was upgraded to newer version. 
If you are using external redis, you don't need to do anything.
Otherwise, when upgrading to chart version 12.x.x from 11.x.x you need to either run helm upgrade with --force flag, or prior to upgrade delete statefulsets for redis master and redis slave. Then run upgrade and it will roll out new statefulsets. Your master redis data will not be lost (PVC is not deleted when you delete statefulset). Your redis slave will now be named redis replica and you can delete PVCs that were used by redis slave after the upgrade.

## Upgrading from 10.x.x version of this Chart to 11.0.0

If you were using clickhouse tabix externally, we disabled it per default.

@Mokto if you have suggestions ^

Done.

[cleveritcz]

when I try to install I got this error:

Error: INSTALLATION FAILED: template: sentry/charts/redis/templates/redis-slave-svc.yaml:5:20: executing "sentry/charts/redis/templates/redis-slave-svc.yaml" at <{{template "redis.fullname" .}}>: template "redis.fullname" not defined

[sagor999]

it would help if you provide information on how you are performing install (helm commands) as well as your values.yaml file.

[granescb]

@cleveritcz You need delete redis folder (not the archive) from charts directory