Skip to content
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

Jump to bottom

Fix db_migrate.py show error and back trace while loading configuration on Linecard #3257

Merged
merged 17 commits into from
Apr 25, 2024
Merged

Fix db_migrate.py show error and back trace while loading configuration on Linecard #3257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2469d6f
Fix multi-asic crash issue
liuh-80 Mar 29, 2024
5c92d04
Improve test coverage
liuh-80 Apr 3, 2024
1c0c074
Imporve UT
liuh-80 Apr 3, 2024
0cebcab
Improve test case
liuh-80 Apr 3, 2024
e4b5c9e
Remove unused import
liuh-80 Apr 3, 2024
b4e03b9
Fix import
liuh-80 Apr 3, 2024
c812b74
Fix UT
liuh-80 Apr 3, 2024
bbc2496
Fix test case
liuh-80 Apr 3, 2024
1652c65
Fix test case
liuh-80 Apr 3, 2024
e17e4a2
Improve testcase
liuh-80 Apr 3, 2024
ce14d3e
Improve UT
liuh-80 Apr 3, 2024
0154882
Improve UT
liuh-80 Apr 3, 2024
40c2f21
Improve UT
liuh-80 Apr 3, 2024
1f41eb3
Improve UT
liuh-80 Apr 3, 2024
4b13a57
Update db_migrator.py
liuh-80 Apr 7, 2024
3930718
Update db_migrator.py
liuh-80 Apr 12, 2024
d28cffe
Update db_migrator_test.py
liuh-80 Apr 12, 2024
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
12 changes: 9 additions & 3 deletions scripts/db_migrator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@
import re

from sonic_py_common import device_info, logger
from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector
from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector, SonicDBConfig
from minigraph import parse_xml
from utilities_common.helper import update_config
from utilities_common.general import load_db_config

INIT_CFG_FILE = '/etc/sonic/init_cfg.json'
MINIGRAPH_FILE = '/etc/sonic/minigraph.xml'
Expand Down Expand Up @@ -1277,7 +1276,14 @@ def main():
socket_path = args.socket
namespace = args.namespace

load_db_config()
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

load_db_config

Is it a general issue? If yes, do we need to fix them all and destroy this function? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a general issue, according to the code of this script and Judy's comments, this script seems designed to be run before ASIC instance database0 and database1 generate database-config.json.
For other code using load_db_config this issue should not happen.

# Can't load global config base on the result of is_multi_asic(), because on multi-asic device, when db_migrate.py
# run on the local database, ASIC instance will have not created the /var/run/redis0/sonic-db/database-config.json
if args.namespace is not None:
if not SonicDBConfig.isGlobalInit():
SonicDBConfig.initializeGlobalConfig()
else:
if not SonicDBConfig.isInit():
SonicDBConfig.initialize()

Copy link
Contributor

@judyjoseph judyjoseph Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I feel it is more like a timing issue as you pointed out, Can we call db_migrator, after /var/run/redis0/sonic-db/database-config.json is created ?

Here, https://github.com/sonic-net/sonic-swss-common/blob/7db6ccf5fb427d4832d6c54ab1f485ddd87b6697/common/dbconnector.h#L107 -- it doesn't make a difference if we pass/don't pass the namespace parameter to load_sonic_global_db_config()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I search sonic code but not found any code use the '-n' parameter of db_migrator.py to pass the namespace parameter.
I will add comments in the sonic-net/sonic-buildimage#18389 to ask author check if this can fix by your suggestion.

For load_sonic_global_db_config(namespace=args.namespace), I think it's better keep the parameter to keep the compatibility, incase we change the code of load_sonic_global_db_config in future.

Copy link
Contributor

Choose a reason for hiding this comment

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

@liuh-80 @judyjoseph all instance database@.service have dependency "Requires=database.service
After=database.service". db_mihgrator script in local host database will not be able to wait for all other instance ready unless we change the dependency.

if socket_path:
dbmgtr = DBMigrator(namespace, socket=socket_path)
Expand Down
18 changes: 17 additions & 1 deletion tests/db_migrator_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from unittest import mock
from deepdiff import DeepDiff

from swsscommon.swsscommon import SonicV2Connector
from swsscommon.swsscommon import SonicV2Connector, SonicDBConfig
from sonic_py_common import device_info

from .mock_tables import dbconnector
Expand Down Expand Up @@ -889,6 +889,22 @@ def test_init(self, mock_args):
import db_migrator
db_migrator.main()

@mock.patch('argparse.ArgumentParser.parse_args')
@mock.patch('swsscommon.swsscommon.SonicDBConfig.isInit', mock.MagicMock(return_value=False))
@mock.patch('swsscommon.swsscommon.SonicDBConfig.initialize', mock.MagicMock())
def test_init_no_namespace(self, mock_args):
mock_args.return_value=argparse.Namespace(namespace=None, operation='version_202405_01', socket=None)
import db_migrator
db_migrator.main()

@mock.patch('argparse.ArgumentParser.parse_args')
@mock.patch('swsscommon.swsscommon.SonicDBConfig.isGlobalInit', mock.MagicMock(return_value=False))
@mock.patch('swsscommon.swsscommon.SonicDBConfig.initializeGlobalConfig', mock.MagicMock())
def test_init_namespace(self, mock_args):
mock_args.return_value=argparse.Namespace(namespace="asic0", operation='version_202405_01', socket=None)
import db_migrator
db_migrator.main()


class TestGNMIMigrator(object):
@classmethod
Expand Down
Loading