[xcvrd] Subscribe port configuration change in xcvrd #212

Junchao-Mellanox · 2021-08-11T10:36:04Z

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?

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

Additional Information (Optional)

lgtm-com · 2021-08-11T10:47:00Z

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

lgtm-com · 2021-08-26T02:24:46Z

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'

lgtm-com · 2021-09-01T08:26:27Z

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'

prgeor · 2021-09-15T12:00:51Z

@vdahiya12 could you review from y-cable perspective?

vdahiya12 · 2021-09-16T05:59:01Z

@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 · 2021-09-17T07:19:58Z

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

lgtm-com · 2021-09-17T07:33:28Z

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 · 2021-09-17T07:50:29Z

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_utilities/port_mapping.py

sonic-xcvrd/xcvrd/xcvrd.py

sonic-xcvrd/xcvrd/xcvrd_utilities/port_mapping.py

sonic-xcvrd/xcvrd/xcvrd.py

lgtm-com · 2021-09-17T10:03:36Z

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 · 2021-09-26T08:14:05Z

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 · 2021-10-11T01:59:36Z

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'

liat-grozovik · 2021-10-11T06:30:52Z

@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

prgeor · 2021-10-11T14:20:44Z

sonic-xcvrd/xcvrd/xcvrd.py

+        # 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, 


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?

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:

User remove logical port Ethernet0

SfpStateUpdateTask get the event first and remove DB

DomInfoUpdateTask does not get the event and still update TRANSCEIVER_DOM_SENSOR table

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:

User remove logical port Ethernet0

DomInfoUpdateTask get the event first and remove DB

SfpStateUpdateTask detect a SFP insert event and update TRANSCEIVER_DOM_SENSOR/TRANSCEIVER_INFO_TABLE table

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

lgtm-com · 2021-10-12T02:55:20Z

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

new alerts:

1 for Unused import

keboliu previously approved these changes Aug 13, 2021

View reviewed changes

Junchao-Mellanox mentioned this pull request Aug 17, 2021

[xcvrd] Handle port config change on fly in xcvrd sonic-net/SONiC#839

Merged

keboliu mentioned this pull request Aug 18, 2021

[DPB] XCVRD not able to fetch the new port SFP info after perform dynamic port breakdown sonic-net/sonic-buildimage#6499

Closed

liat-grozovik requested a review from sujinmkang August 25, 2021 18:54

Junchao-Mellanox added 2 commits August 26, 2021 10:16

[xcvrd] Subscribe port configuration change in xcvrd

0e89108

Fix LGTM warning

261f805

Junchao-Mellanox dismissed keboliu’s stale review via 261f805 August 26, 2021 02:17

Junchao-Mellanox force-pushed the xcvrd-subscribe-port-review branch from c84afe1 to 261f805 Compare August 26, 2021 02:17

Junchao-Mellanox marked this pull request as draft August 27, 2021 02:41

sujinmkang requested review from vdahiya12 and prgeor August 27, 2021 17:45

Fix unit test failure after rebase

e0402a5

Junchao-Mellanox marked this pull request as ready for review September 1, 2021 08:15

keboliu previously approved these changes Sep 14, 2021

View reviewed changes

Merge branch 'master' into xcvrd-subscribe-port-review

a3e7f7a

Y cable task should also subscribe port configuration change

88acbcf

Junchao-Mellanox dismissed keboliu’s stale review via 88acbcf September 17, 2021 07:24

Fix LGTM

1aa2ced

prgeor requested changes Sep 17, 2021

View reviewed changes

Fix review comment and adjust unit test

da1ca2c

Use select in xcvrd tasks

4c7f888

Junchao-Mellanox added 3 commits September 26, 2021 15:49

Use select in xcvrd tasks

da0761c

Merge branch 'xcvrd-subscribe-port-noq' of github.com:Junchao-Mellano…

5f87f74

