[VOQ][saidump] Enhance saidump with new option -r to parser the JSON file and displays/format the right output, fix compiling error #1335

JunhongMao · 2023-12-27T00:40:41Z

Why I did it

Based on the PR1288
#1288

and the comment from gechiang
#1288 (comment)

in 202305, we should use "#include " swss/json.hpp " " other than #include <nlohmann/json.hpp>,

Below is the information on the PR1288.
Fix issue: sonic-net/sonic-buildimage#13561
The existing saidump use https://github.com/sonic-net/sonic-swss-common/blob/master/common/table_dump.lua script which loops the ASIC_DB more than 5 seconds and blocks other processes access.

This solution uses the redis-db SAVE option to save the snapshot of DB each time and recover later, instead of looping through each entry in the table.
Related PRs:
sonic-net/sonic-utilities#2972
sonic-net/sonic-buildimage#16466

MSFT ADO: 25892357

How I did it

To use the Redis-db SAVE option to save the snapshot of DB each time and recover later, instead of looping through each entry in the table and saving it.

    (1) Updated platform/broadcom/docker-syncd-brcm-dnx/Dockerfile.j2, install Python library rdbtools into the syncd containter.
    (2) Updated sonic-buildimage/src/sonic-sairedis/saidump/saidump.cpp, add a new option -r, which updates the rdbtools's output-JSON files' format.
    (3) Updated sonic-buildimage/build_debian.sh, to add a new script file: files/scripts/saidump.sh into the host. This shell file does the below steps:
      For each ASIC0, such as ASIC0,

      1. Save the Redis data.
      sudo sonic-db-cli -n asic$1 SAVE > /dev/null

      2. Move dump files to /var/run/redisX/
      docker exec database$1 sh -c "mv /var/lib/redis/dump.rdb /var/run/redis$1/"

      3. Run rdb command to convert the dump files into JSON files
      docker exec syncd$1 sh -c "rdb --command json /var/run/redis$1/dump.rdb | tee /var/run/redis$1/dump.json > /dev/null"

      4. Run saidump -r to update the JSON files' format as same as the saidump before. Then we can get the saidump result in standard output.
      docker exec syncd$1 sh -c "saidump -r /var/run/redis$1/dump.json -m 100"

      5. clear
      sudo rm -f /var/run/redis$1/dump.rdb
      sudo rm -f /var/run/redis$1/dump.json

    (4) Update sonic-buildimage/src/sonic-utilities/scripts/generate_dump, to check the asic db size and if it is larger than xxx entries, then do with REDIS SAVE, otherwise, to do with old method: looping through each entry of Redis DB.

How to verify it

Method 1

On T2 setup with more than 96K routes, execute CLI command -- generate_dump
No error should be shown
Download the generate_dump result and verify the saidump file after unpacking it.

Method 2

Run syncd0 bash
docker exec -it syncd0 bash
Get saidump output by looping through each entry in the table
root@ixre-egl-board40:/# saidump | sha1sum
821d442938028114b614bd810ecf844d564fd812 -
Get saidump output by using the redis-db SAVE option to save the snapshot of D
root@ixre-egl-board40:/# saidump.sh | sha1sum
821d442938028114b614bd810ecf844d564fd812 -
The sha1sums are the same, so the test passed.

How to do unit tests manually.

Go into compile container bash.
jumao@4b61df49afa7:/sonic/src/sonic-sairedis/unittest
Run saidump unit tests.

