DASH SAI record for remove fails if key specified

[chrispsommers]

The following test case fails on the remove operation. I found experimentally if I delete the extraneous information after the "op" it passes. All I can see in the DUT switch trace (DASH bmv2) is "Match key not found." The sai-thrift -server trace shows a P4Runtime error:

GRPC call Write::DELETE ERROR: 
table_id: 38612462 match { field_id: 1 exact { value: "\000\252\252\252\273\000" } }

I assume this is some kind of key, but I can't associate it with anything SAI Challenger is doing because I can't see any trace info.

I suppose there needs to be some clear rules regarding use of "name" vs "key" in a DASH sai record. Please document.

import json
import time
from pathlib import Path
from pprint import pprint

import pytest

# Constants
SWITCH_ID = 5


class TestSaiVnetVni:

    def test_vnet_eni_ether_address_create(self, dpu):

        commands = [
            {
                "name": "eni_ether_address_map_entry",
                "op": "create",
                "type": "SAI_OBJECT_TYPE_ENI_ETHER_ADDRESS_MAP_ENTRY",
                "key": {
                    "switch_id": "$SWITCH_ID",
                    "address": "00:AA:AA:AA:AA:00"
                },
                "attributes": [
                    "SAI_ENI_ETHER_ADDRESS_MAP_ENTRY_ATTR_ENI_ID",
                    "1"
                ]
            },
        ]
        result = [*dpu.process_commands(commands)]
        print("\n======= SAI commands RETURN values create =======")
        pprint(result)


    def test_vnet_eni_ether_address_remove(self, dpu):

        commands = [
            {
                "name": "eni_ether_address_map_entry",
                "op": "remove",
                # Keeping entries below this causes test to fail; removing makes it pass
                "type": "SAI_OBJECT_TYPE_ENI_ETHER_ADDRESS_MAP_ENTRY",
                "key": {
                    "switch_id": "$SWITCH_ID",
                    "address": "00:AA:AA:AA:BB:00"
                },
                "attributes": [
                    "SAI_ENI_ETHER_ADDRESS_MAP_ENTRY_ATTR_ENI_ID",
                    "$eni_id"
                ]
            },
        ]

        result = [*dpu.process_commands(commands)]
        print("\n======= SAI commands RETURN values remove =======")
        pprint(result)

[anton7811]

It looks like BMv2 does not return failure on the inbound routing entry creation. I verified and see that it returns the correct structure. I even tried to create the same working entry multiple times and saw that it returns each time the new OID. But t shows an error in the saithrift server console: "Match entry exists".

But on the other hand, I found that for remove() it does return the status and it does not check it.
Here is a suggested fix:

diff --git a/common/sai_client/sai_thrift_client/sai_thrift_client.py b/common/sai_client/sai_thrift_client/sai_thrift_client.py
index 8386b5f..3d2b72f 100644
--- a/common/sai_client/sai_thrift_client/sai_thrift_client.py
+++ b/common/sai_client/sai_thrift_client/sai_thrift_client.py
@@ -22,6 +22,12 @@ def assert_status(method):
                 # TODO: pick correct STATUS
                 return sai_headers.SAI_STATUS_FAILURE
         if do_assert and result is not None:
+            # print("\t###", method.__name__, args, kwargs, '-->', result)
+            if method.__name__ != 'create' and isinstance(result, int):
+                # Create returns int, but it is OID
+                assert result == sai_headers.SAI_STATUS_SUCCESS, f"{method.__name__}({args}, {kwargs}) --> {result}"
+            # elif isinstance(result, SaiData):
+                # Returned SAI value - keys, OIDs, get values, etc. No reason to do an assert on success.
             return result
         else:
             return sai_headers.SAI_STATUS_SUCCESS

I will appreciate if you can test it. Especially if you have any other implementation except BMv2 that might return failure on create(). In this fix, I do not check the create() status. It works well for remove() only. FYI: do_assert=True by default, so it will be always verifying status in your case.

@andriy-kokhan maybe you know what should return create() in case of failure?

[andriy-kokhan]

@anton7811 , as per initial design, the create() API returns:

• OID or None on success, in case do_assert=True (

  SAI-Challenger/common/sai.py

  Line 400 in 499848f

  return vid

  )
• (status, OID or None) in case do_assert=False (

  SAI-Challenger/common/sai.py

  Line 402 in 499848f

  return status[2], vid

  )

It's expected that when create() is executed with do_assert=False, it means that the application/test will check returned status instead of relying on the assertion inside CRUD API.

[chrispsommers]

Someone please explain this do_assert parameter, is it relevant for tests using process_commands()?

It looks like BMv2 does not return failure on the inbound routing entry creation. I verified and see that it returns the correct structure. I even tried to create the same working entry multiple times and saw that it returns each time the new OID. But t shows an error in the saithrift server console: "Match entry exists".

But on the other hand, I found that for remove() it does return the status and it does not check it. Here is a suggested fix:

diff --git a/common/sai_client/sai_thrift_client/sai_thrift_client.py b/common/sai_client/sai_thrift_client/sai_thrift_client.py
index 8386b5f..3d2b72f 100644
--- a/common/sai_client/sai_thrift_client/sai_thrift_client.py
+++ b/common/sai_client/sai_thrift_client/sai_thrift_client.py
@@ -22,6 +22,12 @@ def assert_status(method):
                 # TODO: pick correct STATUS
                 return sai_headers.SAI_STATUS_FAILURE
         if do_assert and result is not None:
+            # print("\t###", method.__name__, args, kwargs, '-->', result)
+            if method.__name__ != 'create' and isinstance(result, int):
+                # Create returns int, but it is OID
+                assert result == sai_headers.SAI_STATUS_SUCCESS, f"{method.__name__}({args}, {kwargs}) --> {result}"
+            # elif isinstance(result, SaiData):
+                # Returned SAI value - keys, OIDs, get values, etc. No reason to do an assert on success.
             return result
         else:
             return sai_headers.SAI_STATUS_SUCCESS

I will appreciate if you can test it. Especially if you have any other implementation except BMv2 that might return failure on create(). In this fix, I do not check the create() status. It works well for remove() only. FYI: do_assert=True by default, so it will be always verifying status in your case.

@andriy-kokhan maybe you know what should return create() in case of fail

Hi @anton7811 I believe you answered a different issue I reported to you directly in Slack. I've filed #30 so perhaps yo can move your answer there.

Nobody has responded to the actual problem I'm reporting here (Issue 29) which is about what happens if in a remove operation, the user provides both a name and a key, the remove fails. It works if just name is provided.