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

add HLD md document on add/remove ports dynamically feature #1

Closed
wants to merge 2 commits into from

Conversation

tomer-israel
Copy link
Owner

No description provided.

@tomer-israel tomer-israel force-pushed the port-add-del-dynamically branch 3 times, most recently from fce5c63 to d3fc101 Compare October 3, 2021 14:49
3. Portsyncd is adding the new port info to App DB
4. On portsorch (orchagent) - Port set event is received from App DB.
5. Portsorch is creating the port on SAI.
6. SDK is creating the port and the host interfaces.

Choose a reason for hiding this comment

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

We can add complete steps [not necessary though]...I mean b/w SDK and Netlink.

Copy link
Owner Author

Choose a reason for hiding this comment

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

i will add the complete steps

- Del port: Receive del port operation from port config table, remove this port from APP DB.

#### Sflowmgr:
Add port: Event from config db - Update the speed to sflow internal db.

Choose a reason for hiding this comment

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

Did not get this part , we need to remove speed or polling rate?

Copy link
Owner Author

Choose a reason for hiding this comment

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

its a description of what sflowmgr is doing as a result of a change on config db ports table - it's not a change that we are planning to add.
I will try to change it in a way that it will be more clear.

https://github.com/Azure/sonic-buildimage/pull/8422 <br />
https://github.com/Azure/sonic-platform-daemons/pull/212

#### snampagent:

Choose a reason for hiding this comment

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

Say is SNMP data is fetcched via a remote client for this port and later on port is deleted, what will be MIB entries....STALE or REMOVED

Copy link
Owner Author

Choose a reason for hiding this comment

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

after removing a port the MIB will remove also. the fetched snmp data should be without this port.


We have also possible way for race condition:

![possible buffermgr delete port race condition](images/buffermgr_possible_delete_race.png)

Choose a reason for hiding this comment

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

I feel, there will be multiple such problems, can we change portsorch code to not remove "it entry in map" for buffer till port is added. Even after that we may have problems, If del port is followed by add Port immediately. We may need to validate the config in that case.

Copy link
Owner Author

Choose a reason for hiding this comment

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

"..can we change portsorch code to not remove "it entry in map" for buffer till port is added.." - what is it mean?

" If del port is followed by add Port immediately. We may need to validate the config in that case." - why del port and add port immediately is problematic? can you explain why do you think we need to validate from config?

@liat-grozovik
Copy link

liat-grozovik commented Nov 11, 2021

@tomer-israel this HLD is in your own fork.
Please provide a public PR against aZure, add as comment the feedback and the discussion so it is not lost and refer to the PRs need to be reviewed as well.

@tomer-israel
Copy link
Owner Author

moving to external PR

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.

3 participants