[sairedis/syncd] Implement bulk get support #1509

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

DEPENDS ON sonic-net/sonic-swss-common#966.

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/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

Could you please clarify? Attribute may be not implemented with SET as well.

meta/SaiSerialize.cpp

saiplayer/SaiPlayer.cpp

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

to use m_infile in processBulk method

i see that, but why the reason ? since this is local variable no need to upgrade it to member

SaiPlayer operates on input stream, the input stream is feeding multiple functions with data throuhgout this operation. From that point of view the lifetime of the input stream needs to match the lifetime of SaiPlayer, thus fits to be a member variable. I did this change for ergonomics primaraly - not having to pass the input stream explicitelly everywhere it is needed in downstream code. Besides, passing an input stream explicitelly implies a member function may operate on different input streams throughout SaiPlayer's lifetime which is not the case.
I can make a function parameter though if you prefer so.

@kcudnik can be resolved?

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 ?

to use m_infile in processBulk method

syncd/Syncd.cpp

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

I was not sure what this test actually checks. It seems the test was added to satisfy code coverage rather then check functionality. As for functioanlity check and code coverage I added tests to TestSyncdBrcm and added player tests.

@kcudnik Please share your thoughts on what should be in this test case instead and if needed at all since the code is covered by a set of other tests.

if this is not your tests, then why you are removing it ?

if you added code that changes ouyput of this tests, that now it can succeed, then please update test

@kcudnik Yes, the output of this call changes from returning SAI_STATUS_NOT_IMPLEMENTED to throwing an exception waitForBulkGetResponse: wrong number of statuses, got 0, expected 1 after a 60 sec timeout because no one is responding, so I don't get the reason for such a test. We have bulkGet covered by player test and syncd test

please update this test to run correctly

unittest/vslib/TestSwitchStateBase.cpp

kcudnik

address comments

… count is 0" This reverts commit e4b0f58.

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

mssonicbld · 2025-01-27T10:03:01Z

/azp run

azure-pipelines · 2025-01-27T10:03:14Z

Azure Pipelines successfully started running 1 pipeline(s).

syncd/Syncd.cpp

syncd/VendorSai.cpp

…into bulk-get Signed-off-by: Stepan Blyschak <[email protected]>

mssonicbld · 2025-01-29T09:41:44Z

/azp run

azure-pipelines · 2025-01-29T09:41:55Z

Azure Pipelines successfully started running 1 pipeline(s).

stepanblyschak · 2025-02-05T10:24:09Z

Tests are failing due to build using older swss-common which is missing PR sonic-net/sonic-swss-common#966.

Test is using swss-common built here - https://dev.azure.com/mssonic/build/_build/results?buildId=757465 which is from a previous commit.

Waiting for a new swss-common build

mssonicbld · 2025-02-06T07:47:23Z

/azp run

azure-pipelines · 2025-02-06T07:47:35Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-12T13:16:58Z

/azp run

azure-pipelines · 2025-02-12T13:17:09Z

Azure Pipelines successfully started running 1 pipeline(s).

kcudnik · 2025-02-13T15:52:06Z

please r esolve conflicts

mssonicbld · 2025-02-14T17:18:30Z

/azp run

azure-pipelines · 2025-02-14T17:18:41Z

Azure Pipelines successfully started running 1 pipeline(s).

kcudnik · 2025-02-20T15:30:20Z

/azp run

azure-pipelines · 2025-02-20T15:30:33Z

Azure Pipelines successfully started running 1 pipeline(s).

kcudnik · 2025-02-20T15:40:14Z

since that PR is implementing Bulk Support, in test file: ~/sr/unittest/lib/TestClientServerSai.cpp there is this test:

