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

Switch scoped tunnel attributes #1173

Merged
merged 4 commits into from
Feb 22, 2021
Merged

Switch scoped tunnel attributes #1173

merged 4 commits into from
Feb 22, 2021

Conversation

JaiOCP
Copy link
Contributor

@JaiOCP JaiOCP commented Dec 9, 2020

This PR is the closure of discussion in
#1153

There is switch level tunnel object which can be set for various attributes per tunnel type.

Signed-off-by: Jai Kumar <jai.kumar@broadcom.com>
Signed-off-by: Jai Kumar <jai.kumar@broadcom.com>
inc/saiswitch.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@itaibaz itaibaz left a comment

Choose a reason for hiding this comment

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

I think we need to add a short comment about precedence of specific tunnel ECN settings over switch tunnel type settings

inc/saitypes.h Outdated Show resolved Hide resolved
inc/saitypes.h Outdated Show resolved Hide resolved
* @objects SAI_OBJECT_TYPE_SWITCH_TUNNEL
* @default empty
*/
SAI_SWITCH_ATTR_TUNNEL_OBJECTS_LIST,
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 have this as read only and have switch object ID as attribute in the sai_switch_tunnel_object? This will make it inline with other objects

Copy link
Contributor Author

@JaiOCP JaiOCP Dec 11, 2020

Choose a reason for hiding this comment

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

Apparently switch object can not be used as an attribute.
ASSERT FAILED (on line 779): SAI_SWITCH_TUNNEL_ATTR_SWITCH_OBJECT: switch object type can't be used as object type in any attribute
switch_id is passed as a parameter in the create API we can use it as binding but thats implicit assumption.

Copy link
Collaborator

@kcudnik kcudnik Dec 11, 2020

Choose a reason for hiding this comment

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

yes, switch_id is passed when create object function is called, so you already have it.
i don't see a point to have object_type_switch on any attribute, and even if you want to get switch id from tunnel object id, then use sai_switch_id_query()

what was your use case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kcudnik Sudharsan was thinking if we can create a binding of switch object in SAI_OBJECT_TYPE_SWITCH_TUNNEL. I think thats implicit.
@dgsudharsan Sudharsan, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, thats implicit

Signed-off-by: Jai Kumar <jai.kumar@broadcom.com>
@JaiOCP
Copy link
Contributor Author

JaiOCP commented Dec 11, 2020

I think we need to add a short comment about precedence of specific tunnel ECN settings over switch tunnel type settings

Done
1ed0f2e

/**
* @brief Defines tunnel encap ECN mode
*/
typedef enum _sai_tunnel_encap_ecn_mode_t
Copy link
Collaborator

Choose a reason for hiding this comment

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

why to move those enums to saiswitch.h instead of #include "saiswitch.h" here?

@rlhui
Copy link
Collaborator

rlhui commented Jan 7, 2021

@kcudnik , @itaibaz , would you please sign-off? Thanks.

@JaiOCP
Copy link
Contributor Author

JaiOCP commented Jan 13, 2021

@itaibaz Please approve this PR. I have another set of changes on top this coming.

inc/saiswitch.h Show resolved Hide resolved
inc/saiswitch.h Show resolved Hide resolved
@rlhui
Copy link
Collaborator

rlhui commented Jan 20, 2021

@itaibaz - reminder if there's anything needed from your end on this PR, or you'd sign off? Thanks.

@itaibaz
Copy link
Collaborator

itaibaz commented Jan 20, 2021

@itaibaz - reminder if there's anything needed from your end on this PR, or you'd sign off? Thanks.

There are 2 open comments from Marian, that needs to be addressed before we can sign off

Signed-off-by: Jai Kumar <jai.kumar@broadcom.com>
@JaiOCP
Copy link
Contributor Author

JaiOCP commented Feb 19, 2021

@rlhui Please merge this PR

@rlhui rlhui merged commit b2d4c9a into opencomputeproject:master Feb 22, 2021
@JaiOCP JaiOCP deleted the patch-9 branch February 24, 2021 17:21
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.

6 participants