Skip to content

Fix ConfigDB load config crash with swss-common issue.#254

Closed
liuh-80 wants to merge 1 commit intosonic-net:masterfrom
liuh-80:dev/liuh/fix_configdb_init
Closed

Fix ConfigDB load config crash with swss-common issue.#254
liuh-80 wants to merge 1 commit intosonic-net:masterfrom
liuh-80:dev/liuh/fix_configdb_init

Conversation

@liuh-80
Copy link
Copy Markdown
Contributor

@liuh-80 liuh-80 commented Apr 28, 2022

- What I did
Fix following code issue:
When sonic-py-common change to use sonic-swss-common, sonic_ax_impl will crash because ConfigDB config load multiple times.

    The reason if this issue is because  multi_asic.is_multi_asic() will try load platform information from local environment variable and machine.conf, but if can't load information from local, is_multi_asic() will initialize default sonic DB config and connect to ConfigDB then read platform information.

- How I did it
Code change to load default config before check multi asic.

- How to verify it
Pass all UT and E2E test.

- Description for the changelog

@liuh-80 liuh-80 requested a review from qiluo-msft April 28, 2022 04:30

# Load default config first, because when is_multi_asic() can't get platform info from local config file, is_multi_asic() will connect to ConfigDB for platform information.
SonicDBConfig.load_sonic_db_config()
if multi_asic.is_multi_asic():
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here are more detail about this change:

Step1 to Step4 happened in https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-py-common/sonic_py_common/multi_asic.py

  1. is_multi_asic() will check asic count by get_num_asics()
  2. get_num_asics() need get asic config file path, so it will call get_asic_conf_file_path()
  3. get_asic_conf_file_path() need get current platform:
    platform = get_platform()
    if platform:
    asic_conf_path_candidates.append(os.path.join(HOST_DEVICE_PATH, platform, ASIC_CONF_FILENAME))
  4. then get_platform() will try read platfrom from "PLATFORM" environment variable and machine.conf, but if still can't get platform, it will connect to ConfigDB and read from "DEVICE_METADATA" table: get_localhost_info('platform')
  5. And when connect to ConfigDB, sonic-swss-common code will check if ConfigDB already initialized, if not it will load default config:

SonicDBInfo& SonicDBConfig::getDbInfo(conststd::string &dbName, conststd::string &netns)
{
std::lock_guardstd::recursive_mutex guard(m_db_info_mutex);

SWSS_LOG_ENTER();

if(!m_init)
    initialize(DEFAULT_SONIC_DB_CONFIG_FILE);

if(!netns.empty())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the stack tracing! The key issue is Step 3 and Step 4. I draft a PR sonic-net/sonic-buildimage#10723.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here is the latest code which still will connect to config DB and initialize it, so the code change in this PR still necessary:

def get_localhost_info(field, config_db=None):
try:
# TODO: enforce caller to provide config_db explicitly and remove its default value
if not config_db:
config_db = ConfigDBConnector()
config_db.connect()

    metadata = config_db.get_table('DEVICE_METADATA')

@@ -81,11 +82,16 @@ def get_machine_info():

return machine_vars

@liuh-80
Copy link
Copy Markdown
Contributor Author

liuh-80 commented May 24, 2022

Close this PR because the crash issue fixed by other PR.

@liuh-80 liuh-80 closed this May 24, 2022
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.

2 participants