Skip to content
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

Support Valkey 7 in CentOS Stream 10 #2

Merged
merged 6 commits into from
Aug 20, 2024
Merged

Support Valkey 7 in CentOS Stream 10 #2

merged 6 commits into from
Aug 20, 2024

Conversation

phracek
Copy link
Member

@phracek phracek commented Aug 15, 2024

This pull request enables building and testing valkey-container in CentOS Stream 10

Copy link

github-actions bot commented Aug 16, 2024

Pull Request validation

Failed

🔴 Failed or pending statuses - Testing Farm - CentOS Stream 10 - 7[pending],Testing Farm - Fedora - 7[pending]
🔴 Review - Missing review from a member

@phracek
Copy link
Member Author

phracek commented Aug 16, 2024

Difference between Dockerfile.fedora and redis dockerfile.fedora in version 7

$ diff -u Dockerfile.fedora ../../redis-container/7/Dockerfile.fedora
--- Dockerfile.fedora	2024-08-16 09:53:12
+++ ../../redis-container/7/Dockerfile.fedora	2024-08-15 13:59:00
@@ -1,70 +1,62 @@
-FROM quay.io/fedora/s2i-core:40
+FROM quay.io/fedora/s2i-core:38

-# Valkey image based on Software Collections packages
+# Redis image based on Software Collections packages
 #
 # Volumes:
-#  * /var/lib/valkey/data - Datastore for Valkey
+#  * /var/lib/redis/data - Datastore for Redis
 # Environment:
-#  * $VALKEY_PASSWORD - Database password
+#  * $REDIS_PASSWORD - Database password

-ENV VALKEY_VERSION=7 \
-    HOME=/var/lib/valkey \
-    NAME=valkey
+ENV REDIS_VERSION=7 \
+    HOME=/var/lib/redis

-ENV SUMMARY="Valkey in-memory data structure store, used as database, cache and message broker" \
-    DESCRIPTION="Valkey $VALKEY_VERSION available as container, is an advanced key-value store. \
+ENV SUMMARY="Redis in-memory data structure store, used as database, cache and message broker" \
+    DESCRIPTION="Redis $REDIS_VERSION available as container, is an advanced key-value store. \
 It is often referred to as a data structure server since keys can contain strings, hashes, lists, \
 sets and sorted sets. You can run atomic operations on these types, like appending to a string; \
 incrementing the value in a hash; pushing to a list; computing set intersection, union and difference; \
 or getting the member with highest ranking in a sorted set. In order to achieve its outstanding \
-performance, Valkey works with an in-memory dataset. Depending on your use case, you can persist \
+performance, Redis works with an in-memory dataset. Depending on your use case, you can persist \
 it either by dumping the dataset to disk every once in a while, or by appending each command to a log."

 LABEL summary="$SUMMARY" \
       description="$DESCRIPTION" \
       io.k8s.description="$DESCRIPTION" \
-      io.k8s.display-name="Valkey $VALKEY_VERSION" \
-      io.openshift.expose-services="6379:valkey" \
-      io.openshift.tags="database,valkey,valkey$VALKEY_VERSION,valkey-$VALKEY_VERSION" \
+      io.k8s.display-name="Redis $REDIS_VERSION" \
+      io.openshift.expose-services="6379:redis" \
+      io.openshift.tags="database,redis,redis$REDIS_VERSION,redis-$REDIS_VERSION" \
       com.redhat.component="$NAME" \
-      name="fedora/$NAME-$VALKEY_VERSION" \
-      version="$VALKEY_VERSION" \
-      usage="podman run -d --name valkey_database -p 6379:6379 quay.io/fedora/$NAME-$VALKEY_VERSION" \
+      name="fedora/$NAME-$REDIS_VERSION" \
+      version="$REDIS_VERSION" \
+      usage="podman run -d --name redis_database -p 6379:6379 quay.io/fedora/$NAME-$REDIS_VERSION" \
       maintainer="SoftwareCollections.org <[email protected]>"

 EXPOSE 6379

-# Get prefix path and path to scripts rather than hard-code them in scripts
-ENV CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/valkey \
-    VALKEY_PREFIX=/usr \
-    VALKEY_CONF=/etc/valkey/valkey.conf \
-    VALKEY_SOCK=/run/valkey/valkey.sock \
-    VALKEY_LIB=/var/lib/valkey \
-    VALKEY_RUN=/run/valkey
-
-# Create user for valkey that has known UID
+# Create user for redis that has known UID
 # We need to do this before installing the RPMs which would create user with random UID
 # The UID is the one used by the default user from the parent layer (1001),
 # and since the user exists already, do not create a new one, but only rename
 # the existing
