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

[sonic-platform-common][sonic-platform-daemons] Update submodules #5917

Closed
wants to merge 2 commits into from

Conversation

vdahiya12
Copy link
Contributor

@vdahiya12 vdahiya12 commented Nov 12, 2020

This PR updates the following commits in sonic-platform-daemons

d5c586e [xcvrd] Fix the console-script xcvrd startup (#111)
8c2a5cc [Thermalctld] Update thermal info to CHASSIS_STATE_DB (#101)
05c79de PSUd changes to compute power-budget for Modular chassis (#104)
a14c2bb Introduce chassisd to monitor status of cards on chassis (#97)
850d0c6 [xcvrd] support for integrating Credo Y cable Ports initialization and status updates with xcvrd. (#105)

This PR also updates the following commits in sonic-platform-common
0a13c7b Thermalctld APIs for recording min and max temp (#131)
8813b1d Common power consumption and supply APIs for modular chassis (#136)
b37f156 [sonic-platform-base] Introduce APIs for modular chassis support (#124)
9ba8189 Remove shebangs from non-executable Python files (#140)
fc3c1a0 [sonic_eeprom] Make compatible with Python 2 and 3 (#127)
012dc39 Add unit test infrastructure (#139)

Signed-off-by: vaibhav-dahiya vdahiya@microsoft.com

@vdahiya12 vdahiya12 marked this pull request as draft November 12, 2020 23:32
@vdahiya12 vdahiya12 requested a review from jleveque November 12, 2020 23:32
@jleveque
Copy link
Contributor

jleveque commented Nov 13, 2020

@mprabhu-nokia: The check builds are failing. It seems like you're referencing device_base.DeviceBase.STATUS_LED_COLOR_OFF when you should probably just use STATUS_LED_COLOR_OFF, because it's inherited:

https://github.com/Azure/sonic-platform-common/pull/136/files#diff-47e0b2001a379299645eee00d556fc5a6f6615c88a1c418fb894500750b428a9R30

Can you please fix?

Edit: After looking at this with fresh eyes, I believe this is not the root cause, but just something that should be improved. I believe we are also missing commits in sonic-platform-common. @vdahiya12: Can you please also update the sonic-platform-common submodule here and see if this passes the check builds?

d5c586e [xcvrd] Fix the console-script xcvrd startup  (sonic-net#111)
8c2a5cc [Thermalctld] Update thermal info to CHASSIS_STATE_DB (sonic-net#101)
05c79de PSUd changes to compute power-budget for Modular chassis (sonic-net#104)
a14c2bb Introduce chassisd to monitor status of cards on chassis (sonic-net#97)
850d0c6 [xcvrd] support for integrating Credo Y cable Ports initialization and status updates with xcvrd.  (sonic-net#105)

Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
@vdahiya12
Copy link
Contributor Author

@mprabhu-nokia: The check builds are failing. It seems like you're referencing device_base.DeviceBase.STATUS_LED_COLOR_OFF when you should probably just use STATUS_LED_COLOR_OFF, because it's inherited:

https://github.com/Azure/sonic-platform-common/pull/136/files#diff-47e0b2001a379299645eee00d556fc5a6f6615c88a1c418fb894500750b428a9R30

Can you please fix?

Edit: After looking at this with fresh eyes, I believe this is not the root cause, but just something that should be improved. I believe we are also missing commits in sonic-platform-common. @vdahiya12: Can you please also update the sonic-platform-common submodule here and see if this passes the check builds?

Updated the PR with sonic-platform-common submodule update

@jleveque
Copy link
Contributor

@vdahiya12: Thanks. Can you also update the title to [sonic-platform-common][sonic-platform-daemons] Update submodules and update the description to list all the sonic-platform-common commits included?

@vdahiya12 vdahiya12 changed the title [sonic-platform-daemons] update submodule [sonic-platform-common][sonic-platform-daemons] Update submodules Nov 13, 2020
@mprabhu-nokia
Copy link
Contributor

@jleveque The psud tests still fail. The tests have a dependency on swsscommon.Table and swsscommon.FieldValuePairs and is failing there. I thought it picks up the mocked objects. Apparently, it does not.
Should I back out the UT part of the code? Or How can we trigger it to not run the unit-test?

@jleveque
Copy link
Contributor

@mprabhu-nokia: It appears as though you are not importing mock_swsscommon in test_psud.py. Were these unit tests validated? We cannot disable or back out unit tests. You need to fix the tests, or we need to revert your changes, and you will need to add them back again with the proper fixes.

@mprabhu-nokia
Copy link
Contributor

@mprabhu-nokia: It appears as though you are not importing mock_swsscommon in test_psud.py. Were these unit tests validated? We cannot disable or back out unit tests. You need to fix the tests, or we need to revert your changes, and you will need to add them back again with the proper fixes.

Yes, it was unit-tested. Now, I see that existing unit-tests in thermalctld are also failing when manually run. The ordering of when swsscommon is built could be one of reasons.
We could update sonic-platform-daemons to a14c2bb or we can revert sonic-net/sonic-platform-daemons#104. I will try these too and let you know if I make progress.

@jleveque
Copy link
Contributor

jleveque commented Nov 14, 2020

@mprabhu-nokia: You may be correct about the ordering of when swsscommon is built. The xcvrd and thermalctld makefiles should specify swsscommon as a dependency. I can open a PR to handle this.

Edit: I have opened this PR to add the swsscommon dependencies. While I'm not sure this is the cause of the issue here, it is definitely something that was lacking.

@jleveque
Copy link
Contributor

Superseded by #5924. Closing this PR.

@jleveque jleveque closed this Nov 14, 2020
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.

xcvrd keep on crashing with latest master image 484 on 7050cx3 platform
3 participants