From bd132eccc60f5b12e00004e7d4afae8af0cd593a Mon Sep 17 00:00:00 2001 From: Kamil Cudnik Date: Mon, 1 Jun 2020 19:12:13 +0200 Subject: [PATCH] Add synchronous mode to sairedis library (#617) * [saiplayer] Add sync mode to saiplayer * [syncd] Add RedisClient more redis methods * [syncd] Update redis db in syncd only when success * [test] Add sync mode unittest --- saiplayer/CommandLineOptions.cpp | 2 + saiplayer/CommandLineOptions.h | 2 + saiplayer/CommandLineOptionsParser.cpp | 11 ++- saiplayer/SaiPlayer.cpp | 8 ++ syncd/RedisClient.cpp | 30 +++++++ syncd/RedisClient.h | 7 ++ syncd/Syncd.cpp | 116 +++++++++++++++++++++++-- syncd/Syncd.h | 7 ++ tests/brcm.pl | 15 ++++ tests/utils.pm | 44 +++++++++- 10 files changed, 231 insertions(+), 11 deletions(-) diff --git a/saiplayer/CommandLineOptions.cpp b/saiplayer/CommandLineOptions.cpp index 869c432b823d..f68cfda4f722 100644 --- a/saiplayer/CommandLineOptions.cpp +++ b/saiplayer/CommandLineOptions.cpp @@ -17,6 +17,7 @@ CommandLineOptions::CommandLineOptions() m_skipNotifySyncd = false; m_enableDebug = false; m_sleep = false; + m_syncMode = false; } std::string CommandLineOptions::getCommandLineString() const @@ -30,6 +31,7 @@ std::string CommandLineOptions::getCommandLineString() const ss << " SkipNotifySyncd=" << (m_skipNotifySyncd ? "YES" : "NO"); ss << " EnableDebug=" << (m_enableDebug ? "YES" : "NO"); ss << " Sleep=" << (m_sleep ? "YES" : "NO"); + ss << " SyncMode=" << (m_syncMode ? "YES" : "NO"); return ss.str(); } diff --git a/saiplayer/CommandLineOptions.h b/saiplayer/CommandLineOptions.h index 539eb1602d38..cb4faa77f9cf 100644 --- a/saiplayer/CommandLineOptions.h +++ b/saiplayer/CommandLineOptions.h @@ -31,6 +31,8 @@ namespace saiplayer bool m_sleep; + bool m_syncMode; + std::vector m_files; }; } diff --git a/saiplayer/CommandLineOptionsParser.cpp b/saiplayer/CommandLineOptionsParser.cpp index 80c7ceb107ed..3fb600857828 100644 --- a/saiplayer/CommandLineOptionsParser.cpp +++ b/saiplayer/CommandLineOptionsParser.cpp @@ -16,7 +16,7 @@ std::shared_ptr CommandLineOptionsParser::parseCommandLine( auto options = std::make_shared(); - const char* const optstring = "uiCdsh"; + const char* const optstring = "uiCdsmh"; while(true) { @@ -27,6 +27,7 @@ std::shared_ptr CommandLineOptionsParser::parseCommandLine( { "skipNotifySyncd", no_argument, 0, 'C' }, { "enableDebug", no_argument, 0, 'd' }, { "sleep", no_argument, 0, 's' }, + { "syncMode", no_argument, 0, 'm' }, { "help", no_argument, 0, 'h' }, }; @@ -61,6 +62,10 @@ std::shared_ptr CommandLineOptionsParser::parseCommandLine( options->m_sleep = true; break; + case 'm': + options->m_syncMode = true; + break; + case 'h': printUsage(); exit(EXIT_SUCCESS); @@ -95,7 +100,7 @@ void CommandLineOptionsParser::printUsage() { SWSS_LOG_ENTER(); - std::cout << "Usage: saiplayer [-u] [-i] [-C] [-d] [-s] [-h] recordfile" << std::endl << std::endl; + std::cout << "Usage: saiplayer [-u] [-i] [-C] [-d] [-s] [-m] [-h] recordfile" << std::endl << std::endl; std::cout << " -u --useTempView:" << std::endl; std::cout << " Enable temporary view between init and apply" << std::endl << std::endl; @@ -107,6 +112,8 @@ void CommandLineOptionsParser::printUsage() std::cout << " Enable syslog debug messages" << std::endl << std::endl; std::cout << " -s --sleep:" << std::endl; std::cout << " Sleep after success reply, to notice any switch notifications" << std::endl << std::endl; + std::cout << " -m --syncMode:" << std::endl; + std::cout << " Enable synchronous mode" << std::endl << std::endl; std::cout << " -h --help:" << std::endl; std::cout << " Print out this message" << std::endl << std::endl; diff --git a/saiplayer/SaiPlayer.cpp b/saiplayer/SaiPlayer.cpp index f2d08a835bd9..3da048f11067 100644 --- a/saiplayer/SaiPlayer.cpp +++ b/saiplayer/SaiPlayer.cpp @@ -1524,6 +1524,14 @@ int SaiPlayer::run() EXIT_ON_ERROR(m_sai->set(SAI_OBJECT_TYPE_SWITCH, switch_id, &attr)); } + if (m_commandLineOptions->m_syncMode) + { + attr.id = SAI_REDIS_SWITCH_ATTR_SYNC_MODE; + attr.value.booldata = true; + + EXIT_ON_ERROR(m_sai->set(SAI_OBJECT_TYPE_SWITCH, switch_id, &attr)); + } + int exitcode = 0; if (m_commandLineOptions->m_files.size() > 0) diff --git a/syncd/RedisClient.cpp b/syncd/RedisClient.cpp index ae75aa6c7ac4..cc4705f96947 100644 --- a/syncd/RedisClient.cpp +++ b/syncd/RedisClient.cpp @@ -479,6 +479,16 @@ void RedisClient::removeAsicObject( m_redisClient->del(key); } +void RedisClient::removeTempAsicObject( + _In_ const sai_object_meta_key_t& metaKey) +{ + SWSS_LOG_ENTER(); + + std::string key = (TEMP_PREFIX ASIC_STATE_TABLE ":") + sai_serialize_object_meta_key(metaKey); + + m_redisClient->del(key); +} + void RedisClient::setAsicObject( _In_ const sai_object_meta_key_t& metaKey, _In_ const std::string& attr, @@ -523,6 +533,26 @@ void RedisClient::createAsicObject( } } +void RedisClient::createTempAsicObject( + _In_ const sai_object_meta_key_t& metaKey, + _In_ const std::vector& attrs) +{ + SWSS_LOG_ENTER(); + + std::string key = (TEMP_PREFIX ASIC_STATE_TABLE ":") + sai_serialize_object_meta_key(metaKey); + + if (attrs.size() == 0) + { + m_redisClient->hset(key, "NULL", "NULL"); + return; + } + + for (const auto& e: attrs) + { + m_redisClient->hset(key, fvField(e), fvValue(e)); + } +} + void RedisClient::setVidAndRidMap( _In_ const std::unordered_map& map) { diff --git a/syncd/RedisClient.h b/syncd/RedisClient.h index bb675a93ec2d..42d2f8716f97 100644 --- a/syncd/RedisClient.h +++ b/syncd/RedisClient.h @@ -83,10 +83,17 @@ namespace syncd void removeAsicObject( _In_ const sai_object_meta_key_t& metaKey); + void removeTempAsicObject( + _In_ const sai_object_meta_key_t& metaKey); + void createAsicObject( _In_ const sai_object_meta_key_t& metaKey, _In_ const std::vector& attrs); + void createTempAsicObject( + _In_ const sai_object_meta_key_t& metaKey, + _In_ const std::vector& attrs); + void setVidAndRidMap( _In_ const std::unordered_map& map); diff --git a/syncd/Syncd.cpp b/syncd/Syncd.cpp index 5eb1f0abc958..a93414caa01e 100644 --- a/syncd/Syncd.cpp +++ b/syncd/Syncd.cpp @@ -85,6 +85,8 @@ Syncd::Syncd( m_asicState = std::make_shared(m_dbAsic.get(), ASIC_STATE_TABLE); m_restartQuery = std::make_shared(m_dbAsic.get(), SYNCD_NOTIFICATION_CHANNEL_RESTARTQUERY); + m_asicState->setModifyRedis(m_commandLineOptions->m_enableSyncMode ? false : true); + // TODO to be moved to ASIC_DB m_dbFlexCounter = std::make_shared(m_contextConfig->m_dbFlex, 0); m_flexCounter = std::make_shared(m_dbFlexCounter.get(), FLEX_COUNTER_TABLE); @@ -663,6 +665,13 @@ sai_status_t Syncd::processBulkQuadEventInInitViewMode( { SWSS_LOG_ENTER(); + std::vector statuses(object_ids.size()); + + for (auto &a: statuses) + { + a = SAI_STATUS_SUCCESS; + } + auto info = sai_metadata_get_object_type_info(objectType); switch (api) @@ -673,7 +682,7 @@ sai_status_t Syncd::processBulkQuadEventInInitViewMode( if (info->isnonobjectid) { - sendApiResponse(api, SAI_STATUS_SUCCESS); + sendApiResponse(api, SAI_STATUS_SUCCESS, (uint32_t)statuses.size(), statuses.data()); return SAI_STATUS_SUCCESS; } @@ -689,7 +698,7 @@ sai_status_t Syncd::processBulkQuadEventInInitViewMode( default: - sendApiResponse(api, SAI_STATUS_SUCCESS); + sendApiResponse(api, SAI_STATUS_SUCCESS, (uint32_t)statuses.size(), statuses.data()); return SAI_STATUS_SUCCESS; } @@ -1280,6 +1289,88 @@ void Syncd::processFlexCounterEvent( // TODO must be moved to go via ASIC channe } } +void Syncd::syncUpdateRedisQuadEvent( + _In_ sai_status_t status, + _In_ sai_common_api_t api, + _In_ const swss::KeyOpFieldsValuesTuple &kco) +{ + SWSS_LOG_ENTER(); + + if (!m_commandLineOptions->m_enableSyncMode) + { + return; + } + + if (status != SAI_STATUS_SUCCESS) + { + return; + } + + // When in synchronous mode, we need to modify redis database when status + // is success, since consumer table on synchronous mode is not making redis + // changes and we only want to apply changes when api succeeded. This + // applies to init view mode and apply view mode. + + const std::string& key = kfvKey(kco); + + auto& values = kfvFieldsValues(kco); + + const std::string& strObjectId = key.substr(key.find(":") + 1); + + sai_object_meta_key_t metaKey; + sai_deserialize_object_meta_key(key, metaKey); + + const bool initView = isInitViewMode(); + + switch (api) + { + case SAI_COMMON_API_CREATE: + + { + if (initView) + m_client->createTempAsicObject(metaKey, values); + else + m_client->createAsicObject(metaKey, values); + + return; + } + + case SAI_COMMON_API_REMOVE: + + { + if (initView) + m_client->removeTempAsicObject(metaKey); + else + m_client->removeAsicObject(metaKey); + + return; + } + + case SAI_COMMON_API_SET: + + { + auto& first = values.at(0); + + auto& attr = fvField(first); + auto& value = fvValue(first); + + if (initView) + m_client->setTempAsicObject(metaKey, attr, value); + else + m_client->setAsicObject(metaKey, attr, value); + + return; + } + + case SAI_COMMON_API_GET: + return; // ignore get since get is not modifying db + + default: + + SWSS_LOG_THROW("api %d is not supported", api); + } +} + sai_status_t Syncd::processQuadEvent( _In_ sai_common_api_t api, _In_ const swss::KeyOpFieldsValuesTuple &kco) @@ -1338,7 +1429,11 @@ sai_status_t Syncd::processQuadEvent( if (isInitViewMode()) { - return processQuadEventInInitViewMode(metaKey.objecttype, strObjectId, api, attr_count, attr_list); + sai_status_t status = processQuadEventInInitViewMode(metaKey.objecttype, strObjectId, api, attr_count, attr_list); + + syncUpdateRedisQuadEvent(status, api, kco); + + return status; } if (api != SAI_COMMON_API_GET) @@ -1402,16 +1497,23 @@ sai_status_t Syncd::processQuadEvent( SWSS_LOG_ERROR("attr: %s: %s", fvField(v).c_str(), fvValue(v).c_str()); } - SWSS_LOG_THROW("failed to execute api: %s, key: %s, status: %s", - op.c_str(), - key.c_str(), - sai_serialize_status(status).c_str()); + if (!m_commandLineOptions->m_enableSyncMode) + { + // throw only when sync mode is not enabled + + SWSS_LOG_THROW("failed to execute api: %s, key: %s, status: %s", + op.c_str(), + key.c_str(), + sai_serialize_status(status).c_str()); + } } else // non GET api, status is SUCCESS { sendApiResponse(api, status); } + syncUpdateRedisQuadEvent(status, api, kco); + return status; } diff --git a/syncd/Syncd.h b/syncd/Syncd.h index 47977ee83743..266fb03150a8 100644 --- a/syncd/Syncd.h +++ b/syncd/Syncd.h @@ -214,6 +214,13 @@ namespace syncd _In_ uint32_t attr_count, _In_ sai_attribute_t *attr_list); + private: + + void syncUpdateRedisQuadEvent( + _In_ sai_status_t status, + _In_ sai_common_api_t api, + _In_ const swss::KeyOpFieldsValuesTuple &kco); + public: // TODO to private sai_status_t processEntry( diff --git a/tests/brcm.pl b/tests/brcm.pl index ea56deab14fd..2219448e95d4 100755 --- a/tests/brcm.pl +++ b/tests/brcm.pl @@ -504,6 +504,20 @@ sub test_brcm_warm_boot_port_remove request_warm_shutdown; } +sub test_sync_brcm_warm_boot_port_remove +{ + sync_fresh_start; + + sync_play "wb_port_remove_a.rec"; + + request_warm_shutdown; + sync_start_syncd_warm; + + sync_play "wb_port_remove_b.rec"; + + request_warm_shutdown; +} + sub test_brcm_warm_boot_port_create { fresh_start; @@ -534,6 +548,7 @@ sub test_brcm_query_object_type_get_availability # RUN TESTS +test_sync_brcm_warm_boot_port_remove; test_brcm_warm_boot_port_remove; test_brcm_warm_boot_port_create; test_remove_port; diff --git a/tests/utils.pm b/tests/utils.pm index 8916d3c94747..4c5d76b4d329 100644 --- a/tests/utils.pm +++ b/tests/utils.pm @@ -59,6 +59,20 @@ sub start_syncd_warm sleep 1; } +sub sync_start_syncd +{ + print color('bright_blue') . "Starting syncd" . color('reset') . "\n"; + `./vssyncd -s -SUu -p "$DIR/vsprofile.ini" >/dev/null 2>/dev/null &`; +} + +sub sync_start_syncd_warm +{ + print color('bright_blue') . "Starting syncd warm" . color('reset') . "\n"; + `./vssyncd -s -SUu -t warm -p "$DIR/vsprofile.ini" >/dev/null 2>/dev/null &`; + + sleep 1; +} + sub request_warm_shutdown { print color('bright_blue') . "Requesting syncd warm shutdown" . color('reset') . "\n"; @@ -67,14 +81,15 @@ sub request_warm_shutdown sleep 2; } -sub play +sub play_common { + my $sync = shift; my $file = shift; my $asicop = shift; print color('bright_blue') . "Replay $file" . color('reset') . "\n"; - my @ret = `../saiplayer/saiplayer -u "$DIR/$file"`; + my @ret = `../saiplayer/saiplayer $sync -u "$DIR/$file"`; if ($? != 0) { @@ -108,6 +123,16 @@ sub play } } +sub play +{ + play_common "", @_; +} + +sub sync_play +{ + play_common "-m", @_; +} + sub fresh_start { my $caller = GetCaller(); @@ -121,11 +146,26 @@ sub fresh_start start_syncd; } +sub sync_fresh_start +{ + my $caller = GetCaller(); + + `rm -f applyview.log`; + + print "$caller: " . color('bright_blue') . "Fresh start" . color('reset') . "\n"; + + kill_syncd; + flush_redis; + sync_start_syncd; +} + + BEGIN { our @ISA = qw(Exporter); our @EXPORT = qw/ kill_syncd flush_redis start_syncd play fresh_start start_syncd_warm request_warm_shutdown + sync_start_syncd sync_fresh_start sync_start_syncd_warm sync_start_syncd sync_play /; my $script = $0;