Refactor Orch class to separate recorder implementation by vivekrnv · Pull Request #2837 · sonic-net/sonic-swss

vivekrnv · 2023-06-27T00:37:01Z

What I did

Removed the recorder implementation from the Orch class and added a new Recorder Interface
Removed the exposure of recorder related global variables from the CfgMgrs and Orchagent
Writing the Kfv tuple to the Rec file was moved from the Orch class into the ConsumerBase
Moved the gBatchSize declaration into the Orch and thereby removing the need to explicitly declare the global variable in CfgMgrs

Why I did it

Recorder implementation should not be tied to Orchagent

How I verified it

Install the swss on the switch and verified

root@r-lionfish-16:/home/admin# show logging | grep Recorder
Jun 23 19:26:15.741815 sonic NOTICE swss#orchagent: :- startRec: SwSS Recorder: Recording started at /var/log/swss/swss.rec
Jun 23 19:29:09.514177 sonic INFO swss#orchagent: :- logfileReopen: SwSS Recorder: LogRotate request handled

Verify Logrotate:

root@r-lionfish-16:/home/admin# ls -l /var/log/swss/
total 2752
-rw-r--r-- 1 root root  999877 Jun 23 19:26 sairedis.rec
-rw-r--r-- 1 root root 1559947 Jun 23 19:11 sairedis.rec.1
-rw-r--r-- 1 root root  110563 Jun 23 19:27 swss.rec
-rw-r--r-- 1 root root  141713 Jun 23 19:16 swss.rec.1


root@r-lionfish-16:/home/admin# logrotate -f /etc/logrotate.conf  (Force Logrotate)

root@r-lionfish-16:/home/admin# ls -l /var/log/swss/
total 1232
-rw-r--r-- 1 root root    140 Jun 23 19:28 sairedis.rec
-rw-r--r-- 1 root root 999877 Jun 23 19:26 sairedis.rec.1
-rw-r--r-- 1 root root 126798 Jun 23 19:11 sairedis.rec.2.gz
-rw-r--r-- 1 root root      0 Jun 23 19:28 swss.rec
-rw-r--r-- 1 root root 110563 Jun 23 19:27 swss.rec.1
-rw-r--r-- 1 root root  13089 Jun 23 19:16 swss.rec.2.gz

<After some time>

root@r-lionfish-16:/home/admin# ls -l /var/log/swss/
total 1476
-rw-r--r-- 1 root root 242404 Jun 23 19:29 sairedis.rec
-rw-r--r-- 1 root root 999877 Jun 23 19:26 sairedis.rec.1
-rw-r--r-- 1 root root 126798 Jun 23 19:11 sairedis.rec.2.gz
-rw-r--r-- 1 root root   1764 Jun 23 19:29 swss.rec
-rw-r--r-- 1 root root 110645 Jun 23 19:29 swss.rec.1
-rw-r--r-- 1 root root  13089 Jun 23 19:16 swss.rec.2.gz

Details if related

…iables Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

…actor_orch

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

prsunny · 2023-06-30T18:21:15Z

tagging @qiluo-msft, @yxieca

qiluo-msft · 2023-07-01T02:08:22Z

orchagent/orch.cpp

-extern ofstream gRecordOfs;
-extern bool gLogRotate;
-extern string gRecordFile;
+int gBatchSize = 0;


I see you move the definition of this global variable from main.cpp to orch.cpp. Is it really necessary? #Closed

Yes, if not every daemon that inherits from Orch should define this.

None of the cfgmgrs require the use of gBatchSize var, so it is defined as zero. Only orchagent needs to updated this and thus it is linked with "extern" and update in main.cpp

qiluo-msft · 2023-07-01T02:13:16Z

lib/recorder.h

+    bool enable_rec;
+    bool log_rotate;
+    std::string location;
+    std::string filename;


filename

filename -> m_filename #Closed

qiluo-msft · 2023-07-01T02:15:02Z

lib/recorder.h

+    std::string getName() { return m_name; }
+
+private:
+    bool enable_rec;


enable_rec

Please add verbose documents to new code. For example, what is m_name, if it is a name, why you have another filename. Seems like m_name should be m_somename. #Closed

m_name is just the user readable name of the type of recorder. "SwSS", "Response Publisher" & "SaiRedis". Used while printing the logs

Eg:

SWSS_LOG_NOTICE("%s Recorder: Recording started at %s", getName().c_str(), fname.c_str());

Jun 23 19:26:15.741815 sonic NOTICE swss#orchagent: :- startRec: SwSS Recorder: Recording started at /var/log/swss/swss.rec

qiluo-msft · 2023-07-01T02:18:12Z

lib/recorder.h

+class SaiRedisRec : public RecBase {
+public:
+    SaiRedisRec();
+};


All 3 classes share the same interfaces. They fit better into 3 objects of class RecWriter. #WontFix

And that's why we have a Recorder interface to access these

/* Interface to access recorder classes */ class Recorder { public: static std::unique_ptr<SwSSRec> swss; static std::unique_ptr<SaiRedisRec> sairedis; static std::unique_ptr<ResPubRec> respub; };

The proposal "3 objects of class RecWriter" allow you to the same thing.

Having 3 separate interfaces is flexible since we can easily extend the functionality for any one of these separately, if required.

kcudnik · 2023-07-01T11:13:21Z

lib/recorder.h

+
+class RecBase {
+public:
+    RecBase() {}


use default keyword {}

kcudnik · 2023-07-01T11:14:22Z

lib/recorder.h

+    static std::unique_ptr<SwSSRec> swss;
+    static std::unique_ptr<SaiRedisRec> sairedis;
+    static std::unique_ptr<ResPubRec> respub;


i would prefer to not have those static objects

What would you recommend? The only other option i could think of is to define a singleton of Recorder and have these as public member objects.

But to access the singleton everytime, we might need to define a method like getInstance() and use it to access the object and their methods, kinds like the logger class under swsscomon

Eg: Recorder::getInstance().swss.startRec()

you dont need actually singleton, just a instance of recorder in Orch class, if you have more static members, its harder to test

Defining recorder instance as a member of Orch class will not work. There are multiple orch classes and all write to a same file.

So, logically the scope of recorder is the daemon/process and not just orch. Even if we can workaround with using an instance of the recorder class per orch, we would still need static members for handing ofstream, path etc.

Best case scenario is one static Recorder instance and using it as singleton.

kcudnik · 2023-07-01T11:16:56Z

orchagent/orch.h

    Orch(swss::DBConnector *db, const std::vector<table_name_with_pri_t> &tableNameWithPri);
    Orch(const std::vector<TableConnector>& tables);
-    virtual ~Orch();
+    virtual ~Orch() {};


use default keyword

…actor_orch

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

vivekrnv · 2023-07-04T00:13:03Z

/azpw run Azure.sonic-swss

mssonicbld · 2023-07-04T00:13:05Z

/AzurePipelines run Azure.sonic-swss

azure-pipelines · 2023-07-04T00:13:15Z

Azure Pipelines successfully started running 1 pipeline(s).

vivekrnv · 2023-07-04T00:46:59Z

@kcudnik , @qiluo-msft . I've handled your comments. Please check

vivekrnv · 2023-07-06T13:52:48Z

/azpw run Azure.sonic-swss

mssonicbld · 2023-07-06T13:52:50Z

/AzurePipelines run Azure.sonic-swss

azure-pipelines · 2023-07-06T13:53:00Z

Azure Pipelines successfully started running 1 pipeline(s).

liat-grozovik · 2023-07-06T15:20:42Z

@qiluo-msft @kcudnik could you please help to review following comments?

dgsudharsan · 2023-07-13T22:56:57Z

@qiluo-msft Can you please review and signoff?

**What I did** 1) Removed the recorder implementation from the Orch class and added a new Recorder Interface 2) Removed the exposure of recorder related global variables from the CfgMgrs and Orchagent 3) Writing the Kfv tuple to the Rec file was moved from the Orch class into the ConsumerBase 4) Moved the gBatchSize declaration into the Orch and thereby removing the need to explicitly declare the global variable in CfgMgrs **Why I did it** Recorder implementation should not be tied to Orchagent **How I verified it** Install the swss on the switch and verified ``` root@r-lionfish-16:/home/admin# show logging | grep Recorder Jun 23 19:26:15.741815 sonic NOTICE swss#orchagent: :- startRec: SwSS Recorder: Recording started at /var/log/swss/swss.rec Jun 23 19:29:09.514177 sonic INFO swss#orchagent: :- logfileReopen: SwSS Recorder: LogRotate request handled ``` **Verify Logrotate:** ``` root@r-lionfish-16:/home/admin# ls -l /var/log/swss/ total 2752 -rw-r--r-- 1 root root 999877 Jun 23 19:26 sairedis.rec -rw-r--r-- 1 root root 1559947 Jun 23 19:11 sairedis.rec.1 -rw-r--r-- 1 root root 110563 Jun 23 19:27 swss.rec -rw-r--r-- 1 root root 141713 Jun 23 19:16 swss.rec.1 root@r-lionfish-16:/home/admin# logrotate -f /etc/logrotate.conf (Force Logrotate) root@r-lionfish-16:/home/admin# ls -l /var/log/swss/ total 1232 -rw-r--r-- 1 root root 140 Jun 23 19:28 sairedis.rec -rw-r--r-- 1 root root 999877 Jun 23 19:26 sairedis.rec.1 -rw-r--r-- 1 root root 126798 Jun 23 19:11 sairedis.rec.2.gz -rw-r--r-- 1 root root 0 Jun 23 19:28 swss.rec -rw-r--r-- 1 root root 110563 Jun 23 19:27 swss.rec.1 -rw-r--r-- 1 root root 13089 Jun 23 19:16 swss.rec.2.gz <After some time> root@r-lionfish-16:/home/admin# ls -l /var/log/swss/ total 1476 -rw-r--r-- 1 root root 242404 Jun 23 19:29 sairedis.rec -rw-r--r-- 1 root root 999877 Jun 23 19:26 sairedis.rec.1 -rw-r--r-- 1 root root 126798 Jun 23 19:11 sairedis.rec.2.gz -rw-r--r-- 1 root root 1764 Jun 23 19:29 swss.rec -rw-r--r-- 1 root root 110645 Jun 23 19:29 swss.rec.1 -rw-r--r-- 1 root root 13089 Jun 23 19:16 swss.rec.2.gz ```

vivekrnv added 7 commits June 20, 2023 22:59

Added new Recorder Interface and removed ResponsePublisher Global Var…

5cfc886

…iables Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Removed sairedis.rec global variables

3f60432

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Removed all recorder related global variable and used Recorder interface

b914049

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Handle build failures

aefdb0b

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Refactoring recorder

9500bc8

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

Removed the explicit definiton of gBatchSize from the cfgmgrs

11089de

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

handle spell check failures

0899623

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

vivekrnv requested review from judyjoseph, neethajohn and prsunny as code owners June 27, 2023 00:37

vivekrnv requested a review from dgsudharsan June 27, 2023 00:37

dgsudharsan previously approved these changes Jun 27, 2023

View reviewed changes

vivekrnv added 2 commits June 27, 2023 01:23

Merge branch 'master' of https://github.com/Azure/sonic-swss into ref…

5746702

…actor_orch

Add missing declaration

d8588e9

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

vivekrnv dismissed dgsudharsan’s stale review via d8588e9 June 27, 2023 01:43

dgsudharsan previously approved these changes Jun 27, 2023

View reviewed changes

prsunny requested a review from prabhataravind June 30, 2023 18:20

qiluo-msft requested review from Pterosaur and liuh-80 July 1, 2023 01:59

qiluo-msft reviewed Jul 1, 2023

View reviewed changes

qiluo-msft requested a review from kcudnik July 1, 2023 02:11

qiluo-msft reviewed Jul 1, 2023

View reviewed changes

kcudnik reviewed Jul 1, 2023

View reviewed changes

lib/recorder.h Outdated

class RecBase {

public:

RecBase() {}

Copy link
Copy Markdown

Contributor

kcudnik Jul 1, 2023

Choose a reason for hiding this comment

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

use default keyword {}

kcudnik reviewed Jul 1, 2023

View reviewed changes

vivekrnv added 2 commits July 3, 2023 22:55

Merge branch 'master' of https://github.com/Azure/sonic-swss into ref…

ab31a8b

…actor_orch

Converted Recorder into a singleton and handled other comments

5895a14

Signed-off-by: Vivek Reddy Karri <vkarri@nvidia.com>

vivekrnv dismissed dgsudharsan’s stale review via 5895a14 July 3, 2023 23:53

vivekrnv requested review from kcudnik and qiluo-msft July 6, 2023 14:48

kcudnik approved these changes Jul 6, 2023

View reviewed changes

Merge branch 'master' into refactor_orch

9d9eca6

dgsudharsan approved these changes Jul 13, 2023

View reviewed changes

qiluo-msft merged commit 5b27c20 into sonic-net:master Jul 16, 2023

vivekrnv deleted the refactor_orch branch September 13, 2023 23:07

Conversation

vivekrnv commented Jun 27, 2023

Uh oh!

prsunny commented Jun 30, 2023

Uh oh!

qiluo-msft Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vivekrnv Jul 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vivekrnv Jul 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vivekrnv commented Jul 4, 2023

Uh oh!

mssonicbld commented Jul 4, 2023

Uh oh!

azure-pipelines bot commented Jul 4, 2023

Uh oh!

vivekrnv commented Jul 4, 2023

Uh oh!

vivekrnv commented Jul 6, 2023

Uh oh!

mssonicbld commented Jul 6, 2023

Uh oh!

azure-pipelines bot commented Jul 6, 2023

Uh oh!

liat-grozovik commented Jul 6, 2023

Uh oh!

dgsudharsan commented Jul 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

qiluo-msft Jul 1, 2023 •

edited

Loading

qiluo-msft Jul 1, 2023 •

edited

Loading

qiluo-msft Jul 1, 2023 •

edited

Loading

qiluo-msft Jul 1, 2023 •

edited

Loading

vivekrnv Jul 1, 2023 •

edited

Loading

vivekrnv Jul 15, 2023 •

edited

Loading