213 SAIREDIS_DECLARE_EVERY_ENTRY(TEST_ENTRY)
214
215 #define TEST_BULK_ENTRY(OT,ot)                                                                                               \
216 TEST(ClientServerSai, bulk_ ## OT)                                                                                           \
217 {                                                                                                                            \
218     auto css = std::make_shared<ClientServerSai>();                                                                          \
219     sai_ ## ot ## _t e[2] = {};                                                                                              \
220     EXPECT_EQ(SAI_STATUS_SUCCESS, css->apiInitialize(0, &test_services));                                                       \
221     EXPECT_NE(SAI_STATUS_SUCCESS, css->bulkCreate(0, e, nullptr, nullptr, SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, nullptr));    \
222     EXPECT_NE(SAI_STATUS_SUCCESS, css->bulkSet(2, e, nullptr, SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, nullptr));                \
223     EXPECT_NE(SAI_STATUS_SUCCESS, css->bulkRemove(2, e, SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, nullptr));                      \
224                                                                                                                              \
225     auto ss = std::make_shared<ClientServerSai>();                                                                           \
226     EXPECT_EQ(SAI_STATUS_SUCCESS, ss->apiInitialize(0, &test_client_services));                                                 \
227     EXPECT_NE(SAI_STATUS_SUCCESS, ss->bulkCreate(0, e, nullptr, nullptr, SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, nullptr));     \
228     EXPECT_NE(SAI_STATUS_SUCCESS, ss->bulkSet(0, e, nullptr, SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, nullptr));                 \
229     EXPECT_NE(SAI_STATUS_SUCCESS, ss->bulkRemove(0, e, SAI_BULK_OP_ERROR_MODE_IGNORE_ERROR, nullptr));                       \
230 }

which expects that all bulk apis will fail, and seems like the changes to bulk PR causing this test to fail with:

[ RUN ] ClientServerSai.DIRECTION_LOOKUP_ENTRY
Assertion failed: _owned.empty () (src/own.cpp:197)
/bin/bash: line 6: 27699 Aborted (core dumped) ${dir}$tst
FAIL: tests

@Stepan Blyshchak can you check that locally ?

mssonicbld · 2025-02-20T19:29:29Z

/azp run

kcudnik · 2025-02-20T19:30:11Z

i narrowed this down to : https://gitea.atria-soft.org/generic-library/libzmq/src/commit/092ad50b0c32174cdffdd45b5516b50237a8315d/src/own.cpp
this line:

Assertion failed: _owned.empty () (src/own.cpp:197)

is from zmq library, but beats me why this assert is crashing 😕

kcudnik · 2025-02-20T19:33:43Z

/azp run

azure-pipelines · 2025-02-20T19:33:55Z

Azure Pipelines successfully started running 1 pipeline(s).

kcudnik · 2025-02-21T07:17:06Z

/azp run

azure-pipelines · 2025-02-21T07:17:18Z

Azure Pipelines successfully started running 1 pipeline(s).

kcudnik · 2025-02-21T07:17:32Z

some of the tests are not passing:

Replay BCM56850/port_bulk_get.rec
terminate called after throwing an instance of 'std::runtime_error'
  what():  :- waitForBulkGetResponse: wrong number of statuses, got 0, expected 2
player BCM56850/port_bulk_get.rec: exitcode: 134:
FAIL: BCM56850.pl

this seems like implementation bulk error

stepanblyschak · 2025-02-21T07:29:39Z

@kcudnik I am familiar with this failure, it requires sonic-swss-common PR sonic-net/sonic-swss-common#966. The problem is - there is no new sonic-swss-common build that includes this PR as I pointed out in previous comment:

Tests are failing due to build using older swss-common which is missing PR sonic-net/sonic-swss-common#966.

Test is using swss-common built here - https://dev.azure.com/mssonic/build/_build/results?buildId=757465 which is from a previous commit.

Waiting for a new swss-common build

There's no new sonic-swss-common build for few weeks because of different failures in sonic-swss-common (one of which was the mentioned assertion in zmq lib) and I do not have a way to restart sonic-swss-common build:

kcudnik · 2025-02-21T14:28:24Z

/azp run

azure-pipelines · 2025-02-21T14:28:37Z

Azure Pipelines successfully started running 1 pipeline(s).

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

meta/SaiSerialize.cpp Outdated Show resolved Hide resolved

kcudnik reviewed Jan 24, 2025

View reviewed changes

saiplayer/SaiPlayer.cpp Outdated Show resolved Hide resolved

kcudnik reviewed Jan 24, 2025

View reviewed changes

syncd/Syncd.cpp Show resolved Hide resolved

kcudnik reviewed Jan 24, 2025

View reviewed changes

unittest/vslib/TestSwitchStateBase.cpp Outdated Show resolved Hide resolved

kcudnik requested changes Jan 24, 2025

View reviewed changes

stepanblyschak added 3 commits January 27, 2025 09:37

Revert "Fix transfer_list to return SAI_STATUS_BUFFER_OVERFLOW if dst…

2989f1a

… count is 0" This reverts commit e4b0f58.

tests/portBulkGetObjectListInsufficientBuffer: Resize queue list to 1

a6b72e3

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

Remove comment

ea6740f

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

kcudnik reviewed Jan 27, 2025

View reviewed changes

syncd/Syncd.cpp Show resolved Hide resolved

kcudnik reviewed Jan 27, 2025

View reviewed changes

syncd/VendorSai.cpp Show resolved Hide resolved

Merge branch 'master' of https://github.com/sonic-net/sonic-sairedis …

3ad34fc

…into bulk-get Signed-off-by: Stepan Blyschak <[email protected]>

Merge branch 'master' into bulk-get

098e615

stepanblyschak mentioned this pull request Feb 10, 2025

[portsorch] Ports initialization speedup sonic-net/sonic-swss#3506

Open

Merge branch 'master' into bulk-get

e91d85c

volodymyrsamotiy approved these changes Feb 13, 2025

View reviewed changes

Merge branch 'master' into bulk-get

78c8d4e

dgsudharsan added the Request for msft-202412 Branch label Feb 18, 2025

mssonicbld added the Cherry Pick Conflict_msft-202412 label Feb 18, 2025

kcudnik approved these changes Feb 19, 2025

View reviewed changes

Merge branch 'master' into bulk-get

ae53357


		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 • edited Loading

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

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

mssonicbld commented Jan 27, 2025

azure-pipelines bot commented Jan 27, 2025

mssonicbld commented Jan 29, 2025

azure-pipelines bot commented Jan 29, 2025

stepanblyschak commented Feb 5, 2025

mssonicbld commented Feb 6, 2025

azure-pipelines bot commented Feb 6, 2025

mssonicbld commented Feb 12, 2025

azure-pipelines bot commented Feb 12, 2025

kcudnik commented Feb 13, 2025

mssonicbld commented Feb 14, 2025

azure-pipelines bot commented Feb 14, 2025

kcudnik commented Feb 20, 2025

azure-pipelines bot commented Feb 20, 2025

kcudnik commented Feb 20, 2025 • edited Loading

mssonicbld commented Feb 20, 2025

kcudnik commented Feb 20, 2025

kcudnik commented Feb 20, 2025

azure-pipelines bot commented Feb 20, 2025

kcudnik commented Feb 21, 2025

azure-pipelines bot commented Feb 21, 2025

kcudnik commented Feb 21, 2025

stepanblyschak commented Feb 21, 2025

kcudnik commented Feb 21, 2025

azure-pipelines bot commented Feb 21, 2025

stepanblyschak commented Jan 24, 2025 •

edited

Loading

kcudnik commented Feb 20, 2025 •

edited

Loading