Skip to content

Use unix socket path in ConfigDBConnector for some components #10179

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

Closed
wants to merge 4 commits into from
Closed

Use unix socket path in ConfigDBConnector for some components #10179

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89fa106
Use unix socket path in ConfigDBConnector for some components
stepanblyschak Feb 21, 2022
aefff93
Another place added where unix sock usage is required
stepanblyschak Mar 21, 2022
78eee06
add another use_unix_socket_path
stepanblyschak Mar 28, 2022
8a79393
add param for connect_configdb_for_ns
stepanblyschak Mar 30, 2022
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
2 changes: 1 addition & 1 deletion src/sonic-host-services/scripts/hostcfgd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,7 +954,7 @@ class HostConfigDaemon:
def __init__(self):
# Just a sanity check to verify if the CONFIG_DB has been initialized
# before moving forward
self.config_db = ConfigDBConnector()
self.config_db = ConfigDBConnector(use_unix_socket_path=True)
self.config_db.connect(wait_for_init=True, retry_on=True)
self.dbconn = DBConnector(CFG_DB, 0)
self.selector = Select()
Expand Down
4 changes: 2 additions & 2 deletions src/sonic-py-common/sonic_py_common/multi_asic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def connect_config_db_for_ns(namespace=DEFAULT_NAMESPACE):
Returns:
handle to the config_db for a namespace
"""
config_db = swsscommon.ConfigDBConnector(namespace=namespace)
config_db = swsscommon.ConfigDBConnector(use_unix_socket_path=True, namespace=namespace)
Copy link
Collaborator

@qiluo-msft qiluo-msft Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

use_unix_socket_path

Since this is a library function, it will impact many applications. One impact that the new code requires sudo for the application.

To make migration easier, how about add a parameter to connect_config_db_for_ns() and by default do not use unix socket. And you can migrate the applications one by one. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@qiluo-msft In my test use_unix_socket_path does not require sudo:

admin@r-tigon-20:~$ python
Python 3.9.2 (default, Feb 28 2021, 17:03:44)
[GCC 10.2.1 20210110] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from swsscommon.swsscommon import ConfigDBConnector
>>> c=ConfigDBConnector(use_unix_socket_path=True)
>>> c.connect()
>>>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree. Still application may still want to customize the connection method. Could you add a parameter to connect_config_db_for_ns() and by default do not use unix socket. And you can migrate the applications one by one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@qiluo-msft In some cases it is not possible to do. Considering an application want's to import from a library:

>>> from utilities_common.db import Db
^C
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.9/dist-packages/utilities_common/db.py", line 4, in <module>
    from utilities_common.multi_asic import multi_asic_ns_choices
  File "/usr/local/lib/python3.9/dist-packages/utilities_common/multi_asic.py", line 102, in <module>
    default=multi_asic_display_default_option(),
  File "/usr/local/lib/python3.9/dist-packages/utilities_common/multi_asic.py", line 93, in multi_asic_display_default_option
    if not multi_asic.is_multi_asic() and not device_info.is_chassis():
  File "/usr/local/lib/python3.9/dist-packages/sonic_py_common/device_info.py", line 427, in is_chassis
    return is_voq_chassis() or is_packet_chassis()
  File "/usr/local/lib/python3.9/dist-packages/sonic_py_common/device_info.py", line 417, in is_voq_chassis
    switch_type = get_platform_info().get('switch_type')
  File "/usr/local/lib/python3.9/dist-packages/sonic_py_common/device_info.py", line 353, in get_platform_info
    hw_info_dict['hwsku'] = get_hwsku()
  File "/usr/local/lib/python3.9/dist-packages/sonic_py_common/device_info.py", line 126, in get_hwsku
    return get_localhost_info('hwsku')
  File "/usr/local/lib/python3.9/dist-packages/sonic_py_common/device_info.py", line 47, in get_localhost_info
    config_db.connect()
  File "/usr/local/lib/python3.9/dist-packages/swsssdk/configdb.py", line 84, in connect
    self.db_connect('CONFIG_DB', wait_for_init, retry_on)
  File "/usr/local/lib/python3.9/dist-packages/swsssdk/configdb.py", line 79, in db_connect
    SonicV2Connector.connect(self, self.db_name, retry_on)
  File "/usr/local/lib/python3.9/dist-packages/swsssdk/dbconnector.py", line 268, in connect
    self.dbintf.connect(db_id, db_name, retry_on)
  File "/usr/local/lib/python3.9/dist-packages/swsssdk/interface.py", line 175, in connect
    self._onetime_connect(db_id, db_name)
  File "/usr/local/lib/python3.9/dist-packages/swsssdk/interface.py", line 192, in _onetime_connect
    client.config_set('notify-keyspace-events', self.KEYSPACE_EVENTS)
  File "/usr/local/lib/python3.9/dist-packages/redis/client.py", line 1243, in config_set
    return self.execute_command('CONFIG SET', name, value)
  File "/usr/local/lib/python3.9/dist-packages/redis/client.py", line 898, in execute_command
    conn = self.connection or pool.get_connection(command_name, **options)
  File "/usr/local/lib/python3.9/dist-packages/redis/connection.py", line 1192, in get_connection
    connection.connect()
  File "/usr/local/lib/python3.9/dist-packages/redis/connection.py", line 559, in connect
    sock = self._connect()
  File "/usr/local/lib/python3.9/dist-packages/redis/connection.py", line 603, in _connect
    sock.connect(socket_address)
KeyboardInterrupt

This will connect to DB using TCP. But the question is, how an application doing import needs to tell the library to use unix socket. One may add a boolean flag before doing import but this is a hack which might not work actually when the import is done from another place. I don't think executing much code like connecting to a database during import time is correct for general purpose sonic library. So I would need to refactor that part, which is actually a bit of work in an area which I do not have the right expertise (chassis, multi-asic, etc.). A similar story for the multi_asic.py.
Considering that I don't see a problem using unix socket now (the sudo is no more required) I would prefer to change the default in the library.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to clarify: I am asking adding a parameter to connect_config_db_for_ns(), not get_platform_info().
I agree with you on the case study of get_platform_info().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@qiluo-msft I see, only for connect_config_db_for_ns it is possible. I fixed that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I double checked and there should be permission issue. Only checked swsssdk library. But swsscommon should be the same. Make sure you could not read the file /var/run/redis/redis.sock. So the suggestion is to keep the library behave the old way if there is no explicit option.

>>> c.connect()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/dist-packages/swsssdk/configdb.py", line 74, in connect
    self.db_connect('CONFIG_DB', wait_for_init, retry_on)
  File "/usr/local/lib/python2.7/dist-packages/swsssdk/configdb.py", line 69, in db_connect
    SonicV2Connector.connect(self, self.db_name, retry_on)
  File "/usr/local/lib/python2.7/dist-packages/swsssdk/dbconnector.py", line 250, in connect
    self.dbintf.connect(db_id, retry_on)
  File "/usr/local/lib/python2.7/dist-packages/swsssdk/interface.py", line 171, in connect
    self._onetime_connect(db_id)
  File "/usr/local/lib/python2.7/dist-packages/swsssdk/interface.py", line 183, in _onetime_connect
    client.config_set('notify-keyspace-events', self.KEYSPACE_EVENTS)
  File "/usr/local/lib/python2.7/dist-packages/redis/client.py", line 719, in config_set
    return self.execute_command('CONFIG SET', name, value)
  File "/usr/local/lib/python2.7/dist-packages/redis/client.py", line 673, in execute_command
    connection.send_command(*args)
  File "/usr/local/lib/python2.7/dist-packages/redis/connection.py", line 610, in send_command
    self.send_packed_command(self.pack_command(*args))
  File "/usr/local/lib/python2.7/dist-packages/redis/connection.py", line 585, in send_packed_command
    self.connect()
  File "/usr/local/lib/python2.7/dist-packages/redis/connection.py", line 489, in connect
    raise ConnectionError(self._error_message(e))
redis.exceptions.ConnectionError: Error 13 connecting to unix socket: /var/run/redis/redis.sock. Permission denied.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@qiluo-msft what is your SONiC version ?

config_db.connect()
return config_db

Expand All @@ -65,7 +65,7 @@ def connect_to_all_dbs_for_ns(namespace=DEFAULT_NAMESPACE):
Returns:
handle to all the dbs for a namespaces
"""
db = swsscommon.SonicV2Connector(namespace=namespace)
db = swsscommon.SonicV2Connector(use_unix_socket_path=True, namespace=namespace)
db_list = list(db.get_db_list())
if not is_supervisor():
try:
Expand Down