Avoid loading media_settings.json on mellanox platforms if the host cmis management feature is disabled #8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…en the host CMIS management feature is disabled
|
code change looks good, you may need to add some unit test |
DavidZagury
suggested changes
Jun 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
I added a check to ensure that the media settings JSON is not loaded when the host CMIS management feature is disabled. This prevents unnecessary port-side configurations and potential errors when the feature is not in use.
Motivation and Context
Currently, the xcvrd code always posts the SI parameters from media_settings.json to APP_DB. On Mellanox platforms, this process is required only if the Host CMIS Management feature is enabled. If the feature is disabled when media_settings.json is present on the platform and notify_media_settings() is triggered, it can cause containers to crash or links to flap. This is why I added a check to ensure that on Mellanox platforms, media_settings.json is not loaded if the Host CMIS Management feature is disabled.
How Has This Been Tested?
Manual tests to ensure that containers do not crash when the feature is disabled and media_settings.json exists on the platform.
Additional Information (Optional)