jumao@4b61df49afa7:/sonic/src/sonic-sairedis/unittest/saidump$ make check
make  check-TESTS
make[1]: Entering directory '/sonic/src/sonic-sairedis/unittest/saidump'
[==========] Running 4 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 4 tests from SaiDump
[ RUN      ] SaiDump.printUsage
[       OK ] SaiDump.printUsage (0 ms)
[ RUN      ] SaiDump.handleCmdLine
[       OK ] SaiDump.handleCmdLine (0 ms)
[ RUN      ] SaiDump.dumpFromRedisRdbJson
Failed to open the input file .
[       OK ] SaiDump.dumpFromRedisRdbJson (1 ms)
[ RUN      ] SaiDump.preProcessFile
Failed to open the input file .
Get ./dump.json's size failure or its size 7914 >= 0 MB.
[       OK ] SaiDump.preProcessFile (1 ms)
[----------] 4 tests from SaiDump (2 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 1 test suite ran. (2 ms total)
[  PASSED  ] 4 tests.
PASS: tests
=============
1 test passed
=============

Which release branch to backport (provide reason below if selected)

Tested branch (Please provide the tested image version)

202205

sonic-net#1288 (comment)

kcudnik · 2024-01-03T08:08:23Z

you will need to pass code coverage to merge this

JunhongMao · 2024-01-03T16:22:55Z

you will need to pass code coverage to merge this
OK.
Besides, I have a question. There was an error at the stage of CodeQL / Analyze (cpp) (pull_request_target) .

dpkg-buildpackage: info: host architecture amd64
dpkg-checkbuilddeps: error: Unmet build dependencies: nlohmann-json3-dev
dpkg-buildpackage: warning: build dependencies/conflicts unsatisfied; aborting
dpkg-buildpackage: warning: (Use -d flag to override.)
Error: Process completed with exit code 3.

I checked the files sonic-slave-buster/Dockerfile.j2 and sonic-slave-bullseye/Dockerfile.j2. They all installed the library below.
nlohmann-json3-dev.
So, my question is, how do I fix this issue? Thanks.

kcudnik · 2024-01-03T19:34:11Z

this is for codeql ?

kcudnik · 2024-01-03T19:35:51Z

this is for code ql, edit sonic-sairedis/.github/workflows/codeql-analysis.yml 202305 branch

JunhongMao · 2024-01-03T19:37:07Z

this is for codeql ?

Yes. And for the error during the stage of coverage.Azure.sonic-sairedis.amd64， I think I need add unit test codes to fix them, right?

kcudnik · 2024-01-03T19:41:19Z

yes, for code coverage you need add unittests for that

JunhongMao · 2024-01-03T21:10:33Z

Hi, @kcudnik. I looked into the subdirectories: sonic-sairedis/tests and sonic-sairedis/unittest, there are not any existed saidump test codes. How did the previous saidump codes pass the code coverage test? Need I create a new code file sonic-sairedis/unittest/saidump/TestSaidump.cpp to do the whole unit test of it? Thanks.

kcudnik · 2024-01-04T11:27:49Z

Hi, @kcudnik. I looked into the subdirectories: sonic-sairedis/tests and sonic-sairedis/unittest, there are not any existed saidump test codes. How did the previous saidump codes pass the code coverage test? Need I create a new code file sonic-sairedis/unittest/saidump/TestSaidump.cpp to do the whole unit test of it? Thanks.

previous saidump passed since unittest coverage was enabled when saidump was already present

that project is old, starting from 2016, and its not brought up to todays code quality standard, it would need to be converted to OOP and have classes for command line and command line parser and saidump itsel, and be converted to library with samall main.cpp file, which would gratefully improve testing abbility,

please take a look at sairedis/saiplayer/*cpp and *.h and Makefile.am files as a base how it should be done in todays standard

saiplayer also dont have unittest enabled, if you want to see strucutre how to place test files, you can take a look for vslib and syncd, but those are much complicated, of course you dont need to look everything, just the main concept, and saiplayer is good enough for that to easy understand

kcudnik · 2024-01-04T11:35:06Z

and in the long run it will make sense to convert it o OOP since it will be more manageable to write future code and unittest, but at current state, its a lot of work to bring upfront

gechiang · 2024-01-08T21:04:45Z

@JunhongMao Any update on the requires test cases?
Can we wrap up this PR quick as it is blocking other PRs...
Thanks!

kcudnik · 2024-01-16T10:39:22Z

you can make tests to code coverage this as is, but its not be easy, it will still needs to be converted to oop as suggested before, but that is even more work

JunhongMao · 2024-01-16T21:41:15Z

Hi, @kcudnik @gechiang, @mlok-nokia, the PRs of the similar modification have merged into the master branch. This PR is only intended to fix a compiling error of 202305 branch. Could we bypass this code-testing coverage issue to merge it first and create a new PR to fix the code-testing coverage issue in the future? What are the details of how to convert to OOP as suggested before? I think there will be more communication. As Kcudnik commented, much effort will be needed. Thanks.

kcudnik · 2024-01-16T21:42:50Z

yes, you will need to make more changes in the test area to satisfy coverage

JunhongMao · 2024-01-18T14:51:45Z

@kcudnik , how to update this PR's checking policy to bypass the coverage check blocking?

kcudnik · 2024-01-18T15:32:21Z

you cant bypass, you need to add unittests that will coverage the code in 80%

JunhongMao · 2024-01-22T14:39:19Z

@kcudnik, @gechiang @mlok-nokia , could we bypass this code-testing coverage issue to merge it first and create a new PR to fix the code-testing coverage issue in the future? I'm on leave. I will go back to the office and fix this issue next Monday.

mlok-nokia · 2024-01-22T14:43:24Z

@kcudnik Junhong is new to this module. He may need some guideline how to add the UNIT test case. Is there any example which he can follow? Thanks.

kcudnik · 2024-01-22T15:11:43Z

Usually each PR have unittests, please take a look a recent prs and into unittests directory, for those each file needs to be added corresponding with Test prefix and each function should be tested, if you need more info let me know

kcudnik · 2024-01-22T15:12:39Z

Code coverage is strictly required policy from the top and is checked time to time, and we get into trouble for bypassing unittests

JunhongMao · 2024-01-22T16:08:38Z

@kcudnik Thanks. I'm working on it.

JunhongMao · 2024-02-03T15:05:41Z

Hi @kcudnik, I have added the unit test codes in sonic-sairedis\unittest\saidump, but the Coverage check results showed that the new unit test codes were not called. Could you help to check why? Thanks.

Total: 95 lines
Missing: 95 lines
Coverage: 0%
Threshold: 80%
Diff coverage

kcudnik · 2024-02-04T09:19:57Z

unittest/saidump/main.cpp

@@ -0,0 +1,55 @@
+#include <gtest/gtest.h>
+#include "saidump.cpp"


please don't do that, don't include cpp files in another cpp files, i mentioned in comments that you would need to convert that saidump.cpp to library and then use taht library in the unittests, and saidump binary, then your test cases will be executed, since this file right now is treated as include not actual source, thats why is not covered even if tests are executed

unittest/saidump/tests

JunhongMao · 2024-02-06T14:56:50Z

Hi @kcudnik, I have converted the saidump.cpp to libsaidump.a. But there was still an error below:
dpkg-checkbuilddeps: error: Unmet build dependencies: nlohmann-json3-dev.

I checked the building scripts and found that they didn't include nlohmann-json3-dev. So, I think I need to create a separate PR that only installs nlohmann-json3-dev. After merging that PR, this PR could be checked successfully.
sudo apt-get install -y libxml-simple-perl
aspell
... ...
libjansson-dev
python

What do you think of it?

sonic-net#1335

JunhongMao · 2024-02-06T15:53:11Z

I created new PR:
#1350

kcudnik · 2024-02-06T18:37:28Z

approved

kcudnik · 2024-02-06T18:37:46Z

not sure why tou need json package if so far everything was compiling fine

Why I did it During the process of fix PR1335, #1335 There was an error: dpkg-checkbuilddeps: error: Unmet build dependencies: nlohmann-json3-dev. I checked the building scripts and found that they didn't include nlohmann-json3-dev. So, I think I need to create a separate PR that only installs nlohmann-json3-dev. After merging that PR, the PR1335 could be checked successfully. sudo apt-get install -y libxml-simple-perl aspell

kcudnik · 2024-02-17T15:59:22Z

saidump/saidump.h

+};
+
+void printUsage();
+CmdOptions handleCmdLine(int argc, char **argv);


Do not make global functions make them as static in class look for syncd commandline

Hi @kcudnik, could you please give more details about your comments? If I modified them as below:

static void printUsage(); staic CmdOptions handleCmdLine(int argc, char **argv);

When I compiling unittest/saidump, there were errors below:

jumao@8f4d6eacacca:/sonic/src/sonic-sairedis/unittest/saidump$ make check g++ -DHAVE_CONFIG_H -I. -I../.. -g -I../../SAI/inc -I../../SAI/experimental -I../../SAI/meta -I../../saidump -I../../lib -I../../vslib -I../../meta -ansi -fPIC -std=c++14 -Wall -Wcast-align -Wcast-qual -Wconversion -Wdisabled-optimization -Werror -Wextra -Wfloat-equal -Wformat=2 -Wformat-nonliteral -Wformat-security -Wformat-y2k -Wimport -Winit-self -Wno-inline -Winvalid-pch -Wmissing-field-initializers -Wmissing-format-attribute -Wmissing-include-dirs -Wmissing-noreturn -Wno-aggregate-return -Wno-padded -Wno-switch-enum -Wno-unused-parameter -Wpacked -Wpointer-arith -Wredundant-decls -Wshadow -Wstack-protector -Wstrict-aliasing=3 -Wswitch -Wswitch-default -Wunreachable-code -Wunused -Wvariadic-macros -Wwrite-strings -Wno-switch-default -Wconversion -Wno-psabi -Wcast-align=strict -g -O2 -MT tests-main.o -MD -MP -MF .deps/tests-main.Tpo -c -o tests-main.o `test -f 'main.cpp' || echo './'`main.cpp In file included from main.cpp:3: ../../saidump/saidump.h:16:13: error: 'void printUsage()' declared 'static' but never defined [-Werror=unused-function] 16 | static void printUsage(); | ^~~~~~~~~~ ../../saidump/saidump.h:16:13: error: 'void printUsage()' used but never defined [-Werror] cc1plus: all warnings being treated as errors make: *** [Makefile:496: tests-main.o] Error 1

Did you mean to add a saidump class to include all the functions? Such as below:

class saidump { static void printUsage(); staic CmdOptions handleCmdLine(int argc, char **argv); }

Thank you.

Yes, take a look at command line options.cpp and h in syncs directory you didn't put syncd namespace and class sorsaidump for printusage hence error you get

saidump/main.cpp

JunhongMao · 2024-05-02T19:40:56Z

@kcudnik @gechiang I found the failures of the below checks are wired.

Azure.sonic-sairedis (Build amd64) Failing after 33m — Build amd64 failed
Azure.sonic-sairedis Failing after 33m — Build #20240416.3 failed
CodeQL / Analyze (cpp) (pull_request_target) Failing after 10m

The similar failures are excited in many recent commits below:
https://github.com/sonic-net/sonic-sairedis/commits/202305/
I guess they are not related to this PR. Please help to double-check them. Thanks.

kcudnik · 2024-05-04T08:20:00Z

/azp run

azure-pipelines · 2024-05-04T08:20:11Z

Azure Pipelines successfully started running 1 pipeline(s).

kcudnik · 2024-05-04T08:20:27Z

i think 202305 azpipeline eneeds to be updated

kcudnik · 2024-05-04T15:34:56Z

Checkig for white spaces ...
../saidump/main.cpp:#include "saidump.h"
../saidump/main.cpp:
../saidump/main.cpp:using namespace syncd;
../saidump/main.cpp:
../saidump/main.cpp:int main(int argc, char **argv)
../saidump/main.cpp:{
../saidump/main.cpp:    SWSS_LOG_ENTER();
../saidump/main.cpp:    swss::Logger::getInstance().setMinPrio(swss::Logger::SWSS_INFO);
../saidump/main.cpp:
../saidump/main.cpp:    SaiDump m_saiDump;
../saidump/main.cpp:
../saidump/main.cpp:    if(SAI_STATUS_SUCCESS != m_saiDump.handleCmdLine(argc, argv))
../saidump/main.cpp:    {
../saidump/main.cpp:        return EXIT_FAILURE;
../saidump/main.cpp:    }
../saidump/main.cpp:
../saidump/main.cpp:    m_saiDump.dumpFromRedisDb();
../saidump/main.cpp:    return EXIT_SUCCESS;
../saidump/saidump.h:            sai_status_t preProcessFile(const std::string file_name);            
../unittest/saidump/main.cpp:    syncd::SaiDump m_saiDump;    
../unittest/saidump/main.cpp:    m_saiDump.rdbJsonFile = "./dump.json";    
ERROR: some files contain white spaces at the end of line, please fix
FAIL: checkwhitespace.sh

This is why you get unit tests to fail you have a lot of white spaces in those lines

Please carefully read errors on builds and tests

To fix the issue:

a754579

sonic-net#1288 (comment)

JunhongMao mentioned this pull request Dec 27, 2023

[VOQ][saidump] Enhance saidump with new option -r to parser the JSON file and displays/format the right output #1288

Merged

10 tasks

Fix the CodeQL stage error of sonic-net#1335

dbb9532

Merge remote-tracking branch 'upstream/202305' into 202305

c8b5891

JunhongMao force-pushed the 202305 branch 6 times, most recently from 72e11a1 to 3979646 Compare February 2, 2024 16:40

JunhongMao force-pushed the 202305 branch from 8f9780c to 0cf0ce7 Compare February 2, 2024 17:52

kcudnik reviewed Feb 4, 2024

View reviewed changes

unittest/saidump/tests Outdated Show resolved Hide resolved

JunhongMao force-pushed the 202305 branch from 2fe7748 to 26cd591 Compare February 5, 2024 22:43

JunhongMao added a commit to JunhongMao/sonic-sairedis that referenced this pull request Feb 6, 2024

Install nlohmann-json3-dev for CodeQL analysis of

fc8f22e

sonic-net#1335

JunhongMao mentioned this pull request Feb 6, 2024

[VOQ][saidump] Enhance saidump with new option -r to parser the JSON file and displays/format the right output, fix compiling error #1335, to fix dpkg-checkbuilddeps: error #1350

Merged

Add unittests codes for saidump

5ba869c

JunhongMao force-pushed the 202305 branch from 3a4ae39 to 5ba869c Compare February 16, 2024 15:05

kcudnik reviewed Feb 17, 2024

View reviewed changes

JunhongMao added 2 commits April 15, 2024 20:37

Wrap saidump's functions into a class and update the unittest codes.

5cf3960

Merge remote-tracking branch 'upstream/202305' into 202305

1609aec

kcudnik reviewed Apr 16, 2024

View reviewed changes

saidump/main.cpp Outdated Show resolved Hide resolved

Remove useless codes.

c02410f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VOQ][saidump] Enhance saidump with new option -r to parser the JSON file and displays/format the right output, fix compiling error #1335

[VOQ][saidump] Enhance saidump with new option -r to parser the JSON file and displays/format the right output, fix compiling error #1335

JunhongMao commented Dec 27, 2023 •

edited

Loading

kcudnik commented Jan 3, 2024

JunhongMao commented Jan 3, 2024

kcudnik commented Jan 3, 2024

kcudnik commented Jan 3, 2024

JunhongMao commented Jan 3, 2024

kcudnik commented Jan 3, 2024

JunhongMao commented Jan 3, 2024

kcudnik commented Jan 4, 2024 •

edited

Loading

kcudnik commented Jan 4, 2024

gechiang commented Jan 8, 2024

kcudnik commented Jan 16, 2024

JunhongMao commented Jan 16, 2024

kcudnik commented Jan 16, 2024

JunhongMao commented Jan 18, 2024

kcudnik commented Jan 18, 2024

JunhongMao commented Jan 22, 2024

mlok-nokia commented Jan 22, 2024

kcudnik commented Jan 22, 2024

kcudnik commented Jan 22, 2024

JunhongMao commented Jan 22, 2024

JunhongMao commented Feb 3, 2024

kcudnik Feb 4, 2024

JunhongMao commented Feb 6, 2024

JunhongMao commented Feb 6, 2024

kcudnik commented Feb 6, 2024

kcudnik commented Feb 6, 2024

kcudnik Feb 17, 2024

JunhongMao Feb 19, 2024

kcudnik Feb 20, 2024

JunhongMao commented May 2, 2024

kcudnik commented May 4, 2024

azure-pipelines bot commented May 4, 2024

kcudnik commented May 4, 2024

kcudnik commented May 4, 2024

		@@ -0,0 +1,55 @@
		#include <gtest/gtest.h>
		#include "saidump.cpp"

[VOQ][saidump] Enhance saidump with new option -r to parser the JSON file and displays/format the right output, fix compiling error #1335

Are you sure you want to change the base?

[VOQ][saidump] Enhance saidump with new option -r to parser the JSON file and displays/format the right output, fix compiling error #1335

Conversation

JunhongMao commented Dec 27, 2023 • edited Loading

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Tested branch (Please provide the tested image version)

kcudnik commented Jan 3, 2024

JunhongMao commented Jan 3, 2024

kcudnik commented Jan 3, 2024

kcudnik commented Jan 3, 2024

JunhongMao commented Jan 3, 2024

kcudnik commented Jan 3, 2024

JunhongMao commented Jan 3, 2024

kcudnik commented Jan 4, 2024 • edited Loading

kcudnik commented Jan 4, 2024

gechiang commented Jan 8, 2024

kcudnik commented Jan 16, 2024

JunhongMao commented Jan 16, 2024

kcudnik commented Jan 16, 2024

JunhongMao commented Jan 18, 2024

kcudnik commented Jan 18, 2024

JunhongMao commented Jan 22, 2024

mlok-nokia commented Jan 22, 2024

kcudnik commented Jan 22, 2024

kcudnik commented Jan 22, 2024

JunhongMao commented Jan 22, 2024

JunhongMao commented Feb 3, 2024

kcudnik Feb 4, 2024

Choose a reason for hiding this comment

JunhongMao commented Feb 6, 2024

JunhongMao commented Feb 6, 2024

kcudnik commented Feb 6, 2024

kcudnik commented Feb 6, 2024

kcudnik Feb 17, 2024

Choose a reason for hiding this comment

JunhongMao Feb 19, 2024

Choose a reason for hiding this comment

kcudnik Feb 20, 2024

Choose a reason for hiding this comment

JunhongMao commented May 2, 2024

kcudnik commented May 4, 2024

azure-pipelines bot commented May 4, 2024

kcudnik commented May 4, 2024

kcudnik commented May 4, 2024

JunhongMao commented Dec 27, 2023 •

edited

Loading

kcudnik commented Jan 4, 2024 •

edited

Loading