-# This image must forever use UID 1001 for valkey user so our volumes are
+# This image must forever use UID 1001 for redis user so our volumes are
 # safe in the future. This should *never* change, the last test is there
 # to make sure of that.
-RUN getent group  valkey &> /dev/null || groupadd -r valkey &> /dev/null && \
-    usermod -l valkey -aG valkey -c 'Server' default &> /dev/null && \
+RUN getent group  redis &> /dev/null || groupadd -r redis &> /dev/null && \
+    usermod -l redis -aG redis -c 'Redis Server' default &> /dev/null && \
 # Install gettext for envsubst command
-    dnf install -y dnf-utils gettext policycoreutils && \
-    INSTALL_PKGS="valkey" && \
+    dnf install -y yum-utils gettext policycoreutils && \
+    INSTALL_PKGS="redis" && \
     dnf install -y --setopt=tsflags=nodocs --nogpgcheck $INSTALL_PKGS && \
     rpm -V $INSTALL_PKGS && \
     dnf -y clean all --enablerepo='*' && \
-    valkey-server --version | grep -qe "^Server v=$VALKEY_VERSION\." && echo "Found VERSION $VALKEY_VERSION" && \
-    mkdir -p $VALKEY_LIB/data && chown -R valkey:0 $VALKEY_LIB && \
-    mkdir -p $VALKEY_RUN && chown -R valkey:0 $VALKEY_RUN && \
-    chmod -R ug+rwX $VALKEY_RUN && \
-    chmod -R ug+rwX $VALKEY_LIB && \
-    [[ "$(id valkey)" == "uid=1001(valkey)"* ]]
+    redis-server --version | grep -qe "^Redis server v=$REDIS_VERSION\." && echo "Found VERSION $REDIS_VERSION" && \
+    mkdir -p /var/lib/redis/data && chown -R redis.0 /var/lib/redis && \
+    [[ "$(id redis)" == "uid=1001(redis)"* ]]

+# Get prefix path and path to scripts rather than hard-code them in scripts
+ENV CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/redis \
+    REDIS_PREFIX=/usr \
+    REDIS_CONF=/etc/redis/redis.conf

 COPY root /

@@ -73,7 +65,7 @@
 # script.
 RUN /usr/libexec/container-setup

-VOLUME ["/var/lib/valkey/data"]
+VOLUME ["/var/lib/redis/data"]

 # Using a numeric value because of a comment in [1]:
 # If your S2I image does not include a USER declaration with a numeric user,
@@ -82,4 +74,4 @@
 USER 1001

 ENTRYPOINT ["container-entrypoint"]
-CMD ["run-valkey"]
+CMD ["run-redis"]

Difference between Dockerfile.fedora and Dockerfile.c10s in valkey-container

$ diff -u Dockerfile.c10s Dockerfile.fedora
--- Dockerfile.c10s	2024-08-16 09:53:12
+++ Dockerfile.fedora	2024-08-16 09:53:12
@@ -1,4 +1,4 @@
-FROM quay.io/sclorg/s2i-core-c10s:c10s
+FROM quay.io/fedora/s2i-core:40

 # Valkey image based on Software Collections packages
 #
@@ -25,12 +25,11 @@
       io.k8s.description="$DESCRIPTION" \
       io.k8s.display-name="Valkey $VALKEY_VERSION" \
       io.openshift.expose-services="6379:valkey" \
-      io.openshift.tags="database,valkey,valkey,valkey-$VALKEY_VERSION" \
-      com.redhat.component="valkey-$VALKEY_VERSION-container" \
-      name="sclorg/valkey-$VALKEY_VERSION-c10s" \
+      io.openshift.tags="database,valkey,valkey$VALKEY_VERSION,valkey-$VALKEY_VERSION" \
+      com.redhat.component="$NAME" \
+      name="fedora/$NAME-$VALKEY_VERSION" \
       version="$VALKEY_VERSION" \
-      com.redhat.license_terms="https://www.redhat.com/en/about/red-hat-end-user-license-agreements#rhel" \
-      usage="podman run -d --name valkey_database -p 6379:6379 quay.io/sclorg/valkey-$VALKEY_VERSION-c10s" \
+      usage="podman run -d --name valkey_database -p 6379:6379 quay.io/fedora/$NAME-$VALKEY_VERSION" \
       maintainer="SoftwareCollections.org <[email protected]>"

 EXPOSE 6379
@@ -43,20 +42,20 @@
     VALKEY_LIB=/var/lib/valkey \
     VALKEY_RUN=/run/valkey

-
-# Create user for Valkey that has known UID
+# Create user for valkey that has known UID
 # We need to do this before installing the RPMs which would create user with random UID
 # The UID is the one used by the default user from the parent layer (1001),
 # and since the user exists already, do not create a new one, but only rename
 # the existing
-# This image must forever use UID 1001 for Valkey user so our volumes are
+# This image must forever use UID 1001 for valkey user so our volumes are
 # safe in the future. This should *never* change, the last test is there
 # to make sure of that.
-RUN getent group valkey &> /dev/null || groupadd -r valkey &> /dev/null && \
-    usermod -l valkey -aG valkey -c 'Valkey Server' default &> /dev/null && \
+RUN getent group  valkey &> /dev/null || groupadd -r valkey &> /dev/null && \
+    usermod -l valkey -aG valkey -c 'Server' default &> /dev/null && \
 # Install gettext for envsubst command
-    INSTALL_PKGS="policycoreutils gettext bind valkey" && \
-    dnf install -y --setopt=tsflags=nodocs $INSTALL_PKGS && \
+    dnf install -y dnf-utils gettext policycoreutils && \
+    INSTALL_PKGS="valkey" && \
+    dnf install -y --setopt=tsflags=nodocs --nogpgcheck $INSTALL_PKGS && \
     rpm -V $INSTALL_PKGS && \
     dnf -y clean all --enablerepo='*' && \
     valkey-server --version | grep -qe "^Server v=$VALKEY_VERSION\." && echo "Found VERSION $VALKEY_VERSION" && \
@@ -66,10 +65,6 @@
     chmod -R ug+rwX $VALKEY_LIB && \
     [[ "$(id valkey)" == "uid=1001(valkey)"* ]]

-# Get prefix path and path to scripts rather than hard-code them in scripts
-ENV CONTAINER_SCRIPTS_PATH=/usr/share/container-scripts/valkey \
-    VALKEY_PREFIX=/usr \
-    VALKEY_CONF=/etc/valkey/valkey.conf

 COPY root /

7/root/usr/bin/run-valkey Outdated Show resolved Hide resolved
@pkubatrh
Copy link
Member

Did a quick sweep and looks fine. But it would be nice to have someone with actual redis/valkey expertise to take a look as well.

@phracek phracek requested a review from remicollet August 16, 2024 10:46
@phracek phracek force-pushed the support_c10s branch 3 times, most recently from 76cb117 to a0063ab Compare August 16, 2024 11:37
Signed-off-by: Petr "Stone" Hracek <[email protected]>
@phracek
Copy link
Member Author

phracek commented Aug 16, 2024

[test]

@hhorak
Copy link
Member

hhorak commented Aug 16, 2024

Well done, @phracek, I didn't see a big issues (just a few left-overs of redis stuff, like v6 version in the imagestream) and think it can serve as a reasonable base like this. A similar conversation about imagestreams might be about templates -- whether there is a reason to maintain templates still, or whether helm charts can replace them.

Signed-off-by: Petr "Stone" Hracek <[email protected]>
@phracek
Copy link
Member Author

phracek commented Aug 19, 2024

[test]

@phracek
Copy link
Member Author

phracek commented Aug 19, 2024

@hhorak Helm charts will be added to repository https://github.com/sclorg/helm-charts. Good reminder.

The issue is already filed sclorg/helm-charts#88

@phracek phracek added enhancement New feature or request help wanted Extra attention is needed ready for review This label should be used when a PR is ready for review. labels Aug 19, 2024
@hhorak
Copy link
Member

hhorak commented Aug 19, 2024

@phracek the imagestreams still don't look correct -- I'd say we should either remove them entirely or convert to valkey, but keeping redis mentions there does not add any value IMO. Otherwise LGTM.

Signed-off-by: Petr "Stone" Hracek <[email protected]>
@phracek
Copy link
Member Author

phracek commented Aug 20, 2024

@phracek the imagestreams still don't look correct -- I'd say we should either remove them entirely or convert to valkey, but keeping redis mentions there does not add any value IMO. Otherwise LGTM.

I totally forget to remove it. It would be nice so that tool, who generates sources removes also json's that should not be preset. I will update it.

@phracek
Copy link
Member Author

phracek commented Aug 20, 2024

[test]

@phracek phracek merged commit 5789386 into master Aug 20, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed pr/failing-ci pr/missing-review ready for review This label should be used when a PR is ready for review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants