-
Notifications
You must be signed in to change notification settings - Fork 4
feat: migrate to rapidfort container #587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
jpmckinney
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redis 6.2 is EOL since Feb 2025.
When operating a cluster, we can upgrade to at most 7.8 (though only 7.4 is at https://hub.docker.com/_/redis/): https://redis.io/docs/latest/operate/rs/installing-upgrading/upgrading/upgrade-cluster/#supported-upgrade-paths
However, the Docker images are not a cluster, so I think we can upgrade directly to 8.4. For example, 8.0x can read 6.2 databases: https://redis.io/docs/latest/operate/rs/installing-upgrading/upgrading/upgrade-database/
That said, we can still take the precautions here: https://redis.io/docs/latest/operate/oss_and_stack/install/upgrade/standalone/
I've read these breaking changes and found no issues:
- 6.4 https://redis.io/docs/latest/operate/rs/release-notes/rs-6-4-2-releases/
- 7.0 https://redis.io/docs/latest/embeds/r7-breaking-changes/
- 7.2
- 7.4 no breaking changes
- 7.8 https://redis.io/docs/latest/operate/rs/release-notes/rs-7-8-releases/#breaking-changes
- 7.22 no breaking changes: https://redis.io/docs/latest/operate/rs/release-notes/rs-7-22-releases/
- 8.0
- 8.2 no breaking changes: https://redis.io/docs/latest/develop/whats-new/8-2/
- 8.4 no breaking changes: https://redis.io/docs/latest/develop/whats-new/8-4/
| @@ -0,0 +1,101 @@ | |||
| # Network | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should minimize this file to only the options that change the default and that are truly necessary. This will make it easier to upgrade if options are deprecated in future. For example, *-ziplist-* became *-listpack-*, lua-time-limit is deprecated. I did:
curl https://raw.githubusercontent.com/redis/redis/6.2/redis.conf | grep -v '^#' | grep . | sort > a
curl https://raw.githubusercontent.com/open-contracting/deploy/de667b17e3eae5a245570df3a0a1fff4d29b9623/salt/docker_apps/files/conf/redis.conf | grep -v '^#' | grep . | sort > b
diff -u a bAnd I get (defaults in comments just for clarity in this comment):
bind 0.0.0.0 :: # 127.0.0.1 -::1
appendonly yes # no
daemonize yes # no
dir /data # ./
pidfile /tmp/redis.pid # /var/run/redis_6379.pid
protected-mode no # yes
save "" # not present in default redis.conf
- RapidFort must be doing something special for
daemonize yesto work. If we use the default image, I think we want to leave it asdaemonize no. - If we use the default image, protected mode is off by default (
protected-mode no): https://hub.docker.com/_/redis#security
So I think that leaves us with just:
bind 0.0.0.0 ::
dir /data
pidfile /tmp/redis.pid
# https://redis.io/docs/latest/operate/oss_and_stack/management/persistence/#append-only-file
appendonly yes
# https://redis.io/docs/latest/operate/oss_and_stack/management/persistence/#how-i-can-switch-to-aof-if-im-currently-using-dumprdb-snapshots
save ""
I repeated the diff comparing to 8.4, and, besides new options from later versions and the differences already noted, the default of repl-diskless-sync changed from no to yes. However, we don't use replicas, so it's not relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The customisation in this config file is from the previous Bitnami container which I pulled over to keep the systems running as similarly as possible. It will be better to have the lean configuration you've shared above - I will commit this.
Additionally, I am removing pidfile /tmp/redis.pid. From my reading pidfile /tmp is set to help redis recover from a unclean shutdown - in our case redis-server is the entrypoint and therefore this is managed by Docker. I believe bitnami had a wrapper script which then ran redis so that may be why they needed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I think perhaps pidfile also doesn't make sense with daemonize no.
Closes #586