Support new gnmi config interface in telemetry container. #7

ganglyu · 2022-08-10T08:03:38Z

Signed-off-by: Gang Lv ganglv@microsoft.com

Why I did it

Update telemetry container to support new gnmi config interface.

How I did it

Use new target to support mixed schema, and we can enable/disable telemetry and gnmi config interface.

How to verify it

Build target and run unit test.

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)

linux-foundation-easycla · 2022-09-24T05:08:05Z

The committers listed above are authorized under a signed CLA.

✅ login: ganglyu / name: ganglv (e29e651, fa03135, 1abf2f9, af2fd6c, 5f8b9f0, 0ed45ac, 474b344)

qiluo-msft · 2022-09-26T18:26:31Z

azure-pipelines.yml

@@ -40,7 +40,7 @@ stages:
      DIFF_COVER_WORKING_DIRECTORY: $(System.DefaultWorkingDirectory)/sonic-gnmi

    container:
-      image: sonicdev-microsoft.azurecr.io:443/sonic-slave-buster:latest
+      image: sonicdev-microsoft.azurecr.io:443/sonic-slave-bullseye:latest


bullseye

image: sonicdev-microsoft.azurecr.io:443/sonic-slave-bullseye:latest

Let's separate irrelevant changes such as "bullseye" to another PR and focus "native mode gnmi service" in this PR.

If you mix, it's challenge to revert anything in case. #Closed

qiluo-msft · 2022-09-26T18:27:26Z

go.mod

@@ -5,10 +5,11 @@ go 1.12
 require (
 	github.com/Azure/sonic-mgmt-common v0.0.0-00010101000000-000000000000
 	github.com/Workiva/go-datastructures v1.0.50
-	github.com/agiledragon/gomonkey/v2 v2.8.0
+	github.com/agiledragon/gomonkey/v2 v2.9.0


gomonkey

github.com/agiledragon/gomonkey/v2 v2.9.0

Upgrade existing dep version in another PR.
You can keep newly introduced dep in this PR. #Closed

qiluo-msft · 2022-09-26T18:30:00Z

Makefile

@@ -18,7 +18,15 @@ TEST_FILES=$(wildcard *_test.go)
 TELEMETRY_TEST_DIR = build/tests/gnmi_server
 TELEMETRY_TEST_BIN = $(TELEMETRY_TEST_DIR)/server.test
 ifeq ($(ENABLE_TRANSLIB_WRITE),y)
-BLD_FLAGS := -tags gnmi_translib_write
+  ifeq ($(ENABLE_NATIVE_WRITE),y)


ENABLE_NATIVE_WRITE

ifeq ($(ENABLE_NATIVE_WRITE),y)

You may check each ENABLE_XXX flag, and append to string, and finally set BLD_FLAGS by the string. Current nested if-else is not scalable for future extra flags. #Closed

qiluo-msft · 2022-09-26T18:53:21Z

Makefile

@@ -41,38 +49,48 @@ $(GO_DEPS): go.mod $(PATCHES)
 	patch -d vendor -p0 < patches/gnmi_cli.all.patch
 	patch -d vendor -p0 < patches/gnmi_set.patch
 	patch -d vendor -p0 < patches/gnmi_get.patch
+	patch -d vendor -p0 < patches/path.patch


path.patch

patch -d vendor -p0 < patches/path.patch

The patch filename is too general. #Closed

qiluo-msft · 2022-09-26T18:56:19Z

Makefile

 endif

+# TODO: Create a new repo for this lib, sonic-restapi and sonic-gnmi can share this lib


TODO

# TODO: Create a new repo for this lib, sonic-restapi and sonic-gnmi can share this lib

Instead of todo, let's do it now? #Closed

qiluo-msft · 2022-09-26T19:05:00Z

gnmi_server/gnoi.go

 	"google.golang.org/grpc/status"
 	"google.golang.org/grpc/codes"
 	"os/user"
 	"encoding/json"
 	jwt "github.com/dgrijalva/jwt-go"
 )

