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.

Sign up for GitHub

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

Jump to bottom

[DASH] add PA Validation orch #3357

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[DASH] add PA Validation orch #3357

wants to merge 2 commits into from

Conversation

Yakiv-Huryk
Copy link
Contributor

What I did

  • New DASH PA Validation orch is responsible for PA Validation table processing
  • VnetOrch now uses the new orch to create/remove PA Validation entries
  • Update SAI mock infrastructure to support dash API

Why I did it
To support DASH PA Validation table configuration

How I verified it
Added new unit test (dashpavalidation_ut)

Details if related

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Yakiv-Huryk
Copy link
Contributor Author

This also fixes the issue #3349 via eefa641 commit

kperumalbfn
kperumalbfn reviewed Nov 22, 2024
View reviewed changes
orchagent/dash/dashpavalidationorch.cpp
new_entry = true;
vni_addresses[entry.address] = 1;
} else {
vni_addresses[entry.address]++;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this condition happen? PA validation entry addition for same {VNI, IpAddress}??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as part of DASH_VNET_MAPPING_TABLE processing:

sonic-swss/orchagent/dash/dashvnetorch.cpp

Line 373 in 1847195

auto it = pa_refcount_table_.find(pa_ref_key);

orchagent/dash/dashpavalidationorch.cpp
statuses.emplace_back();
bulker.create_entry(&statuses.back(), &sai_entry, attr_count, &attr);

gCrmOrch->incCrmResUsedCounter(entry.address.isV4() ? CrmResourceType::CRM_DASH_IPV4_PA_VALIDATION : CrmResourceType::CRM_DASH_IPV6_PA_VALIDATION);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this CRM_DASH_IPV4_PA_VALIDATION and CRM_DASH_IPV6_PA_VALIDATION include both outbound and inbound direction entries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, there is no distinction between inbound/outbound PA Validation entries on SAI/DASH level.

orchagent/dash/dashpavalidationorch.cpp
attr.value.u32 = SAI_PA_VALIDATION_ENTRY_ACTION_PERMIT;

statuses.emplace_back();
bulker.create_entry(&statuses.back(), &sai_entry, attr_count, &attr);
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we decrement vni_map refcount if there is any failure in SAI addition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SAI failure leads to the abort(), so reverting the refcount/CRM right before that doesn't have a lot of value.

sonic-swss/orchagent/saihelper.cpp

Line 621 in 1847195

handleSaiFailure(true);

If you prefer to have it (for consistency/clarity) please let me know, and I'll add it.

orchagent/dash/dashpavalidationorch.cpp

SWSS_LOG_ENTER();

statuses.reserve(toCreate.size() + toRemove.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

should we check PA validation CRM before calling bulk addition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CRM checks are done by the CRM Orch, while the Orchs that are adding/removing the resources just update the usage. The API of CRM Orch:

sonic-swss/orchagent/crmorch.h

Lines 73 to 90 in 1847195

void incCrmResUsedCounter(CrmResourceType resource);
void decCrmResUsedCounter(CrmResourceType resource);
// Increment "used" counter for the ACL table/group CRM resources
void incCrmAclUsedCounter(CrmResourceType resource, sai_acl_stage_t stage, sai_acl_bind_point_type_t point);
// Decrement "used" counter for the ACL table/group CRM resources
void decCrmAclUsedCounter(CrmResourceType resource, sai_acl_stage_t stage, sai_acl_bind_point_type_t point, sai_object_id_t oid);
// Increment "used" counter for the per ACL table CRM resources (ACL entry/counter)
void incCrmAclTableUsedCounter(CrmResourceType resource, sai_object_id_t tableId);
// Decrement "used" counter for the per ACL table CRM resources (ACL entry/counter)
void decCrmAclTableUsedCounter(CrmResourceType resource, sai_object_id_t tableId);
// Increment "used" counter for the EXT table CRM resources
void incCrmExtTableUsedCounter(CrmResourceType resource, std::string table_name);
// Decrement "used" counter for the EXT table CRM resources
void decCrmExtTableUsedCounter(CrmResourceType resource, std::string table_name);
// Increment "used" counter for the per DASH ACL CRM resources (ACL group/rule)
void incCrmDashAclUsedCounter(CrmResourceType resource, sai_object_id_t groupId);
// Decrement "used" counter for the per DASH ACL CRM resources (ACL group/rule)
void decCrmDashAclUsedCounter(CrmResourceType resource, sai_object_id_t groupId);

orchagent/dash/dashpavalidationorch.cpp
}

SWSS_LOG_ERROR("Failed to create PA validation entry for VNI %u IP %s", entry.vni, entry.address.to_string().c_str());
task_process_status handle_status = handleSaiCreateStatus((sai_api_t) SAI_API_DASH_PA_VALIDATION, status);
Copy link
Contributor

Choose a reason for hiding this comment

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

handle refcount here for any failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same as above, if you prefer to revert the refcount/CRM before the abort(), let me know.

orchagent/dash/dashvnetorch.cpp
@@ -310,7 +325,7 @@ bool DashVnetOrch::addOutboundCaToPa(const string& key, VnetMapBulkContext& ctxt
else
{
SWSS_LOG_ERROR("Invalid encap type %d for %s", action.encap_type(), key.c_str());
return false;
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

why true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The true means "error", so it will be removed from m_toSync.
While false means "ok, proceed" or "wait for another resource that is not yet available", not removing from the m_toSync.

sonic-swss/orchagent/dash/dashvnetorch.cpp

Lines 737 to 744 in 1847195

if (addVnetMap(key, ctxt))
{
it = consumer.m_toSync.erase(it);
}
else
{
it++;
}

tests/mock_tests/dash/dashpavalidation_ut.cpp
// Remove all the entries for 100, the one created by VnetOrch should still be active
config = PaValidaionConfig {"100", {}};
doConfig(pa, { }, { config });

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 add CRM test as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@Yakiv-Huryk
[DASH] fix VNET Map logic
2940340 
This fixes a bug when adding a VNET map doesn't go to the
addVnetMapPost() flow. The reason is that the addVnetMap() returns
'true' for normal flow, while it should return 'false' to keep the task is
in the m_toSync for a post processing flow.

Signed-off-by: Yakiv Huryk <[email protected]>
@Yakiv-Huryk
[DASH] add PA Validation orch
9b7a7f6 
* new DASH PA Validation orch is responsible for PA Validation table processing
* VnetOrch now uses the new orch to create/remove PA Validation entries
* update sai mock infrastructure to support dash api
* add mock tests for DASH PA Validation orch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kperumalbfn kperumalbfn kperumalbfn left review comments

@oleksandrivantsiv oleksandrivantsiv oleksandrivantsiv approved these changes

@prsunny prsunny Awaiting requested review from prsunny prsunny is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Yakiv-Huryk @mssonicbld @oleksandrivantsiv @kperumalbfn