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

Sfputil base and helper class changes for multi-ASIC #100

Merged
merged 5 commits into from
Aug 25, 2020

Conversation

judyjoseph
Copy link
Contributor

@judyjoseph judyjoseph commented Jul 9, 2020

This is for changes needed to support multi-asic platforms.
(i) Add a new API to parse the port_config.ini files in different directories named based on the asic_ID's
(ii) Add a new dictionary which maps the interface to the asic_id
(iii) Check if it is a backplane interface, if so skip from adding it to logical_port list

sonic_platform_base/sonic_sfp/sfputilbase.py Outdated Show resolved Hide resolved
sonic_platform_base/sonic_sfp/sfputilbase.py Outdated Show resolved Hide resolved
sonic_platform_base/sonic_sfp/sfputilhelper.py Outdated Show resolved Hide resolved
sonic_platform_base/sonic_sfp/sfputilhelper.py Outdated Show resolved Hide resolved
sonic_platform_base/sonic_sfp/sfputilbase.py Outdated Show resolved Hide resolved
@@ -453,12 +456,16 @@ def read_porttab_mappings(self, porttabfile):
# so we use the port's position in the file (zero-based) as bcm_port
portname = line.split()[0]

# Ignore if this is an internal backplane interface
if portname.startswith(daemon_base.get_internal_interface_prefix()):
continue

Choose a reason for hiding this comment

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

Is json format file depracated. I don't see similar check for json format files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do support only the port_config.ini files for multi_asic. The json format files was recently introduced. So I was currently adding the logic to handle multiple port_config.ini files alone.

Copy link
Contributor

@jleveque jleveque Aug 19, 2020

Choose a reason for hiding this comment

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

All port_config.ini/platform.json parsing should eventually be moved to portconfig.py (currently in sonic-config-engine, to be moved to sonic-py-common) so that it is only done in one place. This is tracked by issues: #88, #110

@judyjoseph: Is there already multi-ASIC logic in portconfig.py? If so I would prefer resolving #88 with this PR, because this PR is adding even MORE parsing logic to this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jleveque I could move the json/ini file parsing from here --> to portconfig.py. But what I find is that I will have to write this logic as new API's in portconfig.py -- as the port_config.ini/platform.json parsing logic implemented here in sfpbase module is different to that is already present @ https://github.com/Azure/sonic-buildimage/blob/da69d57a8dffa37e285d7b93671d055ba193c61d/src/sonic-config-engine/portconfig.py#L142.

We could have separate parsing API's for now, and later on unify them in portconfig.py as a single parsing API ?

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, It's probably best to leave it here for now and do the move and unification at the same time, and maybe also move it all to sonic-py-common at the same time, as well.

@smaheshm
Copy link

What's the difference between sfputilbase and sfputilhelper. There's lot of common code between them.

Comment on lines 213 to 216
if logical_port in self.logical_to_asic.keys():
return self.logical_to_asic[logical_port]
else:
return None

Choose a reason for hiding this comment

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

this can be simpler:

return self.logical_to_asic.get(logical_port)

Copy link

@smaheshm smaheshm left a comment

Choose a reason for hiding this comment

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

minor comments

@jleveque
Copy link
Contributor

@smaheshm:

What's the difference between sfputilbase and sfputilhelper. There's lot of common code between them.

sfputilbase.py was the base class for the old platform 1.0 plugins, and is deprecated. sfphelper.py is used as a helper class for the new platform 2.0 API. Hence why there is duplicate code. Once all platforms have moved to using the new 2.0 API, sfputilbase.py will be removed.

@jleveque jleveque dismissed their stale review August 19, 2020 23:35

PR looks good to me now. Wait for @smaheshm to re-review.

@smaheshm
Copy link

Looks good!

@judyjoseph judyjoseph merged commit b60f46c into sonic-net:master Aug 25, 2020
@abdosi
Copy link
Contributor

abdosi commented Sep 4, 2020

@judyjoseph Please create PR for 201911

judyjoseph added a commit to judyjoseph/sonic-platform-common that referenced this pull request Sep 5, 2020
@judyjoseph judyjoseph deleted the pmon_multi_asic branch September 5, 2020 01:59
@judyjoseph
Copy link
Contributor Author

@judyjoseph Please create PR for 201911

Raised this new PR #117

oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-platform-common that referenced this pull request Oct 25, 2024
In pcied, added support to collect AER stats belonging to different severities for AER supported PCIe devices and update it in STATE_DB.

The key used to represent a PCIE device for storing its AER stats in STATE_DB is of the format PCIE_DEVICE|<Bus>:<Dev>.<Fn>.
For every device, AER stats will be stored as key, value pairs where key is of the format <severity>|<AER Error type> and the device ID will be stored with key id.

HLD: sonic-net/SONiC#678, sonic-net/SONiC#720
Depends on: sonic-net#144
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.

4 participants