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

[db_migrator] Remove hardcoded config and migrate config from minigraph #2887

Merged
merged 8 commits into from
Jun 29, 2023
Merged

[db_migrator] Remove hardcoded config and migrate config from minigraph #2887

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3a85220
[db migrator] Remove hardcoded migration and replace w/ config from m…
vaibhavhd Jun 26, 2023
7ae4c94
Remove routing config mode from this PR
vaibhavhd Jun 27, 2023
6e28286
Add sample minigraph.xml for UT
vaibhavhd Jun 28, 2023
2b66fa3
Fix test failures
vaibhavhd Jun 28, 2023
fcfd1fb
Fix test input
vaibhavhd Jun 28, 2023
94bc8bd
Review addressed: added a UT for missing minigraph
vaibhavhd Jun 29, 2023
126c187
Fix exception handling
vaibhavhd Jun 29, 2023
2240d10
Remove unwanted line from unit test
vaibhavhd Jun 29, 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
26 changes: 23 additions & 3 deletions scripts/db_migrator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@

from sonic_py_common import device_info, logger
from swsscommon.swsscommon import SonicV2Connector, ConfigDBConnector, SonicDBConfig
from db_migrator_constants import RESTAPI, TELEMETRY, CONSOLE_SWITCH
from minigraph import parse_xml

INIT_CFG_FILE = '/etc/sonic/init_cfg.json'
MINIGRAPH_FILE = '/etc/sonic/minigraph.xml'

# mock the redis for unit test purposes #
try:
Expand All @@ -22,6 +23,7 @@
sys.path.insert(0, modules_path)
sys.path.insert(0, tests_path)
INIT_CFG_FILE = os.path.join(mocked_db_path, "init_cfg.json")
MINIGRAPH_FILE = os.path.join(mocked_db_path, "minigraph.xml")
except KeyError:
pass

Expand Down Expand Up @@ -51,6 +53,12 @@ def __init__(self, namespace, socket=None):
self.TABLE_KEY = 'DATABASE'
self.TABLE_FIELD = 'VERSION'

# load config data from minigraph to get the default/hardcoded values from minigraph.py
# this is to avoid duplicating the hardcoded these values in db_migrator
self.minigraph_data = None
if os.path.isfile(MINIGRAPH_FILE):
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

isfile

Could you add a testcase for missing MINIGRAPH_FILE? It would be even better if you can add a testcase for L2 switch mode #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.

ack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new testcase for missing minigraph. We can discuss what other fields are expected/unexpected in L2 switch's case and add a test for that in future PR. What do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test for L2 switch in future is okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per offline discussion: from unit test level the test for missing minigraph and absence of default entries is sufficient. L2 switch config and functionality needs to be tested separately as part of sonic-mgmt test. Raised a test gap for this: sonic-net/sonic-mgmt#8777

