Skip to content

Add a new attribute SAI_BFD_SESSION_ATTR_NEXT_HOP_ID to saibfd.h#2127

Merged
tjchadaga merged 1 commit intoopencomputeproject:masterfrom
baorliu:baorliu_sh_bfd_use_nexthop
Mar 14, 2025
Merged

Add a new attribute SAI_BFD_SESSION_ATTR_NEXT_HOP_ID to saibfd.h#2127
tjchadaga merged 1 commit intoopencomputeproject:masterfrom
baorliu:baorliu_sh_bfd_use_nexthop

Conversation

@baorliu
Copy link
Copy Markdown
Contributor

@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.

A document to explain the use case for the proposed attributes:
sonic-net/SONiC#1932

@kcudnik
Copy link
Copy Markdown
Collaborator

kcudnik commented Jan 14, 2025

/app run

@kcudnik
Copy link
Copy Markdown
Collaborator

kcudnik commented Jan 14, 2025

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@kcudnik
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

kcudnik commented Jan 14, 2025

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@kcudnik
Copy link
Copy Markdown
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 <96146196+baorliu@users.noreply.github.com>

remove extra space

Signed-off-by: Baorong Liu <96146196+baorliu@users.noreply.github.com>

move new added attr to the end of enum

Signed-off-by: Baorong Liu <96146196+baorliu@users.noreply.github.com>
@baorliu baorliu force-pushed the baorliu_sh_bfd_use_nexthop branch from 3838d9d to 4fb9d00 Compare January 14, 2025 22:25
@baorliu
Copy link
Copy Markdown
Contributor Author

baorliu commented Jan 14, 2025

updated and squashed old commits to a single commit. thanks

@baorliu
Copy link
Copy Markdown
Contributor Author

baorliu commented Jan 14, 2025

/azp run

@azure-pipelines
Copy link
Copy Markdown

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

@tjchadaga
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@tjchadaga
Copy link
Copy Markdown
Collaborator

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

@baorliu
Copy link
Copy Markdown
Contributor 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.

@tjchadaga
Copy link
Copy Markdown
Collaborator

@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.

@baorliu - checking on this again. Is this needed for SAI upcoming release (v1.16)?

@baorliu
Copy link
Copy Markdown
Contributor Author

baorliu commented Feb 21, 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.

@baorliu - checking on this again. Is this needed for SAI upcoming release (v1.16)?

It is not targeting v1.16 for now. but if the proposal is approved, need to update it to different release.
I also prepared a document to explain one of use case, updated to this PR's description.

@baorliu
Copy link
Copy Markdown
Contributor Author

baorliu commented Feb 25, 2025

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

Hi @tjchadaga, we can discuss this PR in the community meeting, if there is any slot available this week or next week. I created a document to describe the use case:
sonic-net/SONiC#1932

@tjchadaga
Copy link
Copy Markdown
Collaborator

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

Hi @tjchadaga, we can discuss this PR in the community meeting, if there is any slot available this week or next week. I created a document to describe the use case: sonic-net/SONiC#1932

Scheduled for next week (3/6).

Comment thread inc/saibfd.h
* @flags CREATE_ONLY
* @default false
*/
SAI_BFD_SESSION_ATTR_USE_NEXT_HOP,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider adding a VALID ONLY condition for SAI_BFD_SESSION_ATTR_DST_MAC_ADDRESS that it is valid only when SAI_BFD_SESSION_ATTR_USE_NEXT_HOP == false

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

for the SAI_BFD_SESSION_ATTR_DST_MAC_ADDRESS, this is no validonly condition for SAI_BFD_SESSION_ATTR_HW_LOOKUP_VALID == true today.
If it is required for SAI_BFD_SESSION_ATTR_USE_NEXT_HOP == false, it can be added.

Comment thread inc/saibfd.h
@tjchadaga tjchadaga added the reviewed PR is discussed in SAI Meeting label Mar 6, 2025
@tjchadaga tjchadaga requested a review from abdosi March 6, 2025 22:25
@tjchadaga
Copy link
Copy Markdown
Collaborator

@JaiOCP - could you please help review?

@tjchadaga tjchadaga merged commit a3d6844 into opencomputeproject:master Mar 14, 2025
baorliu added a commit to baorliu/SAI that referenced this pull request Mar 14, 2025
Signed-off-by: Baorong Liu <96146196+baorliu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

reviewed PR is discussed in SAI Meeting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants