Thanks for the PR @4n4nd! Are you still seeing the slots issue you mentioned in the ticket? I would assume there are several commands we need to ensure are working with this change. Also we need to determine the migration/upgrade path for our users to get to this new version. Do we want to support 6.x and 7.x for a time? I think we have to do it this way bc we can't guarantee users' clients/applications are ready for 7.x.

Are you still seeing the slots issue you mentioned in the ticket?

I am not, the radix dependency update seems to have fixed it

I would assume there are several commands we need to ensure are working with this change. Also we need to determine the migration/upgrade path for our users to get to this new version.

I was successfully able to perform a rolling update from 6.2.x to 7.0.x just by updating the node container image in the RedisCluster CR.

Do we want to support 6.x and 7.x for a time?

I think we could do that. Once some initial tests are done on 7.0.x, I don't think it would be that difficult to maintain support for it. Are there any specific tests that you'd like me to perform on this?

I think we could do that. Once some initial tests are done on 7.0.x, I don't think it would be that difficult to maintain support for it. Are there any specific tests that you'd like me to perform on this?

I think a good place to start would be ensuring all the commands sent through DoCmd, DoCmdWithRetries, and PipeAppend work properly. I would think we'd see error messages in the logs on failure, but it's possible some errors are squelched. This is probably an opportunity to beef up our integration/e2e tests (if needed) as just bringing up a redis cluster executes most of these commands. Scaling will cause most of the others to be invoked.

❯ grep -Ehor '\.PipeAppend\(.*\)|\.DoCmd\(.*\)|\.DoCmdWithRetries\(.*\)' --include='*.go' | sort | uniq
.DoCmd(ctx, &keys, "CLUSTER", "GETKEYSINSLOT", slot.String(), batch)
.DoCmd(ctx, &rcv, cmd, args...)
.DoCmd(ctx, &resp, "CLIENT", "SETNAME", cnx.clientName)
.DoCmd(ctx, &resp, "CLUSTER", "COUNTKEYSINSLOT", slot.String())
.DoCmd(ctx, &resp, "CLUSTER", "FAILOVER")
.DoCmd(ctx, &resp, "CLUSTER", "FORGET", id)
.DoCmd(ctx, &resp, "CLUSTER", "MEET", ip, port)
.DoCmd(ctx, &resp, "CLUSTER", "NODES")
.DoCmd(ctx, &resp, "CLUSTER", "REPLICATE", primary.ID)
.DoCmd(ctx, &resp, "CLUSTER", "RESET", ResetSoft)
.DoCmd(ctx, &resp, "CLUSTER", "SETSLOT", slot.String(), action, node.ID)
.DoCmd(ctx, &resp, "CLUSTER", "SLOTS")
.DoCmd(ctx, &resp, "CLUSTER", args...)
.DoCmd(ctx, &resp, "CONFIG", "GET", pattern)
.DoCmd(ctx, &resp, "CONFIG", args...)
.DoCmd(ctx, &resp, "DEL", keys...)
.DoCmd(ctx, &resp, "INFO", "SERVER")
.DoCmd(ctx, nil, "FLUSHALL")
.DoCmdWithRetries(ctx, &resp, "MIGRATE", args...)
.PipeAppend(radix.Cmd(nil, "CLUSTER", "RESET", mode))
.PipeAppend(radix.Cmd(nil, "CLUSTER", "SETSLOT", slot.String(), action))
.PipeAppend(radix.Cmd(nil, "CLUSTER", "SETSLOT", slot.String(), action, nodeID))
.PipeAppend(radix.Cmd(nil, "FLUSHALL"))

I was successfully able to perform a rolling update from 6.2.x to 7.0.x just by updating the node container image in the RedisCluster CR.

Did you have data in the cluster? If not, we should definitely test that.

Did you have data in the cluster? If not, we should definitely test that.

Just added a bunch of keys manually and tested the update. All the keys successfully migrated to the updated cluster.

Cluster configuration was: 3 primaries and 2 replication-factor. Updated from version 6.2.x to v7.0.4

I think a good place to start would be ensuring all the commands sent through DoCmd, DoCmdWithRetries, and PipeAppend work properly.

I'll check the redis changelogs to see if any of the commands you listed were updated or not

I just brought up a 7.x cluster for testing and so far things look good. I did see this warning in the logs though. Will investigate.

