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

[portsorch] Expose supported FEC modes to STABE_DB and check whether FEC mode is supported before setting it #2333

Merged
merged 3 commits into from
Jul 24, 2022

Conversation

stephenxs
Copy link
Collaborator

@stephenxs stephenxs commented Jun 16, 2022

What I did
Expose supported FEC modes to STATE_DB.PORT_TABLE|<port>.supported_fecs.
The orchagent calls get_port_attribute to get attribute SAI_PORT_ATTR_SUPPORTED_FEC_MODE during initialization and then records it into internal data.

  • By default, the supported FEC modes will be returned by SAI and exposed to STATE_DB. Eg. rs,none means only rs and none is supported on the port.
    • The orchagent will check whether the FEC mode is supported on the port before calling the SAI API to set it.
    • The CLI will check whether the FEC mode is in supported_fecs before setting FEC mode on a port to the CONFIG_DB
  • In case the SAI API does not support any FEC mode on the port, N/A will be exposed to STATE_DB
    • The orchagent will deny any FEC setting and prints log.
    • The CLI will deny FEC setting.
  • In case the SAI API get_port_attribute returns Not implemented
    • No check will be performed before setting a FEC mode to SAI on the port.
    • The CLI will check whether the FEC mode is defined before setting it to CONFIG_DB.

Why I did it
It is not supported to set FEC mode on some platforms. To avoid error, we need to expose the supported FEC list.

How I verified it
Manually test and mock test.

Details if related

@stephenxs
Copy link
Collaborator Author

vstest failure is under investigation.

@stephenxs
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephenxs
Copy link
Collaborator Author

@bingwang-ms
looks like test_dscp_to_tc_map_applied_to_switch is failing. can you please help to check it?
thanks

2022-06-21T02:43:00.0247335Z =================================== FAILURES ===================================
2022-06-21T02:43:00.0253184Z ____________ TestDscpToTcMap.test_dscp_to_tc_map_applied_to_switch _____________
2022-06-21T02:43:00.0254092Z 
2022-06-21T02:43:00.0254640Z self = <test_qos_map.TestDscpToTcMap object at 0x7f1af01ba070>
2022-06-21T02:43:00.0255221Z dvs = <conftest.DockerVirtualSwitch object at 0x7f1af02f2730>
2022-06-21T02:43:00.0255469Z 
2022-06-21T02:43:00.0256057Z     def test_dscp_to_tc_map_applied_to_switch(self, dvs):
2022-06-21T02:43:00.0256575Z         self.init_test(dvs)
2022-06-21T02:43:00.0257171Z         dscp_to_tc_map_id = None
2022-06-21T02:43:00.0257756Z         created_new_map = False
2022-06-21T02:43:00.0258242Z         try:
2022-06-21T02:43:00.0258992Z             existing_map = self.dscp_to_tc_table.getKeys()
2022-06-21T02:43:00.0259567Z             if "AZURE" not in existing_map:
2022-06-21T02:43:00.0260162Z                 # Create a DSCP_TO_TC map
2022-06-21T02:43:00.0260898Z                 dscp_to_tc_map = [(str(i), str(i)) for i in range(0, 63)]
2022-06-21T02:43:00.0261698Z                 self.dscp_to_tc_table.set("AZURE", swsscommon.FieldValuePairs(dscp_to_tc_map))
2022-06-21T02:43:00.0262132Z     
2022-06-21T02:43:00.0262981Z                 self.asic_db.wait_for_n_keys(self.ASIC_QOS_MAP_STR, self.asic_qos_map_count + 1)
2022-06-21T02:43:00.0263419Z     
2022-06-21T02:43:00.0263932Z                 # Get the DSCP_TO_TC map ID
2022-06-21T02:43:00.0264602Z                 dscp_to_tc_map_id = self.get_qos_id()
2022-06-21T02:43:00.0265218Z                 assert(dscp_to_tc_map_id is not None)
2022-06-21T02:43:00.0265598Z     
2022-06-21T02:43:00.0266271Z                 # Assert the expected values
2022-06-21T02:43:00.0267079Z                 fvs = self.asic_db.get_entry(self.ASIC_QOS_MAP_STR, dscp_to_tc_map_id)
2022-06-21T02:43:00.0267936Z                 assert(fvs.get("SAI_QOS_MAP_ATTR_TYPE") == "SAI_QOS_MAP_TYPE_DSCP_TO_TC")
2022-06-21T02:43:00.0268431Z                 created_new_map = True
2022-06-21T02:43:00.0268913Z             else:
2022-06-21T02:43:00.0269586Z                 for id in self.asic_qos_map_ids:
2022-06-21T02:43:00.0270315Z                     fvs = self.asic_db.get_entry(self.ASIC_QOS_MAP_STR, id)
2022-06-21T02:43:00.0271056Z                     if fvs.get("SAI_QOS_MAP_ATTR_TYPE") == "SAI_QOS_MAP_TYPE_DSCP_TO_TC":
2022-06-21T02:43:00.0271562Z                         dscp_to_tc_map_id = id
2022-06-21T02:43:00.0272077Z                         break
2022-06-21T02:43:00.0272731Z             switch_oid = dvs.getSwitchOid()
2022-06-21T02:43:00.0275914Z             # Check switch level DSCP_TO_TC_MAP doesn't before PORT_QOS_MAP|global is created
2022-06-21T02:43:00.0276548Z             fvs = self.asic_db.get_entry(self.ASIC_SWITCH_STR, switch_oid)
2022-06-21T02:43:00.0277054Z >           assert("SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP" not in fvs)
2022-06-21T02:43:00.0278412Z E           AssertionError: assert 'SAI_SWITCH_ATTR_QOS_DSCP_TO_TC_MAP' not in {'NULL': 'NULL', 'SAI_SWITCH_ATTR_FDB_EVENT_NOTIFY': '0x56102c5bda69', 'SAI_SWITCH_ATTR_INIT_SWITCH': 'true', 'SAI_SWITCH_ATTR_PORT_STATE_CHANGE_NOTIFY': '0x56102c5bda89', ...}

@stephenxs
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephenxs stephenxs changed the title Handle supported FEC modes [portsorch] Expose supported FEC modes to STABE_DB and check whether FEC mode is supported before setting it Jun 22, 2022
@stephenxs stephenxs marked this pull request as ready for review June 22, 2022 13:40
@stephenxs stephenxs requested a review from prsunny as a code owner June 22, 2022 13:40
@prsunny prsunny requested a review from prgeor June 22, 2022 16:23
@liat-grozovik
Copy link
Collaborator

@prsunny , @prgeor could you please help to review and signoff?

orchagent/portsorch.cpp Outdated Show resolved Hide resolved
tests/mock_tests/portsorch_ut.cpp Outdated Show resolved Hide resolved
tests/mock_tests/portsorch_ut.cpp Show resolved Hide resolved
tests/mock_tests/portsorch_ut.cpp Outdated Show resolved Hide resolved
@stephenxs
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

dgsudharsan
dgsudharsan previously approved these changes Jul 1, 2022
@stephenxs
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephenxs
Copy link
Collaborator Author

None of the failures is caused by the PR itself. Retried

2022-07-01T18:52:06.9786125Z ____________ TestPGDropCounter.test_pg_drop_counter_port_add_remove ____________
2022-07-01T18:52:06.9787450Z 
2022-07-01T18:52:06.9789516Z self = <test_pg_drop_counter.TestPGDropCounter object at 0x7f7859e07ac0>
2022-07-01T18:52:06.9790341Z dvs = <conftest.DockerVirtualSwitch object at 0x7f7859d71880>
2022-07-01T18:52:06.9791343Z 
2022-07-01T18:52:06.9792232Z     def test_pg_drop_counter_port_add_remove(self, dvs):
2022-07-01T18:52:06.9793442Z         self.setup_dbs(dvs)
2022-07-01T18:52:06.9794186Z     
2022-07-01T18:52:06.9794531Z         try:
2022-07-01T18:52:06.9794899Z             # configure pg drop flex counter
2022-07-01T18:52:06.9795321Z             self.set_up_flex_counter()
2022-07-01T18:52:06.9795702Z     
2022-07-01T18:52:06.9796126Z             # receive port info
2022-07-01T18:52:06.9796508Z             fvs = self.config_db.get_entry("PORT", PORT)
2022-07-01T18:52:06.9796932Z             assert len(fvs) > 0
2022-07-01T18:52:06.9797206Z     
2022-07-01T18:52:06.9797542Z             # save all the oids of the pg drop counters
2022-07-01T18:52:06.9797899Z             oid_list = []
2022-07-01T18:52:06.9798236Z             for priority in range(0,7):
2022-07-01T18:52:06.9798821Z >               oid_list.append(dvs.get_counters_db().get_entry("COUNTERS_PG_NAME_MAP", "")["%s:%d"%(PORT, priority)])
2022-07-01T18:52:06.9800042Z E               KeyError: 'Ethernet0:0'
2022-07-01T18:52:06.9800225Z 
2022-07-01T18:52:06.9800558Z test_pg_drop_counter.py:114: KeyError
2022-07-01T18:52:06.9801283Z ---------------------------- Captured stdout setup -----------------------------
2022-07-01T18:52:06.9801724Z remove extra link dummy
2022-07-01T18:52:06.9802386Z ---------------------------- Captured stderr setup -----------------------------
2022-07-01T18:52:06.9803147Z Exception ignored in: <function ApplDbValidator.__del__ at 0x7f785b4e4af0>
2022-07-01T18:52:06.9803618Z Traceback (most recent call last):
2022-07-01T18:52:06.9804045Z   File "/agent/_work/1/s/tests/conftest.py", line 164, in __del__
2022-07-01T18:52:06.9804498Z     neighbors = self.get_keys(self.NEIGH_TABLE)
2022-07-01T18:52:06.9804992Z   File "/agent/_work/1/s/tests/dvslib/dvs_database.py", line 115, in get_keys
2022-07-01T18:52:06.9805499Z     table = swsscommon.Table(self.db_connection, table_name)
2022-07-01T18:52:06.9806288Z   File "/usr/lib/python3/dist-packages/swsscommon/swsscommon.py", line 2263, in __init__
2022-07-01T18:52:06.9806843Z     _swsscommon.Table_swiginit(self, _swsscommon.new_Table(*args))
2022-07-01T18:52:06.9807628Z RuntimeError: Unable to connect to redis (unix-socket): Cannot assign requested address
2022-07-01T18:52:06.9808222Z _________________ TestSubPortIntf.test_sub_port_intf_creation __________________
2022-07-01T18:52:06.9808526Z 
2022-07-01T18:52:06.9808898Z self = <test_sub_port_intf.TestSubPortIntf object at 0x7f7859d46f40>
2022-07-01T18:52:06.9809404Z dvs = <conftest.DockerVirtualSwitch object at 0x7f7859d71880>
2022-07-01T18:52:06.9809647Z 
2022-07-01T18:52:06.9809972Z     def test_sub_port_intf_creation(self, dvs):
2022-07-01T18:52:06.9810360Z >       self.connect_dbs(dvs)
2022-07-01T18:52:06.9810604Z 
2022-07-01T18:52:06.9810909Z test_sub_port_intf.py:585: 

