Skip to content
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

Memory Statistics Config and Show Commands #3575

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

kanza-latif
Copy link

@kanza-latif kanza-latif commented Oct 14, 2024

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

@kanza-latif kanza-latif changed the title added show and config commands Memory Statistics Config and Show Commands Oct 16, 2024
@ridahanif96
Copy link
Contributor

/azpw run Azure.sonic-utilities

@kanza-latif
Copy link
Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ridahanif96
Copy link
Contributor

@qiluo-msft @xincunli-sonic @zbud-msft @FengPan-Frank We have linked the code PRs. Please help review the feature.
Target 202411, please help with expedite review.

@FengPan-Frank
Copy link
Contributor

pls fix pretest analysis failure. thanks

@kanza-latif
Copy link
Author

@FengPan-Frank I've resolved it, kindly review.

@FengPan-Frank
Copy link
Contributor

@FengPan-Frank I've resolved it, kindly review.

seems python test is failed. could you double check?

@kanza-latif
Copy link
Author

@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

@FengPan-Frank
Copy link
Contributor

@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
you can create dummy PR for verify if it's current pipeline issue or your code issue.

@kanza-latif
Copy link
Author

kanza-latif commented Nov 5, 2024

@FengPan-Frank I've made all the necessary changes and all the checks have passed. Kindly review.

@ridahanif96
Copy link
Contributor

@qiluo-msft @xincunli-sonic pls help review this feature

@@ -0,0 +1,114 @@
import click
from swsscommon.swsscommon import ConfigDBConnector
# from utilities_common.cli import AbbreviationGroup
Copy link
Contributor

Choose a reason for hiding this comment

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

If not needed, please remove it.

Copy link
Author

Choose a reason for hiding this comment

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

Thankyou for reviewing. I just removed it

@memory_statistics.command(name="enable", short_help="Enable the Memory Statistics feature")
def memory_statistics_enable():
"""Enable the Memory Statistics feature"""
db = ConfigDBConnector()
Copy link
Contributor

Choose a reason for hiding this comment

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

ConfigDBConnector

Is this only considering single asic?

return # Exit gracefully on error

try:
db.mod_entry("MEMORY_STATISTICS", "memory_statistics", {"enabled": "true", "disabled": "false"})
Copy link
Contributor

Choose a reason for hiding this comment

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

{"enabled": "true", "disabled": "false"}

Could you explain why do we use two same meaning flags?



@memory_statistics.command(name="disable", short_help="Disable the Memory Statistics feature")
def memory_statistics_disable():
Copy link
Contributor

Choose a reason for hiding this comment

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

memory_statistics_disable

Since enable and disable functions are most of identical, please consider having a common helper function.

click.echo("Unable to retrieve key 'memory_statistics' from MEMORY_STATISTICS table.", err=True)
return False

return True
Copy link
Contributor

Choose a reason for hiding this comment

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

True

Consider return boolean and error, then caller to handle positive and negative cases.


def get_memory_statistics_table(db):
"""Get the MEMORY_STATISTICS table from the database."""
return db.get_table("MEMORY_STATISTICS")
Copy link
Contributor

Choose a reason for hiding this comment

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

MEMORY_STATISTICS

Define a constant?

click.echo("Unable to retrieve 'MEMORY_STATISTICS' table from Config DB.", err=True)
return False

if "memory_statistics" not in memory_statistics_table:
Copy link
Contributor

Choose a reason for hiding this comment

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

memory_statistics

Define a constant?


@memory_statistics.command(name="retention-period", short_help="Configure the retention period for Memory Statistics")
@click.argument('retention_period', metavar='<retention_period>', required=True, type=int)
def memory_statistics_retention_period(retention_period):
Copy link
Contributor

Choose a reason for hiding this comment

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

retention_period

Please add verify and validation for input parameter.


@memory_statistics.command(name="sampling-interval", short_help="Configure the sampling interval for Memory Statistics")
@click.argument('sampling_interval', metavar='<sampling_interval>', required=True, type=int)
def memory_statistics_sampling_interval(sampling_interval):
Copy link
Contributor

Choose a reason for hiding this comment

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

sampling_interval

Please add verify and validation for input parameter.

except Exception as e:
click.echo(f"Error setting retention period: {str(e)}", err=True)

click.echo("Save SONiC configuration using 'config save' to persist the changes.")
Copy link
Contributor

Choose a reason for hiding this comment

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

click.echo("Save SONiC configuration using 'config save' to persist the changes.")

After each command (e.g., enable, disable), there’s a reminder to run 'config save' to persist changes. It could be helpful to wrap these reminders with conditional checks to only show the message when a change is successful.

def check_memory_statistics_table_existence(memory_statistics_table):
"""Checks whether the 'MEMORY_STATISTICS' table is configured in Config DB."""
if not memory_statistics_table:
click.echo("Unable to retrieve 'MEMORY_STATISTICS' table from Config DB.", err=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

echo

for all click.echo, suggest to add corresponding message to syslog?

@memory_statistics.command(name="retention-period", short_help="Configure the retention period for Memory Statistics")
@click.argument('retention_period', metavar='<retention_period>', required=True, type=int)
def memory_statistics_retention_period(retention_period):
"""Set the retention period for Memory Statistics"""
Copy link
Contributor

Choose a reason for hiding this comment

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

"""Set the retention period for Memory Statistics"""

Can we add an explaining the expected input and any limitations, like valid range or unit. Same as sampling_interval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants