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

Add redis cli to nightly image #1415

Merged
merged 5 commits into from
May 5, 2023

Conversation

ColinChamber
Copy link
Contributor

solve #1376

Hi @PragmaTwice
We have been working on improving kvrocks-operator recently and have found that many redis-operators use redis-cli for Kubernetes liveness probes. Although we can use commands like 'echo "PING" | nc localhost 6666', it can be a bit tricky. Additionally, redis-cli is more user-friendly for operations and makes it convenient to query service information within the container. In complex cloud network environments with audit policies, it is not possible for redis-cli from any node to connect to the redis-server

I pushed the image to my own repository and ran some tests.
image
image

Dockerfile Outdated
@@ -37,6 +37,10 @@ WORKDIR /kvrocks

COPY --from=build /kvrocks/build/kvrocks ./bin/

ARG TARGETARCH
COPY tools/redis-cli-${TARGETARCH} /usr/bin/redis-cli
RUN chmod a+x /usr/bin/redis-cli
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO if a user run docker build towards this dockerfile in him/her device instead of CI, a fatal error will appear since there is no tools dir.

So I think It is not a reasonable change.

Copy link
Member

@PragmaTwice PragmaTwice May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I want to emphasize my point: the dockerfile cannot have different behavior between built by a user and triggered in the CI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these conditional branches related to arch in the current changes is not necessary. You can just build redis-cli in the dockerfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@ColinChamber ColinChamber requested a review from PragmaTwice May 5, 2023 07:06
Dockerfile Outdated
@@ -37,6 +44,9 @@ WORKDIR /kvrocks

COPY --from=build /kvrocks/build/kvrocks ./bin/

COPY --from=build /kvrocks/tools/redis-cli ./bin/
RUN ln -s /kvrocks/bin/redis-cli /usr/bin/redis-cli
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to add this symlink?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/kvrocks/bin is not in the PATH environment variable, so I created a symlink. Are you suggesting that I should either copy it directly to /usr/bin or add /kvrocks/bin to the PATH?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the latter is better.

@ColinChamber ColinChamber requested a review from PragmaTwice May 5, 2023 11:49
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@PragmaTwice
Copy link
Member

Thanks @ColinChamber , merging...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants