Skip to content
This repository has been archived by the owner on Mar 2, 2024. It is now read-only.

Fix command for manually running redis #6

Conversation

mariasfiraiala
Copy link
Contributor

Signed-off-by: Maria Sfiraiala [email protected]

@razvand razvand self-requested a review August 1, 2022 13:46
@razvand razvand self-assigned this Aug 1, 2022
Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Hi, @mariasfiraiala . What is this useful for? I was able to run the Redis Unikraft application with the initial command.

Your version of the command is required only if the Redis application is built with 9pfs support when running:

make menuconfig

But 9pfs support is not required. Maybe there should be a fix in the documentation of lib-redis to point out that 9pfs support is not required (unlike lib-nginx - referenced there).

@mariasfiraiala
Copy link
Contributor Author

Alright then, closing this PR. I'll take a look into the documentation for lib-redis.

@razvand razvand reopened this Aug 4, 2022
@razvand
Copy link
Contributor

razvand commented Aug 4, 2022

Hi, @mariasfiraiala . Based on our discussion, this PR is indeed useful. We require a redis.conf configuration file to be passed to Redis, and that needs to be done via a filesystem (such as 9pfs).

Please update this commit with the fs0/redis.conf file.

This redis.conf file worked for me, maybe you can simplify it:

bind 172.44.0.2
port 6379
tcp-backlog 511
timeout 0
tcp-keepalive 300

@razvand razvand self-requested a review August 4, 2022 15:22
@razvand
Copy link
Contributor

razvand commented Aug 4, 2022

Also add instructions on adding 9pfs support with the proper tag (rootfs in the command above), similar to the lib-nginx documentation. You could also add this to the lib-redis repository for consistency.

I think consistency is important, so I would aim to make Redis and Nginx as close to each other as possible, documentation-wise.

@mariasfiraiala mariasfiraiala force-pushed the fix-manually-run-command branch 2 times, most recently from f3b40da to 3410155 Compare August 4, 2022 16:27
@razvand razvand added the enhancement New feature or request label Nov 9, 2022
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Only one comment, looks good besides that.

README.md Outdated Show resolved Hide resolved
@mariasfiraiala
Copy link
Contributor Author

Only one comment, looks good besides that.

Thank you @StefanJum . Removed it.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Few more comments.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

One more little comment.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

All good, thank you @mariasfiraiala.
Reviewed-by: Stefan Jumarea [email protected]

@razvand
Copy link
Contributor

razvand commented Nov 10, 2022

@mariasfiraiala , please mark solved items with Resolve conversation and re-request reviews from reviewers (@StefanJum in this case).

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

This works. Thanks, @mariasfiraiala

Reviewed-by: Razvan Deaconescu [email protected]
Approved-by: Razvan Deaconescu [email protected]

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ci/merged enhancement New feature or request
Projects
Status: Done
Status: Done!
Development

Successfully merging this pull request may close these issues.

4 participants