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

[DASH] Add Routing Group API #2026

Merged
merged 1 commit into from
Jun 27, 2024
Merged

Conversation

marian-pritsak
Copy link
Contributor

Add intermediate container object for outbound
routing entries to attach them in all together
to ENI.

@@ -81,6 +82,8 @@ typedef enum _sai_api_extensions_t

SAI_API_DASH_HA,

SAI_API_DASH_ROUTING_GROUP,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this is not added at the end ? such changes will not be backward compatible later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@r12f is there a way to control ordering in this enum?

Copy link
Contributor

Choose a reason for hiding this comment

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

The current logic of generation is that we will scan all the existing attributes and ignore if the attribute is already added.

Since you are hitting this issue, it should be caused by you have generated the SAI headers in multiple branches without resetting. If you revert all the SAI changes by git reset --hard and regenerate it, it will be added in the end.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@r12f is there a way to control ordering in this enum?

you can assign number to enum explicitly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no longer a separate API, this change is removed

@@ -74,6 +74,8 @@ typedef enum _sai_object_type_extensions_t

SAI_OBJECT_TYPE_HA_SCOPE,

SAI_OBJECT_TYPE_ROUTING_GROUP,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this is not added at the end ? such changes will not be backward compatible later

Copy link
Contributor

Choose a reason for hiding this comment

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

this is the same problem as the API type. we need to reset the generated SAI header before generating the new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to the end thanks to new api gen infra.

@prsunny prsunny self-requested a review June 5, 2024 20:31
* @flags CREATE_AND_SET
* @default false
*/
SAI_ROUTING_GROUP_ATTR_ADMIN_STATE = SAI_ROUTING_GROUP_ATTR_START,
Copy link
Contributor

@r12f r12f Jun 6, 2024

Choose a reason for hiding this comment

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

I remember this should be changed to disabled instead of admin state. But somehow it is missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to disabled

Copy link
Contributor

@r12f r12f left a comment

Choose a reason for hiding this comment

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

Hi Marian, please check my comment in the PR. We will need some update in the code.

Copy link
Contributor

@r12f r12f left a comment

Choose a reason for hiding this comment

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

Hi Marian, please check my comment in the PR. We will need some update in the code.

_In_ uint32_t attr_count,
_Inout_ sai_attribute_t *attr_list);

typedef struct _sai_dash_routing_group_api_t
Copy link
Contributor

Choose a reason for hiding this comment

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

since the routing entry in DASH is outbound_routing_entry, do you mind to change this to outbound_routing_group to make them align?

also, I believe it is better to merged into outbound routing header, we can use the annotation dash_api in the p4 file to specify the API group.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed and merged

@marian-pritsak marian-pritsak force-pushed the rg branch 3 times, most recently from d5b973b to 27ba5ae Compare June 27, 2024 05:22
Signed-off-by: Marian Pritsak <marianp@mellanox.com>
@prsunny prsunny merged commit 18ba20f into opencomputeproject:master Jun 27, 2024
3 checks passed
siqbal1986 pushed a commit to siqbal1986/SAI that referenced this pull request Sep 30, 2024
Add intermediate container object for outbound
routing entries to attach them in all together to ENI.

Signed-off-by: siqbal1986 <shahzad.iqbal@microsoft.com>
erohsik pushed a commit to erohsik/SAI that referenced this pull request Nov 7, 2024
Add intermediate container object for outbound 
routing entries to attach them in all together to ENI.
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.

4 participants