[gNOI] Added initial implementation of gNOI.OS Install RPC.#429
[gNOI] Added initial implementation of gNOI.OS Install RPC.#429kanchanavelusamy wants to merge 1 commit intosonic-net:masterfrom
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
447617b to
84306e0
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
84306e0 to
d9c1b24
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
d9c1b24 to
bc7007c
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
bc7007c to
f8b9a37
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
f8b9a37 to
bee5763
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
bee5763 to
f8e268f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
f8e268f to
e055610
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
e055610 to
14e6dcf
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
14e6dcf to
c238272
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
c238272 to
bcb5a40
Compare
gnmi_server/gnoi_os.go
Outdated
| }, | ||
| } | ||
| // If the file doesn't exist, create it, or append to the file | ||
| f, err := os.OpenFile(imgPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) |
There was a problem hiding this comment.
Does this require us to mount /tmp into the container?
There was a problem hiding this comment.
Does this require us to mount /tmp into the container?
Based on the HLD, my understanding is that the gNMI server, which runs in a container, receives the OS image data in chunks via the os.Install RPC stream. The processTransferContent function then temporarily saves these chunks to a local file within the container.
Once the entire file has been transferred, the container's role is to pass the completed file to the host for the actual installation using the DBUS communication method as described in the HLD. Therefore, the file only needs to be in the container for the duration of this transfer and doesn't require persistent storage.
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
e551f01 to
10ca04f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@kanchanavelusamy Got it. Meanwhile, is the description (test) stale? |
hdwhdw
left a comment
hdwhdw
left a comment
There was a problem hiding this comment.
Also a general improvement suggestion. We need some more logging on the server side as well. In your PR description, can you also share your gnmi server log and make sure it reflect the entire workflow of an install?
| ProcessTransferReady func(string) (string, error) // Function that handles TransferReady request. | ||
| ProcessTransferEnd func(string) (string, error) // Function that handles TransferEnd request. |
There was a problem hiding this comment.
I don't think this is an appropriate use of Config. if we want to inject implementation for testing, we can inject it into the server struct or monkey patch it, but don't call it a config (let alone putting it into the telemetry binary) .
If everybody starts to do this, the config is going to get unmanageable very soon.
There was a problem hiding this comment.
Copying the example I raised in another comment
// Define clear interfaces for dependencies
type OSInstaller interface {
ProcessTransferReady(req string) (string, error)
ProcessTransferEnd(req string) (string, error)
// Could also be more specific:
// ValidateTransfer(*ospb.TransferRequest) error
// StoreImage(version string, content []byte) error
// CompleteInstallation(version string) error
}
type OSServer struct {
installer OSInstaller // Inject via constructor
imgDir string // Configuration stays separate
auth Authenticator
}
// Constructor injection
func NewOSServer(installer OSInstaller, imgDir string, auth Authenticator) *OSServer {
return &OSServer{
installer: installer,
imgDir: imgDir,
auth: auth,
}
}
gnmi_server/gnoi_os.go
Outdated
| log.Infof("Response string from backend= %s", respStr) | ||
| if err != nil { | ||
| // If the error is about unimplemented OS install, return nil to trigger gRPC Unimplemented code | ||
| if strings.Contains(err.Error(), "OS Install not supported") { |
There was a problem hiding this comment.
This is extremely fragile, is there a more robust way to handle this. For example comparing error code?
There was a problem hiding this comment.
This is extremely fragile, is there a more robust way to handle this. For example comparing error code?
Addressed.
gnmi_server/gnoi_os.go
Outdated
| }, | ||
| } | ||
| // If the file doesn't exist, create it, or append to the file | ||
| f, err := os.OpenFile(imgPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) |
There was a problem hiding this comment.
We should validate the path, to prevent potential traversal vulnerability.
There was a problem hiding this comment.
We should validate the path, to prevent potential traversal vulnerability.
Addressed.
| } | ||
| defer sem.Unlock() | ||
| // Receive TransferReq message. | ||
| req, err := stream.Recv() |
There was a problem hiding this comment.
There is an implicit state machine here. If you are from start, you are expecting a TransferRequest, then you are expecting a TransferContent, etc. Can we implement a state machine here? For example, keep track of the current state and the next potential/valid state?
gnmi_server/gnoi_os.go
Outdated
| return &ospb.InstallResponse{ | ||
| Response: &ospb.InstallResponse_TransferProgress{ | ||
| TransferProgress: &ospb.TransferProgress{ | ||
| BytesReceived: uint64(len(transferContent)), |
There was a problem hiding this comment.
This is a bug isn't it? We should return the total content received so far, not the length of the current trunk.
There was a problem hiding this comment.
This is a bug isn't it? We should return the total content received so far, not the length of the current trunk.
Addressed.
| } | ||
| func (srv *OSServer) processTransferEnd(req *ospb.InstallRequest) *ospb.InstallResponse { | ||
| // Front end marshals the request, and sends to the sonic-host-service. | ||
| // Back end is expected to return the response in JSON format. |
There was a problem hiding this comment.
We can probably use some logging in places like this, i.e. whenever an "event" happen like client has signal transfer being completed, there should be some logs so people can debug.
There was a problem hiding this comment.
We can probably use some logging in places like this, i.e. whenever an "event" happen like client has signal transfer being completed, there should be some logs so people can debug.
Addressed.
Sample Logs for Component Test Results
I0829 10:19:35.164142 1453383 gnoi_os.go:274] === [OS Install Start] ===
I0829 10:19:35.164201 1453383 gnoi_os.go:299] Install: Received TransferRequest version=os1.1
I0829 10:19:35.164220 1453383 gnoi_os.go:44] processTransferReq: transfer_request:{version:"os1.1"}
I0829 10:19:35.164346 1453383 gnoi_os.go:71] processTransferReq: Backend response {"transferReady":{}}
I0829 10:19:35.164394 1453383 gnoi_os.go:99] processTransferReq: complete.
I0829 10:19:35.164422 1453383 gnoi_os.go:315] Install: Response received transfer_ready:{}
I0829 10:19:35.164863 1453383 gnoi_os.go:339] Install: Received TransferContent stream
I0829 10:19:35.164899 1453383 gnoi_os.go:148] processTransferContent: file /tmp/os1.1
I0829 10:19:35.165038 1453383 gnoi_os.go:234] processTransferContent: complete. Wrote 54 bytes to /tmp/os1.1
I0829 10:19:35.165063 1453383 gnoi_os.go:387] Install: Response received transfer_progress:{bytes_received:54}
I0829 10:19:35.165389 1453383 gnoi_os.go:339] Install: Received TransferContent stream
I0829 10:19:35.165420 1453383 gnoi_os.go:365] Install: TransferContent is complete.
I0829 10:19:35.165437 1453383 gnoi_os.go:410] Install: Received TransferEnd
I0829 10:19:35.165462 1453383 gnoi_os.go:106] processTransferEnd: transfer_end:{}
I0829 10:19:35.165580 1453383 gnoi_os.go:120] processTransferEnd: Backend response {"validated":{}}
I0829 10:19:35.165630 1453383 gnoi_os.go:143] processTransferEnd: complete.
I0829 10:19:35.165652 1453383 gnoi_os.go:412] Install: Response received validated:{}
I0829 10:19:35.165721 1453383 gnoi_os.go:426] === [OS Install Complete] ===
Hi @hdwhdw |
I've updated the PR description to include the gNMI server log, which reflects the expected "Backend returned unimplemented error" for the Install RPC's current workflow. Server-side logs indicating successful OS installation scenarios: |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
10ca04f to
07dae98
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
07dae98 to
23cb327
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
23cb327 to
3660e51
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
3660e51 to
14c6080
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
14c6080 to
5d139a7
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kanchanavelusamy
force-pushed
the
gnoi_os_fe_pr1
branch
from
5d139a7 to
f1f4dd5
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
4 participants
Client Implementation [https://github.com/kanchanavelusamy/pull/3]
Dependent Backend PR: [https://github.com/sonic-net/sonic-host-services/pull/270]
[UMF] gNOI OS Service - Test Results
Manual CLI Testing
Verify RPC
Activate RPC
Install RPC
gnmi.log for reference.
UT Results:
Why I did it
How I did it
How to verify it
Which release branch to backport (provide reason below if selected)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)