Add gNOI OS Management services #270
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
5e68f04 to
a091f31
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
a091f31 to
0f5e957
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
0f5e957 to
c109864
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
c109864 to
7bd5be8
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
7bd5be8 to
915f4d3
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
915f4d3 to
10ae78a
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
10ae78a to
a17ed57
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
a17ed57 to
1e4522c
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
1e4522c to
15f8374
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
15f8374 to
bdf0afc
Compare
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
0322c6b to
62974d3
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
62974d3 to
fbd2b3a
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
fbd2b3a to
8b08715
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
8b08715 to
402190b
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
402190b to
8ce4cce
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
8ce4cce to
86e874c
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
86e874c to
82eecb8
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Kanchana-HCLTech
force-pushed
the
gnoi_os_service_be
branch
from
82eecb8 to
7f1829f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
Adds gNOI OS management services integration, including service registration, protobuf definitions, and an infrastructure module for command execution.
- Register new
gnoi_os_mgmtandinfra_hosthandlers in the D-Bus server script - Introduce shared protobuf constants in
os_mgmt/gnoi_os_proto_defs.py - Implement
InfraHostmodule for executing shell commands and reporting critical state
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/mocks/host_service.py | Add mock HostModule and Logger for unit tests |
| scripts/sonic-host-server | Register new gNOI OS management and infra host modules |
| os_mgmt/gnoi_os_proto_defs.py | Define shared gNOI OS protobuf field constants |
| host_modules/infra_host.py | Implement InfraHost with command execution and state reporting |
| timestamp_in_seconds = current_time.timestamp() | ||
| tv_sec = int(timestamp_in_seconds) | ||
| tv_nsec = int((timestamp_in_seconds - tv_sec)*10**9) | ||
| result = "%s timestamp \"%s\" timestamp-seconds %s timestamp-nanoseconds %s" % ( |
There was a problem hiding this comment.
REPORT_CRITICAL_STATE_COMMAND has no trailing space, so concatenating it directly with 'timestamp' produces '...essential truetimestamp'. Add a space after the base command or include it in the format string.
Copilot uses AI. Check for mistakes.
| REPORT_CRITICAL_STATE_COMMAND, timestamp_string, str(tv_sec), str(tv_nsec)) | ||
| return result | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
This method is marked as @staticmethod but still takes 'self'. Either remove the @staticmethod decorator or refactor to use an instance method signature.
| @staticmethod | |
Copilot uses AI. Check for mistakes.
| @staticmethod | ||
| def raise_critical_state(self): | ||
| """Raise critical state when reboot fails""" | ||
| rc, stdout, stderr = self._run_command(self.generate_critical_state_command(self)) |
There was a problem hiding this comment.
Calling generate_critical_state_command(self) passes 'self' twice. It should be invoked as self.generate_critical_state_command().
Copilot uses AI. Check for mistakes.
| else: | ||
| logger.error("%s: !Encountered while processing empty param", | ||
| MOD_NAME) | ||
| return (1, None) |
There was a problem hiding this comment.
The method signature declares an int and string ('is') return, but returns None for the string. Consider returning an empty string instead of None to match the expected type.
| return (1, None) | |
| return (1, "") |
Copilot uses AI. Check for mistakes.
|
Can you see if you can take advantage of https://github.com/sonic-net/sonic-host-services/blob/master/host_modules/image_service.py? We should avoid writing different modules for the same service. Thanks! |
There was a problem hiding this comment.
Please take a look at existing services to see if you can reuse. We should avoid reinventing the wheel. If the implementation here is much more robust feel free to propose substituting the existing service (and also in sonic-gnmi), but we should avoid having two services doing the same thing.
There was a problem hiding this comment.
Can you share why this is needed? Ideally the host services should not need to know the exact definition of the gnoi service.
|
|
||
| import host_service | ||
| import infra_host | ||
| import os_mgmt.gnoi_os_proto_defs as ospb |
There was a problem hiding this comment.
We should try to create a separate, stable and well-defined API for sonic-host-service that can be used regardless whether you are from gnoi, i.e. use what you write here to implement gnoi, instead of directly implement gnoi here. This avoid churns when the upstream gnoi definition changes.
No description provided.