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.

Sign up for GitHub

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

Jump to bottom

Added front panel port prefix regex to schema.h #598

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added front panel port prefix regex to schema.h #598

wants to merge 1 commit into from

Conversation

itamar-talmon
Copy link

@itamar-talmon itamar-talmon commented Apr 5, 2022 •
edited
Loading

- What I did
Removed the dependency on the "Ethernet" string in the SONiC code base and added support
for extending the front panel port name pattern.

- How I did it
1. Introduced FRONT_PANEL_PORT_PREFIX_REGEX that extends the old FRONT_PANEL_PORT_PREFIX ("Ethernet")
2. Updated all the relevant usage of the "Ethernet" throughout the code base to use the new regex pattern

- How to verify it
Pass all UT and CI testing.

- Why I did it
In order to support distinguishing between different types of front panel ports in a maintainable fashion.
Specifically, we are planning to bring up a system with 'service' ports (in addition to the regular ethernet data ports) - these
are lower speed ports that used for connection to accelerators, internal loopbacks and more.

- Related Commits and Merge Strategy
This is part of a group of related commits and should be merged first.
The full merge order is:

  1. swss-common - Added front panel port prefix regex to schema.h #598
  2. sonic-buildimage - Added support for front panel port prefix regex sonic-buildimage#10471
  3. swsssdk - Added support for front panel port prefix regex sonic-py-swsssdk#121
  4. all the rest
    https://github.com/Azure/sonic-utilities/pull/2127
    https://github.com/Azure/sonic-snmpagent/pull/251
    https://github.com/Azure/sonic-swss/pull/2223
    https://github.com/Azure/sonic-platform-daemons/pull/252
    https://github.com/Azure/sonic-platform-common/pull/274

@prsunny prsunny mentioned this pull request Apr 6, 2022
Open
This was referenced Apr 6, 2022
Open
Open
Open
Open
Open
Open
@prgeor prgeor requested review from prsunny and lguohan April 11, 2022 19:48
@prgeor
Copy link

prgeor commented Apr 11, 2022

Please run sonic-mgmt test as well

@prgeor
Copy link

prgeor commented Apr 25, 2022

- What I did Removed the dependancy on the "Ethernet" string in the SONiC code base and added support for extending the front panel port name pattern.

- How I did it 1. Introduced FRONT_PANEL_PORT_PREFIX_REGEX that extends the old FRONT_PANEL_PORT_PREFIX ("Ethernet") 2. Updated all the relevant usage of the "Ethernet" throughout the code base to use the new regex pattern

- How to verify it Pass all UT and CI testing.

Please also add a section "why you did this change"

@itamar-talmon
Copy link
Author

itamar-talmon commented May 11, 2022 •
edited
Loading

- What I did Removed the dependancy on the "Ethernet" string in the SONiC code base and added support for extending the front panel port name pattern.
- How I did it 1. Introduced FRONT_PANEL_PORT_PREFIX_REGEX that extends the old FRONT_PANEL_PORT_PREFIX ("Ethernet") 2. Updated all the relevant usage of the "Ethernet" throughout the code base to use the new regex pattern
- How to verify it Pass all UT and CI testing.

Please also add a section "why you did this change"

@prgeor Added, please check it out

qiluo-msft
qiluo-msft reviewed May 30, 2022
View reviewed changes
common/schema.h
* e.g. if we want to add an "SwitchPort" prefix, we will add a new
* #define FRONT_PANEL_SWP_PORT_PREFIX "SwitchPort"
* and update the regex as follows -
* #define FRONT_PANEL_PORT_PREFIX_REGEX "^(" FRONT_PANEL_PORT_PREFIX "|" FRONT_PANEL_SWP_PORT_PREFIX ")"
Copy link
Contributor

@qiluo-msft qiluo-msft May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why need define a SWP prefix and use |?
Is it possible just change FRONT_PANEL_PORT_PREFIX value? #Closed

Copy link
Author

@itamar-talmon itamar-talmon May 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We want to support few options in the same time (e.g. we will have something like normal_front_panel_ports and special_front_panel_ports) and if we would like to check which of possible options was matched and act differently on it somehow - it would be easier and more maintainable to have something defined to compare it to for each option.

@itamar-talmon itamar-talmon force-pushed the front_panel_port_name_regex branch 2 times, most recently from ce5b358 to 9796efb Compare May 31, 2022 11:32
kcudnik
kcudnik previously approved these changes May 31, 2022
View reviewed changes
@itamar-talmon
Copy link
Author

/azpw run Azure.sonic-swss-common

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss-common

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@itamar-talmon
Copy link
Author

@kcudnik @qiluo-msft - could you please check it out after my update (I have changed the definition in response to CR comments on related commits). Thanks!
The checkers failure are not related to my change (happens on all PRs lately) - I reported the issue to the admins.

qiluo-msft
qiluo-msft previously approved these changes Jun 16, 2022
View reviewed changes
@itamar-talmon
Copy link
Author

/azpw run Azure.sonic-swss-common

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss-common

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@itamar-talmon
Copy link
Author

/azpw run Azure.sonic-swss-common

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss-common

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@itamar-talmon
Copy link
Author

/azpw run Azure.sonic-swss-common

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss-common

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@itamar-talmon
Copy link
Author

/azpw run Azure.sonic-swss-common

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss-common

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@itamar-talmon
Copy link
Author

/azpw run Azure.sonic-swss-common

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss-common

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@itamar-talmon
Copy link
Author

@qiluo-msft - hi, I rebased my branch and I see all checks are passing now.

@lguohan
Copy link
Contributor

lguohan commented Jul 27, 2022

the overall motivation is not quite clear. can you describe what are you trying to achieve here? here is the sonic port naming convention. for new port names convention to be introduced, we need to make a proposal first here. https://github.com/sonic-net/SONiC/blob/master/doc/sonic-port-name.md

lguohan
lguohan requested changes Jul 27, 2022
View reviewed changes
Copy link
Contributor

@lguohan lguohan left a comment

Choose a reason for hiding this comment

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

we need a design doc to describe the overall efforts here.

@itamar-talmon
Copy link
Author

itamar-talmon commented Aug 2, 2022 •
edited
Loading

@lguohan @prgeor - I have added a PR with the design doc - sonic-net/SONiC#1092

@itamar-talmon itamar-talmon requested review from lguohan and removed request for prsunny September 6, 2022 13:31
@itamar-talmon
Copy link
Author

itamar-talmon commented Nov 3, 2022 •
edited
Loading

we need a design doc to describe the overall efforts here.

@lguohan @prgeor Hi, please check my (approved) PR - sonic-net/SONiC#1092

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 12, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prgeor prgeor prgeor left review comments

@qiluo-msft qiluo-msft qiluo-msft left review comments

@kcudnik kcudnik kcudnik left review comments

@lguohan lguohan Awaiting requested review from lguohan

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@itamar-talmon @prgeor @mssonicbld @lguohan @qiluo-msft @kcudnik