From a821af0421de316555146615cffd4108ec566e20 Mon Sep 17 00:00:00 2001
From: bingwang-ms <66248323+bingwang-ms@users.noreply.github.com>
Date: Mon, 22 Apr 2024 13:23:29 -0700
Subject: [PATCH] [ACL] Remove flex counter when updating ACL rule (#3118)

What I did
This PR is to fix sonic-net/sonic-buildimage#18719

When ACL rule is created for the first time, a flex counter is created and registered. When the same ACL rule is being updated, the FlexCounter created before is not removed, and another FlexCounter is created and registered.

Why I did it
Fix the issue that FlexCounter is duplicated when updating existing ACL rule.
---
 orchagent/aclorch.cpp   |  6 ++++++
 tests/dvslib/dvs_acl.py | 18 +++++++++++++++++-
 tests/test_acl.py       | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/orchagent/aclorch.cpp b/orchagent/aclorch.cpp
index 707462799f..b31a06b10f 100644
--- a/orchagent/aclorch.cpp
+++ b/orchagent/aclorch.cpp
@@ -2585,6 +2585,12 @@ bool AclTable::add(shared_ptr<AclRule> newRule)
     if (ruleIter != rules.end())
     {
         // If ACL rule already exists, delete it first
+        if (ruleIter->second->hasCounter())
+        {
+            // Deregister the flex counter before deleting the rule
+            // A new flex counter will be created when the new rule is added
+            m_pAclOrch->deregisterFlexCounter(*(ruleIter->second));
+        }
         if (ruleIter->second->remove())
         {
             rules.erase(ruleIter);
diff --git a/tests/dvslib/dvs_acl.py b/tests/dvslib/dvs_acl.py
index 4315da3798..236ccaa0fc 100644
--- a/tests/dvslib/dvs_acl.py
+++ b/tests/dvslib/dvs_acl.py
@@ -685,6 +685,17 @@ def _match_acl_range(sai_acl_range):
             return True
 
         return _match_acl_range
+    
+    def get_acl_counter_oid(self, acl_rule_id=None) -> str:
+        if not acl_rule_id:
+            acl_rule_id = self._get_acl_rule_id()
+        
+        entry = self.asic_db.wait_for_entry("ASIC_STATE:SAI_OBJECT_TYPE_ACL_ENTRY", acl_rule_id)
+        counter_oid = entry.get("SAI_ACL_ENTRY_ATTR_ACTION_COUNTER")
+        return counter_oid
+    
+    def get_acl_rule_id(self) -> str:
+        return self._get_acl_rule_id()
 
     def _get_acl_rule_id(self) -> str:
         num_keys = len(self.asic_db.default_acl_entries) + 1
@@ -742,7 +753,12 @@ def _check_acl_entry_counters_map(self, acl_entry_oid: str):
             return
         rule_to_counter_map = self.counters_db.get_entry("ACL_COUNTER_RULE_MAP", "")
         counter_to_rule_map = {v: k for k, v in rule_to_counter_map.items()}
-        assert counter_oid in counter_to_rule_map
+        assert counter_oid in counter_to_rule_map       
+
+    def check_acl_counter_not_in_counters_map(self, acl_counter_oid: str):
+        rule_to_counter_map = self.counters_db.get_entry("ACL_COUNTER_RULE_MAP", "")
+        counter_to_rule_map = {v: k for k, v in rule_to_counter_map.items()}
+        assert acl_counter_oid not in counter_to_rule_map
 
     def verify_acl_table_status(
             self,
diff --git a/tests/test_acl.py b/tests/test_acl.py
index cf68d1516e..ed5789b2b0 100644
--- a/tests/test_acl.py
+++ b/tests/test_acl.py
@@ -1,5 +1,6 @@
 import pytest
 from requests import request
+import time
 
 L3_TABLE_TYPE = "L3"
 L3_TABLE_NAME = "L3_TEST"
@@ -131,6 +132,38 @@ def test_InvalidAclRuleCreation(self, dvs_acl, l3_acl_table):
         dvs_acl.verify_acl_rule_status(L3_TABLE_NAME, "INVALID_RULE", None)
         dvs_acl.verify_no_acl_rules()
 
+    def test_AclRuleUpdate(self, dvs_acl, l3_acl_table):
+        """The test is to verify there is no duplicated flex counter when updating an ACL rule
+        """
+        config_qualifiers = {"SRC_IP": "10.10.10.10/32"}
+        expected_sai_qualifiers = {
+            "SAI_ACL_ENTRY_ATTR_FIELD_SRC_IP": dvs_acl.get_simple_qualifier_comparator("10.10.10.10&mask:255.255.255.255")
+        }
+
+        dvs_acl.create_acl_rule(L3_TABLE_NAME, L3_RULE_NAME, config_qualifiers)
+        dvs_acl.verify_acl_rule(expected_sai_qualifiers)
+        
+        acl_rule_id = dvs_acl.get_acl_rule_id()
+        counter_id = dvs_acl.get_acl_counter_oid()
+        
+        new_config_qualifiers = {"SRC_IP": "10.10.10.11/32"}
+        new_expected_sai_qualifiers = {
+            "SAI_ACL_ENTRY_ATTR_FIELD_SRC_IP": dvs_acl.get_simple_qualifier_comparator("10.10.10.11&mask:255.255.255.255")
+        }
+        dvs_acl.update_acl_rule(L3_TABLE_NAME, L3_RULE_NAME, new_config_qualifiers)
+        # Verify the rule has been updated
+        retry = 5
+        while dvs_acl.get_acl_rule_id() == acl_rule_id and retry >= 0:
+            retry -= 1
+            time.sleep(1)
+        assert retry > 0
+        dvs_acl.verify_acl_rule(new_expected_sai_qualifiers)
+        # Verify the previous counter is removed
+        if counter_id:
+            dvs_acl.check_acl_counter_not_in_counters_map(counter_id)
+        dvs_acl.remove_acl_rule(L3_TABLE_NAME, L3_RULE_NAME)
+        dvs_acl.verify_no_acl_rules()
+
     def test_AclRuleL4SrcPort(self, dvs_acl, l3_acl_table):
         config_qualifiers = {"L4_SRC_PORT": "65000"}
         expected_sai_qualifiers = {