sonic-host-services changes for gNOI Cold Reboot #181
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@hdwhdw @vvolam @qiluo-msft Pls review |
rkavitha-hcl
force-pushed
the
gnoi_cr_host_services
branch
from
efb38de to
0a7c80e
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
rkavitha-hcl
force-pushed
the
gnoi_cr_host_services
branch
from
0a7c80e to
41233ee
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
host_modules/gnoi_reboot.py
Outdated
| return 1, "Delayed reboot is not supported" | ||
| return 0, "" | ||
|
|
||
| def execute_reboot(self, rebootmethod): |
There was a problem hiding this comment.
https://github.com/sonic-net/sonic-host-services/blob/41233eed49a3325429625a403a3b7fb8b0c6b1dc/host_modules/systemd_service.py#L59C44-L60C9 - This older execute_reboot needs to be removed and all those methods needs to be merged here I think.
rkavitha-hcl
force-pushed
the
gnoi_cr_host_services
branch
from
41233ee to
98b00cd
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@hdwhdw The code has been updated to rename all gnoi_reboot to reboot, PTAL. |
|
Pls look at unit test results at sonic-net/sonic-buildimage#20713 (comment) |
There was a problem hiding this comment.
Can we move this into tests/host_modules?
Also suggest following the format of these tests
| valid_method = False | ||
| for values in [REBOOTMETHOD_COLD_BOOT_VALUES, REBOOTMETHOD_NSF_VALUES]: | ||
| if rebootmethod in values: | ||
| valid_method = True | ||
| if not valid_method: | ||
| return 1, "Invalid reboot method: " + str(rebootmethod) |
There was a problem hiding this comment.
Nits: Suggest simplifying:
valid_reboot_method=REBOOTMETHOD_COLD_BOOT_VALUES | REBOOTMETHOD_NSF_VALUES
if not reboot_method in VALID_REBOOT_METHOD:
return errno.EINVAL, ....
| return 1, "Reboot request must contain a reboot method" | ||
|
|
||
| # Check whether reboot method is valid. | ||
| rebootmethod = reboot_request["method"] |
There was a problem hiding this comment.
Nits: be consistent in variable naming: reboot_method. Here and everywhere else.
| self.lock.release() | ||
| # Return without issuing the reboot if the previous reboot is ongoing | ||
| if is_reboot_ongoing: | ||
| return 1, "Previous reboot is ongoing" |
There was a problem hiding this comment.
Nits: error code can be useful in determining what to return in gNOI response. Potentially useful errno can be:
EINVAL (invalid argument), EBUSY (reboot ongoing), EPERM (not permitted). ENOENT ((value) not found)
Python Error also carries a error code, which can be used to return.
This is up to you if it can make it easier to figure out the context in the gnmi/framework code.
| def register(): | ||
| """Return the class name""" | ||
| return Reboot, MOD_NAME |
| assert result[1] == "Failed to start thread to execute reboot with error: test raise RuntimeError exception" | ||
|
|
||
| def test_get_reboot_status_active(self): | ||
| self.reboot_module.populate_reboot_status_flag(True, 1617811205, "testing reboot response") |
There was a problem hiding this comment.
1617811205, we should make this a constant to make it easier to update.
| self.reboot_module.populate_reboot_status_flag(True, 1617811205, "testing reboot response") | ||
| result = self.reboot_module.get_reboot_status() | ||
| assert result[0] == 0 | ||
| assert result[1] == TEST_ACTIVE_RESPONSE_DATA |
There was a problem hiding this comment.
TEST_ACTIVE_RESPONSE_DATA is only used once, putting it inside a constant at the top of the file make it hard for readers to associate input with output. We should use a raw string here. Suggest:
TIME=123456
MSG="testing reboot_response"
self.reboot_module.populate_reboot_status_flag(True, TIME, MSG)
....
assert result[0] == TIME, "Wrong time."
....
| self.reboot_module.populate_reboot_status_flag(False, 0, "") | ||
| result = self.reboot_module.get_reboot_status() | ||
| assert result[0] == 0 | ||
| assert result[1] == TEST_INACTIVE_RESPONSE_DATA |
There was a problem hiding this comment.
Same as above. We should not use a constant here.
| REBOOTMETHOD_NSF_ENUM = 5 | ||
|
|
||
| TEST_TIMESTAMP = 1618942253.831912040 | ||
| REPORT_CRITICAL_STATE_FULL_COMMAND = "redis-cli -n 6 HSET COMPONENT_STATE_TABLE|host state ERROR reason \"cold reboot has failed\" essential true timestamp \"2021-04-20 18:10:53\" timestamp-seconds 1618942253 timestamp-nanoseconds 831912040" |
No description provided.