[Mellanox] thermal control enhancement for dynamic minimum fan speed and PSU fan speed policy #4403

Junchao-Mellanox · 2020-04-09T02:35:52Z

- What I did

Support dynamic minimum fan speed policy. Before this change, the default fan speed is 60% in Mellanox platform which means that the fan speed will be set to 60% if all fans work well. But 60% might be too high for cold environment which consumes a lot power and makes noise. The idea of dynamic minimum fan speed is to set minimum fan speed dynamically according to thermal zone temperature, air flow direction. The dynamic minimum fan speed policy check air flow direction and thermal zone temperature every 60 seconds and determine a proper minimum fan speed according to a "minimum table". Set fan speed to lower than minimum fan speed is not allowed.
Support PSU fan speed policy. Before this change, the PSU fan speed is a fix value in Mellanox platform. It means that the PSU fan speed won't be adjusted according to the system's thermal zone status. The PSU fan speed policy checks the system cooling level periodically and set PSU fan speed according to it.
Adjust the current "all fan and psu presence" policy. Before this change, when all fan and psu work well, this policy just set fan speed to 60% and enable kernel thermal algorithm. Within this change, the policy will check all thermal zones and make sure all thermal zones return to their normal state before setting fan speed to 60%.
Add a fan fault policy. If any fan in fault state, set other fan speed to 100%.

- How I did it

Add a private policy for dynamic minimum fan speed. The policy contains a new condition to detect that if thermal zone temperature or air flow direction changes. The policy also contains a new action to set the minimum fan speed.
Add a private policy for PSU fan speed. The policy contains a new condition to check that if system cooling level changes. The policy also contains a new action to set PSU fan speed via i2c tool.

- How to verify it

Manual test and regression test. Added two test cases in sonic-net/sonic-mgmt#1552.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

…policy and PSU speed policy

lgtm-com · 2020-04-09T02:52:46Z

This pull request introduces 1 alert when merging 3b77d71 into 768d18d - view on LGTM.com

new alerts:

1 for Unused local variable

Junchao-Mellanox · 2020-04-10T09:50:26Z

retest vsimage please

Junchao-Mellanox · 2020-04-12T06:17:52Z

retest vsimage please

liat-grozovik · 2020-04-13T15:35:01Z

platform/mellanox/mlnx-platform-api/sonic_platform/device_data.py

@@ -0,0 +1,134 @@
+DEVICE_DATA = {
+    'ACS-MSN2700': {


seems like for the different SKU which are different flavour of the same Platform type we need to define the same table over and over. can you please find a better way to do so?
for example: ACS-MSN2700, LS-SN2700, Mellanox-SN2700, Mellanox-SN2700-D48C8, etc.
soon when we will have a dynamic SKU creator we will not be able to support it thus it cannot be based on SKU.
we have the same limitation in sfputil and this is wrong.

liat-grozovik · 2020-04-13T15:35:35Z

platform/mellanox/mlnx-platform-api/sonic_platform/device_data.py

+            }
+        }
+    },
+}


You are missing SN4700 which is also supported.

We don't have minimum table for 4700. We set minimum FAN speed to 60% for 4700 or any other platform who has no minimum table.

liat-grozovik · 2020-04-13T15:36:34Z

platform/mellanox/mlnx-platform-api/sonic_platform/fan.py

@@ -22,17 +23,24 @@

 FAN_PATH = "/var/run/hw-management/thermal/"
 LED_PATH = "/var/run/hw-management/led/"
+CONFIG_PATH = "/var/run/hw-management/config"


i think we have this config path on different files. is this correct to have it this way instead of having it inherited?

I think this is correct. The config path here is a directory and I only need it to find some i2c address related to PSU fan. I don't see difference among differenct platforms.

liat-grozovik · 2020-04-13T15:37:00Z

platform/mellanox/mlnx-platform-api/sonic_platform/fan.py

 # fan_dir isn't supported on Spectrum 1. It is supported on Spectrum 2 and later switches
 FAN_DIR = "/var/run/hw-management/system/fan_dir"
+COOLING_STATE_PATH = "/var/run/hw-management/thermal/cooling_cur_state"

 # SKUs with unplugable FANs:
 # 1. don't have fanX_status and should be treated as always present
 hwsku_dict_with_unplugable_fan = ['ACS-MSN2010', 'ACS-MSN2100']


same here, should not be SKU but rather a system type

liat-grozovik · 2020-04-13T15:37:56Z

platform/mellanox/mlnx-platform-api/sonic_platform/fan.py

+        if not isinstance(level, int):
+            raise RuntimeError("Failed to set cooling level, input parameter must be integer")
+
+        if level < 1 or level > 10:


suggest to have min, max defines and use it for both if case and prints.

liat-grozovik · 2020-04-13T15:41:35Z

platform/mellanox/mlnx-platform-api/sonic_platform/thermal.py

+THERMAL_ZONE_TEMPERATURE = "thermal_zone_temp"
+THERMAL_ZONE_NORMAL_TEMPERATURE = "temp_trip_norm"
+
+MODULE_COUNTER_PATH = "/var/run/hw-management/config/module_counter"


Are you sure you are using it?

liat-grozovik · 2020-04-13T15:41:40Z

platform/mellanox/mlnx-platform-api/sonic_platform/thermal.py

+THERMAL_ZONE_NORMAL_TEMPERATURE = "temp_trip_norm"
+
+MODULE_COUNTER_PATH = "/var/run/hw-management/config/module_counter"
+GEARBOX_COUNTER_PATH = "/var/run/hw-management/config/gearbox_counter"


Are you sure you are using it?

liat-grozovik · 2020-04-13T15:42:21Z

platform/mellanox/mlnx-platform-api/sonic_platform/thermal.py

+
+
+    @classmethod
+    def _write_generic_file(cls, filename, content):


what the purpose of the generic file? can you please add more description for it?

Updated description.

liat-grozovik · 2020-04-13T15:43:43Z

platform/mellanox/mlnx-platform-api/sonic_platform/thermal.py

+            logger.log_info("Fail to write file {} due to {}".format(filename, repr(e)))
+
+    @classmethod
+    def set_thermal_algorithm_status(cls, status, force=True):


what is the behaviour expected when disabled?

When disable the thermal algorithm, kernel no longer adjust the FAN speed according to thermal zone temperature. We usually disable the thermal algorithm when FAN absence or PSU absence. If a fan unit or PSU is removed from switch, there will be a "air hole" at switch, we need set fan speed to 100% and prevent thermal algorithm adjust it.

liat-grozovik · 2020-04-13T15:50:50Z

platform/mellanox/mlnx-platform-api/tests/test_thermal_policy.py

@@ -159,6 +185,44 @@ def test_all_fan_presence_condition():
    fan_info.collect(chassis)
    assert condition.is_match({'fan_info': fan_info})

+def test_any_fan_fault_condition():


should you consider to have a test file for the tests instead of the general code used for production? not must but i think it is better

…level to dynamic minimum value

jleveque · 2020-04-14T20:44:59Z

Please fix conflicts

abdosi · 2020-05-28T00:57:07Z

@Junchao-Mellanox Please create PR for 201911. There is merge conflict.
Removing Request for 201911 Label

@rlhui @liat-grozovik

…and PSU fan speed policy (sonic-net#4403) Conflicts: platform/mellanox/mlnx-platform-api/sonic_platform/fan.py

[Mellanox] thermal control enhancement for dynamic minimum FAN speed …

3b77d71

…policy and PSU speed policy

Remove unused variable

6ab058a

Enable thermal algorithm when thermal control daemon exist

5a75eae

liat-grozovik requested changes Apr 13, 2020

View reviewed changes

1. fix review comment by liat; 2. add a new policy to update cooling …

981bfb5

…level to dynamic minimum value

Junchao-Mellanox and others added 3 commits April 15, 2020 08:57

fix unit test issues

df85565

Strip content read from file

4398275

Merge branch 'master' into thermal-enhance

3a4f559

Junchao-Mellanox requested a review from liat-grozovik April 17, 2020 01:28

liat-grozovik approved these changes Apr 21, 2020

View reviewed changes

jleveque approved these changes Apr 21, 2020

View reviewed changes

jleveque merged commit c730f3e into sonic-net:master Apr 21, 2020

Junchao-Mellanox deleted the thermal-enhance branch May 7, 2020 06:10

liat-grozovik added the Request for 201911 Branch label May 21, 2020

abdosi removed the Request for 201911 Branch label May 28, 2020

Junchao-Mellanox mentioned this pull request Jun 4, 2020

[201911][thermal control] Backport feature from master branch #4677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mellanox] thermal control enhancement for dynamic minimum fan speed and PSU fan speed policy #4403

[Mellanox] thermal control enhancement for dynamic minimum fan speed and PSU fan speed policy #4403

Junchao-Mellanox commented Apr 9, 2020 •

edited

Loading

lgtm-com bot commented Apr 9, 2020

Junchao-Mellanox commented Apr 10, 2020

Junchao-Mellanox commented Apr 12, 2020

liat-grozovik Apr 13, 2020

liat-grozovik Apr 13, 2020

Junchao-Mellanox Apr 14, 2020 •

edited

Loading

liat-grozovik Apr 13, 2020

Junchao-Mellanox Apr 14, 2020

liat-grozovik Apr 13, 2020

liat-grozovik Apr 13, 2020

liat-grozovik Apr 13, 2020

liat-grozovik Apr 13, 2020

Junchao-Mellanox Apr 14, 2020

liat-grozovik Apr 13, 2020

Junchao-Mellanox Apr 14, 2020

liat-grozovik Apr 13, 2020

Junchao-Mellanox Apr 14, 2020

liat-grozovik Apr 13, 2020

jleveque commented Apr 14, 2020

abdosi commented May 28, 2020



		@classmethod
		def _write_generic_file(cls, filename, content):

[Mellanox] thermal control enhancement for dynamic minimum fan speed and PSU fan speed policy #4403

[Mellanox] thermal control enhancement for dynamic minimum fan speed and PSU fan speed policy #4403

Conversation

Junchao-Mellanox commented Apr 9, 2020 • edited Loading

lgtm-com bot commented Apr 9, 2020

Junchao-Mellanox commented Apr 10, 2020

Junchao-Mellanox commented Apr 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Junchao-Mellanox Apr 14, 2020 • edited Loading

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

jleveque commented Apr 14, 2020

abdosi commented May 28, 2020

Junchao-Mellanox commented Apr 9, 2020 •

edited

Loading

Junchao-Mellanox Apr 14, 2020 •

edited

Loading