GNOI Implementation of OS.Activate #351

hdwhdw · 2025-01-31T00:39:58Z

Why I did it

Support OS.Activate for GNOI.

How I did it

Implement OS.Activate.

How to verify it

On physical switch:

admin@switch:~$ sudo sonic-installer list
Current: SONiC-OS-20240510.24
Next: SONiC-OS-master.762661-a3792f837
Available:
SONiC-OS-master.762661-a3792f837
SONiC-OS-20240510.24
admin@switch:~$ docker exec gnmi gnoi_client -target 127.0.0.1:50052 -logtostderr -insecure -module OS -rpc Activate -jsonin  '{"version": "SONiC-OS-20240510.24"}'
OS Activate
{"Response":{"ActivateOk":{}}}
admin@switch:~$ sudo sonic-installer list
Current: SONiC-OS-20240510.24
Next: SONiC-OS-20240510.24
Available:
SONiC-OS-master.762661-a3792f837
SONiC-OS-20240510.24
admin@switch:~$ docker exec gnmi gnoi_client -target 127.0.0.1:50052 -logtostderr -insecure -module OS -rpc Activate -jsonin  '{"version": "Foo"}'
OS Activate
{"Response":{"ActivateError":{"type":1,"detail":"Error: Image does not exist\n"}}}

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)

mssonicbld · 2025-01-31T00:40:00Z

/azp run

linux-foundation-easycla · 2025-01-31T00:40:02Z

The committers listed above are authorized under a signed CLA.

✅ login: hdwhdw / name: Dawei Huang (d841c0c, ed49269, 88783ef, eb39685, f869de2, 0c02ca1, 006fbbe, ff16bf3, ecbaab2, 7b5c71f, bfa7c3d, 2d4b009)

azure-pipelines · 2025-01-31T00:40:10Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-01-31T00:41:59Z

/azp run

azure-pipelines · 2025-01-31T00:42:09Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-01-31T02:49:51Z

/azp run

azure-pipelines · 2025-01-31T02:50:00Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-01-31T04:26:54Z

/azp run

azure-pipelines · 2025-01-31T04:27:03Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-01-31T14:56:47Z

/azp run

azure-pipelines · 2025-01-31T14:56:57Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-01-31T16:56:11Z

/azp run

azure-pipelines · 2025-01-31T16:56:21Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-03T20:39:47Z

/azp run

azure-pipelines · 2025-02-03T20:39:57Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-03T20:48:57Z

/azp run

azure-pipelines · 2025-02-03T20:49:06Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (2)

sonic_service_client/dbus_client_test.go: Evaluated as low risk
common_utils/context.go: Evaluated as low risk

Comments suppressed due to low confidence (3)

gnoi_client/os/os.go:35

[nitpick] The error message could be more descriptive. Consider changing it to 'Failed to unmarshal JSON: ' + err.Error()'.

panic(err.Error())

gnoi_client/os/os.go:39

[nitpick] The error message could be more descriptive. Consider changing it to 'Failed to activate OS: ' + err.Error()'.

panic(err.Error())

gnoi_client/os/os.go:43

[nitpick] The error message could be more descriptive. Consider changing it to 'Failed to marshal response: ' + err.Error()'.

panic(err.Error())

mssonicbld · 2025-02-03T20:59:09Z

/azp run

azure-pipelines · 2025-02-03T20:59:19Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.

Files not reviewed (3)

gnoi_client/os/verify.go: Evaluated as low risk
common_utils/context.go: Evaluated as low risk
gnoi_client/gnoi_client.go: Evaluated as low risk

xincunli-sonic · 2025-02-03T23:12:23Z

gnmi_server/gnoi.go

+	image := req.GetVersion()
+	log.Infof("Requested to activate image %s", image)
+
+	dbus, err := ssc.NewDbusClient()


dbus

Please call close after the client created?

defer dbus.Close() #Closed

xincunli-sonic · 2025-02-03T23:13:49Z

gnmi_server/gnoi.go

+	}
+
+	var resp gnoi_os_pb.ActivateResponse
+	err = dbus.ActivateImage(image)


ActivateImage

The logic checking for "not" and "exist" in err.Error() is fragile. It relies on string matching rather than structured error handling, which may break if error messages change.

Suggestion: If dbus.ActivateImage(image) provides structured error types, check against those instead of relying on string searches.

I did try to do this previously but sonic-installer set-next-boot does not provide proper error code (it just return 1 instead, arguably it should return 2 (ENOENT)). The error code is propagated through DBUS and then to GNMI so I have to rely on the message.

I will add a check here and correct this in both host-services and sonic-installer script.

I understand the sonic-installer set-next-boot command line just returned string, what I meant is actual place where calls command line then it did wrap over there.

admin@vslab1:~$ sudo sonic-installer set-next-boot foo Error: Image does not exist

Doing it in sonic-net/sonic-host-services#210, add a check for errno here. We can still keep the message check in an "either-or" condition just to be doubly safe.

xincunli-sonic · 2025-02-03T23:14:51Z

gnmi_server/gnoi.go

+				},
+			}
+		}
+		return &resp, nil


nil

Why would not we return actual error?

I don't think this is a server error. See https://github.com/openconfig/gnoi/blob/main/os/os.proto#L372, this is an application level error instead of server error.

But this is server side handling? And since the err is not nil, you did wrap the response, how would the client of this server handle the error, only relying on response?