@stephenxs
Copy link
Collaborator Author

failure is tracked by issue #2365

orchagent/portsorch.cpp Outdated Show resolved Hide resolved
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
@stephenxs
Copy link
Collaborator Author

Failure is not related to PR. Retried.


        Reads at most n bytes from socket
        """
    
        recoverable_errors = (errno.EINTR, errno.EDEADLK, errno.EWOULDBLOCK)
    
        if six.PY3 and not isinstance(socket, NpipeSocket):
>           select.select([socket], [], [])
E           ValueError: filedescriptor out of range in select()

/usr/local/lib/python3.8/dist-packages/docker/utils/socket.py:31: ValueError
---------------------------- Captured stdout setup -----------------------------
remove extra link dummy
---------------------------- Captured stderr setup -----------------------------
Exception ignored in: <function ApplDbValidator.__del__ at 0x7f15d5e80820>
Traceback (most recent call last):
  File "/agent/_work/1/s/tests/conftest.py", line 164, in __del__
    neighbors = self.get_keys(self.NEIGH_TABLE)
  File "/agent/_work/1/s/tests/dvslib/dvs_database.py", line 115, in get_keys
    table = swsscommon.Table(self.db_connection, table_name)
  File "/usr/lib/python3/dist-packages/swsscommon/swsscommon.py", line 2331, in __init__
    _swsscommon.Table_swiginit(self, _swsscommon.new_Table(*args))
RuntimeError: Unable to connect to redis (unix-socket): Cannot assign requested address
=============================== warnings summary ===============================

@stephenxs
Copy link
Collaborator Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…FEC mode is supported before setting it

<!--
Please make sure you have read and understood the contribution guildlines:
https://github.com/Azure/SONiC/blob/gh-pages/CONTRIBUTING.md

1. Make sure your commit includes a signature generted with `git commit -s`
2. Make sure your commit title follows the correct format: [component]: description
3. Make sure your commit message contains enough details about the change and related tests
4. Make sure your pull request adds related reviewers, asignees, labels

Please also provide the following information in this pull request:
-->

**What I did**
Expose supported FEC modes to `STATE_DB.PORT_TABLE|<port>.supported_fecs`.
The orchagent calls `get_port_attribute` to get attribute `SAI_PORT_ATTR_SUPPORTED_FEC_MODE` during initialization and then records it into internal data.
   - By default, the supported FEC modes will be returned by SAI and exposed to `STATE_DB`. Eg. `rs,none` means only `rs` and `none` is supported on the port.
     - The orchagent will check whether the FEC mode is supported on the port before calling the SAI API to set it.
     - The CLI will check whether the FEC mode is in `supported_fecs` before setting FEC mode on a port to the `CONFIG_DB`
   - In case the SAI API does not support any FEC mode on the port, `N/A` will be exposed to `STATE_DB`
     - The orchagent will deny any FEC setting and prints log.
     - The CLI will deny FEC setting.
   - In case the SAI API `get_port_attribute` returns `Not implemented`
     - No check will be performed before setting a FEC mode to SAI on the port.
     - The CLI will check whether the FEC mode is defined before setting it to `CONFIG_DB`.

**Why I did it**
It is not supported to set FEC mode on some platforms. To avoid error, we need to expose the supported FEC list.

**How I verified it**
Manually test and mock test.

**Details if related**
dgsudharsan
dgsudharsan previously approved these changes Jul 10, 2022
@liat-grozovik
Copy link
Collaborator

@prsunny , @prgeor could you please help to review and signoff?

orchagent/portsorch.cpp Show resolved Hide resolved
orchagent/portsorch.h Show resolved Hide resolved
orchagent/portsorch.h Show resolved Hide resolved
orchagent/portsorch.cpp Show resolved Hide resolved
Signed-off-by: Stephen Sun <[email protected]>
Signed-off-by: Stephen Sun <[email protected]>
@liat-grozovik liat-grozovik merged commit dc8bc1c into sonic-net:master Jul 24, 2022
@stephenxs stephenxs deleted the supported_feds branch July 25, 2022 03:53
prsunny added a commit that referenced this pull request Jul 25, 2022
…whether FEC mode is supported before setting it (#2333)"

This reverts commit dc8bc1c.
prsunny added a commit that referenced this pull request Jul 25, 2022
…whether FEC mode is supported before setting it (#2333)" (#2396)

This reverts commit dc8bc1c.
*Revert "[portsorch] Expose supported FEC modes to STABE_DB and check whether FEC mode is supported before setting it"
stephenxs added a commit to stephenxs/sonic-swss that referenced this pull request Jul 27, 2022
…d check whether FEC mode is supported before setting it (sonic-net#2333)" (sonic-net#2396)"

This reverts commit 6565b50.
@yxieca
Copy link
Contributor

yxieca commented Jul 28, 2022

@stephenxs this change is an empty cherry-pick for 202205. Can you double check?

@stephenxs
Copy link
Collaborator Author

@stephenxs this change is an empty cherry-pick for 202205. Can you double check?

Thanks @yxieca.
maybe it is because the PR has been reverted by #2396 due to a compiling issue.
I removed the Request for 202205 branch and opened another PR to take the code back. I will add the tag there.

stephenxs added a commit to stephenxs/sonic-swss that referenced this pull request Aug 4, 2022
…d check whether FEC mode is supported before setting it (sonic-net#2333)" (sonic-net#2396)"

This reverts commit 6565b50.
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
…FEC mode is supported before setting it (sonic-net#2333)

- What I did
Expose supported FEC modes to STATE_DB.PORT_TABLE|<port>.supported_fecs.
The orchagent calls get_port_attribute to get attribute SAI_PORT_ATTR_SUPPORTED_FEC_MODE during initialization and then records it into internal data.
1. By default, the supported FEC modes will be returned by SAI and exposed to STATE_DB. Eg. rs,none means only rs and none is supported on the port.
   The orchagent will check whether the FEC mode is supported on the port before calling the SAI API to set it.
   The CLI will check whether the FEC mode is in supported_fecs before setting FEC mode on a port to the CONFIG_DB
2. In case the SAI API does not support any FEC mode on the port, N/A will be exposed to STATE_DB
    The orchagent will deny any FEC setting and prints log.
    The CLI will deny FEC setting.
3. In case the SAI API get_port_attribute returns Not implemented
    No check will be performed before setting a FEC mode to SAI on the port.
    The CLI will check whether the FEC mode is defined before setting it to CONFIG_DB.

- Why I did it
It is not supported to set FEC mode on some platforms. To avoid error, we need to expose the supported FEC list.

- How I verified it
Manually test and mock test.
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
…whether FEC mode is supported before setting it (sonic-net#2333)" (sonic-net#2396)

This reverts commit dc8bc1c.
*Revert "[portsorch] Expose supported FEC modes to STABE_DB and check whether FEC mode is supported before setting it"
prgeor pushed a commit that referenced this pull request Aug 8, 2022
…FEC mode is supported before setting it (#2400)

* Revert "Revert "[portsorch] Expose supported FEC modes to STABE_DB and check whether FEC mode is supported before setting it (#2333)" (#2396)"

This reverts commit 6565b50.

* Adjust the prototype of setPortFec

Signed-off-by: Stephen Sun <[email protected]>
yxieca pushed a commit that referenced this pull request Aug 11, 2022
…FEC mode is supported before setting it (#2400)

* Revert "Revert "[portsorch] Expose supported FEC modes to STABE_DB and check whether FEC mode is supported before setting it (#2333)" (#2396)"

This reverts commit 6565b50.

* Adjust the prototype of setPortFec

Signed-off-by: Stephen Sun <[email protected]>
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.

6 participants