Fix ACL rule creation in packet trimming tests (Broadcom)

[rgarofano-arista]

Description of PR

Summary:
Fixes # (issue)

Type of change

• Bug fix
• Testbed and Framework(new/improvement)
• New Test case
  • Skipped for non-supported platforms
• Test case improvement

Back port request

• 202205
• 202305
• 202311
• 202405
• 202411
• 202505

Approach

What is the motivation for this PR?

Packet trimming tests use acl for verifying that the disable trim action works. We couldn't get this testcase to pass on Broadcom th5. One of the issues was that acl rules weren't being created.

How did you do it?

After inspecting the sairedis logs, it was discovered that it was trying and failing to create a counter for the acl rules the test is trying to create. This is because the action list specified in the configuration does not include a counter, but the orchagent seems to attempt to create one anyways. By adding a counter to the action list we no longer fail to create the ACL rule.

How did you verify/test it?

Confirmed test_acl_action_with_trimming testcase was passing with the change.

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sdszhang]

@rgarofano-arista pls fix the pr checker failure.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[r12f]

the test change it not broadcom only, but seems to be reasonable. + @nazariig and @dgsudharsan to double confirm.

[nazariig]

the test change it not broadcom only, but seems to be reasonable. + @nazariig and @dgsudharsan to double confirm.

@rgarofano-arista the change looks to me redundant: should be working without specifying action explicitly

Please double check:
https://github.com/sonic-net/sonic-swss/blob/master/orchagent/aclorch.cpp#L2559

bool AclTable::addMandatoryActions()
...
    sai_acl_action_type_t acl_action = SAI_ACL_ACTION_TYPE_COUNTER;
    if (m_pAclOrch->isAclActionSupported(stage, acl_action))
    {
        SWSS_LOG_INFO("Add counter acl action");
        type.addAction(acl_action);
    }
...

[r12f]

Change to 202412: Azure/sonic-mgmt.msft#882

[r12f]

hey Ryan @rgarofano-arista do you mind to help check Nazarii's comment above? 202412 also has this change, so the ACL tables should be ok, but maybe something override this logic in swss?

[rgarofano-arista]

Yes, SWSS is trying to create the counter without the config. However the SAI complains, failing to create the counter if the config is not present.

[StormLiangMS]

hi @nazariig what I heard is that this PR would cause failure on nVidia platform, could you confirm? @rgarofano-arista @r12f FYI.

[kperumalbfn]

@rgarofano-arista could you check if SAI capability query fails for ACL counter?

[rgarofano-arista]

hey Ryan @rgarofano-arista do you mind to help check Nazarii's comment above? 202412 also has this change, so the ACL tables should be ok, but maybe something override this logic in swss?

@r12f This function only adds the counter action if the action list is not empty (see https://github.com/sonic-net/sonic-swss/blob/0ad109f7e573f44e73bc80021efd9701440a06da/orchagent/aclorch.cpp#L2575). In the packet trimming test we are specifying the action list in the configuration, hence we need to add the counter to the action list in the config. How is this working for Mellanox without this change?

[nazariig]

@rgarofano-arista we have tested this change. Looks good. No further concerns

[bingwang-ms]

hey Ryan @rgarofano-arista do you mind to help check Nazarii's comment above? 202412 also has this change, so the ACL tables should be ok, but maybe something override this logic in swss?

@r12f This function only adds the counter action if the action list is not empty (see https://github.com/sonic-net/sonic-swss/blob/0ad109f7e573f44e73bc80021efd9701440a06da/orchagent/aclorch.cpp#L2575). In the packet trimming test we are specifying the action list in the configuration, hence we need to add the counter to the action list in the config. How is this working for Mellanox without this change?

@rgarofano-arista I think we may need to improve orchagent to handle this scenario. If counter is a mandatory and missing it leads to crash, it should be better to be handled in orchagent.

[mssonicbld]

Cherry-pick PR to msft-202412: Azure/sonic-mgmt.msft#1029

[r12f]

Previous manual pick to 202412: Azure/sonic-mgmt.msft#882

[mssonicbld]

Cherry-pick PR to 202511: #22534