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 a new attribute SAI_BFD_SESSION_ATTR_NEXT_HOP_ID to saibfd.h #2127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

baorliu
Copy link

@baorliu baorliu commented Jan 13, 2025

<re-created the PR, because the original one #2117 was using master branch in forked repo cause the commits were lost when do fork sync.
Please see comments in #2117 for review history>

Adding a new attribute SAI_BFD_SESSION_ATTR_NEXT_HOP_ID to saibfd.h to support forwarding single hop bfd packet to specific nexthop.

The proposed usage is:
1, this attribute can be provided both in create_bfd_session and set_bfd_session_attribute.
2, if SAI_BFD_SESSION_ATTR_USE_NEXT_HOP (optional, default is false) is set to true, BFD session will get next hop from SAI_BFD_SESSION_ATTR_NEXT_HOP_ID value and forward the bfd packet to the next hop. If SAI_BFD_SESSION_ATTR_USE_NEXT_HOP is false, attribute SAI_BFD_SESSION_ATTR_NEXT_HOP_ID will be ignored.
3, when using both SAI_BFD_SESSION_ATTR_HW_LOOKUP_VALID and SAI_BFD_SESSION_ATTR_USE_NEXT_HOP, the implementation is vender dependent.

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 14, 2025

/app run

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 14, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 14, 2025

Skipping cap file sample.cap
WARNING: too many spaces after *\s+ saibfd.h 167: * @condition SAI_BFD_SESSION_ATTR_HW_LOOKUP_VALID == true and SAI_BFD_SESSION_ATTR_USE_NEXT_HOP == false
Running

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 14, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 14, 2025

you are causing enum shift:

enum SAI_BFD_SESSION_ATTR_NEXT_HOP_ID only defined in ../inc//saibfd.h:453
ERROR: value of SAI_BFD_SESSION_ATTR_SELECTIVE_COUNTER_LIST differ: ../inc//saibfd.h:473 vs temp/inc//saibfd.h:455 => (44 != 42)
ERROR: value of SAI_BFD_SESSION_ATTR_STATS_COUNT_MODE differ: ../inc//saibfd.h:461 vs temp/inc//saibfd.h:443 => (43 != 41)
enum SAI_BFD_SESSION_ATTR_USE_NEXT_HOP only defined in ../inc//saibfd.h:443
ERROR: value of SAI_BFD_SESSION_ATTR_SELECTIVE_COUNTER_LIST differ: temp/inc//saibfd.h:455 vs ../inc//saibfd.h:473 => (42 != 44)
ERROR: value of SAI_BFD_SESSION_ATTR_STATS_COUNT_MODE differ: temp/inc//saibfd.h:443 vs ../inc//saibfd.h:461 => (41 != 43)
ERROR: please correct all 4 error(s) before continue

all attributes needs to be added at the end, also you need to force push to single commit all changes with enum shift

Signed-off-by: Baorong Liu <[email protected]>

remove extra space

Signed-off-by: Baorong Liu <[email protected]>

move new added attr to the end of enum

Signed-off-by: Baorong Liu <[email protected]>
@baorliu baorliu force-pushed the baorliu_sh_bfd_use_nexthop branch from 3838d9d to 4fb9d00 Compare January 14, 2025 22:25
@baorliu
Copy link
Author

baorliu commented Jan 14, 2025

updated and squashed old commits to a single commit. thanks

@baorliu
Copy link
Author

baorliu commented Jan 14, 2025

/azp run

Copy link

Commenter does not have sufficient privileges for PR 2127 in repo opencomputeproject/SAI

@tjchadaga
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tjchadaga
Copy link
Collaborator

@baorliu - would you like to discuss this PR in this week's community meeting?

@baorliu
Copy link
Author

baorliu commented Jan 16, 2025

@baorliu - would you like to discuss this PR in this week's community meeting?

in the middle of validating it in a system now. we can discuss it after I verify the use case with the new attributes.

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