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] Subscribe port configuration change in xcvrd #212

Merged
merged 14 commits into from
Oct 13, 2021

Conversation

Junchao-Mellanox
Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox commented Aug 11, 2021

Features like dynamic port breakout allow user to change port configuration on fly, xcvrd need detects such changes and update transceiver information to STATE_DB accordingly. See HLD change PR: sonic-net/SONiC#839

Description

Currently, xcvrd assumes that port mapping information is never changed, so it always read static port mapping information from platform.json/port_config.ini and save it to a global data structure. However, things changed since dynamic port breakout feature introduced. Port can be added/created on the fly, xcvrd cannot update transceiver information, DOM information and transceiver status information without knowing the ports change. This causes data in state db not aligned with config db. To address this issue, xcvrd should subscribe CONFIG_DB PORT table change and update port mapping information accordingly. Observer pattern will be used here to handle port mapping information change:

  • Main process works as a "Subject", it subscribes CONFIG_DB PORT table change and publish port change event.
  • State machine process and DOM sensor update thread work as "Observer", it subscribes port change event and update local port mapping accordingly.

Motivation and Context

How Has This Been Tested?

  1. Manual test
  2. sonic-mgmt platform related test, 100% pass
  3. new unit test covers all new changes

Additional Information (Optional)

@lgtm-com
Copy link

lgtm-com bot commented Aug 11, 2021

This pull request introduces 2 alerts when merging 780444a into f63fc94 - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Variable defined multiple times

keboliu
keboliu previously approved these changes Aug 13, 2021
@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2021

This pull request introduces 3 alerts and fixes 1 when merging 261f805 into e038bc2 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Unreachable code

fixed alerts:

  • 1 for Except block handles 'BaseException'

@Junchao-Mellanox Junchao-Mellanox marked this pull request as draft August 27, 2021 02:41
@Junchao-Mellanox Junchao-Mellanox marked this pull request as ready for review September 1, 2021 08:15
@lgtm-com
Copy link

lgtm-com bot commented Sep 1, 2021

This pull request introduces 11 alerts and fixes 1 when merging e0402a5 into ef0e791 - view on LGTM.com

new alerts:

  • 8 for Wrong number of arguments in a call
  • 2 for Unused local variable
  • 1 for Unreachable code

fixed alerts:

  • 1 for Except block handles 'BaseException'

keboliu
keboliu previously approved these changes Sep 14, 2021
@prgeor
Copy link
Collaborator

prgeor commented Sep 15, 2021

@vdahiya12 could you review from y-cable perspective?

@vdahiya12
Copy link
Contributor

@vdahiya12 could you review from y-cable perspective?

LGTM, but is there any testing done with xcvrs/cables pulled in and out and see if the logic works ?

@Junchao-Mellanox
Copy link
Collaborator Author

Hi @vdahiya12 , i tested normal cable, but i did not test y cable.

@lgtm-com
Copy link

lgtm-com bot commented Sep 17, 2021

This pull request introduces 11 alerts and fixes 1 when merging 88acbcf into 84386e6 - view on LGTM.com

new alerts:

  • 8 for Wrong number of arguments in a call
  • 2 for Unused local variable
  • 1 for Unreachable code

fixed alerts:

  • 1 for Except block handles 'BaseException'

@lgtm-com
Copy link

lgtm-com bot commented Sep 17, 2021

This pull request introduces 3 alerts and fixes 1 when merging 1aa2ced into 84386e6 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Unreachable code

fixed alerts:

  • 1 for Except block handles 'BaseException'

sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved
sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved
sonic-xcvrd/xcvrd/xcvrd.py Show resolved Hide resolved
sonic-xcvrd/xcvrd/xcvrd.py Show resolved Hide resolved
sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved
sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved
sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved
sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Sep 17, 2021

This pull request introduces 3 alerts and fixes 1 when merging da1ca2c into 84386e6 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Unreachable code

fixed alerts:

  • 1 for Except block handles 'BaseException'

…x/sonic-platform-daemons into xcvrd-subscribe-port-noq

Conflicts:
	sonic-xcvrd/tests/test_xcvrd.py
	sonic-xcvrd/xcvrd/xcvrd.py
Conflicts:
	sonic-xcvrd/tests/test_xcvrd.py
	sonic-xcvrd/xcvrd/xcvrd.py
@lgtm-com
Copy link

lgtm-com bot commented Sep 26, 2021

This pull request introduces 3 alerts and fixes 1 when merging eb7fe30 into 84386e6 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Unreachable code

fixed alerts:

  • 1 for Except block handles 'BaseException'

@lgtm-com
Copy link

lgtm-com bot commented Oct 11, 2021

This pull request introduces 3 alerts and fixes 1 when merging 1521f1a into 6e4cf6f - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Unreachable code

fixed alerts:

  • 1 for Except block handles 'BaseException'

keboliu
keboliu previously approved these changes Oct 11, 2021
@liat-grozovik
Copy link
Collaborator

@vdahiya12 and @prgeor would you please refer to the replies on your comments?
if no additional comments please approve so we can merge.

Note: multi asic should be tested by this PR as VS only.

sonic-xcvrd/xcvrd/xcvrd.py Show resolved Hide resolved
sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved
sonic-xcvrd/xcvrd/xcvrd.py Show resolved Hide resolved
# To avoid race condition, remove the entry TRANSCEIVER_DOM_INFO, TRANSCEIVER_STATUS_INFO and TRANSCEIVER_INFO table.
# The operation to remove entry from TRANSCEIVER_DOM_INFO is duplicate with DomInfoUpdateTask.on_remove_logical_port,
# but it is necessary because TRANSCEIVER_DOM_INFO is also updated in this sub process when a new SFP is inserted.
del_port_sfp_dom_info_from_db(port_change_event.port_name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

del_port_sfp_dom_info_from_db() will be called by both tasks DomInfoUpdateTask and SfpStateUpdateTask when there a port config change, so I am not sure why do you want to invoke from two different task? Is the key deletion from the dom table thread safe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This overlap is needed. Both DomInfoUpdateTask and SfpStateUpdateTask update TRANSCEIVER_DOM_SENSOR and TRANSCEIVER_INFO_TABLE table. When a logical port is removed, we don't know which task(DomInfoUpdateTask/SfpStateUpdateTask) will get the event first.

If we don't do remove in DomInfoUpdateTask, consider following flow:

  1. User remove logical port Ethernet0
  2. SfpStateUpdateTask get the event first and remove DB
  3. DomInfoUpdateTask does not get the event and still update TRANSCEIVER_DOM_SENSOR table
  4. Ethernet0 will be left over in TRANSCEIVER_DOM_SENSOR and no one can clear it.

If we don't do remove in SfpStateUpdateTask, consider following flow:

  1. User remove logical port Ethernet0
  2. DomInfoUpdateTask get the event first and remove DB
  3. SfpStateUpdateTask detect a SFP insert event and update TRANSCEIVER_DOM_SENSOR/TRANSCEIVER_INFO_TABLE table
  4. Ethernet0 will be left over in those tables and no one can clear it.

It is thread safe because redis can guarantee it.

sonic-xcvrd/xcvrd/xcvrd_utilities/y_cable_helper.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2021

This pull request introduces 1 alert when merging a45d0c8 into 6e4cf6f - view on LGTM.com

new alerts:

  • 1 for Unused import

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants