-
Notifications
You must be signed in to change notification settings - Fork 693
[hft]: Remove IPFIX template size estimation, query actual size from SAI instead #4304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3822842
c7aa750
7a0dc41
a368dd2
30a96cd
669ec8a
ceadd11
ca07d7b
18257be
cde3f65
2c68f63
82530a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -955,47 +955,36 @@ void HFTelProfile::updateTemplates(sai_object_id_t tam_tel_type_obj) | |
| SWSS_LOG_THROW("The object type is not found"); | ||
| } | ||
|
|
||
| // Estimate the template size | ||
| auto counters = m_sai_tam_counter_subscription_objs.find(object_type); | ||
| if (counters == m_sai_tam_counter_subscription_objs.end()) | ||
| { | ||
| SWSS_LOG_THROW("The counter subscription object is not found"); | ||
| } | ||
| size_t counters_count = 0; | ||
| for (const auto &item : counters->second) | ||
| { | ||
| counters_count += item.second.size(); | ||
| } | ||
|
|
||
| const size_t COUNTER_SIZE (8LLU); | ||
| const size_t IPFIX_TEMPLATE_MAX_SIZE (0xffffLLU); | ||
| const size_t IPFIX_HEADER_SIZE (16LLU); | ||
| const size_t IPFIX_TEMPLATE_METADATA_SIZE (12LLU); | ||
| const size_t IPFIX_TEMPLATE_MAX_STATS_COUNT (((IPFIX_TEMPLATE_MAX_SIZE - IPFIX_HEADER_SIZE - IPFIX_TEMPLATE_METADATA_SIZE) / COUNTER_SIZE) - 1LLU); | ||
| size_t estimated_template_size = (counters_count / IPFIX_TEMPLATE_MAX_STATS_COUNT + 1) * IPFIX_TEMPLATE_MAX_SIZE; | ||
|
|
||
| vector<uint8_t> buffer(estimated_template_size, 0); | ||
|
|
||
| sai_attribute_t attr; | ||
| // Query the required buffer size first by passing count=0 and list=nullptr, | ||
| // then allocate and fetch the actual data. | ||
| sai_attribute_t attr{}; | ||
| attr.id = SAI_TAM_TEL_TYPE_ATTR_IPFIX_TEMPLATES; | ||
| attr.value.u8list.count = static_cast<uint32_t>(buffer.size()); | ||
| attr.value.u8list.list = buffer.data(); | ||
| attr.value.u8list.count = 0; | ||
| attr.value.u8list.list = nullptr; | ||
|
|
||
| auto status = sai_tam_api->get_tam_tel_type_attribute(tam_tel_type_obj, 1, &attr); | ||
| if (status == SAI_STATUS_BUFFER_OVERFLOW) | ||
| if (status != SAI_STATUS_SUCCESS && status != SAI_STATUS_BUFFER_OVERFLOW) | ||
| { | ||
| buffer.resize(attr.value.u8list.count); | ||
| attr.value.u8list.list = buffer.data(); | ||
| status = sai_tam_api->get_tam_tel_type_attribute(tam_tel_type_obj, 1, &attr); | ||
| SWSS_LOG_THROW("Failed to query the IPFIX template size for TAM telemetry type object %s: %d", | ||
| sai_serialize_object_id(tam_tel_type_obj).c_str(), status); | ||
| } | ||
|
|
||
| if (status != SAI_STATUS_SUCCESS) | ||
| vector<uint8_t> buffer; | ||
| if (status == SAI_STATUS_BUFFER_OVERFLOW && attr.value.u8list.count > 0) | ||
| { | ||
| SWSS_LOG_THROW("Failed to get the TAM telemetry type object %s attributes: %d", | ||
| sai_serialize_object_id(tam_tel_type_obj).c_str(), status); | ||
| } | ||
| buffer.resize(attr.value.u8list.count, 0); | ||
| attr.value.u8list.list = buffer.data(); | ||
| status = sai_tam_api->get_tam_tel_type_attribute(tam_tel_type_obj, 1, &attr); | ||
|
|
||
| buffer.resize(attr.value.u8list.count); | ||
| if (status != SAI_STATUS_SUCCESS) | ||
| { | ||
| SWSS_LOG_THROW("Failed to get the TAM telemetry type object %s attributes: %d", | ||
| sai_serialize_object_id(tam_tel_type_obj).c_str(), status); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: The second
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. I added a brief comment explaining that the second resize is intentional because SAI may return fewer bytes than initially requested. Commit: 2c68f63 |
||
| // SAI may return fewer bytes than originally requested. | ||
| buffer.resize(attr.value.u8list.count); | ||
| } | ||
|
|
||
| m_sai_tam_tel_type_templates[object_type] = move(buffer); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,239 @@ | ||
| // Pre-include standard library and third-party headers that conflict with | ||
| // the #define private public hack (they use 'private' internally). | ||
| #include <sstream> | ||
| #include <string> | ||
| #include <vector> | ||
| #include <map> | ||
| #include <unordered_map> | ||
| #include <set> | ||
| #include <memory> | ||
|
|
||
| #define private public | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. This pattern already exists in multiple tests in this area, so I am leaving it as-is in this PR and will revisit it later if we move to a cleaner test-access pattern. |
||
| #define protected public | ||
| #include "high_frequency_telemetry/hftelprofile.h" | ||
| #undef private | ||
| #undef protected | ||
|
|
||
| #include "ut_helper.h" | ||
| #include "mock_orchagent_main.h" | ||
| #include <gtest/gtest.h> | ||
|
|
||
| extern sai_tam_api_t *sai_tam_api; | ||
|
|
||
| namespace hftelprofile_ut | ||
| { | ||
| using namespace std; | ||
|
|
||
| /* | ||
| * Mock state for sai_tam_api->get_tam_tel_type_attribute. | ||
| * Controls what the mock returns on each successive call. | ||
| */ | ||
| struct MockGetTelTypeAttrState | ||
| { | ||
| sai_status_t first_call_status = SAI_STATUS_BUFFER_OVERFLOW; | ||
| uint32_t first_call_count = 0; | ||
| sai_status_t second_call_status = SAI_STATUS_SUCCESS; | ||
| vector<uint8_t> template_data; | ||
| int call_count = 0; | ||
| }; | ||
|
|
||
| static MockGetTelTypeAttrState g_mock; | ||
|
|
||
| static sai_status_t mock_get_tam_tel_type_attribute( | ||
| sai_object_id_t /*id*/, uint32_t attr_count, sai_attribute_t *attr_list) | ||
| { | ||
| ++g_mock.call_count; | ||
|
|
||
| if (attr_count != 1 || !attr_list || | ||
| attr_list[0].id != SAI_TAM_TEL_TYPE_ATTR_IPFIX_TEMPLATES) | ||
| { | ||
| return SAI_STATUS_INVALID_PARAMETER; | ||
| } | ||
|
|
||
| if (g_mock.call_count == 1) | ||
| { | ||
| /* First call — size query (count=0, list=nullptr). */ | ||
| attr_list[0].value.u8list.count = g_mock.first_call_count; | ||
| return g_mock.first_call_status; | ||
| } | ||
|
|
||
| /* Second call — data fetch. */ | ||
| if (g_mock.second_call_status == SAI_STATUS_SUCCESS && | ||
| !g_mock.template_data.empty()) | ||
| { | ||
| auto n = min(static_cast<uint32_t>(g_mock.template_data.size()), | ||
| attr_list[0].value.u8list.count); | ||
| memcpy(attr_list[0].value.u8list.list, | ||
| g_mock.template_data.data(), n); | ||
| attr_list[0].value.u8list.count = n; | ||
| } | ||
| return g_mock.second_call_status; | ||
| } | ||
|
|
||
| /* | ||
| * Fixture: swaps sai_tam_api->get_tam_tel_type_attribute with our mock | ||
| * for each test, restoring the original on tear-down. | ||
| */ | ||
| struct UpdateTemplatesTest : public ::testing::Test | ||
| { | ||
| sai_tam_api_t ut_api; | ||
| sai_tam_api_t *orig_api; | ||
|
|
||
| /* Minimal state to call updateTemplates(). */ | ||
| sai_object_id_t fake_tel_type_oid = 0x100; | ||
| HFTelProfile::sai_guard_t guard; | ||
| CounterNameCache empty_cache; | ||
|
|
||
| void SetUp() override | ||
| { | ||
| if (sai_tam_api == nullptr) | ||
| { | ||
| static sai_tam_api_t default_tam_api{}; | ||
| sai_tam_api = &default_tam_api; | ||
| } | ||
| ut_api = *sai_tam_api; | ||
| orig_api = sai_tam_api; | ||
| ut_api.get_tam_tel_type_attribute = mock_get_tam_tel_type_attribute; | ||
| sai_tam_api = &ut_api; | ||
|
|
||
| g_mock = MockGetTelTypeAttrState{}; | ||
| guard = make_shared<sai_object_id_t>(fake_tel_type_oid); | ||
| } | ||
|
|
||
| void TearDown() override { sai_tam_api = orig_api; } | ||
|
|
||
| /* | ||
| * Build a *partially-constructed* HFTelProfile that is just enough | ||
| * for updateTemplates() to run. We skip the real constructor | ||
| * (which calls initTelemetry → SAI) by using raw allocation + | ||
| * placement construction of only the members we need. | ||
| * | ||
| * This is deliberately minimal; we only touch the three maps that | ||
| * updateTemplates() and getObjectType() read/write. | ||
| */ | ||
| struct Stub | ||
| { | ||
| alignas(HFTelProfile) unsigned char buf[sizeof(HFTelProfile)]; | ||
| HFTelProfile *p = nullptr; | ||
|
|
||
| void init(HFTelProfile::sai_guard_t &guard) | ||
| { | ||
| /* | ||
| * Zero the storage so any incidental reads of uninitialised | ||
| * scalar members are safe (e.g. m_poll_interval). | ||
| */ | ||
| memset(buf, 0, sizeof(buf)); | ||
| p = reinterpret_cast<HFTelProfile *>(static_cast<void *>(buf)); | ||
|
|
||
| /* Placement-new the containers and strings that may be | ||
| * accessed (directly or via logging) by updateTemplates(). | ||
| * Today that means: | ||
| * - m_profile_name | ||
| * - m_sai_tam_tel_type_objs | ||
| * - m_sai_tam_tel_type_templates | ||
| * If updateTemplates() starts touching additional members, | ||
| * extend this partial construction accordingly. */ | ||
| new (const_cast<string*>(&p->m_profile_name)) string(); | ||
| new (&p->m_sai_tam_tel_type_objs) | ||
| decay_t<decltype(p->m_sai_tam_tel_type_objs)>(); | ||
| new (&p->m_sai_tam_tel_type_templates) | ||
| decay_t<decltype(p->m_sai_tam_tel_type_templates)>(); | ||
|
|
||
| p->m_sai_tam_tel_type_objs[SAI_OBJECT_TYPE_PORT] = guard; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: This Stub pattern with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I added a comment documenting which members the partial Stub currently constructs/touches so future updates to updateTemplates() are less likely to silently drift into UB territory. Commit: ca07d7b |
||
| } | ||
|
|
||
| ~Stub() | ||
| { | ||
| if (!p) return; | ||
| p->m_profile_name.~basic_string(); | ||
| p->m_sai_tam_tel_type_objs.~unordered_map(); | ||
| p->m_sai_tam_tel_type_templates.~unordered_map(); | ||
| p = nullptr; | ||
| } | ||
| }; | ||
| }; | ||
|
|
||
| /* ---- SAI returns BUFFER_OVERFLOW then SUCCESS (happy path) ---- */ | ||
| TEST_F(UpdateTemplatesTest, BufferOverflow_ThenSuccess) | ||
| { | ||
| Stub s; | ||
| s.init(guard); | ||
|
|
||
| g_mock.first_call_status = SAI_STATUS_BUFFER_OVERFLOW; | ||
| g_mock.first_call_count = 4; | ||
| g_mock.second_call_status = SAI_STATUS_SUCCESS; | ||
| g_mock.template_data = {0xAA, 0xBB, 0xCC, 0xDD}; | ||
|
|
||
| ASSERT_NO_THROW(s.p->updateTemplates(fake_tel_type_oid)); | ||
|
|
||
| auto &tpl = s.p->m_sai_tam_tel_type_templates[SAI_OBJECT_TYPE_PORT]; | ||
| ASSERT_EQ(tpl.size(), 4u); | ||
| EXPECT_EQ(tpl[0], 0xAA); | ||
| EXPECT_EQ(tpl[3], 0xDD); | ||
| } | ||
|
|
||
| /* ---- SAI returns SUCCESS on first call (count stays 0) ---- */ | ||
| TEST_F(UpdateTemplatesTest, Success_EmptyTemplate) | ||
| { | ||
| Stub s; | ||
| s.init(guard); | ||
|
|
||
| g_mock.first_call_status = SAI_STATUS_SUCCESS; | ||
| g_mock.first_call_count = 0; | ||
|
|
||
| ASSERT_NO_THROW(s.p->updateTemplates(fake_tel_type_oid)); | ||
|
|
||
| auto &tpl = s.p->m_sai_tam_tel_type_templates[SAI_OBJECT_TYPE_PORT]; | ||
| EXPECT_TRUE(tpl.empty()); | ||
| } | ||
|
|
||
| /* ---- BUFFER_OVERFLOW with count=0 stores an empty template ---- */ | ||
| TEST_F(UpdateTemplatesTest, BufferOverflow_EmptyTemplate) | ||
| { | ||
| Stub s; | ||
| s.init(guard); | ||
|
|
||
| g_mock.first_call_status = SAI_STATUS_BUFFER_OVERFLOW; | ||
| g_mock.first_call_count = 0; | ||
|
|
||
| ASSERT_NO_THROW(s.p->updateTemplates(fake_tel_type_oid)); | ||
|
|
||
| auto &tpl = s.p->m_sai_tam_tel_type_templates[SAI_OBJECT_TYPE_PORT]; | ||
| EXPECT_TRUE(tpl.empty()); | ||
| EXPECT_EQ(g_mock.call_count, 1); | ||
| } | ||
|
|
||
| /* ---- First query fails with unexpected status ---- */ | ||
| TEST_F(UpdateTemplatesTest, FirstCall_UnexpectedFailure) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Consider adding a test for the edge case where SAI returns
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good suggestion. I added an explicit unit test for the |
||
| { | ||
| Stub s; | ||
| s.init(guard); | ||
|
|
||
| g_mock.first_call_status = SAI_STATUS_FAILURE; | ||
|
|
||
| EXPECT_THROW(s.p->updateTemplates(fake_tel_type_oid), runtime_error); | ||
| } | ||
|
|
||
| /* ---- Second call (data fetch) fails ---- */ | ||
| TEST_F(UpdateTemplatesTest, SecondCall_Failure) | ||
| { | ||
| Stub s; | ||
| s.init(guard); | ||
|
|
||
| g_mock.first_call_status = SAI_STATUS_BUFFER_OVERFLOW; | ||
| g_mock.first_call_count = 4; | ||
| g_mock.second_call_status = SAI_STATUS_FAILURE; | ||
|
|
||
| EXPECT_THROW(s.p->updateTemplates(fake_tel_type_oid), runtime_error); | ||
| } | ||
|
|
||
| /* ---- Unknown tel-type OID → object type not found ---- */ | ||
| TEST_F(UpdateTemplatesTest, UnknownOID_Throws) | ||
| { | ||
| Stub s; | ||
| s.init(guard); | ||
|
|
||
| sai_object_id_t bad_oid = 0xDEAD; | ||
| EXPECT_THROW(s.p->updateTemplates(bad_oid), runtime_error); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
sai_attribute_t attr;leaves padding/unused union members uninitialized. Considersai_attribute_t attr{};for defensive zero-initialization — trivial cost and avoids any future surprises if the struct layout changes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I changed this to
sai_attribute_t attr{}for defensive zero-initialization. Commit: 18257be