-
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] use sonic-db-cli PING and fix wrong multiDB API in NAT #4541
Conversation
until [[ $(/usr/bin/sonic-netns-exec "$NET_NS" sonic-db-cli PING | grep -c PONG) -gt 0 ]]; do | ||
sleep 1; | ||
done | ||
|
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.
@judyjoseph Could you please experiment with this part? It supposed to fix the corrupted config file issue. I wish your PR 4477 merge after this one.
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.
@judyjoseph Could you please experiment with this part? It supposed to fix the corrupted config file issue. I wish your PR 4477 merge after this one.
the corrupted config file issue will be fixed after the swsssdk submodule updated plus this PR
Advance sonic-py-swsssdk pointer #4496
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 am not sure if this will fix the issue I saw earlier. The issue I saw was that the config json files was present but empty as the database_config.json and database_global.json files are generated using "j2".
So now again sonic-db-cli PING, we will indirectly try to open the database_config.json/database_global.json file which could be in same state of "present but don't have contents" and json.load will fail.
I believe the solution would be some sort of synchronization between where we generate the json files ( which is in the database docker ) .. and the other scripts ..were we use to check with PING-PONG.
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 am not sure if this will fix the issue I saw earlier. The issue I saw was that the config json files was present but empty as the database_config.json and database_global.json files are generated using "j2".
So now again sonic-db-cli PING, we will indirectly try to open the database_config.json/database_global.json file which could be in same state of "present but don't have contents" and json.load will fail.
I believe the solution would be some sort of synchronization between where we generate the json files ( which is in the database docker ) .. and the other scripts ..were we use to check with PING-PONG.
IF open file failed or file context is not correct , sonic-db-cli PING won't return 'PONG' and the until loop continue to try. This behavior will be there after swsssdk submodule updated to latest
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.
Thanks, this logic with the sonic-db-cli PONG works correctly even though there are certain json load errors initially.
Btw I find there is no dependency on sonic-py-swsssdk pointer #4496. It works ok with the current PING/PONG implementation in the swsssdk submodule in sonic-buildimage. Please confirm if that is the case ?
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.
Thanks, this logic with the sonic-db-cli PONG works correctly even though there are certain json load errors initially.
Btw I find there is no dependency on sonic-py-swsssdk pointer #4496. It works ok with the current PING/PONG implementation in the swsssdk submodule in sonic-buildimage. Please confirm if that is the case ?
The logic is working, if something bad happened when loading json file, it not returning 'PONG' and keep trying in the loop. From functionality point of view, it does not depend on 4496. But there is a exception internally, after 4496 is merged , the exception is handled as a failure case. This is the only difference. So we can merge this PR for now , there is no functionality effect and will work as you said
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.
LGTM
Signed-off-by: Dong Zhang d.zhang@alibaba-inc.com