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

Use get() to fetch default value from dictionary for port admin_status #286

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

anamehra
Copy link
Contributor

Signed-off-by: anamehra anamehra@cisco.com

Description

The config db dictionary may not have admin_status available for a

port if the same is not set in config db file. Use get() function to
prevent crashes and set the default status to 'down'.

Motivation and Context

xcvrd crash is observed during boot on multi-asic chassis system.
The crash is reported in sonic-net/sonic-buildimage#11707

On debugging the issue, found that at the point of the crash, the data being used is from config db.
It was observed that the Ethernet ports, which are not configured for admin_state field explicitly in the config is causing the crash. This is a valid config as admin_status is not a mandatory field and is considerd by default 'down' if missing in port config in config db file.

for example, the Ethernet31 is configured for a far-end, and has this field populated while Ethernet32, as was not being used, has this field missing.

"Ethernet31": {
    "admin_status": "up",
    "alias": "Eth0/2/31",
    "asic_port_name": "Eth31-ASIC2",
    "description": "ARISTA32T3:Ethernet1",
    "fec": "rs",
    "index": "31",
    "lanes": "512,513,514,515",
    "mtu": "9100",
    "pfc_asym": "off",
    "role": "Ext",
    "speed": "100000",
    "tpid": "0x8100"
},
"Ethernet32": {
    "alias": "Eth0/2/32",
    "asic_port_name": "Eth32-ASIC2",
    "description": "Eth0/2/32",
    "fec": "rs",
    "index": "32",
    "lanes": "264,265,266,267",
    "mtu": "9100",
    "pfc_asym": "off",
    "role": "Ext",
    "speed": "100000",
    "tpid": "0x8100"
},

closes sonic-net/sonic-buildimage#11707

How Has This Been Tested?

Loaded image on single and multi-asic system. Tested for normal boot. No crash observed.

Additional Information (Optional)

    The config db dictionary may not have admin_status available for a
port if the same is not set in config db file. Use get() function to
prevent crash and set default status to 'down'.

Signed-off-by: anamehra <anamehra@cisco.com>
@anamehra
Copy link
Contributor Author

@prgeor , @abdosi , please review.

@prgeor
Copy link
Collaborator

prgeor commented Aug 25, 2022

@anamehra can you please check your minigraph why 'admin_status' is missing for some ports?

@anamehra
Copy link
Contributor Author

@anamehra can you please check your minigraph why 'admin_status' is missing for some ports?

@abdosi , could you please comment on minigraph?
@prgeor , a config db json with Ethernet ports missing admin_status should be a valid config. If admin_status is not defined, its assumed as down. It has been the case always. In the fix, I am just handling the same scenario. If a dictionary entry is missing, use the default instead of crashing.

@abdosi abdosi self-requested a review September 20, 2022 15:21
@abdosi
Copy link
Contributor

abdosi commented Sep 20, 2022

@anamehra can you please check your minigraph why 'admin_status' is missing for some ports?

@abdosi , could you please comment on minigraph? @prgeor , a config db json with Ethernet ports missing admin_status should be a valid config. If admin_status is not defined, its assumed as down. It has been the case always. In the fix, I am just handling the same scenario. If a dictionary entry is missing, use the default instead of crashing.

@prgeor i checked for the ports that do not have any neighbor associated minigraph parser do not assign admin_status attribute in port table. We can fix minigraph parser to always have admin_status attribute but I think this PR change is good and is independent of minigraph parser change

@abdosi abdosi changed the title Use get() to fetch data from dictionary Use get() to fetch default value from dictionary for port admin_status Sep 20, 2022
@abdosi abdosi merged commit 8ff5f37 into sonic-net:master Sep 20, 2022
@anamehra anamehra deleted the anamehra/xcvrd_crash branch September 20, 2022 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xcvrd crash observed during boot
4 participants