[sairedis/syncd] Implement bulk get support #1509

stepanblyschak · 2025-01-24T11:05:27Z

Implement bulk get support for SAIRedis, Syncd, SaiPlayer and VSlib.

Signed-off-by: Stepan Blyschak <[email protected]>

mssonicbld · 2025-01-24T11:05:29Z

/azp run

azure-pipelines · 2025-01-24T11:05:41Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-01-24T11:05:47Z

/azp run

azure-pipelines · 2025-01-24T11:05:58Z

Azure Pipelines successfully started running 1 pipeline(s).

meta/Meta.cpp

+
+        status = meta_generic_validation_get(meta_key, attr_count[idx], attr_list[idx]);
+
+        // FIXME: This macro returns on failure.


saiplayer/SaiPlayer.cpp

    switch (api)
    {
        case SAI_COMMON_API_BULK_CREATE:

        {
            sai_object_id_t switch_id = m_sai->switchIdQuery(local_ids[0]);
            std::vector<sai_object_id_t> ids(object_count);

            for (uint32_t it = 0; it < object_count; it++)
            {
                if (m_sai->switchIdQuery(local_ids[it]) != switch_id ||
                    switch_id == SAI_NULL_OBJECT_ID)
                {
                    SWSS_LOG_THROW("invalid switch_id translated from VID %s",
                            sai_serialize_object_id(local_ids[it]).c_str());
                }
            }

            std::vector<uint32_t> attr_counts(object_count);

            std::vector<const sai_attribute_t*> attr_lists(object_count);

            for (uint32_t idx = 0; idx < object_count; idx++)
            {
                attr_counts[idx] = attributes[idx]->get_attr_count();
                attr_lists[idx] = attributes[idx]->get_attr_list();
            }

            switch_id = translate_local_to_redis(switch_id);

            status = m_sai->bulkCreate(object_type,
                                            switch_id,
                                            object_count,
                                            attr_counts.data(),
                                            attr_lists.data(),
                                            mode,
                                            ids.data(),
                                            statuses.data());

            if (status == SAI_STATUS_SUCCESS)
            {
                for (uint32_t it = 0; it < object_count; it++)
                {
                    match_redis_with_rec(ids[it], local_ids[it]);

                    SWSS_LOG_INFO("saved VID %s to RID %s",
                            sai_serialize_object_id(local_ids[it]).c_str(),
                            sai_serialize_object_id(ids[it]).c_str());
                }
            }

            return status;
        }
        break;

        case SAI_COMMON_API_BULK_REMOVE:

        {
            std::vector<sai_object_id_t> ids(object_count);

            for (uint32_t it = 0; it < object_count; it++)
            {
                ids[it] = translate_local_to_redis(local_ids[it]);
            }

            status = m_sai->bulkRemove(object_type, object_count, ids.data(), mode, statuses.data());
        }
        break;

        case SAI_COMMON_API_BULK_SET:

        {
            std::vector<sai_object_id_t> ids(object_count);

            for (uint32_t it = 0; it < object_count; it++)
            {
                ids[it] = translate_local_to_redis(local_ids[it]);
            }

            std::vector<sai_attribute_t> attr_list;

            // route can have multiple attributes, so we need to handle them all
            for (const auto &alist: attributes)
            {
                attr_list.push_back(alist->get_attr_list()[0]);
            }

            status = m_sai->bulkSet(object_type, object_count, ids.data(), attr_list.data(), mode, statuses.data());
        }
        break;

+        case SAI_COMMON_API_BULK_GET:
+
+        {


syncd/Syncd.cpp

+            switch (api)
+            {
+            case SAI_COMMON_API_BULK_GET:
+                sendBulkGetResponse(objectType, objectIds, all, attributes, statuses);
+                break;
+            default:
+                sendApiResponse(api, all, (uint32_t)objectIds.size(), statuses.data());
+                break;
+            }


syncd/Syncd.cpp

+    switch (api)
+    {
+    case SAI_COMMON_API_BULK_GET:
+        sendBulkGetResponse(objectType, objectIds, all, attributes, statuses);
+        break;
+    default:
+        sendApiResponse(api, all, (uint32_t)objectIds.size(), statuses.data());
+        break;
+    }


syncd/VendorSai.cpp

+    switch (object_type)
+    {
+        case SAI_OBJECT_TYPE_PORT:
+            ptr = m_apis.port_api->get_ports_attribute;
+            break;

-    SWSS_LOG_ERROR("not implemented, FIXME");
+        default:
+            SWSS_LOG_ERROR("not implemented %s, FIXME", sai_serialize_object_type(object_type).c_str());
+            return SAI_STATUS_NOT_IMPLEMENTED;
+    }


kcudnik · 2025-01-24T12:32:59Z

meta/Meta.cpp

+
+        // FIXME: This macro returns on failure.
+        // When mode is SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR we should continue instead of return.
+        // This issue exists for all bulk operations.


should we fix it here for this api ? since GET operation is specific, so maybe some attributes maybe not implemented and they will definitely return failure

kcudnik · 2025-01-24T12:33:54Z

meta/SaiSerialize.cpp

@@ -90,24 +90,27 @@ static sai_status_t transfer_list(
 {
    SWSS_LOG_ENTER();

-    if (countOnly || dst_element.count == 0)


why this logic is changed ?

kcudnik · 2025-01-24T12:34:35Z

saiplayer/SaiPlayer.cpp

+                                            attr_counts.data(),
+                                            attr_lists.data(),
+                                            mode,
+                                            statuses.data()); // STATUS_NOT_EXECUTED


why this comment means not executed ?

kcudnik · 2025-01-24T12:35:43Z

saiplayer/SaiPlayer.cpp

@@ -2522,17 +2629,17 @@ int SaiPlayer::replay()

    SWSS_LOG_NOTICE("using file: %s", filename.c_str());

-    std::ifstream infile(filename);


why this logic is changed ? not need for this variable to be member of class

kcudnik · 2025-01-24T12:36:03Z

saiplayer/SaiPlayer.h

@@ -259,6 +260,8 @@ namespace saiplayer

            std::shared_ptr<CommandLineOptions> m_commandLineOptions;

+            std::ifstream m_infile;


why this is moved to member ?

kcudnik · 2025-01-24T13:00:28Z

syncd/Syncd.cpp

-    for (auto &a: statuses)
-    {
-        a = SAI_STATUS_SUCCESS;
-    }


why this is removeD ?

kcudnik · 2025-01-24T13:01:14Z

unittest/lib/TestRedisRemoteSaiInterface.cpp

@@ -4,27 +4,3 @@
 #include <gtest/gtest.h>

 using namespace sairedis;
-
-TEST(RedisRemoteSaiInterface, bulkGet)


please don't remove tests, implement correct TEST to validate your code

kcudnik · 2025-01-24T13:02:17Z

unittest/vslib/TestSwitchStateBase.cpp

@@ -69,7 +69,7 @@ TEST_F(SwitchStateBaseTest, switchHashGet)
    attr.id = SAI_HASH_ATTR_NATIVE_HASH_FIELD_LIST;
    attr.value.s32list.list = nullptr;
    attr.value.s32list.count = 0;
-    ASSERT_EQ(m_ss->get(SAI_OBJECT_TYPE_HASH, sai_serialize_object_id(ecmpHashOid), 1, &attr), SAI_STATUS_SUCCESS);
+    ASSERT_EQ(m_ss->get(SAI_OBJECT_TYPE_HASH, sai_serialize_object_id(ecmpHashOid), 1, &attr), SAI_STATUS_BUFFER_OVERFLOW);


why this suddenly chhanges status ?

kcudnik

address comments

stepanblyschak added 5 commits January 15, 2025 09:43

[sairedis/syncd] Implement bulk get support

34ea88b

Signed-off-by: Stepan Blyschak <[email protected]>

Implement bulkGet for Recorder and SaiPlayer

2fcdd29

Signed-off-by: Stepan Blyschak <[email protected]>

Fix transfer_list to return SAI_STATUS_BUFFER_OVERFLOW if dst count is 0

e4b0f58

Signed-off-by: Stepan Blyschak <[email protected]>

Add UT for BUFFER_OVERFLOW

65c38b7

Signed-off-by: Stepan Blyschak <[email protected]>

Implement bulk get in vslib

922068e

Signed-off-by: Stepan Blyschak <[email protected]>

Merge branch 'master' into bulk-get

1c8389e

github-advanced-security bot found potential problems Jan 24, 2025

View reviewed changes

kcudnik reviewed Jan 24, 2025

View reviewed changes

syncd/Syncd.cpp

for (auto &a: statuses)

{

a = SAI_STATUS_SUCCESS;

}

Copy link

Collaborator

kcudnik Jan 24, 2025

Choose a reason for hiding this comment

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

why this is removeD ?

kcudnik reviewed Jan 24, 2025

View reviewed changes

kcudnik requested changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sairedis/syncd] Implement bulk get support #1509

[sairedis/syncd] Implement bulk get support #1509

stepanblyschak commented Jan 24, 2025

mssonicbld commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

mssonicbld commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

kcudnik Jan 24, 2025

kcudnik Jan 24, 2025

kcudnik Jan 24, 2025

kcudnik Jan 24, 2025

kcudnik Jan 24, 2025

kcudnik Jan 24, 2025

kcudnik Jan 24, 2025

kcudnik Jan 24, 2025

kcudnik left a comment


		status = meta_generic_validation_get(meta_key, attr_count[idx], attr_list[idx]);

		// FIXME: This macro returns on failure.

		@@ -2522,17 +2629,17 @@ int SaiPlayer::replay()

		SWSS_LOG_NOTICE("using file: %s", filename.c_str());

		std::ifstream infile(filename);

		@@ -259,6 +260,8 @@ namespace saiplayer

		std::shared_ptr<CommandLineOptions> m_commandLineOptions;

		std::ifstream m_infile;

[sairedis/syncd] Implement bulk get support #1509

Are you sure you want to change the base?

[sairedis/syncd] Implement bulk get support #1509

Conversation

stepanblyschak commented Jan 24, 2025

mssonicbld commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

mssonicbld commented Jan 24, 2025

azure-pipelines bot commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcudnik left a comment

Choose a reason for hiding this comment