rediscluster-rc-node-for-redis-mhw7v redis-node I0914 20:23:02.779987       1 log.go:16] 20:M 14 Sep 2022 20:23:02.779 * No cluster configuration found, I'm 28d90c9c229d417102bc774a847307eb01083c5e
rediscluster-rc-node-for-redis-mhw7v redis-node I0914 20:23:02.873994       1 log.go:16] 20:M 14 Sep 2022 20:23:02.873 * Running mode=cluster, port=6379.
rediscluster-rc-node-for-redis-mhw7v redis-node 20:M 14 Sep 2022 20:23:02.873 # Server initialized
rediscluster-rc-node-for-redis-mhw7v redis-node 20:M 14 Sep 2022 20:23:02.873 # WARNING overcommit_memory is set to 0! Background save may fail under low memory condition. To fix this issue add 'vm.overcommit_memor
y = 1' to /etc/sysctl.conf and then reboot or run the command 'sysctl vm.overcommit_memory=1' for this to take effect.

Changes in commands that might affect operator behavior:

CLUSTER-

• https://github.com/redis/redis/blob/7.0/00-RELEASENOTES#L547 CLUSTER SLOTS command now supports hostnames in addition to ip addresses. This shouldn't break anything for us.
• https://github.com/redis/redis/blob/7.0/00-RELEASENOTES#L130 CLUSTER SHARDS returns slots as RESP integers, not strings. I don't see this command being used anywhere in the operator.
• https://github.com/redis/redis/blob/7.0/00-RELEASENOTES#L285 CLUSTER SLOTS command has been deprecated and replaced by CLUSTER SHARDS. This could become an issue later. Doc

CONFIG-

• https://github.com/redis/redis/blob/7.0/00-RELEASENOTES#L291 CONFIG GET response returned in a non-deterministic order. This command is being used here. But I am not sure if we depend on the order of the response.

@cin I couldn't find any other changes that would affect the operator's functionality.

@cin any luck with this PR?

Hey @4n4nd, sorry for the delay. I was out unexpectedly on Thursday and Friday. I actually did test this a bit last Wednesday. Things seem to work though I didn't get to dig into the warning noted above yet. I think the main thing that needs to change here is maintaining support for 6.x. I know at least for us internally, we need to support 6.x until apps are ready to upgrade to 7.x. I don't think this should be too hard to do luckily. We basically just copy the dockerfile and build two different images. I'm not sure how they should be named off-hand. Will need to think about that...open to suggestions...

Hey @4n4nd, sorry for the delay. I was out unexpectedly on Thursday and Friday.

No worries! Please take your time. I hope everything is going well for you.

I think the main thing that needs to change here is maintaining support for 6.x.

Yeah, since we are not making any changes to the operator, I think we could just add another Dockerfile (Dockerfile.node.6.2.x) which can be removed once Redis v6.2 is EOL in August 2023.

I'm not sure how they should be named off-hand. Will need to think about that...open to suggestions...

I had one suggestion. Since Redis Node images are essentially Redis images, we could just tag them as whatever Redis version they are. That way we could maintain builds for both.

Example: node-for-redis:7.0.4 and node-for-redis:6.2.7

Maybe we could also re-tag some older images to match this naming convention. wdyt?

Actually the node image contains code (see pkg/redisnode). It's responsible for configuring and managing Redis (among other things). I've never actually looked at it closely. That said, we'll need to version the image w/the github tag as well. So I think we should do something like node-for-redis:x.y.z-7.0.4 and node-for-redis:x.y.z-6.2.7, where x.y.z is the github tag.

Maybe we could also re-tag some older images to match this naming convention. wdyt?

I wouldn't worry about re-tagging any images bc they align with the helm charts. We'll definitely want to bump the minor version with this change though.

So I think we should do something like node-for-redis:x.y.z-7.0.4 and node-for-redis:x.y.z-6.2.7, where x.y.z is the github tag.

I think this is a good idea. We would have unique images for any operator code changes as well as any redis updates.

@cin I updated the release workflow. Could you please give it a look?

@4n4nd, I think this looks good from an automation standpoint. However I think we need to update the Makefile as well so it builds the proper images when make container PREFIX= TAG=cin is called locally (for testing).

Maybe something like this would work?