…x/sonic-platform-daemons into xcvrd-subscribe-port-noq Conflicts: sonic-xcvrd/tests/test_xcvrd.py sonic-xcvrd/xcvrd/xcvrd.py

Merge branch 'xcvrd-subscribe-port-noq' into xcvrd-subscribe-port-review

eb7fe30

Conflicts: sonic-xcvrd/tests/test_xcvrd.py sonic-xcvrd/xcvrd/xcvrd.py

sujinmkang requested a review from prgeor September 27, 2021 03:53

Fix conflict

1521f1a

keboliu previously approved these changes Oct 11, 2021

View reviewed changes

keboliu mentioned this pull request Oct 6, 2021

[sfp-refactoring] xcvrd: add initial support for CMIS application initialization #217

Merged

5 tasks

prgeor requested changes Oct 11, 2021

View reviewed changes

Fix review comment

a45d0c8

Junchao-Mellanox dismissed keboliu’s stale review via a45d0c8 October 12, 2021 02:46

Junchao-Mellanox requested review from prgeor and keboliu October 12, 2021 02:46

Fix LGTM warning

4a0777c

prgeor approved these changes Oct 13, 2021

View reviewed changes

prgeor merged commit 65cdc63 into sonic-net:master Oct 13, 2021

tomer-israel mentioned this pull request Nov 11, 2021

HLD document on add/remove ports dynamically feature sonic-net/SONiC#900

Merged

vdahiya12 pushed a commit to vdahiya12/sonic-platform-daemons that referenced this pull request Apr 4, 2022

Fix QSFP-DD power class mask (sonic-net#212)

221fb8a

chiourung mentioned this pull request Aug 5, 2022

[port_breakout] Add eeprom information for breakout ports sonic-net/sonic-utilities#1816

Closed

keboliu mentioned this pull request Dec 6, 2022

Add new fields to status/dom_sensor/pm tables in STATE_DB for CMIS/C-CMIS #304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[xcvrd] Subscribe port configuration change in xcvrd #212

[xcvrd] Subscribe port configuration change in xcvrd #212

Junchao-Mellanox commented Aug 11, 2021 •

edited

lgtm-com bot commented Aug 11, 2021

lgtm-com bot commented Aug 26, 2021

lgtm-com bot commented Sep 1, 2021

prgeor commented Sep 15, 2021

vdahiya12 commented Sep 16, 2021

Junchao-Mellanox commented Sep 17, 2021

lgtm-com bot commented Sep 17, 2021

lgtm-com bot commented Sep 17, 2021

lgtm-com bot commented Sep 17, 2021

lgtm-com bot commented Sep 26, 2021

lgtm-com bot commented Oct 11, 2021

liat-grozovik commented Oct 11, 2021

prgeor Oct 11, 2021

Junchao-Mellanox Oct 12, 2021

lgtm-com bot commented Oct 12, 2021

[xcvrd] Subscribe port configuration change in xcvrd #212

[xcvrd] Subscribe port configuration change in xcvrd #212

Conversation

Junchao-Mellanox commented Aug 11, 2021 • edited

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

lgtm-com bot commented Aug 11, 2021

lgtm-com bot commented Aug 26, 2021

lgtm-com bot commented Sep 1, 2021

prgeor commented Sep 15, 2021

vdahiya12 commented Sep 16, 2021

Junchao-Mellanox commented Sep 17, 2021

lgtm-com bot commented Sep 17, 2021

lgtm-com bot commented Sep 17, 2021

lgtm-com bot commented Sep 17, 2021

lgtm-com bot commented Sep 26, 2021

lgtm-com bot commented Oct 11, 2021

liat-grozovik commented Oct 11, 2021

prgeor Oct 11, 2021

Choose a reason for hiding this comment

Junchao-Mellanox Oct 12, 2021

Choose a reason for hiding this comment

lgtm-com bot commented Oct 12, 2021

Junchao-Mellanox commented Aug 11, 2021 •

edited