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

[DPB] Fix a potential command failure when break out a port that is a member of portchannel. #3106

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[DPB] Fix a potential command failure when break out a port that is a member of portchannel. #3106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d4fcbf9
[DPB] Fix a potential command failure when break out a port that is a…
sandyw777 Dec 28, 2023
a2851e3
[DPB] Fix test case assertion error.
sandyw777 Dec 28, 2023
9476f6b
[DPB] Refactor the previous code change to address test coverage issue.
sandyw777 Dec 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
32 changes: 24 additions & 8 deletions config/config_mgmt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,8 +432,8 @@ def breakOutPort(self, delPorts=list(), portJson=dict(), force=False, \
'''
MAX_WAIT = 60
try:
# delete Port and get the Config diff, deps and True/False
delConfigToLoad, deps, ret = self._deletePorts(ports=delPorts, \
# delete Port and get the Config diff for delete port, delete dependencies, deps and True/False
delConfigToLoad, delConfigDepToLoad, deps, ret = self._deletePorts(ports=delPorts, \
force=force)
# return dependencies if delete port fails
if ret == False:
Expand All @@ -453,10 +453,14 @@ def breakOutPort(self, delPorts=list(), portJson=dict(), force=False, \

# If we are here, then get ready to update the Config DB as below:
# -- shutdown the ports,
# -- Update deletion of dependencies in Config DB,
# -- Wait 1 sec to complete the deletion of dependencies,
# -- Update deletion of ports in Config DB,
# -- verify Asic DB for port deletion,
# -- then update addition of ports in config DB.
self._shutdownIntf(delPorts)
self.writeConfigDB(delConfigDepToLoad)
tsleep(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a static value for sleep, Can we check if the PORT is removed from APPL_DB and proceed further when it is deleted

Copy link
Member

Choose a reason for hiding this comment

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

As I recall, delConfigToLoad should contain all the dependencies as well. Kindly paste an example, how delConfigDepToLoad and delConfigToLoad are different. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Line 466 verifies in ASIC_DB, is that not enough ? do we still need extra sleep. Sleep(1) is not deterministic.

Copy link
Member

Choose a reason for hiding this comment

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

Okie, got the gist now. Except sleep(1), rest from logic POV looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a static value for sleep, Can we check if the PORT is removed from APPL_DB and proceed further when it is deleted

@vivekrnv Just a confirmation, are you saying a busy waiting loop to replace the sleep?

Copy link
Contributor

@vivekrnv vivekrnv Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you paste an example b/w delConfigDepToLoad and delConfigToLoad? If self.writeConfigDB(delConfigDepToLoad) is for dependencies, then there is no reliable way to check if the corresponding object is deleted in the ASIC. In that case, static sleep looks fine to me

Copy link
Member

Choose a reason for hiding this comment

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

@sandyw777, Kindly add a comment that 'delConfigToLoad' will contain only ports after this fix, if not done already.
Seems like, we have to live with static sleep as of now. @puffc @vivekrnv

Copy link
Author

Choose a reason for hiding this comment

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

@vivekrnv Here is the example of delConfigDepToLoad and delConfigToLoad:

  • delConfigDepToLoad: {'BUFFER_PG': {'Ethernet32|0': None, 'Ethernet32|1-2': None, 'Ethernet32|3-4': None, 'Ethernet32|5-7': None}, 'BUFFER_QUEUE': {'Ethernet32|0-2': None, 'Ethernet32|3-4': None, 'Ethernet32|5-7': None}, 'CABLE_LENGTH': {'AZURE': {'Ethernet32': None}}, 'PORTCHANNEL_MEMBER': {'PortChannel1|Ethernet32': None}, 'PORT_QOS_MAP': {'Ethernet32': None}, 'QUEUE': {'Ethernet32|0': None, 'Ethernet32|1': None, 'Ethernet32|2': None, 'Ethernet32|3': None, 'Ethernet32|4': None, 'Ethernet32|5': None, 'Ethernet32|6': None}}

  • delConfigToLoad: {'PORT': {'Ethernet32': None}}

In this example, we breakout the Ethernet32 which is in a portchannel.
And below is the log of ConfigMgmt in this example:

admin@sonic:~$  show logging | grep "ConfigMgmt" 
Aug 19 07:04:48.170632 sonic INFO ConfigMgmt: Reading data from Redis configDb
Aug 19 07:04:48.271659 sonic INFO ConfigMgmt: delPorts ports:['Ethernet32'] force:True
Aug 19 07:04:48.271765 sonic INFO ConfigMgmt: Start Port Deletion
Aug 19 07:04:48.271843 sonic INFO ConfigMgmt: Find dependecies for port Ethernet32
Aug 19 07:04:48.274979 sonic INFO ConfigMgmt: Deleting /sonic-portchannel:sonic-portchannel/PORTCHANNEL_MEMBER/PORTCHANNEL_MEMBER_LIST[name='PortChannel1'][port='Ethernet32']/port
Aug 19 07:04:48.275250 sonic INFO ConfigMgmt: Deleting /sonic-buffer-pg:sonic-buffer-pg/BUFFER_PG/BUFFER_PG_LIST[port='Ethernet32'][pg_num='0']/port
Aug 19 07:04:48.276100 sonic INFO ConfigMgmt: Deleting /sonic-buffer-pg:sonic-buffer-pg/BUFFER_PG/BUFFER_PG_LIST[port='Ethernet32'][pg_num='1-2']/port
Aug 19 07:04:48.276939 sonic INFO ConfigMgmt: Deleting /sonic-buffer-pg:sonic-buffer-pg/BUFFER_PG/BUFFER_PG_LIST[port='Ethernet32'][pg_num='3-4']/port
Aug 19 07:04:48.277746 sonic INFO ConfigMgmt: Deleting /sonic-buffer-pg:sonic-buffer-pg/BUFFER_PG/BUFFER_PG_LIST[port='Ethernet32'][pg_num='5-7']/port
Aug 19 07:04:48.278513 sonic INFO ConfigMgmt: Deleting /sonic-buffer-queue:sonic-buffer-queue/BUFFER_QUEUE/BUFFER_QUEUE_LIST[port='Ethernet32'][qindex='0-2']/port
Aug 19 07:04:48.279201 sonic INFO ConfigMgmt: Deleting /sonic-buffer-queue:sonic-buffer-queue/BUFFER_QUEUE/BUFFER_QUEUE_LIST[port='Ethernet32'][qindex='3-4']/port
Aug 19 07:04:48.279827 sonic INFO ConfigMgmt: Deleting /sonic-buffer-queue:sonic-buffer-queue/BUFFER_QUEUE/BUFFER_QUEUE_LIST[port='Ethernet32'][qindex='5-7']/port
Aug 19 07:04:48.280442 sonic INFO ConfigMgmt: Deleting /sonic-cable-length:sonic-cable-length/CABLE_LENGTH/CABLE_LENGTH_LIST[name='AZURE']/CABLE_LENGTH[port='Ethernet32']/port
Aug 19 07:04:48.280766 sonic INFO ConfigMgmt: Deleting /sonic-port-qos-map:sonic-port-qos-map/PORT_QOS_MAP/PORT_QOS_MAP_LIST[ifname='Ethernet32']/ifname
Aug 19 07:04:48.281273 sonic INFO ConfigMgmt: Deleting /sonic-queue:sonic-queue/QUEUE/QUEUE_LIST[ifname='Ethernet32'][qindex='0']/ifname
Aug 19 07:04:48.283087 sonic INFO ConfigMgmt: Deleting /sonic-queue:sonic-queue/QUEUE/QUEUE_LIST[ifname='Ethernet32'][qindex='1']/ifname
Aug 19 07:04:48.284947 sonic INFO ConfigMgmt: Deleting /sonic-queue:sonic-queue/QUEUE/QUEUE_LIST[ifname='Ethernet32'][qindex='2']/ifname
Aug 19 07:04:48.286838 sonic INFO ConfigMgmt: Deleting /sonic-queue:sonic-queue/QUEUE/QUEUE_LIST[ifname='Ethernet32'][qindex='3']/ifname
Aug 19 07:04:48.288563 sonic INFO ConfigMgmt: Deleting /sonic-queue:sonic-queue/QUEUE/QUEUE_LIST[ifname='Ethernet32'][qindex='4']/ifname
Aug 19 07:04:48.290267 sonic INFO ConfigMgmt: Deleting /sonic-queue:sonic-queue/QUEUE/QUEUE_LIST[ifname='Ethernet32'][qindex='5']/ifname
Aug 19 07:04:48.291955 sonic INFO ConfigMgmt: Deleting /sonic-queue:sonic-queue/QUEUE/QUEUE_LIST[ifname='Ethernet32'][qindex='6']/ifname
Aug 19 07:04:48.318297 sonic INFO ConfigMgmt: Data Validation successful
Aug 19 07:04:48.332257 sonic INFO ConfigMgmt: Generate Final Config to write in DB
Aug 19 07:04:48.335846 sonic INFO ConfigMgmt: Deleting Port: Ethernet32
Aug 19 07:04:48.360377 sonic INFO ConfigMgmt: Data Validation successful
Aug 19 07:04:48.373320 sonic INFO ConfigMgmt: Generate Final Config to write in DB
Aug 19 07:04:48.376482 sonic INFO ConfigMgmt: Start Port Addition
Aug 19 07:04:48.376587 sonic INFO ConfigMgmt: addPorts Args portjson: {'PORT': {'Ethernet32': {'alias': 'Eth5-1', 'lanes': '217,218', 'speed': '100000', 'index': '5'}, 'Ethernet34': {'alias': 'Eth5-2', 'lanes': '219,220', 'speed': '100000', 'index': '5'}, 'Ethernet36': {'alias': 'Eth5-3', 'lanes': '221,222', 'speed': '100000', 'index': '5'}, 'Ethernet38': {'alias': 'Eth5-4', 'lanes': '223,224', 'speed': '100000', 'index': '5'}}} loadDefConfig: False
Aug 19 07:04:48.458260 sonic INFO ConfigMgmt: Data Validation successful
Aug 19 07:04:48.458499 sonic INFO ConfigMgmt: Generate Final Config to write in DB
Aug 19 07:04:48.463975 sonic INFO ConfigMgmt: shutdown Interfaces: {'PORT': {'Ethernet32': {'admin_status': 'down'}}}
Aug 19 07:04:48.464167 sonic INFO ConfigMgmt: Writing in Config DB
Aug 19 07:04:48.464353 sonic INFO ConfigMgmt: Write in DB: {'PORT': {'Ethernet32': {'admin_status': 'down'}}}
Aug 19 07:04:48.464493 sonic INFO ConfigMgmt: Writing in Config DB
Aug 19 07:04:48.465082 sonic INFO ConfigMgmt: Write in DB: {'BUFFER_PG': {('Ethernet32', '0'): None, ('Ethernet32', '1-2'): None, ('Ethernet32', '3-4'): None, ('Ethernet32', '5-7'): None}, 'BUFFER_QUEUE': {('Ethernet32', '0-2'): None, ('Ethernet32', '3-4'): None, ('Ethernet32', '5-7'): None}, 'CABLE_LENGTH': {'AZURE': {'Ethernet32': None}}, 'PORTCHANNEL_MEMBER': {('PortChannel1', 'Ethernet32'): None}, 'PORT_QOS_MAP': {'Ethernet32': None}, 'QUEUE': {('Ethernet32', '0'): None, ('Ethernet32', '1'): None, ('Ethernet32', '2'): None, ('Ethernet32', '3'): None, ('Ethernet32', '4'): None, ('Ethernet32', '5'): None, ('Ethernet32', '6'): None}}
Aug 19 07:04:49.476798 sonic INFO ConfigMgmt: Writing in Config DB
Aug 19 07:04:49.476964 sonic INFO ConfigMgmt: Write in DB: {'PORT': {'Ethernet32': None}}
Aug 19 07:04:49.477238 sonic INFO ConfigMgmt: Verify Port Deletion from Asic DB, Wait...
Aug 19 07:04:49.477688 sonic INFO ConfigMgmt: Check Key in Asic DB: ASIC_STATE:SAI_OBJECT_TYPE_PORT:oid:0x1000000000153
Aug 19 07:04:50.480308 sonic INFO ConfigMgmt: Check Key in Asic DB: ASIC_STATE:SAI_OBJECT_TYPE_PORT:oid:0x1000000000153
Aug 19 07:04:50.480524 sonic INFO ConfigMgmt: Writing in Config DB
Aug 19 07:04:50.480658 sonic INFO ConfigMgmt: Write in DB: {'PORT': {'Ethernet32': {'alias': 'Eth5-1', 'lanes': '217,218', 'speed': '100000', 'index': '5'}, 'Ethernet34': {'alias': 'Eth5-2', 'lanes': '219,220', 'speed': '100000', 'index': '5'}, 'Ethernet36': {'alias': 'Eth5-3', 'lanes': '221,222', 'speed': '100000', 'index': '5'}, 'Ethernet38': {'alias': 'Eth5-4', 'lanes': '223,224', 'speed': '100000', 'index': '5'}}}
admin@sonic:~$

self.writeConfigDB(delConfigToLoad)
# Verify in Asic DB,
self._verifyAsicDB(db=dataBase, ports=delPorts, portMap=if_name_map, \
Expand All @@ -479,10 +483,10 @@ def _deletePorts(self, ports=list(), force=False):
force (bool): if false return dependecies, else delete dependencies.

Returns:
(configToLoad, deps, ret) (tuple)[dict, list, bool]: config, dependecies
(configToLoad, configDepToLoad, deps, ret) (tuple)[dict, list, bool]: config, config, dependecies
and success/fail.
'''
configToLoad = None; deps = None
configToLoad = None; configDepToLoad = None; deps = None
try:
self.sysLog(msg="delPorts ports:{} force:{}".format(ports, force))

Expand All @@ -500,7 +504,7 @@ def _deletePorts(self, ports=list(), force=False):

# No further action with no force and deps exist
if not force and deps:
return configToLoad, deps, False
return configToLoad, configDepToLoad, deps, False

# delets all deps, No topological sort is needed as of now, if deletion
# of deps fails, return immediately
Expand All @@ -510,6 +514,18 @@ def _deletePorts(self, ports=list(), force=False):
self.sy.deleteNode(str(dep))
# mark deps as None now,
deps = None

if not self.validateConfigData():
return configToLoad, configDepToLoad, deps, False

# Dependencies are deleted, Lets get the diff
self.configdbJsonOut = self.sy.getData()

# Update configToLoad
configDepToLoad = self._updateDiffConfigDB()

# Update self.configdbJsonOut which removed dependencies to self.configdbJsonIn
self.configdbJsonIn = self.configdbJsonOut

# all deps are deleted now, delete all ports now
for port in ports:
Expand All @@ -519,7 +535,7 @@ def _deletePorts(self, ports=list(), force=False):

# Let`s Validate the tree now
if not self.validateConfigData():
return configToLoad, deps, False
return configToLoad, configDepToLoad, deps, False

# All great if we are here, Lets get the diff
self.configdbJsonOut = self.sy.getData()
Expand All @@ -530,9 +546,9 @@ def _deletePorts(self, ports=list(), force=False):
self.sysLog(doPrint=True, logLevel=syslog.LOG_ERR, msg=str(e))
self.sysLog(doPrint=True, logLevel=syslog.LOG_ERR, \
msg="Port Deletion Failed")
return configToLoad, deps, False
return configToLoad, configDepToLoad, deps, False

return configToLoad, deps, True
return configToLoad, configDepToLoad, deps, True

def _addPorts(self, portJson=dict(), loadDefConfig=True):
'''
Expand Down
2 changes: 1 addition & 1 deletion tests/config_mgmt_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ def test_upper_case_mac_fix(self):
['00:11:22:BB:CC:DD', '00:11:22:bb:cc:dd']

# verify correct function call to writeConfigDB
assert cmdpb.writeConfigDB.call_count == 3
assert cmdpb.writeConfigDB.call_count == 4
print(cmdpb.writeConfigDB.call_args_list[0])
# args is populated if call is done as writeConfigDB(a, b), kwargs is
# populated if call is done as writeConfigDB(A=a, B=b)
Expand Down
Loading