Skip to content

Conversation

@theasianpianist
Copy link
Contributor

What I did
Only set certain CA to PA attributes if they are valid for a given mapping type

Why I did it
Some SAI attributes are only valid for privatelink and some are only valid for non-privatelink. This is enforced by the SAI metadata layer, so add this check in orchagent to prevent crashes.

How I verified it

Details if related

Copilot AI review requested due to automatic review settings December 3, 2025 02:05
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR reorganizes the DASH vnet orchestration code to conditionally set SAI attributes for outbound CA to PA entries based on routing type (privatelink vs non-privatelink). This prevents crashes that occur when attempting to set attributes that are only valid for specific routing types, as enforced by the SAI metadata layer.

Key changes:

  • Refactored attribute setting logic to separate privatelink-specific attributes from non-privatelink-specific attributes
  • Improved test error messages to include operation type (SET/DEL)
  • Added explicit return values to mock expectations in tests

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
orchagent/dash/dashvnetorch.cpp Reorganizes CA to PA attribute setting based on routing type; moves encap_type and tunnel_key extraction to variables for conditional use
tests/mock_tests/mock_dash_orch_test.cpp Changes op variable from auto to std::string and includes operation type in error messages
tests/mock_tests/dashvnetorch_ut.cpp Adds explicit Return(SAI_STATUS_SUCCESS) to mock expectations for better test clarity

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@prabhataravind prabhataravind left a comment

Choose a reason for hiding this comment

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

lgtm. Please verify this on cisco SS as well.

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