container-%: buildlinux-%
  if [ "$*" == "node" ]; then \
		docker build -t $(PREFIX)$*-for-redis:$(TAG)-6.2.7 --build-arg "REDIS_VERSION=6.2.7" -f Dockerfile.$* . \
		docker build -t $(PREFIX)$*-for-redis:$(TAG)-7.0.4 --build-arg "REDIS_VERSION=7.0.4" -f Dockerfile.$* . \
	else \
		docker build -t $(PREFIX)$*-for-redis:$(TAG) -f Dockerfile.$* . \
	fi

I also use skaffold locally to build images (especially if i'm pushing to a CR instead of local kind clusters). I'm looking into a way to override the image tag now.

@cin tested and updated the Makefile with your suggestion.
I am not very familiar with skaffold, but you could pass the version as make container-node REDIS_VERSION_6=6.2.6 to build a specific version.

I just noticed the build info strings. build.go and the corresponding set call here.

So the way this is being done now will not have the right REDIS_VERSION listed in the logs when it starts. Not the biggest deal but it'd be confusing. Should we maybe just allow the user to call the Makefile w/REDIS_VERSION? If they don't set it, we default it to 6.2.7. If they do set it, let it go through and at least try to build the image, etc. They'd have to call the makefile again to build another version, but I think that's fine (given that we'll have automation building both). Thoughts?

We also need to update the image in the helm chart. Right now, we don't have a Redis version override; but in order to default the image properly, we'll need it. You can still get this to work now but simply specifying the tag but we should make it as easy as possible on users of the operator.

Nitpicking here, but I think we should swap the github tag and redis version in the image tag name. I just looked at how we've done this in other projects, and we used the following format: node-for-redis:7.0.4-x.y.z. Sorry, I should have looked that up before posting about it.

I just noticed the build info strings. build.go and the corresponding set call here.

taking a look 👀

Should we maybe just allow the user to call the Makefile w/REDIS_VERSION? If they don't set it, we default it to 6.2.7. If they do set it, let it go through and at least try to build the image, etc. They'd have to call the makefile again to build another version, but I think that's fine (given that we'll have automation building both). Thoughts?

Yeah, I think letting the users set REDIS_VERSION would be the simplest solution. For the default value, I would want to keep it to 7.0.4 since we want new users to start with a newer version of Redis (6.2 will EOL in August 2023).

We also need to update the image in the helm chart. Right now, we don't have a Redis version override; but in order to default the image properly, we'll need it. You can still get this to work now but simply specifying the tag but we should make it as easy as possible on users of the operator.

What if we use the latest tag as default in the helm charts? Anyone running in prod would want to fix it to a version any way and users who are trying out the operator won't care as much. If we do this, we will have one less value change on every redis version update.

Nitpicking here, but I think we should swap the github tag and redis version in the image tag name. I just looked at how we've done this in other projects, and we used the following format: node-for-redis:7.0.4-x.y.z. Sorry, I should have looked that up before posting about it.

No worries 👍 I can just make the changes.

I would want to keep it to 7.0.4

Fair enough.

What if we use the latest tag as default in the helm charts?

I've never been a fan of using latest for anything. :) It can be convenient at times, but it always ends up biting you somewhere down the line. Also, check out the other comment I made about the appVersion. These things impact each other, so it complicates things a bit.

I just noticed the build info strings. build.go and the corresponding set call here.

@cin The linter didn't seem to mind it. So, I updated the Makefile to make it consistent with the release.yaml. wdyt?

Also, the whole go build runs again in the Docker build. So, the REDIS_VERSION would still be consistent when a user runs make container-node.

@cin any luck with this PR?

Hey @4n4nd. Sorry I got pulled into some other things and haven't had a chance to get back to this since last Thursday. There's one place that needs some cleanup that I know of offhand though and it's defaulting the chart image. As it stands right now, the user would always have to override the tag in order to set the node-for-redis image properly. I think it's good after that (but will need to test to make sure).

@cin sorry for the delay. I couldn't really come up with a perfect solution.
How about we stick to 7.0.x as default and add a release_v6.yaml github workflow that builds node images for Redis v6.x updates? This way we would have support for both versions and once Redis v6 is EoL, we could remove the workflow yaml file.

@4n4nd I'm sorry for the delay; I thought I responded to this. I think I'm okay with breaking out v6 into a separate workflow. This will simplify things a good bit especially as you mentioned when v6 is EOL. I think we should also look into fixing up the Makefile for this as well (not the biggest issue but will is nice to have it usable). There's probably a cleaner way to do this but something like this maybe could work. I'm no Makefile expert so there may be a better way to do this here.

build-%:
	@if [ "$*" = "node" ]; then\
		echo "v6: ${REDIS_VERSION_6} and v7: ${REDIS_VERSION_7}" ;\
		LDFLAGS=" -ldflags '"'-w -X ${BUILDINFOPKG}.TAG=${TAG} -X ${BUILDINFOPKG}.COMMIT=${COMMIT} -X ${BUILDINFOPKG}.OPERATOR_VERSION=${OPERATOR_VERSION} -X ${BUILDINFOPKG}.REDIS_VERSION=${REDIS_VERSION_6} -X ${BUILDINFOPKG}.BUILDTIME=${DATE} -s'"'" ;\
		CGO_ENABLED=0 go build -installsuffix cgo ${LDFLAGS} -o bin/$* ./cmd/$* ;\
		LDFLAGS=" -ldflags '"'-w -X ${BUILDINFOPKG}.TAG=${TAG} -X ${BUILDINFOPKG}.COMMIT=${COMMIT} -X ${BUILDINFOPKG}.OPERATOR_VERSION=${OPERATOR_VERSION} -X ${BUILDINFOPKG}.REDIS_VERSION=${REDIS_VERSION_7} -X ${BUILDINFOPKG}.BUILDTIME=${DATE} -s'"'" ;\
		CGO_ENABLED=0 go build -installsuffix cgo ${LDFLAGS} -o bin/$* ./cmd/$* ;\
	else\
		LDFLAGS=" -ldflags '"'-w -X ${BUILDINFOPKG}.TAG=${TAG} -X ${BUILDINFOPKG}.COMMIT=${COMMIT} -X ${BUILDINFOPKG}.OPERATOR_VERSION=${OPERATOR_VERSION} -X ${BUILDINFOPKG}.REDIS_VERSION=${REDIS_VERSION_7} -X ${BUILDINFOPKG}.BUILDTIME=${DATE} -s'"'" ;\
		CGO_ENABLED=0 go build -installsuffix cgo ${LDFLAGS} -o bin/$* ./cmd/$* ;\
	fi

buildlinux-%: ${SOURCES}
	@if [ "$*" = "node" ]; then\
		LDFLAGS=" -ldflags '"'-w -X ${BUILDINFOPKG}.TAG=${TAG} -X ${BUILDINFOPKG}.COMMIT=${COMMIT} -X ${BUILDINFOPKG}.OPERATOR_VERSION=${OPERATOR_VERSION} -X ${BUILDINFOPKG}.REDIS_VERSION=${REDIS_VERSION_6} -X ${BUILDINFOPKG}.BUILDTIME=${DATE} -s'"'" ;\
		CGO_ENABLED=0 GOOS=linux go build -installsuffix cgo ${LDFLAGS} -o docker/$*/$* ./cmd/$*/main.go ;\
		LDFLAGS=" -ldflags '"'-w -X ${BUILDINFOPKG}.TAG=${TAG} -X ${BUILDINFOPKG}.COMMIT=${COMMIT} -X ${BUILDINFOPKG}.OPERATOR_VERSION=${OPERATOR_VERSION} -X ${BUILDINFOPKG}.REDIS_VERSION=${REDIS_VERSION_7} -X ${BUILDINFOPKG}.BUILDTIME=${DATE} -s'"'" ;\
		CGO_ENABLED=0 GOOS=linux go build -installsuffix cgo ${LDFLAGS} -o docker/$*/$* ./cmd/$*/main.go ;\
	else\
		LDFLAGS=" -ldflags '"'-w -X ${BUILDINFOPKG}.TAG=${TAG} -X ${BUILDINFOPKG}.COMMIT=${COMMIT} -X ${BUILDINFOPKG}.OPERATOR_VERSION=${OPERATOR_VERSION} -X ${BUILDINFOPKG}.REDIS_VERSION=${REDIS_VERSION_7} -X ${BUILDINFOPKG}.BUILDTIME=${DATE} -s'"'" ;\
		CGO_ENABLED=0 GOOS=linux go build -installsuffix cgo ${LDFLAGS} -o docker/$*/$* ./cmd/$*/main.go ;\
	fi