self.minigraph_data = parse_xml(MINIGRAPH_FILE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to protect against parse_xml throwing error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Added now.


db_kwargs = {}
if socket:
db_kwargs['unix_socket_path'] = socket
Expand Down Expand Up @@ -527,6 +535,9 @@ def migrate_vxlan_config(self):

def migrate_restapi(self):
# RESTAPI - add missing key
if not self.minigraph_data or 'RESTAPI' not in self.minigraph_data:
return
RESTAPI = self.minigraph_data['RESTAPI']
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

RESTAPI

RESTAPI -> restapi_data. It's not a constant anymore. Many occurences in this file. #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.

Updated now

log.log_notice('Migrate RESTAPI configuration')
config = self.configDB.get_entry('RESTAPI', 'config')
if not config:
Expand All @@ -537,6 +548,9 @@ def migrate_restapi(self):

def migrate_telemetry(self):
# TELEMETRY - add missing key
if not self.minigraph_data or 'TELEMETRY' not in self.minigraph_data:
return
TELEMETRY = self.minigraph_data['TELEMETRY']
log.log_notice('Migrate TELEMETRY configuration')
gnmi = self.configDB.get_entry('TELEMETRY', 'gnmi')
if not gnmi:
Expand All @@ -547,6 +561,9 @@ def migrate_telemetry(self):

def migrate_console_switch(self):
# CONSOLE_SWITCH - add missing key
if not self.minigraph_data or 'CONSOLE_SWITCH' not in self.minigraph_data:
return
CONSOLE_SWITCH = self.minigraph_data['CONSOLE_SWITCH']
log.log_notice('Migrate CONSOLE_SWITCH configuration')
console_mgmt = self.configDB.get_entry('CONSOLE_SWITCH', 'console_mgmt')
if not console_mgmt:
Expand All @@ -555,10 +572,13 @@ def migrate_console_switch(self):

def migrate_device_metadata(self):
# DEVICE_METADATA - synchronous_mode entry
log.log_notice('Migrate DEVICE_METADATA missing configuration (synchronous_mode=enable)')
if not self.minigraph_data or 'DEVICE_METADATA' not in self.minigraph_data:
return
log.log_notice('Migrate DEVICE_METADATA missing configuration')
metadata = self.configDB.get_entry('DEVICE_METADATA', 'localhost')
DEVICE_METADATA = self.minigraph_data["DEVICE_METADATA"]["localhost"]
if 'synchronous_mode' not in metadata:
metadata['synchronous_mode'] = 'enable'
metadata['synchronous_mode'] = DEVICE_METADATA.get("synchronous_mode")
self.configDB.set_entry('DEVICE_METADATA', 'localhost', metadata)

def migrate_port_qos_map_global(self):
Expand Down
32 changes: 0 additions & 32 deletions scripts/db_migrator_constants.py
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@
'scripts/coredump-compress',
'scripts/configlet',
'scripts/db_migrator.py',
'scripts/db_migrator_constants.py',
'scripts/decode-syseeprom',
'scripts/dropcheck',
'scripts/disk_check.py',
Expand Down
6 changes: 3 additions & 3 deletions tests/db_migrator_input/config_db/cross_branch_upgrade_to_version_2_0_2_expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
},
"RESTAPI|certs": {
"server_key": "/etc/sonic/credentials/restapiserver.key",
"ca_crt": "/etc/sonic/credentials/AME_ROOT_CERTIFICATE.pem",
"server_crt": "/etc/sonic/credentials/restapiserver.crt",
"client_crt_cname": "client.restapi.sonic.gbl"
"ca_crt": "/etc/sonic/credentials/restapica.crt",
"server_crt": "/etc/sonic/credentials/restapiserver.crt",
"client_crt_cname": "client.restapi.sonic"
},
"TELEMETRY|gnmi": {
"client_auth": "true",
Expand Down
29 changes: 29 additions & 0 deletions tests/db_migrator_input/minigraph.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<DeviceMiniGraph xmlns="Microsoft.Search.Autopilot.Evolution" xmlns:i="http://www.w3.org/2001/XMLSchema-instance">
<CpgDec/>
<DpgDec>
<DeviceDataPlaneInfo>
<LoopbackIPInterfaces/>
<ManagementIPInterfaces/>
<Hostname>SONiC-Dummy</Hostname>
<PortChannelInterfaces/>
<VlanInterfaces/>
<IPInterfaces/>
<AclInterfaces/>
</DeviceDataPlaneInfo>
</DpgDec>
<PngDec>
<Devices>
<Device i:type="TorRouter">
<Hostname>SONiC-Dummy</Hostname>
</Device>
</Devices>
</PngDec>
<MetadataDeclaration>
<Devices xmlns:a="http://schemas">
<a:DeviceMetadata>
<a:Properties/>
</a:DeviceMetadata>
</Devices>
</MetadataDeclaration>
<Hostname>SONiC-Dummy</Hostname>
</DeviceMiniGraph>