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

Initial version of APPL STATE DB & Response Path HLD #846

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mint570
Copy link
Collaborator

@mint570 mint570 commented Aug 18, 2021

No description provided.

### Functional Requirements

* A notification channel to notify the applications that their requests have been fulfilled or not. The result should carry the status code and failure reason in case of failure.
* The new DB that represents the real system state for applications to access the system state information.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we support multiple application to subscribe the same app state db channel?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes.
APPL STATE DB is just a regular Redis hash. There is no special channel, nothing special on it.
Applications can use the Redis key space notification channel to subscribe to APPL STATE DB.
SONiC already has a library for this purpose: SubscriberStateTable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot to mention that the SubscriberStateTable API can support multiple clients.
So yes, multiple applications can subscribe/listen to the same APPL STATE DB table.

We are working in progress to update this doc to address some of the review comments in the last meeting.


A list of return codes will be defined for application layer response. The string representation of the code is encoded in the "operation" field in the notification channel as mentioned before. For SAI errors, orchagent will map the SAI error into the corresponding return code.

<table>
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use markdown format here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

<tr>
<td>SWSS_RC_IN_USE
</td>
<td>SAI_STATUS_OBJECT_IN_USE
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a state about in progress? for example, route sync is waiting for nexthop object to be syncd.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is designed to be synchronous. The request comes form application all the way down to the hardware; and the response comes back to the application.

In p4orch, we avoid retry. If we retry, we should not send the response. We should only send one response for each request. So an "in progress" error code does not sound synchronous. Should we send another response after it completes? If a request failed due to missing dependencies at the moment, p4orch will return "invalid param" and not retry.

In the P4RT use case, controller won't send a batch request with internal dependencies. So dependency is not an issue in the P4RT request. That's one reason that p4orch does not do retry like other orchs.

Choose a reason for hiding this comment

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

To be more specific, the P4 programming is synchronous, that's what Runming is describing the behavior of P4Orch using the response path. For existing orch that does retry, and if the applications want to know an request is being retry or not, we can extend the design to add a "status" attribute (pending/done...etc). But that is currently not in the scope of the initial MVP

@mint570 mint570 marked this pull request as ready for review October 13, 2021 22:38
@mint570
Copy link
Collaborator Author

mint570 commented Oct 13, 2021

@lguohan
PTAL
Thanks


## Configuration and management

A new configuration file will be added to enable the response channel and the APPL STATE DB for the selected APPL DB tables. There are two configurations: response channel and APPL STATE DB. They are defined in separate section of the configuration file. The configuration will contain a list of APPL DB table names. Only the tables in the list will have the feature enabled.
Copy link

Choose a reason for hiding this comment

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

Can you please list the location of this new configuration file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@donNewtonAlpha

We did not implement this approach.
We used a different approach to configure it in the request entry.
I have updated this section.

Copy link

Choose a reason for hiding this comment

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

Thanks for the update. It looks like some events in P4RT depend on updates from PORT_TABLE in APPL_STATE_DB. How will they now work?
A better question is how will non P4RT created objects get added to APPL_STATE_DB?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the MVP release, the response path support only applies to P4Orch and VrfOrch.
Non-p4rt objects are not in APPL STATE DB yet. They will need code change in each orch.

P4RT will wait for all ports ready before it starts.

@zhangyanzhao
Copy link
Collaborator

@mint570 can you please sign the EasyCLA which is required to merge your code? Thanks.

@zhangyanzhao
Copy link
Collaborator

@mint570 can you please sign the EasyCLA which is required to merge this PR? Thanks.

@mint570
Copy link
Collaborator Author

mint570 commented Feb 28, 2023

My account has signed EasyCLA already. Just need to rebase the 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.

None yet

5 participants