+func RebootSystem(fileName string) error {


RebootSystem

Is it "reboot" or "config reload"? #Closed

Use reboot to implement "config reload", and please refer to: https://github.com/openconfig/gnoi/blob/main/system/system.proto#L131

qiluo-msft · 2022-09-26T19:05:23Z

gnmi_server/gnoi.go

 func (srv *Server) Reboot(ctx context.Context, req *gnoi_system_pb.RebootRequest) (*gnoi_system_pb.RebootResponse, error) {
+	fileName := common_utils.GNMI_WORK_PATH + "/config_db.json.tmp"
+	//return nil, status.Errorf(codes.Unimplemented, "hit test point")


//return nil, status.Errorf(codes.Unimplemented, "hit test point")

//return nil, status.Errorf(codes.Unimplemented, "hit test point")

Remove test code, not just commented. #Closed

qiluo-msft · 2022-09-26T21:17:00Z

gnmi_server/server.go

@@ -274,26 +284,32 @@ func (s *Server) checkEncodingAndModel(encoding gnmipb.Encoding, models []*gnmip

 // Get implements the Get RPC in gNMI spec.
 func (s *Server) Get(ctx context.Context, req *gnmipb.GetRequest) (*gnmipb.GetResponse, error) {
+	common_utils.IncCounter("GNMI get")


IncCounter

common_utils.IncCounter("GNMI get")

The counter and dump are quite useful new features, not directly tie to the native mode gnmi service feature. Could you separate them into separate PR(s)? #Closed

qiluo-msft · 2022-09-26T21:25:18Z

func (srv *Server) CancelReboot(ctx context.Context, req *gnoi_system_pb.CancelRebootRequest) (*gnoi_system_pb.CancelRebootResponse, error) {

Is it a placeholder function (with TODO comment)? or how did you cancel a reboot?

In reply to: 1258650635

Refers to: gnmi_server/gnoi.go:66 in 71e5e09. [](commit_id = 71e5e09, deletion_comment = False)

qiluo-msft · 2022-09-26T21:29:12Z

patches/path.patch

@@ -0,0 +1,37 @@
+--- ./github.com/jipanyang/gnxi/utils/xpath/path.go


---

Could you add some file level comment to explain why a patch is needed, and how do you achieve it?
ref: https://stackoverflow.com/a/20911434/2514803

qiluo-msft · 2022-09-26T21:29:52Z

sonic_data_client/events_client.go

@@ -275,7 +275,7 @@ func get_events(evtc *EventClient) {
    for {

        rc, evt := C_recv_evt(evtc.subs_handle)
-        log.V(7).Infof("C.event_receive_wrap rc=%d evt:%s", rc, (*C.char)(str_ptr))
+        log.V(7).Infof("C.event_receive_wrap rc=%d evt:%v", rc, (*C.char)(str_ptr))


v

For valid bug fix, separate into a separate PR.

qiluo-msft · 2022-09-26T21:31:53Z

sonic_data_client/transl_data_client.go

@@ -204,7 +204,7 @@ func (c *TranslClient) StreamRun(q *queue.PriorityQueue, stop chan struct{}, w *
 				return
 			}
 		default:
-			log.V(1).Infof("Bad Subscription Mode for client %s ", c)


s

The same

linux-foundation-easycla · 2022-11-14T03:28:22Z

The committers listed above are authorized under a signed CLA.

✅ login: ganglyu / name: ganglv (e103b09, 2cd6dfa, f019892)

qiluo-msft · 2022-11-21T05:45:55Z

libcswsscommon/include/capi/dbconnector.h

+typedef void *db_connector_t;
+
+// DBConnector::DBConnector(int db, std::string hostname, int port, unsigned int timeout)
+db_connector_t db_connector_new(int db, const char *hostname, int port, unsigned int timeout);


db_connector_new

Did you try SWIG?
http://zacg.github.io/blog/2013/06/06/calling-c-plus-plus-code-from-go-with-swig/ #Closed

lgtm-com · 2022-11-24T01:04:24Z

This pull request introduces 1 alert when merging 81700b4 into 6b0253a - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

ganglyu · 2022-11-25T12:46:12Z

/azp run sonic-net.sonic-gnmi

azure-pipelines · 2022-11-25T12:46:22Z

Azure Pipelines successfully started running 1 pipeline(s).

qiluo-msft · 2022-11-26T09:55:41Z

gnmi_server/server.go

+}
+
+func IsNativeOrigin(origin string) bool {
+	return origin == "sonic-db" || origin == "sonic-yang"


"sonic-db"

Origin sonic-db is named as native since the gnmi client directly manipulate raw ConfigDB. sonic-yang is not considered native because there will be a translation between sonic-yang payload to ConfigDB schema. #Closed

qiluo-msft · 2022-11-26T09:58:11Z

gnmi_server/server.go

+		return origin, nil
+	}
+	for i, path := range paths {
+		if origin == "" {


origin

Why treat empty input origin differently?

I will fix this.

qiluo-msft · 2022-11-26T09:58:52Z

gnmi_server/server.go

@@ -274,6 +277,26 @@ func (s *Server) checkEncodingAndModel(encoding gnmipb.Encoding, models []*gnmip
 	return nil
 }

+func ParseOrigin(origin string, paths []*gnmipb.Path) (string, error) {


origin

why a parse function has an extra input parameter origin? Could you add a function level comment?

I will fix this.

ganglyu · 2022-11-28T12:59:57Z

/azp run sonic-net.sonic-gnmi

azure-pipelines · 2022-11-28T13:00:09Z

Azure Pipelines successfully started running 1 pipeline(s).

ganglyu · 2022-11-28T22:51:56Z

/azp run sonic-net.sonic-gnmi

azure-pipelines · 2022-11-28T22:52:07Z

Azure Pipelines successfully started running 1 pipeline(s).

Why I did it Submodule update for sonic-gnmi Incorporates: 8226e46 Upgrade pipeline to use bullseye. (sonic-net/sonic-gnmi#58) ae72767 Add gnmi_dump tool for debug and unit test (sonic-net/sonic-gnmi#60) 6b0253a Add conditional check for split (sonic-net/sonic-gnmi#55) 99bfa8f Remove LOGLEVEL DB since is no longer used (sonic-net/sonic-gnmi#56) 54806a8 Support new gnmi config interface in telemetry container. (sonic-net/sonic-gnmi#7) How I did it Move submodule How to verify it Check build pipeline.

ganglyu requested a review from qiluo-msft August 10, 2022 08:03

ganglyu mentioned this pull request Sep 25, 2022

Add configuration parameter for gnmi native write sonic-net/sonic-buildimage#12182

Open

7 tasks

qiluo-msft reviewed Sep 26, 2022

View reviewed changes

ganglyu added 2 commits November 15, 2022 02:47

Support GNMI native write

e103b09

Fix test issue

2cd6dfa

ganglyu mentioned this pull request Nov 18, 2022

[HLD] SONiC GNMI Server Interface Design sonic-net/SONiC#996

Merged

ganglyu added 2 commits November 21, 2022 02:22

Merge branch 'master' into gnmi_native_write

f019892

Fix lost src file

71e5e09

ganglyu requested a review from qiluo-msft November 21, 2022 02:57

qiluo-msft reviewed Nov 21, 2022

View reviewed changes

ganglyu added 3 commits November 22, 2022 12:53

Use swig wrapper for swsscommon

3ffe171

Fix make dependency

5b43489

Support GNMI native write with origin

81700b4

ganglyu requested a review from qiluo-msft November 24, 2022 01:01

ganglyu requested a review from zbud-msft November 24, 2022 01:04

Fix LGTM alert.

de25049

ganglyu and others added 4 commits November 24, 2022 05:22

Update close

d8792b8

Update close

8f7bc2a

Merge branch 'master' into config_support

267f418

Update

9261480

qiluo-msft reviewed Nov 26, 2022

View reviewed changes

Address comments

1e2a040

ganglyu requested a review from qiluo-msft November 26, 2022 11:44

Reuse swsscommon.i from libswsscommon-dev

93e9bf3

qiluo-msft approved these changes Nov 28, 2022

View reviewed changes

Merge branch 'master' into config_support

7619240

ganglyu merged commit 54806a8 into sonic-net:master Nov 28, 2022

This was referenced Nov 29, 2022

[sonic-gnmi] Support GNMI native write sonic-net/sonic-buildimage#10948

Merged

Update submodule for sonic-gnmi sonic-net/sonic-buildimage#12860

Merged

EdenGri mentioned this pull request Dec 4, 2022

[202211][submodule] Advance sonic-gnmi pointer sonic-net/sonic-buildimage#12929

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support new gnmi config interface in telemetry container. #7

Support new gnmi config interface in telemetry container. #7

ganglyu commented Aug 10, 2022

linux-foundation-easycla bot commented Sep 24, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

ganglyu Sep 27, 2022

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft commented Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022

qiluo-msft Sep 26, 2022

qiluo-msft Sep 26, 2022

linux-foundation-easycla bot commented Nov 14, 2022 •

edited

Loading

qiluo-msft Nov 21, 2022 •

edited

Loading

lgtm-com bot commented Nov 24, 2022

ganglyu commented Nov 25, 2022

azure-pipelines bot commented Nov 25, 2022

qiluo-msft Nov 26, 2022 •

edited

Loading

qiluo-msft Nov 26, 2022

ganglyu Nov 26, 2022

qiluo-msft Nov 26, 2022

ganglyu Nov 26, 2022

ganglyu commented Nov 28, 2022

azure-pipelines bot commented Nov 28, 2022

ganglyu commented Nov 28, 2022

azure-pipelines bot commented Nov 28, 2022

		endif

		# TODO: Create a new repo for this lib, sonic-restapi and sonic-gnmi can share this lib

		@@ -0,0 +1,37 @@
		--- ./github.com/jipanyang/gnxi/utils/xpath/path.go

Support new gnmi config interface in telemetry container. #7

Support new gnmi config interface in telemetry container. #7

Conversation

ganglyu commented Aug 10, 2022

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)

linux-foundation-easycla bot commented Sep 24, 2022 • edited Loading

qiluo-msft Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

image: sonicdev-microsoft.azurecr.io:443/sonic-slave-bullseye:latest

qiluo-msft Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

github.com/agiledragon/gomonkey/v2 v2.9.0

qiluo-msft Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

ifeq ($(ENABLE_NATIVE_WRITE),y)

qiluo-msft Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

patch -d vendor -p0 < patches/path.patch

qiluo-msft Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

# TODO: Create a new repo for this lib, sonic-restapi and sonic-gnmi can share this lib

qiluo-msft Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

//return nil, status.Errorf(codes.Unimplemented, "hit test point")

qiluo-msft Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

common_utils.IncCounter("GNMI get")

qiluo-msft commented Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Nov 14, 2022 • edited Loading

qiluo-msft Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Nov 24, 2022

ganglyu commented Nov 25, 2022

azure-pipelines bot commented Nov 25, 2022

qiluo-msft Nov 26, 2022 • 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

ganglyu commented Nov 28, 2022

azure-pipelines bot commented Nov 28, 2022

ganglyu commented Nov 28, 2022

azure-pipelines bot commented Nov 28, 2022

linux-foundation-easycla bot commented Sep 24, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft Sep 26, 2022 •

edited

Loading

qiluo-msft commented Sep 26, 2022 •

edited

Loading

linux-foundation-easycla bot commented Nov 14, 2022 •

edited

Loading

qiluo-msft Nov 21, 2022 •

edited

Loading

qiluo-msft Nov 26, 2022 •

edited

Loading