hostcfgd service enhancements to support Management interface config handling in SONiC#4546
hostcfgd service enhancements to support Management interface config handling in SONiC#4546rvasanthm wants to merge 1 commit intosonic-net:masterfrom
Conversation
…f Management port config(MTU, Speed, auto negotiation, admin status and Description config attributes), 2. Added support for Dynamic handling of Management interface config (IPv4/IPv6 DHCP client, IPv4/IPv6 default gateway, IPv4/IPv6 addresses config and respective gateways config attributes) and Management VRF handling.
| echo "dhcp_mgmt_conf_handle, interface : $IF_NAME" | ||
|
|
||
| if [ "$IF_NAME" = "eth0" ]; then | ||
| echo "DHCP exit hook is called for $IF_NAME, reason : $reason" |
There was a problem hiding this comment.
Can you updated this echo "... called for $IF_NAME for updating ip addr in app db, reason : $reason" ?
| @@ -0,0 +1,53 @@ | |||
| #!/bin/sh | |||
| # | |||
| # DHCLIENT exit hook for ip address update in app db | |||
There was a problem hiding this comment.
please add some more comments to explain why this information needs to be added in APP DB.
| fi | ||
| ;; | ||
| RENEW|REBIND|RENEW6|REBIND6) | ||
| if [ -n "$old_ip_address" ] && [ -n "$old_subnet_mask" ]; then |
There was a problem hiding this comment.
Can you add a comment to explain where and how the old_ip_address and old_subnet_mask are updated?
|
|
||
| ifdown --force eth0 | ||
|
|
||
| # Check if ZTP DHCP policy has been installed |
There was a problem hiding this comment.
Is there a reason to remove the "ifdown --force eth0"? This line was present in this file from beginning. Are you sure that removal of this line wont affect any functionality?
|
|
||
| return | ||
|
|
||
| def handle_mgmt_intf_gwaddr_cfg(self, op, ifname, gwaddr): |
There was a problem hiding this comment.
I think its better to rename all functions related to management VRF using a prefix "mvrf". For example, this def "handle_mgmt_intf_gwaddr_cfg" is actually adding default route only for the management VRF case. Hence, its better to rename this to mvrf_configure_default_route. By the by, there is already a def "mgmt_intf_gwaddr_set",which handles default route. Is there a need for a separate function for "handle_mgmt_intf_gwaddr_cfg"?
|
|
||
| return rc | ||
|
|
||
| def handle_mgmt_intf_creation(self, mgmt_intf_key): |
There was a problem hiding this comment.
I dont see this def being used in code. Can we remove? If we remove, the other def mgmt_vrf_add_default_routes can also be removed. Or, is the calling place for this function missed out? When mvrf is created, there should be a call for this to add default routes and also for taking all other actions as given in interfaces.j2 earlier.
| entry = self.config_db.get_entry(CFG_MGMT_VRF, CFG_MGMT_VRF_KEY) | ||
| if entry: | ||
| if (('mgmtVrfEnabled' in entry) and (entry['mgmtVrfEnabled'] == 'true')): | ||
| vrf_table = '5000' |
There was a problem hiding this comment.
Use the same macro in code instead of 5000.
| mgmt_intf_gwaddr = entry['gwaddr'] | ||
| self.handle_mgmt_intf_gwaddr_cfg("ADD", it[0], mgmt_intf_gwaddr) | ||
|
|
||
| def unconfigure_mgmt_vrf(self): |
There was a problem hiding this comment.
unconfigure_mgmt_vrf definition is deleting "lo-m" and calls delete_mgmt_vrf_table which deletes the "mgmt" interface. When mvrf is disabled, where does it delink eth0 from master mgmt and where does it add the default routes and other routes back to default routing table? Same comment applies while enabling mvrf.
| self.unconfigure_mgmt_vrf() | ||
| else: | ||
| self.unconfigure_mgmt_vrf() | ||
| except Exception as inst: |
There was a problem hiding this comment.
what are the possible reasons for this exception? If mvrf enable/disable fails, will eth0 continue to remain in default/mgmt vrf and work normally in default/mgmt vrf?
| def configure_mgmt_vrf(self): | ||
| self.create_mgmt_vrf_table() | ||
|
|
||
| cmd = 'ip link set dev mgmt up' |
There was a problem hiding this comment.
If any of these steps fails, is it better to do a syslog to debug?
|
retest broadcom please |
3 participants
1.Added support for Dynamic handling of Management port config(MTU, Speed, auto negotiation, admin status and Description config attributes).
hostcfgd service enhancement details:
Hostcfgd populates default config for a management interface (“eth0”) and register’s callback handlers to handle events/notifications from the following tables MGMT_PORT, MGMT_INTERFACE and MGMT_VRF_CONFIG in ConfigDB and starts listening for events at INIT phase. As soon as any change observed in above mentioned tables respective event handle will be called, and it determines the config change and programs the same in kernel and populates data in AppDb.
List of Event handlers added:
mgmt_port_handler is a callback method to handle port related config which is populated in MGMT_PORT table in ConfigDB.
mgmt_intf_handler is a callback method to handle interface IP config which is populated in MGMT_INTERFACE table in ConfigDB.
mgmt_vrf_handler is callback method to handle MGMT vrf config which is populated in MGMT_VRF_CONFIG table in ConfigDB.
How to verify it
Update MGMT_PORT, MGMT_INTERFACE and MGMT_VRF_CONFIG config attributes mentioned below, config should be applied in kernel and it should get populated in AppDB.
Description for the change log
Management interface config handling in SONiC by hostcfgd service.
Signed-off-by: Ravi Vasanthm [email protected]