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.

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

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

Merged

Conversation

liuh-80
Copy link
Contributor

@liuh-80 liuh-80 commented Apr 3, 2024

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

Why I did it

Fix [issue @](https://github.com/sonic-net/sonic-buildimage/issues/18389)

How I did it

Revert code change by https://github.com/sonic-net/sonic-utilities/pull/3100
Check DB config initialize state and ignore when initialized.

How to verify it

Pass all UT.
Manually test.
Work item tracking
  • Microsoft ADO (number only): 27384235

Which release branch to backport (provide reason below if selected)

N/A

Description for the changelog

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

A picture of a cute animal (not mandatory but encouraged)

scripts/db_migrator.py Outdated Show resolved Hide resolved
ganglyu
ganglyu previously approved these changes Apr 3, 2024
@@ -1277,7 +1276,14 @@ def main():
socket_path = args.socket
namespace = args.namespace

load_db_config()
# Can't load global config base on the resoult of is_multi_asic(), because on multi-asic device, when db_migrate.py
Copy link
Contributor

@qiluo-msft qiluo-msft Apr 3, 2024

Choose a reason for hiding this comment

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

resoult

typo #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.

Fixed.

@@ -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

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.

@rlhui rlhui requested a review from judyjoseph April 3, 2024 17:54
SonicDBConfig.load_sonic_global_db_config(namespace=args.namespace)
else:
if not SonicDBConfig.isInit():
SonicDBConfig.load_sonic_db_config()

Copy link
Contributor

@judyjoseph judyjoseph Apr 4, 2024

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.

wumiaont added a commit to wumiaont/sonic-utilities that referenced this pull request Apr 22, 2024
…nfig or initialize(). This is a porting fix effort from master sonic-net#3257.
wumiaont added a commit to wumiaont/sonic-utilities that referenced this pull request Apr 22, 2024
…GlobalConfig or initialize(). This is a porting fix effort from master sonic-net#3257."

This reverts commit 5b281bb.
Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

Please check with other reviewers.

@liuh-80 liuh-80 merged commit cd5c058 into sonic-net:master Apr 25, 2024
5 checks passed
arfeigin pushed a commit to arfeigin/sonic-utilities that referenced this pull request Jun 16, 2024
…on on Linecard (sonic-net#3257)

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

#### Why I did it
    Fix [issue @](sonic-net/sonic-buildimage#18389)

#### How I did it
    Revert code change by sonic-net#3100
    Check DB config initialize state and ignore when initialized.

#### How to verify it
    Pass all UT.
    Manually test.

##### Work item tracking
- Microsoft ADO **(number only)**: 27384235

#### Which release branch to backport (provide reason below if selected)
    N/A

#### Description for the changelog
    Fix db_migrate.py show error and back trace while loading configuration on Linecard

#### A picture of a cute animal (not mandatory but encouraged)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants