-
Notifications
You must be signed in to change notification settings - Fork 543
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
Suppress Cable length errors in case pg profile lookup file is not provided #509
Conversation
cfgmgr/buffermgr.cpp
Outdated
@@ -34,6 +34,7 @@ void BufferMgr::readPgProfileLookupFile(string file) | |||
ifstream infile(file); | |||
if (!infile.is_open()) | |||
{ | |||
m_pgfile_exist = false; | |||
return; |
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.
Should we log an error here?
SWSS_LOG_ERROR #Closed
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.
It is not necessarily an error if we don't need this feature. I can put an NOTICE or WARNING log message here. #Closed
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.
If you are sure the feature is optional, then a WARNING should be ok.
In reply to: 189676820 [](ancestors = 189676820)
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.
Done.
cfgmgr/buffermgr.cpp
Outdated
@@ -34,6 +34,7 @@ void BufferMgr::readPgProfileLookupFile(string file) | |||
ifstream infile(file); | |||
if (!infile.is_open()) | |||
{ | |||
m_pgfile_exist = false; |
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.
m_pgfile_exist [](start = 8, length = 14)
m_pgfile_exist [](start = 8, length = 14)
m_pgfile_readable #Closed
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.
Done.
cfgmgr/buffermgr.cpp
Outdated
@@ -34,6 +34,7 @@ void BufferMgr::readPgProfileLookupFile(string file) | |||
ifstream infile(file); | |||
if (!infile.is_open()) | |||
{ | |||
m_pgfile_exist = false; | |||
return; | |||
} |
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.
assign m_pgfile_exist true here, and you could remove class member initialization in header file.
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.
Either way should do the work, any reason you prefer to put here rather than in the header file?
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.
For better readability.
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.
I don't see any confusion with the initialization in header file. it is easily readable so I kept the code here.
@yxieca , @volodymyrsamotiy , can you review? |
cfgmgr/buffermgr.cpp
Outdated
@@ -34,6 +34,8 @@ void BufferMgr::readPgProfileLookupFile(string file) | |||
ifstream infile(file); | |||
if (!infile.is_open()) | |||
{ | |||
m_pgfile_readable = false; |
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.
Do you think m_pgfile_processed a more suitable name for this variable? I would think setting it to false by default, and flip it to true once the file processing is done will serve your purpose better?
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.
The purpose was to check whether the "pg-lookup-file" was there or not, if not, we don't do the buffer settings. It renamed to cover the case where the file somehow not readable.
By looking at the code, there wasn't any exist after this "is_open" check and before the entire function. This means if we had m_pgfile_processed defined, we would have always set it to true after this check. I,E, there was no difference between the current approach and if we had a new naming.
Anyway, I changed it so everybody is happy.
cfgmgr/buffermgr.cpp
Outdated
@@ -34,6 +34,8 @@ void BufferMgr::readPgProfileLookupFile(string file) | |||
ifstream infile(file); | |||
if (!infile.is_open()) | |||
{ | |||
m_pgfile_processed = false; |
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.
Thanks for making the changes.
You can move the above line to the beginning of the function.
Hi, I am still observing these messages, Let me know if have to do any configuration/something. Regards, root@sonic:/# show version Docker images: root@sonic:/# Sep 10 12:43:02.267414 sonic WARNING buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet104. Cable length is not set |
Hi, I am still observing these messages, Let me know if have to do any configuration/something. Regards, root@sonic:/var/log# show version Docker images: |
Signed-off-by: Nazarii Hnydyn <nazariig@mellanox.com>
* Provide generic stat api to switch object * Add generic api to tam_int * Provide vs generic stat api to vs switch * Add vs generic api to vs tam_int Signed-off-by: Wenda Ni <wenni@microsoft.com>
What I did
Suppress the error messages like below
May 19 07:39:02.473125 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet0. Cable length is not set
If we don't have pg_profile_lookup.ini file provided.
Why I did it
In some platform, pg_profile_lookup.ini is not provided and we don't need this buffer management feature.
Today the code is repeating these messages:
May 19 07:39:02.473125 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet0. Cable length is not set
May 19 07:39:02.473362 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet1. Cable length is not set
May 19 07:39:02.473558 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet10. Cable length is not set
May 19 07:39:02.473717 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet100. Cable length is not set
May 19 07:39:02.473872 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet102. Cable length is not set
May 19 07:39:02.474061 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet104. Cable length is not set
May 19 07:39:02.474248 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet106. Cable length is not set
May 19 07:39:02.474404 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet108. Cable length is not set
May 19 07:39:02.474557 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet11. Cable length is not set
May 19 07:39:02.474710 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet110. Cable length is not set
May 19 07:39:02.474864 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet112. Cable length is not set
May 19 07:39:02.475018 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet114. Cable length is not set
May 19 07:39:02.475171 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet116. Cable length is not set
May 19 07:39:02.475325 sonic NOTICE buffermgrd: :- doSpeedUpdateTask: Unable to create/update PG profile for port Ethernet118. Cable length is not set
We should disable this error messages.
How I verified it
With the fix, no error messages were seen.
Details if related