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

Make PFC commands use a class #3057

Merged
merged 11 commits into from
May 31, 2024
Merged

Conversation

bktsim-arista
Copy link
Contributor

@bktsim-arista bktsim-arista commented Nov 30, 2023

What I did

This change puts contents originally in pfc/main.py into a class, to support the usage of the multi-asic helper in a future change. This change is required, as multi-asic helper being used expects members self.config_db and self.db to exist when a function with the decorator run_on_multi_asic is called. The multi-asic class helper will be used to add multi-asic support to pfc commands in a following pull request.

This is a part of the set of changes being pushed for sonic-net/sonic-buildimage#15148

How I did it

Moved contents of PFC commands into a class. There are no functional changes.

@arista-nwolfe
Copy link

/azpw run Azure.sonic-utilities

@wenyiz2021
Copy link
Contributor

/azpw run

@arlakshm
Copy link
Contributor

/azpw run Azure.sonic-utilities

@arlakshm
Copy link
Contributor

/azp run Azure.sonic-utilities

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

vmittal-msft
vmittal-msft previously approved these changes Mar 22, 2024
@vmittal-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

pfc/main.py Outdated
@@ -1,160 +1,182 @@
#!/usr/bin/env python3

import os
Copy link
Contributor

Choose a reason for hiding this comment

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

please sort imports

Copy link
Contributor

Choose a reason for hiding this comment

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

Done


class TestPfc(TestPfcBase):
@classmethod
def setup_class(cls):
Copy link
Contributor

Choose a reason for hiding this comment

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

teardown needs to be called for TestPfc to set the UTILITIES_UNIT_TESTING to zero

Choose a reason for hiding this comment

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

teardown_class is defined in the base class TestPfcBase, it should inherit that method.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the latest revision, I removed the need for the environment variable UTILITIES_UNIT_TESTING.

pfc/main.py Outdated
PFC handler to display asymmetric PFC information.
"""
header = ('Interface', 'Asymmetric')
if "UTILITIES_UNIT_TESTING" in os.environ and os.environ["UTILITIES_UNIT_TESTING"] == "2":
Copy link
Contributor

Choose a reason for hiding this comment

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

why do need this change?

Choose a reason for hiding this comment

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

I don't think we need it, it looks like it was just to make writing the test easier but we can just as easily call the config command followed by the show command in the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed this and changed the way we validate the test case. Please see latest posted change.


def test_pfc_config_priority(self):
self.executor(pfc.cli, ['config', 'priority', 'on', 'Ethernet0', '5'],
expected_output=pfc_config_priority_on)
Copy link
Contributor

Choose a reason for hiding this comment

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

add test for showPfcPrio ?

Choose a reason for hiding this comment

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

It looks like there already is one for showPfcPrio see test_pfc_show_priority_all, test_pfc_show_priority_intf and test_pfc_show_asymmetric_intf_fake

expected_output=pfc_config_asymmetric)

def test_pfc_config_priority(self):
self.executor(pfc.cli, ['config', 'priority', 'on', 'Ethernet0', '5'],
Copy link
Contributor

Choose a reason for hiding this comment

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

@bktsim-arista Can we add "show pfc counter" test as well ? Also, Could we test this change on pizza box as well as chassis ?

Copy link
Contributor

@kenneth-arista kenneth-arista May 28, 2024

Choose a reason for hiding this comment

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

@vmittal-msft Adding a test for show pfc counter is out of scope for this PR as show pfc counter is implemented in the pfcstat script. See

def counters(namespace, display, verbose):
"""Show pfc counters"""
cmd = ['pfcstat', '-s', str(display)]
if namespace is not None:
cmd += ['-n', str(namespace)]
run_command(cmd, display_cmd=verbose)
.

Also, Could we test this change on pizza box as well as chassis ?

This can be done. Though this PR is about refactoring main.py to implement the config/show methods in a class. New unit tests are added as there were no tests prior to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I confirmed on a T0 switch that the config/show commands in pfc/main.py work as expected. This is not surprising as there is no functional product change in this PR. This PR encapsulates globals methods into a class and adds missing UTs.

bktsim-arista and others added 9 commits May 28, 2024 02:40
This change puts contents originally in pfc/main.py into a class,
to support the usage of the multi-asic helper in a future change.
This change is required, as multi-asic helper relies on certain members
of the class to exist. The multi-asic class helper will be used to add
multi-asic support to pfc commands.
@kenneth-arista
Copy link
Contributor

Because of the conversion of global methods to class methods, there are lots of white space changes. The best way to review this PR is to ignore whitespace.

@kenneth-arista kenneth-arista force-pushed the multi-asic-pfc branch 3 times, most recently from 28dc1d5 to bad2beb Compare May 29, 2024 03:37
Removed the use of the environment variable to test `config asymmetric
on`. Instead replaced the validation method with a direct check for the
expected CONFIG_DB entry. Also removed the unnecessary classmethods in
TestPfcBase.
@arlakshm arlakshm merged commit 6447308 into sonic-net:master May 31, 2024
7 checks passed
@kenneth-arista kenneth-arista deleted the multi-asic-pfc branch June 2, 2024 02:58
arfeigin pushed a commit to arfeigin/sonic-utilities that referenced this pull request Jun 16, 2024
What I did
This change puts contents originally in pfc/main.py into a class, to support the usage of the multi-asic helper in a future change. This change is required, as multi-asic helper being used expects members self.config_db and self.db to exist when a function with the decorator run_on_multi_asic is called. The multi-asic class helper will be used to add multi-asic support to pfc commands in a following pull request.

This is a part of the set of changes being pushed for sonic-net/sonic-buildimage#15148

How I did it
Moved contents of PFC commands into a class. There are no functional changes.

Co-authored-by: rdjeric <rdjeric@arista.com>
Co-authored-by: Kenneth Cheung <kennethcheung@arista.com>
@bingwang-ms
Copy link
Contributor

@bktsim-arista Please file a direct PR to 202405 branch to address the conflict.

@kenneth-arista
Copy link
Contributor

@bktsim-arista Please file a direct PR to 202405 branch to address the conflict.

There's no need to cherry-pick this PR into 202405 because commit 6447308 is already included. 😄 Can you change the label to already included in 202405?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants