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

Default implementation of under/over speed checks #382

Merged
merged 5 commits into from
Jul 11, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
46 changes: 46 additions & 0 deletions sonic_platform_base/fan_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,52 @@ def get_speed_tolerance(self):
"""
raise NotImplementedError

def is_under_speed(self):
"""
Calculates if the fan speed is under the tolerated low speed threshold

Default calculation requires get_speed_tolerance to be implemented, and checks
if the current fan speed (expressed as a percentage) is lower than <get_speed_tolerance>
percent below the target fan speed (expressed as a percentage)

Returns:
A boolean, True if fan speed is under the low threshold, False if not
"""
speed = self.get_speed()
target_speed = self.get_target_speed()
tolerance = self.get_speed_tolerance()

for param, value in (('speed', speed), ('target speed', target_speed), ('speed tolerance', tolerance)):
if not isinstance(value, int):
raise TypeError(f'Fan {param} is not an integer value: {param}={value}')
if value < 0 or value > 100:
raise ValueError(f'Fan {param} is not a valid percentage value: {param}={value}')

return speed * 100 < target_speed * (100 - tolerance)

def is_over_speed(self):
"""
Calculates if the fan speed is over the tolerated high speed threshold

Default calculation requires get_speed_tolerance to be implemented, and checks
if the current fan speed (expressed as a percentage) is higher than <get_speed_tolerance>
percent above the target fan speed (expressed as a percentage)

Returns:
A boolean, True if fan speed is over the high threshold, False if not
"""
speed = self.get_speed()
Comment on lines +69 to +103
Copy link
Collaborator

Choose a reason for hiding this comment

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

@spilkey-cisco This is an abstract class. Where is the HLD changes?

Copy link
Contributor Author

@spilkey-cisco spilkey-cisco Jun 14, 2023

Choose a reason for hiding this comment

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

My understanding is that https://github.com/sonic-net/sonic-platform-common/blob/master/sonic_fan/fan_base.py is the (obsolete, from what I can tell) fan abstract base class, and the file in this changeset follows the design from https://github.com/sonic-net/SONiC/blob/master/doc/platform_api/new_platform_api.md indicating ABCs are no longer used, 'No longer abstract base classes: All abstract methods simply raise "NotImplementedError"'. The new methods are not abstract since they have a default implementation, but can be overridden with vendor specific implementations if desired. If an HLD is needed, please advise on where it should go.

Please correct me if this understanding is inaccurate or incomplete.

target_speed = self.get_target_speed()
tolerance = self.get_speed_tolerance()

for param, value in (('speed', speed), ('target speed', target_speed), ('speed tolerance', tolerance)):
if not isinstance(value, int):
raise TypeError(f'Fan {param} is not an integer value: {param}={value}')
if value < 0 or value > 100:
raise ValueError(f'Fan {param} is not a valid percentage value: {param}={value}')

return speed * 100 > target_speed * (100 + tolerance)

def set_speed(self, speed):
"""
Sets the fan speed
Expand Down
158 changes: 158 additions & 0 deletions tests/fan_base_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
'''
Test FanBase module
'''

from unittest import mock
from sonic_platform_base.fan_base import FanBase

class TestFanBase:
'''
Collection of FanBase test methods
'''

@staticmethod
def test_is_under_speed():
'''
Test fan.is_under_speed default implementation
'''
fan = FanBase()
fan.get_speed = mock.MagicMock(return_value=100)
fan.get_target_speed = mock.MagicMock(return_value=50)
fan.get_speed_tolerance = mock.MagicMock(return_value=10)

for func in (fan.get_speed, fan.get_target_speed, fan.get_speed_tolerance):
return_val = func()

# Check type and bounds errors
for value, exc_type in ((None, TypeError), (-1, ValueError), (101, ValueError)):
func.return_value = value
expected_exception = False
try:
fan.is_under_speed()
except Exception as exc:
expected_exception = isinstance(exc, exc_type)
assert expected_exception

# Reset function return value
func.return_value = return_val

# speed=100, minimum tolerated speed=45, not under speed
assert not fan.is_under_speed()

# speed=46, minimum tolerated speed=45, not under speed
fan.get_speed.return_value = 46
assert not fan.is_under_speed()

# speed=45, minimum tolerated speed=45, not under speed
fan.get_speed.return_value = 45
assert not fan.is_under_speed()

# speed=44, minimum tolerated speed=45, under speed
fan.get_speed.return_value = 44
assert fan.is_under_speed()

# speed=44, minimum tolerated speed=40, not under speed
fan.get_speed_tolerance.return_value = 20
assert not fan.is_under_speed()

# speed=41, minimum tolerated speed=40, not under speed
fan.get_speed.return_value = 41
assert not fan.is_under_speed()

# speed=40, minimum tolerated speed=40, not under speed
fan.get_speed.return_value = 40
assert not fan.is_under_speed()

# speed=39, minimum tolerated speed=40, under speed
fan.get_speed.return_value = 39
assert fan.is_under_speed()

# speed=1, minimum tolerated speed=40, under speed
fan.get_speed.return_value = 1
assert fan.is_under_speed()

@staticmethod
def test_is_over_speed():
'''
test fan.is_over_speed default implementation
'''
fan = FanBase()
fan.get_speed = mock.MagicMock(return_value=1)
fan.get_target_speed = mock.MagicMock(return_value=50)
fan.get_speed_tolerance = mock.MagicMock(return_value=10)

for func in (fan.get_speed, fan.get_target_speed, fan.get_speed_tolerance):
return_val = func()

# Check type and bounds errors
for value, exc_type in ((None, TypeError), (-1, ValueError), (101, ValueError)):
func.return_value = value
expected_exception = False
try:
fan.is_under_speed()
except Exception as exc:
expected_exception = isinstance(exc, exc_type)
assert expected_exception

# Reset function return value
func.return_value = return_val

# speed=1, maximum tolerated speed=55, not over speed
assert not fan.is_over_speed()

# speed=54, maximum tolerated speed=55, not over speed
fan.get_speed.return_value = 54
assert not fan.is_over_speed()

# speed=55, maximum tolerated speed=55, not over speed
fan.get_speed.return_value = 55
assert not fan.is_over_speed()

# speed=56, maximum tolerated speed=55, over speed
fan.get_speed.return_value = 56
assert fan.is_over_speed()

# speed=56, maximum tolerated speed=60, not over speed
fan.get_speed_tolerance.return_value = 20
assert not fan.is_over_speed()

# speed=59, maximum tolerated speed=60, not over speed
fan.get_speed.return_value = 59
assert not fan.is_over_speed()

# speed=60, maximum tolerated speed=60, not over speed
fan.get_speed.return_value = 60
assert not fan.is_over_speed()

# speed=61, maximum tolerated speed=60, over speed
fan.get_speed.return_value = 61
assert fan.is_over_speed()

# speed=100, maximum tolerated speed=60, over speed
fan.get_speed.return_value = 100
assert fan.is_over_speed()

@staticmethod
def test_fan_base():
'''
Verify unimplemented methods
'''
fan = FanBase()
not_implemented_methods = [
(fan.get_direction,),
(fan.get_speed,),
(fan.get_target_speed,),
(fan.get_speed_tolerance,),
(fan.set_speed, 50),
(fan.set_status_led, 'green'),
(fan.get_status_led,)]

for method in not_implemented_methods:
expected_exception = False
try:
func = method[0]
args = method[1:]
func(*args)
except Exception as exc:
expected_exception = isinstance(exc, NotImplementedError)
assert expected_exception
Loading