[acl] Update CRM to include LAG bindings for ACL tables (sonic-net#1487)

Signed-off-by: Danny Allen <daall@microsoft.com>

orchagent/aclorch.cpp

@@ -1367,6 +1367,8 @@ bool AclTable::create()
  {
  gCrmOrch->incCrmAclUsedCounter(
  CrmResourceType::CRM_ACL_TABLE, (sai_acl_stage_t)attr.value.s32, SAI_ACL_BIND_POINT_TYPE_PORT);
+ gCrmOrch->incCrmAclUsedCounter(
+ CrmResourceType::CRM_ACL_TABLE, (sai_acl_stage_t)attr.value.s32, SAI_ACL_BIND_POINT_TYPE_LAG);
  }
 
  return status == SAI_STATUS_SUCCESS;
@@ -1551,6 +1553,7 @@ bool AclTable::create()
  if (status == SAI_STATUS_SUCCESS)
  {
  gCrmOrch->incCrmAclUsedCounter(CrmResourceType::CRM_ACL_TABLE, acl_stage, SAI_ACL_BIND_POINT_TYPE_PORT);
+ gCrmOrch->incCrmAclUsedCounter(CrmResourceType::CRM_ACL_TABLE, acl_stage, SAI_ACL_BIND_POINT_TYPE_LAG);
  }
 
  return status == SAI_STATUS_SUCCESS;
@@ -2837,9 +2840,16 @@ bool AclOrch::removeAclTable(string table_id)
  if (deleteUnbindAclTable(table_oid) == SAI_STATUS_SUCCESS)
  {
  auto stage = m_AclTables[table_oid].stage;
+ auto type = m_AclTables[table_oid].type;
+
  sai_acl_stage_t sai_stage = (stage == ACL_STAGE_INGRESS) ? SAI_ACL_STAGE_INGRESS : SAI_ACL_STAGE_EGRESS;
  gCrmOrch->decCrmAclUsedCounter(CrmResourceType::CRM_ACL_TABLE, sai_stage, SAI_ACL_BIND_POINT_TYPE_PORT, table_oid);
 
+ if (type != ACL_TABLE_PFCWD)
+ {
+ gCrmOrch->decCrmAclUsedCounter(CrmResourceType::CRM_ACL_TABLE, sai_stage, SAI_ACL_BIND_POINT_TYPE_LAG, table_oid);
+ }
+
  SWSS_LOG_NOTICE("Successfully deleted ACL table %s", table_id.c_str());
  m_AclTables.erase(table_oid);
 

tests/mock_tests/aclorch_ut.cpp

@@ -593,67 +593,80 @@ namespace aclorch_test
  return true;
  }
 
- // consistency validation with CRM
+ // Validate that ACL table resource count is consistent with CRM
  bool validateResourceCountWithCrm(const AclOrch *aclOrch, CrmOrch *crmOrch)
  {
- // Verify ACL Tables
  auto const &resourceMap = Portal::CrmOrchInternal::getResourceMap(crmOrch);
- uint32_t crm_acl_table_cnt = 0;
- for (auto const &kv : resourceMap.at(CrmResourceType::CRM_ACL_TABLE).countersMap)
+
+ // Verify the ACL tables
+ uint32_t crmAclTableBindingCount = 0;
+ for (auto const &kv: resourceMap.at(CrmResourceType::CRM_ACL_TABLE).countersMap)
  {
- crm_acl_table_cnt += kv.second.usedCounter;
+ crmAclTableBindingCount += kv.second.usedCounter;
+ }
+
+ uint32_t aclorchAclTableBindingCount = 0;
+ for (auto const &kv: Portal::AclOrchInternal::getAclTables(aclOrch))
+ {
+ if (kv.second.type == ACL_TABLE_PFCWD)
+ {
+ aclorchAclTableBindingCount += 1; // port binding only
+ }
+ else
+ {
+ aclorchAclTableBindingCount += 2; // port + LAG binding
+ }
  }
 
- if (crm_acl_table_cnt != Portal::AclOrchInternal::getAclTables(aclOrch).size())
+ if (crmAclTableBindingCount != aclorchAclTableBindingCount)
  {
- ADD_FAILURE() << "ACL table size is not consistent between CrmOrch (" << crm_acl_table_cnt
- << ") and AclOrch " << Portal::AclOrchInternal::getAclTables(aclOrch).size();
+ ADD_FAILURE() << "ACL table binding count is not consistent between CrmOrch ("
+ << crmAclTableBindingCount << ") and AclOrch (" 
+ << aclorchAclTableBindingCount << ")";
  return false;
  }
 
+ // Verify ACL rules and counters
 
- // Verify ACL Rules
- //
- // for each CRM_ACL_ENTRY and CRM_ACL_COUNTER's entry => the ACL TABLE should exist
- //
- for (auto acl_entry_or_counter : { CrmResourceType::CRM_ACL_ENTRY, CrmResourceType::CRM_ACL_COUNTER })
+ // For each CRM_ACL_ENTRY and CRM_ACL_COUNTER entry, there should be a corresponding ACL table
+ for (auto aclResourceType: {CrmResourceType::CRM_ACL_ENTRY, CrmResourceType::CRM_ACL_COUNTER})
  {
- auto const &resourceMap = Portal::CrmOrchInternal::getResourceMap(crmOrch);
- for (auto const &kv : resourceMap.at(acl_entry_or_counter).countersMap)
+ for (auto const &kv: resourceMap.at(aclResourceType).countersMap)
  {
- auto acl_oid = kv.second.id;
+ auto aclOid = kv.second.id;
 
  const auto &aclTables = Portal::AclOrchInternal::getAclTables(aclOrch);
- if (aclTables.find(acl_oid) == aclTables.end())
+ if (aclTables.find(aclOid) == aclTables.end())
  {
- ADD_FAILURE() << "Can't find ACL '" << sai_serialize_object_id(acl_oid) << "' in AclOrch";
+ ADD_FAILURE() << "Can't find ACL '" << sai_serialize_object_id(aclOid)
+ << "' in AclOrch";
  return false;
  }
 
- if (kv.second.usedCounter != aclTables.at(acl_oid).rules.size())
+ if (kv.second.usedCounter != aclTables.at(aclOid).rules.size())
  {
  ADD_FAILURE() << "CRM usedCounter (" << kv.second.usedCounter
- << ") is not equal rule in ACL (" << aclTables.at(acl_oid).rules.size() << ")";
+ << ") is not equal rule in ACL ("
+ << aclTables.at(aclOid).rules.size() << ")";
  return false;
  }
  }
  }
 
- //
- // for each ACL TABLE with rule count larger than one => it shoule exist a corresponding entry in CRM_ACL_ENTRY and CRM_ACL_COUNTER
- //
- for (const auto &kv : Portal::AclOrchInternal::getAclTables(aclOrch))
+ // For each ACL table with at least one rule, there should be corresponding entries for CRM_ACL_ENTRY and CRM_ACL_COUNTER
+ for (const auto &kv: Portal::AclOrchInternal::getAclTables(aclOrch))
  {
- if (0 < kv.second.rules.size())
+ if (kv.second.rules.size() > 0)
  {
  auto key = Portal::CrmOrchInternal::getCrmAclTableKey(crmOrch, kv.first);
- for (auto acl_entry_or_counter : { CrmResourceType::CRM_ACL_ENTRY, CrmResourceType::CRM_ACL_COUNTER })
+ for (auto aclResourceType: {CrmResourceType::CRM_ACL_ENTRY, CrmResourceType::CRM_ACL_COUNTER})
  {
- const auto &cntMap = Portal::CrmOrchInternal::getResourceMap(crmOrch).at(acl_entry_or_counter).countersMap;
+ const auto &cntMap = resourceMap.at(aclResourceType).countersMap;
  if (cntMap.find(key) == cntMap.end())
  {
  ADD_FAILURE() << "Can't find ACL (" << sai_serialize_object_id(kv.first)
- << ") in " << (acl_entry_or_counter == CrmResourceType::CRM_ACL_ENTRY ? "CrmResourceType::CRM_ACL_ENTRY" : "CrmResourceType::CRM_ACL_COUNTER");
+ << ") in "
+ << (aclResourceType == CrmResourceType::CRM_ACL_ENTRY ? "CRM_ACL_ENTRY" : "CRM_ACL_COUNTER");
  return false;
  }
  }

tests/test_acl.py

@@ -450,6 +450,57 @@ def test_AclRuleRedirect(self, dvs, dvs_acl, l3_acl_table, setup_teardown_neighb
  dvs_acl.verify_no_acl_rules()
 
 
+class TestAclCrmUtilization:
+ @pytest.fixture(scope="class", autouse=True)
+ def configure_crm_polling_interval_for_test(self, dvs):
+ dvs.runcmd("crm config polling interval 1")
+
+ yield
+
+ dvs.runcmd("crm config polling interval 300")
+
+ def test_ValidateAclTableBindingCrmUtilization(self, dvs, dvs_acl):
+ counter_db = dvs.get_counters_db()
+
+ crm_port_stats = counter_db.get_entry("CRM", "ACL_STATS:INGRESS:PORT")
+ initial_acl_table_port_bindings_used = int(crm_port_stats.get("crm_stats_acl_table_used", 0))
+
+ crm_lag_stats = counter_db.get_entry("CRM", "ACL_STATS:INGRESS:LAG")
+ initial_acl_table_lag_bindings_used = int(crm_lag_stats.get("crm_stats_acl_table_used", 0))
+
+ dvs_acl.create_acl_table(L3_TABLE_NAME, L3_TABLE_TYPE, L3_BIND_PORTS)
+ dvs_acl.verify_acl_table_count(1)
+
+ counter_db.wait_for_field_match(
+ "CRM",
+ "ACL_STATS:INGRESS:PORT",
+ {"crm_stats_acl_table_used": str(initial_acl_table_port_bindings_used + 1)}
+ )
+
+ counter_db.wait_for_field_match(
+ "CRM",
+ "ACL_STATS:INGRESS:LAG",
+ {"crm_stats_acl_table_used": str(initial_acl_table_lag_bindings_used + 1)}
+ )
+
+ dvs_acl.remove_acl_table(L3_TABLE_NAME)
+ dvs_acl.verify_acl_table_count(0)
+
+ counter_db.wait_for_field_match(
+ "CRM",
+ "ACL_STATS:INGRESS:PORT",
+ {"crm_stats_acl_table_used": str(initial_acl_table_port_bindings_used)}
+ )
+
+ counter_db.wait_for_field_match(
+ "CRM",
+ "ACL_STATS:INGRESS:LAG",
+ {"crm_stats_acl_table_used": str(initial_acl_table_lag_bindings_used)}
+ )
+
+
+# TODO: Need to improve the clean-up/post-checks for these tests as currently we can't run anything
+# afterwards.
 class TestAclRuleValidation:
  """Test class for cases that check if orchagent corectly validates ACL rules input."""