-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
multiDB : add database_config.json into vs images #3757
Conversation
@qiluo-msft after this change, we can do the replacement on C++ APIs and old "redis-cli" wrapper without affect current vs tests running on single instance based on the database_config.json. Later we will change all vs tests to adapt multiple instances case. |
@@ -29,7 +29,8 @@ rm -f /var/run/rsyslogd.pid | |||
|
|||
supervisorctl start rsyslogd | |||
|
|||
mkdir -p /var/run/redis | |||
mkdir -p /var/run/redis/sonic-db | |||
cp /etc/default/sonic-db/database_config.json /var/run/redis/sonic-db/ |
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.
database_config.json [](start = 25, length = 20)
redis-server is not reading this file right now. #Pending
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.
yes. Let's replace the core C++ APIs and redis-cli scripts first. Then I'll go back to change the vs images redis-server stuff. For now , this change only make sure the vs tests are not broken when doing the replacement.
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.
This is still applicable. If I unblock now, it will be forgotten easily.
In reply to: 346558103 [](ancestors = 346558103)
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 can open an issue and assign it to me for reminder, at this moment I don't want to do this part first. Later when we change the vs tests, this is a must do stuff, it cannot be forgotten.
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.
I'd like to finish the core stuff first(those APIs in swss and redis-cli replacement), then the image can be run on DUT and we can start testing on local . After that we can focus on vs tests.
@@ -106,6 +106,7 @@ COPY ["files/configdb-load.sh", "/usr/bin/"] | |||
COPY ["files/arp_update", "/usr/bin/"] | |||
COPY ["files/buffers_config.j2", "files/qos_config.j2", "/usr/share/sonic/templates/"] | |||
COPY ["files/sonic_version.yml", "/etc/sonic/"] | |||
COPY ["database_config.json", "/etc/default/sonic-db/"] | |||
|
|||
# Workaround the tcpdump issue |
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.
Will you add
VOLUME /var/run/redis/sonic-db/
#Pending
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 don't need add VOULME again, it already added when we start the dvs in conftest.py.
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.
Dockerfile VOLUME
is different from docker run -v
. If you mark it as VOLUME
, anything already there (built into docker image) will be copied out to host when you mount. I believe this is useful in some test cases. #Closed
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 can verify and use this VOLUME MODE later. For now, there is nothing in /var/run/redis/ built into docker image. And today we only use this /var/run/redis after docker image is running. I prefer not to change current behavior which is not related to multiDB changes.
retest this please |
1 similar comment
retest this please |
@qiluo-msft Can we make this change merged first ? Then I can submit the C++ API replacement PR and won't break the vs test? |
like what we did in docker-database running on DUT, on vs image we should have the database_config.json in place as well
vs image has a default startup config at /etc/default/sonic-db/
after mount /var/run/redis, in start.sh we copy startup config to running config at /var/run/redis/sonic-db/
verified when running vs tests, we can see the file mounted at /var/run/redis-vs/{sw-name}/sonic-db on host which is mapping to /var/run/redis/sonic-db/ on vs