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

[Xcvrd] Soak duplicate events and process only updated interested events #285

Merged
merged 4 commits into from
Sep 16, 2022

Conversation

prgeor
Copy link
Collaborator

@prgeor prgeor commented Aug 18, 2022

Description

Xcvrd should be listening to
CONFIG_DB - For changes to speed, lane mapping, tx_power, frequency, admin_status
STATE_DB - For optics insertion/removal, host_tx_ready
APPL_DB - only during start to know if PortConfigDone is completed

Duplicate events are now soaked such that only the last event in a barrage of duplicate events is considered for further processing. Also, if a filter is installed then filter events having fields of interest that got changed from last time.

Motivation and Context

Portmgrd is writing unmodified fields to the APL_DB whenever there is configuration change resulting in too many event updates causing CMIS task manager that listens to these events to be woken up and restart the state machine unnecessarily.

There can be events like port operationally going up in STATE_DB's PORT_TABLE that can restart the CMIS state machine while the Rx and Tx datapaths are provisioned independently.

How Has This Been Tested?

Verified 400G ZR and 400G DR4 optics are initializing fine

Additional Information (Optional)

@prgeor
Copy link
Collaborator Author

prgeor commented Aug 18, 2022

@jaganbal-a please review

@shyam77git
Copy link

@prgeor Please update the 'Description' section with following info
Besides the list above in 'Description' section, xcvrd would be listening to following events too from these DBs:
admin up, down events from CONFIG_DB
host tx_ready from STATE_DB

@prgeor prgeor changed the title [Xcvrd] Subscribe to CONFIG_DB instead of APPL_DB [Xcvrd] Soak duplicate events and process only updated interested events Sep 7, 2022
@lgtm-com
Copy link

lgtm-com bot commented Sep 7, 2022

This pull request introduces 1 alert when merging db73f4a into 3acb171 - view on LGTM.com

new alerts:

  • 1 for Too few arguments in formatting call

@prgeor
Copy link
Collaborator Author

prgeor commented Sep 7, 2022

@prgeor Please update the 'Description' section with following info Besides the list above in 'Description' section, xcvrd would be listening to following events too from these DBs: admin up, down events from CONFIG_DB host tx_ready from STATE_DB

@shyam77git updated now

@prgeor
Copy link
Collaborator Author

prgeor commented Sep 7, 2022

This PR should fix this issue sonic-net/sonic-buildimage#11953

asic_context[port_tbl] = asic_id
sel.addSelectable(port_tbl)
logger.log_warning("subscribing to port_tbl {} - {} DB of namespace {} ".format(
port_tbl, list(d.values())[0]), namespace)
Copy link

Choose a reason for hiding this comment

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

Misaligned ')' causing script to bail out

Choose a reason for hiding this comment

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

Agreed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

now fixed

@snider-nokia
Copy link

Aside from the misaligned ')' in port_mapp.py line 133/134, the only problem remaining here is that these changes do not build under 2205 (build target target/python-wheels/bullseye/sonic_xcvrd-1.0-py3-none-any.whl fails). Building of the aforementioned target needs to be fixed.

@prgeor
Copy link
Collaborator Author

prgeor commented Sep 14, 2022

Aside from the misaligned ')' in port_mapp.py line 133/134, the only problem remaining here is that these changes do not build under 2205 (build target target/python-wheels/bullseye/sonic_xcvrd-1.0-py3-none-any.whl fails). Building of the aforementioned target needs to be fixed.

@snider-nokia please check now.

@snider-nokia
Copy link

Aside from the misaligned ')' in port_mapp.py line 133/134, the only problem remaining here is that these changes do not build under 2205 (build target target/python-wheels/bullseye/sonic_xcvrd-1.0-py3-none-any.whl fails). Building of the aforementioned target needs to be fixed.

@snider-nokia please check now.

Yes, confirmed that target now builds properly under 2205.

Copy link

@arpp93 arpp93 left a comment

Choose a reason for hiding this comment

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

LGTM

@prgeor prgeor merged commit b03cc74 into sonic-net:master Sep 16, 2022
@prgeor
Copy link
Collaborator Author

prgeor commented Sep 16, 2022

@yxieca please pick to 202205. Added the label.

yxieca pushed a commit that referenced this pull request Sep 21, 2022
…nts (#285)

* Subscribe to CONFIG_DB instead of APPL_DB

* Filter out events

* Fix build

* improve code coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants