Memory Statistics Config and Show Commands #3575
Conversation
kanza-latif
changed the title
|
/azpw run Azure.sonic-utilities |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@qiluo-msft @xincunli-sonic @zbud-msft @FengPan-Frank We have linked the code PRs. Please help review the feature. |
|
pls fix pretest analysis failure. thanks |
|
@FengPan-Frank I've resolved it, kindly review. |
seems python test is failed. could you double check? |
|
@FengPan-Frank there are some test cases failing but those are not related to memory statistics feature. The memory_statistics_test have passed, could you guide what should i do |
|
|
This PR has been approved with the latest updates and modifications, kindly help merge this into the SONiC master. |
Signed-off-by: Arham-Nasir <[email protected]>
Arham-Nasir
dismissed stale reviews from xincunli-sonic and FengPan-Frank
via
dab65d4
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Reviewers, |
config/memory_statistics.py
Outdated
| click.echo(msg) | ||
| log_to_syslog(msg) | ||
| return True, None | ||
| except Exception as e: |
There was a problem hiding this comment.
Thank you for your suggestion. I have updated the code to catch more specific exception types, such as KeyError, ValueError, and ConnectionError, instead of using a generic Exception.
There was a problem hiding this comment.
Thanks for the new code! however, I still see the "except Exception as e" branch. Is it really needed?
There was a problem hiding this comment.
Thanks for the feedback! I have now removed the except Exception as e branch, and only specific exceptions (e.g., KeyError, ConnectionError, RuntimeError, OSError, ValueError) are handled.
config/memory_statistics.py
Outdated
| the action and returns a tuple indicating whether the operation was successful. | ||
|
|
||
| Args: | ||
| status (str): The status to set for memory statistics ("true" or "false"). |
There was a problem hiding this comment.
Thank you for your suggestion. I have modified the code to use a boolean type instead of a string when setting the memory statistics status. This ensures better type safety and improves code readability.
config/memory_statistics.py
Outdated
| click.echo(success_msg) | ||
| log_to_syslog(success_msg) | ||
| click.echo("Reminder: Please run 'config save' to persist changes.") | ||
| except Exception as e: |
There was a problem hiding this comment.
I have updated the exception handling to catch specific exceptions rather than using generic Exception handling.
There was a problem hiding this comment.
Thanks for the new code! however, I still see the "except Exception as e" branch. Is it really needed?
There was a problem hiding this comment.
Thanks for the feedback! I have now removed the except Exception as e branch, and only specific exceptions (e.g., KeyError, ConnectionError, RuntimeError, OSError, ValueError) are handled. Let me know if you have any further suggestions.
show/memory_statistics.py
Outdated
| self.config_db.connect() | ||
| syslog.syslog(syslog.LOG_INFO, "Successfully connected to SONiC config database") | ||
| return | ||
| except Exception as e: |
There was a problem hiding this comment.
Thank you for the suggestion. I have updated the exception handling to catch specific exceptions rather than using generic Exception handling.
| syslog.syslog(syslog.LOG_WARNING, | ||
| f"Failed to connect to database" | ||
| f"(attempt {retries}/{max_retries}): {str(e)}") | ||
| time.sleep(retry_delay) |
There was a problem hiding this comment.
I have refactored the exception handling to keep the except block minimal. The retry logic and logging are now handled outside the exception block, ensuring cleaner code and better separation of concerns.
show/memory_statistics.py
Outdated
| except Exception as e: | ||
| error_msg = f"Error retrieving memory statistics configuration: {str(e)}" | ||
| syslog.syslog(syslog.LOG_ERR, error_msg) | ||
| raise RuntimeError(error_msg) |
There was a problem hiding this comment.
Thank You your suggestion. To preserve the full exception stack trace, I have updated the code to use raise ... from e. This ensures that the original exception context is retained, making debugging easier by retaining full context while adding additional error messages when necessary.
|
|
||
| self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | ||
| self.sock.settimeout(Config.SOCKET_TIMEOUT) | ||
| self.sock.connect(self.socket_path) |
There was a problem hiding this comment.
Thank you for pointing this out.The socket file permissions are now explicitly set to 0o600 (read and write for the owner only), ensuring that unauthorized users cannot access or modify the socket. Additionally, ownership is restricted to the root user, preventing potential security risks.
…c, and enhance socket security. Signed-off-by: Arham-Nasir <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi everyone, I have addressed all review comments and pushed the updated code. Some previous approvals were dismissed due to the new push. Kindly review the latest changes at your convenience. Thank you! |
Signed-off-by: Arham-Nasir <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@qiluo-msft We have incorporated all the suggested changes. Please help review and merge this |
What I did
Provided support for the configuration and show commands of memory statistics feature.
How I did it
I added a new config file config/memory_statistics.py for the configuration commands of memory statistics feature.
Added a show file show/memory_statistics.py for the show commands of memory statistics feature.
Added a test file tests/memory_statistics_test.py to test the config and show commands of memory statistics feature
How to verify it
Cli support for config and show file is available