Memory Statistics Config and Show Commands #3575

kanza-latif · 2024-10-14T09:32:34Z

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

ridahanif96 · 2024-10-22T12:05:52Z

/azpw run Azure.sonic-utilities

kanza-latif · 2024-10-23T06:30:05Z

/azpw run

mssonicbld · 2024-10-23T06:30:07Z

/AzurePipelines run

azure-pipelines · 2024-10-23T06:30:18Z

Azure Pipelines successfully started running 1 pipeline(s).

ridahanif96 · 2024-10-25T10:12:10Z

@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 · 2024-10-28T07:39:26Z

pls fix pretest analysis failure. thanks

kanza-latif · 2024-10-28T11:30:56Z

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

FengPan-Frank · 2024-10-28T11:36:14Z

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

seems python test is failed. could you double check?

kanza-latif · 2024-10-28T11:45:07Z

@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 · 2024-11-05T02:30:14Z

@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 · 2024-11-05T07:20:11Z

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

ridahanif96 · 2024-11-06T08:49:46Z

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

xincunli-sonic · 2024-11-06T18:01:24Z

config/memory_statistics.py

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


If not needed, please remove it.

Thankyou for reviewing. I just removed it

xincunli-sonic · 2024-11-06T18:03:27Z

config/memory_statistics.py

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


ConfigDBConnector

Is this only considering single asic?

Can you please elaborate on asic ?

xincunli-sonic · 2024-11-06T18:05:59Z

config/memory_statistics.py

+        return  # Exit gracefully on error
+
+    try:
+        db.mod_entry("MEMORY_STATISTICS", "memory_statistics", {"enabled": "true", "disabled": "false"})


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

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

xincunli-sonic · 2024-11-06T18:07:13Z

config/memory_statistics.py

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


memory_statistics_disable

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

xincunli-sonic · 2024-11-06T18:09:27Z

config/memory_statistics.py

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


True

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

xincunli-sonic · 2024-11-06T18:11:38Z

config/memory_statistics.py

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


MEMORY_STATISTICS

Define a constant?

xincunli-sonic · 2024-11-06T18:11:53Z

config/memory_statistics.py

+        click.echo("Unable to retrieve 'MEMORY_STATISTICS' table from Config DB.", err=True)
+        return False
+
+    if "memory_statistics" not in memory_statistics_table:


memory_statistics

Define a constant?

xincunli-sonic · 2024-11-06T18:12:53Z

config/memory_statistics.py

+
+@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):


retention_period

Please add verify and validation for input parameter.

xincunli-sonic · 2024-11-06T18:13:02Z

config/memory_statistics.py

+
+@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):


sampling_interval

Please add verify and validation for input parameter.

xincunli-sonic · 2024-11-06T18:15:10Z

config/memory_statistics.py

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


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.

xincunli-sonic · 2024-11-06T18:18:10Z

config/memory_statistics.py

+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)


echo

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

xincunli-sonic · 2024-11-06T18:19:09Z

config/memory_statistics.py

+@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"""


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

…t and socket communication Signed-off-by: Arham-Nasir <[email protected]>

kanza-latif added 3 commits October 14, 2024 14:30

added show and config commands

ac16864

resolved the errors

f4811d1

resolved the errors

b8d7274

kanza-latif changed the title ~~added show and config commands~~ Memory Statistics Config and Show Commands Oct 16, 2024

ridahanif96 mentioned this pull request Oct 17, 2024

HLD for Memory_Statistics sonic-net/SONiC#1760

Open

added test filw

636f30a

kanza-latif added 2 commits October 23, 2024 14:06

updated test cases

81df5f0

updated test cases

1ad1222

kanza-latif added 2 commits October 28, 2024 11:38

updated the mock db

29afad4

removed pre-commit errors

aecbc72

kanza-latif added 8 commits October 28, 2024 12:52

resolivng pre-commit errors

2d066ee

resolivng pre-commit errors

0cf9043

added mock db in config file

52d7c0a

removed incomaptibilities from the config and test files

43f1f9c

removed incomaptibilities from the config and test files

b4716c3

updated the test file

b17f226

updated the test file

a6680d9

corrected the failing test case

732612f

kanza-latif added 2 commits November 5, 2024 11:35

added testfile for show commands

7b17295

added testfile for show commands

5e72f6b

xincunli-sonic reviewed Nov 6, 2024

View reviewed changes

removed the commented import

a51c84d

xincunli-sonic reviewed Nov 6, 2024

View reviewed changes

Add show memory-stats CLI command with Dict2Obj handling, clean outpu…

07fbf15

…t and socket communication Signed-off-by: Arham-Nasir <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory Statistics Config and Show Commands #3575

Memory Statistics Config and Show Commands #3575

kanza-latif commented Oct 14, 2024 •

edited

Loading

ridahanif96 commented Oct 22, 2024

kanza-latif commented Oct 23, 2024

mssonicbld commented Oct 23, 2024

azure-pipelines bot commented Oct 23, 2024

ridahanif96 commented Oct 25, 2024

FengPan-Frank commented Oct 28, 2024

kanza-latif commented Oct 28, 2024

FengPan-Frank commented Oct 28, 2024

kanza-latif commented Oct 28, 2024

FengPan-Frank commented Nov 5, 2024

kanza-latif commented Nov 5, 2024 •

edited

Loading

ridahanif96 commented Nov 6, 2024

xincunli-sonic Nov 6, 2024

kanza-latif Nov 6, 2024

xincunli-sonic Nov 6, 2024

kanza-latif Nov 7, 2024

xincunli-sonic Nov 6, 2024

xincunli-sonic Nov 6, 2024

xincunli-sonic Nov 6, 2024

xincunli-sonic Nov 6, 2024

xincunli-sonic Nov 6, 2024

xincunli-sonic Nov 6, 2024

xincunli-sonic Nov 6, 2024

xincunli-sonic Nov 6, 2024

xincunli-sonic Nov 6, 2024

xincunli-sonic Nov 6, 2024



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

Memory Statistics Config and Show Commands #3575

Are you sure you want to change the base?

Memory Statistics Config and Show Commands #3575

Conversation

kanza-latif commented Oct 14, 2024 • edited Loading

What I did

How I did it

How to verify it

ridahanif96 commented Oct 22, 2024

kanza-latif commented Oct 23, 2024

mssonicbld commented Oct 23, 2024

azure-pipelines bot commented Oct 23, 2024

ridahanif96 commented Oct 25, 2024

FengPan-Frank commented Oct 28, 2024

kanza-latif commented Oct 28, 2024

FengPan-Frank commented Oct 28, 2024

kanza-latif commented Oct 28, 2024

FengPan-Frank commented Nov 5, 2024

kanza-latif commented Nov 5, 2024 • edited Loading

ridahanif96 commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanza-latif commented Oct 14, 2024 •

edited

Loading

kanza-latif commented Nov 5, 2024 •

edited

Loading