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

Port configuration incremental update support #2192

Closed
wants to merge 8 commits into from
Closed

Port configuration incremental update support #2192

wants to merge 8 commits into from

Conversation

Junchao-Mellanox
Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox commented Mar 22, 2022 •
edited
Loading

HLD: sonic-net/SONiC#985

What I did

  1. portsyncd no longer handle port configuration change and put it to APP DB
  2. Implement incremental configuration change in portmgr
  3. Adjust portsorch to meet incremental configuration change requirement

Why I did it

  1. portsyncd and portmgrd both handles port configuration change, this causes unnecessary handling in ports orch
  2. portmgrd shall do increment configuration update

How I verified it

  1. manual test
  2. regression test
  3. added new unit test case

Details if related

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prsunny
Copy link
Collaborator

prsunny commented Mar 23, 2022

Could you please provide HLD or any issue submitted for this PR?

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

Could you please provide HLD or any issue submitted for this PR?

@prsunny the design was done over emails. We can share the detailed in the PR or sent it over google group. I don't think any change in the current design goes with an HLD. What do you say?

@prsunny prsunny requested a review from prgeor March 24, 2022 22:25
dgsudharsan
dgsudharsan previously approved these changes Mar 24, 2022
View reviewed changes
@Junchao-Mellanox
Copy link
Collaborator Author

Wait for #2195 to fix the DPB vs test failure.

@prsunny
Copy link
Collaborator

prsunny commented Mar 25, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Collaborator Author

/azp run Azure.sonic-swss

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2192 in repo Azure/sonic-swss

@prgeor
Copy link
Contributor

prgeor commented Mar 29, 2022

@Junchao-Mellanox are these changes in this context ?

@Junchao-Mellanox
Copy link
Collaborator Author

@Junchao-Mellanox are these changes in this context ?

Yes

@Junchao-Mellanox
Copy link
Collaborator Author

/azp run Azure.sonic-swss

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2192 in repo Azure/sonic-swss

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-swss

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prgeor
Copy link
Contributor

prgeor commented Apr 21, 2022

@qiluo-msft could you review?

@prsunny
Copy link
Collaborator

prsunny commented Apr 21, 2022

IMO, this is a critical change and may have wider implications. IIRC, there are legacy reasons why portsyncd is listening to CONFIG_DB table. For e.g, some implementations configure kernel MTU differently than H/W MTU. In such cases, if portsyncd is the source of truth, then H/W MTU will be same as kernel MTU. Also, I don't see the HLD reviewed/signed-off. This needs to have some thorough review.

@prgeor
Copy link
Contributor

prgeor commented Apr 21, 2022

HLD: Azure/SONiC#985

I am not sure if auto-neg HLD would be the right place to put this design change which is not AN specific.

@liat-grozovik
Copy link
Collaborator

/azp run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

@prgeor kindly reminder to review and signoff

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Junchao-Mellanox
Junchao-Mellanox commented May 27, 2022
View reviewed changes
portsyncd/portsyncd.cpp
}
port_cfg_map[key] = entry;
}
handlePortConfig(p, port_cfg_map);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

put a log here to prove that: one field change will trigger all port configuration change

@prsunny
Copy link
Collaborator

prsunny commented May 27, 2022

Discussion notes:

  1. Split portsyncd change to another PR.
  2. In the same PR, change portmgr to add more attributes and remove config_cache from cgrmgr
  3. For every port config change, portsyncd loops through the portcfg as the SELECT is invoked. Junchao to attach logs that shows any port attribute change shall invoke "handlePortConfig" function
  4. Revisit the portsorch to only handle the final admin state with a m_config_admin_status.

@Junchao-Mellanox
Copy link
Collaborator Author

Close this. Will create new PR.

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

@dgsudharsan dgsudharsan dgsudharsan left review comments

@keboliu keboliu keboliu left review comments

@prsunny prsunny Awaiting requested review from prsunny prsunny is a code owner

@qiluo-msft qiluo-msft Awaiting requested review from qiluo-msft

@prgeor prgeor Awaiting requested review from prgeor

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

Successfully merging this pull request may close these issues.
7 participants
@Junchao-Mellanox @mssonicbld @prsunny @liat-grozovik @prgeor @dgsudharsan @keboliu