Okay, I saw the gnoi_client just handles response. Do you think is there any other caller?

This function is a grpc internal server implementation, so the only caller is grpc server. If we return error, that means the grpc handler cannot proceed because we encountered an internal error. "Image not found" is not that and based on the server definition in openconfig, we should report that directly to the client, instead of making it a server failure. Whatever clients want to do with the response error is up to the client.

mssonicbld · 2025-02-04T01:24:48Z

/azp run

azure-pipelines · 2025-02-04T01:24:58Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-04T01:27:58Z

/azp run

azure-pipelines · 2025-02-04T01:28:08Z

Azure Pipelines successfully started running 1 pipeline(s).

qiluo-msft · 2025-02-04T21:11:37Z

gnoi_client/gnoi_client.go

@@ -56,6 +56,8 @@ func main() {
 		switch *config.Rpc {
 		case "Verify":
 			gnoi_os.Verify(conn, ctx)
+		case "Activate":


Activate

wondering whether upstream repo already has a feature rich gnoi_client?

Yes in https://google.github.io/gnxi/gnoi_os/.

ganglyu · 2025-02-05T00:48:42Z

gnmi_server/gnoi.go

@@ -161,6 +161,56 @@ func (srv *OSServer) Verify(ctx context.Context, req *gnoi_os_pb.VerifyRequest)
 	return resp, nil
 }

+func (srv *OSServer) Activate(ctx context.Context, req *gnoi_os_pb.ActivateRequest) (*gnoi_os_pb.ActivateResponse, error) {
+	_, err := authenticate(srv.config, ctx, false)


Need RW permission

mssonicbld · 2025-02-05T16:33:07Z

/azp run

azure-pipelines · 2025-02-05T16:33:18Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-07T22:12:56Z

/azp run

azure-pipelines · 2025-02-07T22:13:06Z

Azure Pipelines successfully started running 1 pipeline(s).

hdwhdw force-pushed the os_activate branch from d68fc29 to 015f89b Compare January 31, 2025 00:41

hdwhdw force-pushed the os_activate branch from 11d2211 to 21396b9 Compare February 3, 2025 20:48

hdwhdw requested a review from Copilot February 3, 2025 20:49

Copilot AI reviewed Feb 3, 2025

View reviewed changes

hdwhdw requested a review from Copilot February 3, 2025 20:59

Copilot AI reviewed Feb 3, 2025

View reviewed changes

hdwhdw marked this pull request as ready for review February 3, 2025 21:35

hdwhdw requested review from qiluo-msft, ganglyu, FengPan-Frank and xincunli-sonic February 3, 2025 21:35

xincunli-sonic reviewed Feb 3, 2025

View reviewed changes

qiluo-msft reviewed Feb 4, 2025

View reviewed changes

ganglyu reviewed Feb 5, 2025

View reviewed changes

hdwhdw added 12 commits February 7, 2025 22:12

common_utils/context.go

f869de2

sonic_service_client

eb39685

gnmi_server code.

ff16bf3

success case unit test.

d841c0c

nonexistent test.

88783ef

fix typo.

0c02ca1

gnoi_client.

bfa7c3d

gnoi_client

ed49269

bugfix after test.

7b5c71f

address copilot response.

006fbbe

address comment.

ecbaab2

address comment.

2d4b009

hdwhdw force-pushed the os_activate branch from 675650e to 2d4b009 Compare February 7, 2025 22:12

GNOI Implementation of OS.Activate #351

Are you sure you want to change the base?

GNOI Implementation of OS.Activate #351

Conversation

hdwhdw commented Jan 31, 2025 • edited Loading

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)

mssonicbld commented Jan 31, 2025

linux-foundation-easycla bot commented Jan 31, 2025 • edited Loading

azure-pipelines bot commented Jan 31, 2025

mssonicbld commented Jan 31, 2025

azure-pipelines bot commented Jan 31, 2025

mssonicbld commented Jan 31, 2025

azure-pipelines bot commented Jan 31, 2025

mssonicbld commented Jan 31, 2025

azure-pipelines bot commented Jan 31, 2025

mssonicbld commented Jan 31, 2025

azure-pipelines bot commented Jan 31, 2025

mssonicbld commented Jan 31, 2025

azure-pipelines bot commented Jan 31, 2025

mssonicbld commented Feb 3, 2025

azure-pipelines bot commented Feb 3, 2025

mssonicbld commented Feb 3, 2025

azure-pipelines bot commented Feb 3, 2025

Choose a reason for hiding this comment

mssonicbld commented Feb 3, 2025

azure-pipelines bot commented Feb 3, 2025

Choose a reason for hiding this comment

xincunli-sonic Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xincunli-sonic Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssonicbld commented Feb 4, 2025

azure-pipelines bot commented Feb 4, 2025

mssonicbld commented Feb 4, 2025

azure-pipelines bot commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssonicbld commented Feb 5, 2025

azure-pipelines bot commented Feb 5, 2025

mssonicbld commented Feb 7, 2025

azure-pipelines bot commented Feb 7, 2025

hdwhdw commented Jan 31, 2025 •

edited

Loading

linux-foundation-easycla bot commented Jan 31, 2025 •

edited

Loading

xincunli-sonic Feb 3, 2025 •

edited

Loading

xincunli-sonic Feb 4, 2025 •

edited

Loading