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

[system-health] Add fan direction check for system health #14509

Merged

Conversation

Junchao-Mellanox
Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox commented Apr 4, 2023

Why I did it

Add fan direction check to system health, all fans should be in the same direction

How I did it

Add fan direction check to system health, all fans should be in the same direction

How to verify it

Manual test
Unit test
Added sonic-mgmt test case to verify

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

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

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik liat-grozovik requested a review from prgeor April 13, 2023 11:30
keboliu
keboliu previously approved these changes Apr 14, 2023
@Junchao-Mellanox
Copy link
Collaborator Author

Hi @qiluo-msft , @prgeor , could you please help review this?

},
'FAN_INFO|fan2': {
'presence': 'False',
'status': 'True',
'speed': '60',
'speed_target': '60',
'speed_tolerance': '20'
'speed_tolerance': '20',

Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this extra line is needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@liat-grozovik
Copy link
Collaborator

@prgeor your comments are very welcome. can you please review?

@@ -82,6 +82,7 @@ def _check_fan_status(self, config):
self.set_object_not_ok('Fan', 'Fan', 'Failed to get fan information')
return

Copy link
Contributor

Choose a reason for hiding this comment

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

@Junchao-Mellanox please update the description of the _check_fan_status() to include fan direction check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

if not expect_fan_direction:
# initialize the expect fan direction
expect_fan_direction = (name, direction)
elif direction != expect_fan_direction[1]:
Copy link
Contributor

Choose a reason for hiding this comment

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

@Junchao-Mellanox I don't understand why expect_fan_direction depends upon the direction value against which it is checking?

Copy link
Collaborator Author

@Junchao-Mellanox Junchao-Mellanox May 4, 2023

Choose a reason for hiding this comment

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

Hi, it is depending on the first fan's direction. There is no platform API to get expected fan direction, so we try to get the first fan direction and set it as the expected value.

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Collaborator Author

Hi @StormLiangMS , could you please help cherry-pick to 202211?

@Junchao-Mellanox Junchao-Mellanox deleted the check-fan-dir-system-health branch May 18, 2023 07:01
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Jun 10, 2023
…14509)

- Why I did it
Add fan direction check to system health, all fans should be in the same direction

- How I did it
Add fan direction check to system health, all fans should be in the same direction

- How to verify it
Manual test
Unit test
Added sonic-mgmt test case to verify
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202211: #15415

mssonicbld pushed a commit that referenced this pull request Jun 10, 2023
- Why I did it
Add fan direction check to system health, all fans should be in the same direction

- How I did it
Add fan direction check to system health, all fans should be in the same direction

- How to verify it
Manual test
Unit test
Added sonic-mgmt test case to verify
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants