Skip to content

Force use unix domain socket when namespace not empty #797

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.

Sign up for GitHub

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

Merged
qiluo-msft merged 7 commits into from
Sep 11, 2023
Merged

Force use unix domain socket when namespace not empty #797

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c2b282c
Force use UDS when namespace not empty
liuh-80 Jun 16, 2023
e71877a
Merge branch 'master' into dev/liuh/fix-cli-connection
qiluo-msft Jun 29, 2023
5a52108
Fix code issue
liuh-80 Jul 14, 2023
598d77c
Merge remote-tracking branch 'origin' into dev/liuh/fix-cli-connection
liuh-80 Jul 14, 2023
cfddc57
Fix UT socket config issue
liuh-80 Jul 14, 2023
90ed89f
Merge remote-tracking branch 'origin' into dev/liuh/fix-cli-connection
liuh-80 Aug 3, 2023
e04a24c
Fix PR comments
liuh-80 Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 19 additions & 22 deletions sonic-db-cli/sonic-db-cli.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,17 @@ string handleSingleOperation(
const string& netns,
const string& db_name,
const string& operation,
bool isTcpConn)
bool useUnixSocket)
{
shared_ptr<DBConnector> client;
auto host = SonicDBConfig::getDbHostname(db_name, netns);
string message = "Could not connect to Redis at " + host + ":";
try
{
auto db_id = SonicDBConfig::getDbId(db_name, netns);
if (!isTcpConn && db_name != "redis_chassis.server")
if (useUnixSocket && db_name != "redis_chassis.server")
{
auto db_socket = SonicDBConfig::getDbSock(db_name);
auto db_socket = SonicDBConfig::getDbSock(db_name, netns);
message += db_name + ": Connection refused";
client = make_shared<DBConnector>(db_id, db_socket, 0);
}
Expand Down Expand Up @@ -89,15 +89,15 @@ string handleSingleOperation(
int handleAllInstances(
const string& netns,
const string& operation,
bool isTcpConn)
bool useUnixSocket)
{
auto db_names = SonicDBConfig::getDbList(netns);
// Operate All Redis Instances in Parallel
// TODO: if one of the operations failed, it could fail quickly and not necessary to wait all other operations
list<future<string>> responses;
for (auto& db_name : db_names)
{
future<string> response = std::async(std::launch::async, handleSingleOperation, netns, db_name, operation, isTcpConn);
future<string> response = std::async(std::launch::async, handleSingleOperation, netns, db_name, operation, useUnixSocket);
responses.push_back(std::move(response));
}

Expand Down Expand Up @@ -133,22 +133,22 @@ int executeCommands(
const string& db_name,
vector<string>& commands,
const string& netns,
bool isTcpConn)
bool useUnixSocket)
{
shared_ptr<DBConnector> client = nullptr;
try
{
int db_id = SonicDBConfig::getDbId(db_name, netns);
if (isTcpConn)
if (useUnixSocket)
{
auto host = SonicDBConfig::getDbHostname(db_name, netns);
auto port = SonicDBConfig::getDbPort(db_name, netns);
client = make_shared<DBConnector>(db_id, host, port, 0);
auto db_socket = SonicDBConfig::getDbSock(db_name, netns);
client = make_shared<DBConnector>(db_id, db_socket, 0);
}
else
{
auto db_socket = SonicDBConfig::getDbSock(db_name);
client = make_shared<DBConnector>(db_id, db_socket, 0);
auto host = SonicDBConfig::getDbHostname(db_name, netns);
auto port = SonicDBConfig::getDbPort(db_name, netns);
client = make_shared<DBConnector>(db_id, host, port, 0);
}
}
catch (const exception& e)
Expand Down Expand Up @@ -279,19 +279,16 @@ int sonic_db_cli(
{
auto dbOrOperation = options.m_db_or_op;
auto netns = options.m_namespace;
bool isTcpConn = !options.m_unixsocket;
bool useUnixSocket = options.m_unixsocket;
// Load the database config for the namespace
if (!netns.empty())
{
initializeGlobalConfig();

// Use the unix domain connectivity if namespace not empty.
useUnixSocket = true;
}
else
{
// Use the tcp connectivity if namespace is local and unixsocket cmd_option is present.
isTcpConn = true;
netns = "";
}


if (options.m_cmd.size() != 0)
{
auto commands = options.m_cmd;
Expand All @@ -301,7 +298,7 @@ int sonic_db_cli(
initializeConfig();
}

return executeCommands(dbOrOperation, commands, netns, isTcpConn);
return executeCommands(dbOrOperation, commands, netns, useUnixSocket);
}
else if (dbOrOperation == "PING"
|| dbOrOperation == "SAVE"
Expand All @@ -317,7 +314,7 @@ int sonic_db_cli(
initializeConfig();
}

return handleAllInstances(netns, dbOrOperation, isTcpConn);
return handleAllInstances(netns, dbOrOperation, useUnixSocket);
}
catch (const exception& e)
{
Expand Down
2 changes: 1 addition & 1 deletion tests/redis_multi_db_ut_config/database_config0.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"redis":{
"hostname" : "127.0.0.1",
"port": 6379,
"unix_socket_path": "/var/run/redis0/redis.sock"
"unix_socket_path": "/var/run/redis/redis.sock"
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

redis

why changing test data? #Closed

Copy link
Copy Markdown
Contributor Author

@liuh-80 liuh-80 Aug 3, 2023

Choose a reason for hiding this comment

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

This value is not correct.
Our UT never using unix_socket_path before.
But when change sonic-db-cli to user unix domian socket. some UT failed, so need this this value.

Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft Aug 4, 2023

Choose a reason for hiding this comment

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

I agree that we need to fix test code/data if it just start under testing.
This is used in multi-db feature, and you can see a pattern that database_configN.json will have a path /var/run/redisN/redis.sock. So if it is not working due to missing sock file, we need to start multiple redis-server, each with proper config on sock files.

Copy link
Copy Markdown
Contributor Author

@liuh-80 liuh-80 Aug 31, 2023

Choose a reason for hiding this comment

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

Fixed, keeps the original config no change and add new config file for asic2 and asic3.

}
},
"DATABASES" : {
Expand Down