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
Open
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 114 additions & 0 deletions config/memory_statistics.py
Original file line number Diff line number Diff line change
@@ -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



class AbbreviationGroup(click.Group):
def get_command(self, ctx, cmd_name):
return super().get_command(ctx, cmd_name)


@click.group(cls=AbbreviationGroup, name="memory-statistics")
def memory_statistics():
"""Configure the Memory Statistics feature"""
pass


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?

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?

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?



@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?

db.connect()

memory_statistics_table = get_memory_statistics_table(db)
if not check_memory_statistics_table_existence(memory_statistics_table):
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?

click.echo("Memory Statistics feature enabled.")
except Exception as e:
click.echo(f"Error enabling Memory Statistics feature: {str(e)}", err=True)

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


@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.

"""Disable the Memory Statistics feature"""
db = ConfigDBConnector()
db.connect()

memory_statistics_table = get_memory_statistics_table(db)
if not check_memory_statistics_table_existence(memory_statistics_table):
return # Exit gracefully on error

try:
db.mod_entry("MEMORY_STATISTICS", "memory_statistics", {"enabled": "false", "disabled": "true"})
click.echo("Memory Statistics feature disabled.")
except Exception as e:
click.echo(f"Error disabling Memory Statistics feature: {str(e)}", err=True)

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


@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.

"""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.

db = ConfigDBConnector()
db.connect()

memory_statistics_table = get_memory_statistics_table(db)
if not check_memory_statistics_table_existence(memory_statistics_table):
return # Exit gracefully on error

try:
db.mod_entry("MEMORY_STATISTICS", "memory_statistics", {"retention_period": retention_period})
click.echo(f"Memory Statistics retention period set to {retention_period} days.")
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.



@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.

"""Set the sampling interval for Memory Statistics"""
db = ConfigDBConnector()
db.connect()

memory_statistics_table = get_memory_statistics_table(db)
if not check_memory_statistics_table_existence(memory_statistics_table):
return # Exit gracefully on error

try:
db.mod_entry("MEMORY_STATISTICS", "memory_statistics", {"sampling_interval": sampling_interval})
click.echo(f"Memory Statistics sampling interval set to {sampling_interval} minutes.")
except Exception as e:
click.echo(f"Error setting sampling interval: {str(e)}", err=True)

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


if __name__ == "__main__":
memory_statistics()
111 changes: 111 additions & 0 deletions show/memory_statistics.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
import click
from tabulate import tabulate

import utilities_common.cli as clicommon


#
# 'memory-statistics' group (show memory-statistics ...)
#
@click.group(cls=clicommon.AliasedGroup, name="memory-statistics")
def memory_statistics():
"""Show memory statistics configuration and logs"""
pass


def get_memory_statistics_config(field_name, db_connector):
"""Fetches the configuration of memory_statistics from `CONFIG_DB`.

Args:
field_name: A string containing the field name in the sub-table of 'memory_statistics'.
db_connector: The database connector.

Returns:
field_value: If field name was found, then returns the corresponding value.
Otherwise, returns "Unknown".
"""
field_value = "Unknown"
memory_statistics_table = db_connector.get_table("MEMORY_STATISTICS")
if (memory_statistics_table and
"memory_statistics" in memory_statistics_table and
field_name in memory_statistics_table["memory_statistics"]):
field_value = memory_statistics_table["memory_statistics"][field_name]

return field_value


@memory_statistics.command(name="memory_statistics", short_help="Show the configuration of memory statistics")
@click.pass_context
def config(ctx):
"""Show the configuration of memory statistics."""
db_connector = ctx.obj['db_connector'] # Get the database connector from the context
admin_mode = "Disabled"
admin_enabled = get_memory_statistics_config("enabled", db_connector)
if admin_enabled == "true":
admin_mode = "Enabled"

click.echo("Memory Statistics administrative mode: {}".format(admin_mode))

retention_time = get_memory_statistics_config("retention_period", db_connector)
click.echo("Memory Statistics retention time (days): {}".format(retention_time))

sampling_interval = get_memory_statistics_config("sampling_interval", db_connector)
click.echo("Memory Statistics sampling interval (minutes): {}".format(sampling_interval))


def fetch_memory_statistics(starting_time=None, ending_time=None, additional_options=None, db_connector=None):
"""Fetch memory statistics from the database.

Args:
starting_time: The starting time for filtering the statistics.
ending_time: The ending time for filtering the statistics.
additional_options: Any additional options for filtering or formatting.
db_connector: The database connector.

Returns:
A list of memory statistics entries.
"""
memory_statistics_table = db_connector.get_table("MEMORY_STATISTICS")
filtered_statistics = []

# Ensure you access the right table structure
if "memory_statistics" in memory_statistics_table:
for key, entry in memory_statistics_table["memory_statistics"].items():
# Add filtering logic here based on starting_time, ending_time, and additional options
if (not starting_time or entry.get("time", "") >= starting_time) and \
(not ending_time or entry.get("time", "") <= ending_time):
filtered_statistics.append(entry)

return filtered_statistics


@memory_statistics.command(name="logs", short_help="Show memory statistics logs with optional filtering")
@click.argument('starting_time', required=False)
@click.argument('ending_time', required=False)
@click.argument('additional_options', required=False, nargs=-1)
@click.pass_context
def show_memory_statistics_logs(ctx, starting_time, ending_time, additional_options):
"""Show memory statistics logs with optional filtering by time and additional options."""
db_connector = ctx.obj['db_connector'] # Get the database connector from the context

# Fetch memory statistics
memory_statistics = fetch_memory_statistics(
starting_time,
ending_time,
additional_options,
db_connector=db_connector
)
if not memory_statistics:
click.echo("No memory statistics available for the given parameters.")
return

# Display the memory statistics
headers = ["Time", "Statistic", "Value"] # Adjust according to the actual fields
table_data = [
[
entry.get("time", "N/A"),
entry.get("statistic", "N/A"),
entry.get("value", "N/A"),
] for entry in memory_statistics
]
click.echo(tabulate(table_data, headers=headers, tablefmt="grid"))
Loading
Loading