From da3ff953e23949ac181205888ca187f2b175e156 Mon Sep 17 00:00:00 2001 From: Murat Acikgoz Date: Sun, 22 Nov 2020 00:06:32 -0800 Subject: [PATCH 1/2] New data set to query Sonic build version. --- gnmi_server/server_test.go | 168 ++++++++++++++++++----------- sonic_data_client/non_db_client.go | 81 ++++++++++++-- 2 files changed, 178 insertions(+), 71 deletions(-) diff --git a/gnmi_server/server_test.go b/gnmi_server/server_test.go index 5a3a1932c..6b7a4eb0e 100644 --- a/gnmi_server/server_test.go +++ b/gnmi_server/server_test.go @@ -6,10 +6,19 @@ package gnmi import ( "crypto/tls" "encoding/json" + "fmt" + testcert "github.com/Azure/sonic-telemetry/testdata/tls" "github.com/go-redis/redis" "github.com/golang/protobuf/proto" + "io/ioutil" + "os" + "os/exec" + "reflect" + "testing" + "time" + "github.com/kylelemons/godebug/pretty" "github.com/openconfig/gnmi/client" pb "github.com/openconfig/gnmi/proto/gnmi" @@ -19,17 +28,11 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials" "google.golang.org/grpc/status" - "io/ioutil" - "os" - "os/exec" - "reflect" - "testing" - "time" + // Register supported client types. sdc "github.com/Azure/sonic-telemetry/sonic_data_client" sdcfg "github.com/Azure/sonic-telemetry/sonic_db_config" gclient "github.com/jipanyang/gnmi/client/gnmi" - ) var clientTypes = []string{gclient.Type} @@ -121,7 +124,7 @@ func runTestGet(t *testing.T, ctx context.Context, gClient pb.GNMIClient, pathTa t.Log("err: ", err) t.Fatalf("got return code %v, want %v", gotRetStatus.Code(), wantRetCode) } - + // Check response value if valTest { var gotVal interface{} @@ -389,7 +392,7 @@ func prepareDbTranslib(t *testing.T) { rclient := getRedisClient(t) rclient.FlushDB() rclient.Close() - + //Enable keysapce notification os.Setenv("PATH", "/usr/bin:/sbin:/bin:/usr/local/bin") cmd := exec.Command("redis-cli", "config", "set", "notify-keyspace-events", "KEA") @@ -512,8 +515,6 @@ func TestGnmiSet(t *testing.T) { s.s.Stop() } - - func TestGnmiGet(t *testing.T) { //t.Log("Start server") s := createServer(t) @@ -578,14 +579,47 @@ func TestGnmiGet(t *testing.T) { t.Fatalf("read file %v err: %v", fileName, err) } - tds := []struct { + type testCase struct { desc string pathTarget string textPbPath string wantRetCode codes.Code wantRespVal interface{} valTest bool - }{{ + testInit func() + } + + // A helper function create test cases for 'osversion/build' queries. + createBuildVersionTestCase := func(desc string, wantedVersion string, versionFileContent string, fileReadErr error) testCase { + return testCase{ + desc: desc, + pathTarget: "OTHERS", + textPbPath: ` + elem: + elem: + `, + wantRetCode: codes.OK, + valTest: true, + wantRespVal: []byte(wantedVersion), + testInit: func() { + // Override file read function to mock file content. + sdc.ImplIoutilReadFile = func(filePath string) ([]byte, error) { + if filePath == sdc.SonicVersionFilePath { + if fileReadErr != nil { + return nil, fileReadErr + } + return []byte(versionFileContent), nil + } + return ioutil.ReadFile(filePath) + } + + // Reset the cache so that the content gets loaded again. + sdc.InvalidateVersionFileStash() + }, + } + } + + tds := []testCase{{ desc: "Test non-existing path Target", pathTarget: "MY_DB", textPbPath: ` @@ -702,9 +736,17 @@ func TestGnmiGet(t *testing.T) { wantRetCode: codes.OK, wantRespVal: countersEthernetWildcardPfcwdByte, }, + createBuildVersionTestCase("get osversion/build", `{"build_version": "sonic.12345678.90"}`, "build_version: '12345678.90'\ndebian_version: '9.13'", nil), + createBuildVersionTestCase("get osversion/build file load error", `{"build_version": "sonic.NA"}`, "", fmt.Errorf("Cannot access '%v' ", sdc.SonicVersionFilePath)), + createBuildVersionTestCase("get osversion/build file parse error", `{"build_version": "sonic.NA"}`, "no a valid YAML content", nil), + createBuildVersionTestCase("get osversion/build different value", `{"build_version": "sonic.23456789.01"}`, "build_version: '23456789.01'\ndebian_version: '9.15'", nil), } for _, td := range tds { + if td.testInit != nil { + td.testInit() + } + t.Run(td.desc, func(t *testing.T) { runTestGet(t, ctx, gClient, td.pathTarget, td.textPbPath, td.wantRetCode, td.wantRespVal, td.valTest) }) @@ -745,56 +787,56 @@ func TestGnmiGetTranslib(t *testing.T) { }{ //These tests only work on the real switch platform, since they rely on files in the /proc and another running service - // { - // desc: "Get OC Platform", - // pathTarget: "OC_YANG", - // textPbPath: ` - // elem: - // `, - // wantRetCode: codes.OK, - // wantRespVal: emptyRespVal, - // valTest: false, - // }, - // { - // desc: "Get OC System State", - // pathTarget: "OC_YANG", - // textPbPath: ` - // elem: elem: - // `, - // wantRetCode: codes.OK, - // wantRespVal: emptyRespVal, - // valTest: false, - // }, - // { - // desc: "Get OC System CPU", - // pathTarget: "OC_YANG", - // textPbPath: ` - // elem: elem: - // `, - // wantRetCode: codes.OK, - // wantRespVal: emptyRespVal, - // valTest: false, - // }, - // { - // desc: "Get OC System memory", - // pathTarget: "OC_YANG", - // textPbPath: ` - // elem: elem: - // `, - // wantRetCode: codes.OK, - // wantRespVal: emptyRespVal, - // valTest: false, - // }, - // { - // desc: "Get OC System processes", - // pathTarget: "OC_YANG", - // textPbPath: ` - // elem: elem: - // `, - // wantRetCode: codes.OK, - // wantRespVal: emptyRespVal, - // valTest: false, - // }, + // { + // desc: "Get OC Platform", + // pathTarget: "OC_YANG", + // textPbPath: ` + // elem: + // `, + // wantRetCode: codes.OK, + // wantRespVal: emptyRespVal, + // valTest: false, + // }, + // { + // desc: "Get OC System State", + // pathTarget: "OC_YANG", + // textPbPath: ` + // elem: elem: + // `, + // wantRetCode: codes.OK, + // wantRespVal: emptyRespVal, + // valTest: false, + // }, + // { + // desc: "Get OC System CPU", + // pathTarget: "OC_YANG", + // textPbPath: ` + // elem: elem: + // `, + // wantRetCode: codes.OK, + // wantRespVal: emptyRespVal, + // valTest: false, + // }, + // { + // desc: "Get OC System memory", + // pathTarget: "OC_YANG", + // textPbPath: ` + // elem: elem: + // `, + // wantRetCode: codes.OK, + // wantRespVal: emptyRespVal, + // valTest: false, + // }, + // { + // desc: "Get OC System processes", + // pathTarget: "OC_YANG", + // textPbPath: ` + // elem: elem: + // `, + // wantRetCode: codes.OK, + // wantRespVal: emptyRespVal, + // valTest: false, + // }, { desc: "Get OC Interfaces", pathTarget: "OC_YANG", diff --git a/sonic_data_client/non_db_client.go b/sonic_data_client/non_db_client.go index bc0723b69..5a7a40d8c 100644 --- a/sonic_data_client/non_db_client.go +++ b/sonic_data_client/non_db_client.go @@ -3,13 +3,17 @@ package client import ( "encoding/json" "fmt" + "io/ioutil" + "sync" + "time" + + "gopkg.in/yaml.v2" + spb "github.com/Azure/sonic-telemetry/proto" + "github.com/Workiva/go-datastructures/queue" linuxproc "github.com/c9s/goprocinfo/linux" log "github.com/golang/glog" gnmipb "github.com/openconfig/gnmi/proto/gnmi" - "github.com/Workiva/go-datastructures/queue" - "sync" - "time" ) // Non db client is to Handle @@ -17,6 +21,9 @@ import ( const ( statsRingCap uint64 = 3000 // capacity of statsRing. + + // SonicVersionFilePath is the path of build version YML file. + SonicVersionFilePath = "/etc/sonic/sonic_version.yml" ) type dataGetFunc func() ([]byte, error) @@ -32,14 +39,36 @@ type statsRing struct { mu sync.RWMutex // Mutex for data protection } +// SonicVersionInfo is a data model to serialize '/etc/sonic/sonic_version.yml' +type SonicVersionInfo struct { + BuildVersion string `yaml:"build_version" json:"build_version"` +} + +// sonicVersionYmlStash caches the content of '/etc/sonic/sonic_version.yml' +// Assumed that the content of the file doesn't change during the lifetime of telemetry service. +type sonicVersionYmlStash struct { + once sync.Once // sync object to make sure file is loaded only once. + versionInfo SonicVersionInfo +} + +// InvalidateVersionFileStash invalidates the cache that keeps version file content. +func InvalidateVersionFileStash() { + versionFileStash = sonicVersionYmlStash{} +} + var ( clientTrie *Trie statsR statsRing - // path2DataFuncTbl is used to populate trie tree which is reponsible + versionFileStash sonicVersionYmlStash + + // ImplIoutilReadFile points to the implementation of ioutil.ReadFile. Should be overridden by UTs only. + ImplIoutilReadFile func(string) ([]byte, error) = ioutil.ReadFile + + // path2DataFuncTbl is used to populate trie tree which is responsible // for getting data at the path specified path2DataFuncTbl = []path2DataFunc{ - { // Get cpu utilizaation + { // Get cpu utilization path: []string{"OTHERS", "platform", "cpu"}, getFunc: dataGetFunc(getCpuUtil), }, @@ -63,6 +92,10 @@ var ( path: []string{"OTHERS", "proc", "stat"}, getFunc: dataGetFunc(getProcStat), }, + { // OS build version + path: []string{"OTHERS", "osversion", "build"}, + getFunc: dataGetFunc(getBuildVersion), + }, } ) @@ -246,6 +279,39 @@ func getProcStat() ([]byte, error) { return b, nil } +func getBuildVersion() ([]byte, error) { + + // Load and parse the content of version file + versionFileStash.once.Do(func() { + versionFileStash.versionInfo.BuildVersion = "sonic.NA" + + fileContent, err := ImplIoutilReadFile(SonicVersionFilePath) + if err != nil { + log.Errorf("Failed to read '%v', %v", SonicVersionFilePath, err) + return + } + + err = yaml.Unmarshal(fileContent, &versionFileStash.versionInfo) + if err != nil { + log.Errorf("Failed to parse '%v', %v", SonicVersionFilePath, err) + return + } + + // Prepend 'sonic.' to the build version string. + if versionFileStash.versionInfo.BuildVersion != "sonic.NA" { + versionFileStash.versionInfo.BuildVersion = "sonic." + versionFileStash.versionInfo.BuildVersion + } + }) + + b, err := json.Marshal(versionFileStash.versionInfo) + if err != nil { + log.V(2).Infof("%v", err) + return b, err + } + log.V(4).Infof("getBuildVersion, output %v", string(b)) + return b, nil +} + func pollStats() { for { stat, err := linuxproc.ReadStat("/proc/stat") @@ -411,10 +477,9 @@ func (c *NonDbClient) Close() error { return nil } -func (c *NonDbClient) Set(path *gnmipb.Path, t *gnmipb.TypedValue, flagop int) error { +func (c *NonDbClient) Set(path *gnmipb.Path, t *gnmipb.TypedValue, flagop int) error { return nil } -func (c *NonDbClient) Capabilities() ([]gnmipb.ModelData) { +func (c *NonDbClient) Capabilities() []gnmipb.ModelData { return nil } - From 4e5ca27a3fbecf1e9a4bc055d471d4a3a58ffc62 Mon Sep 17 00:00:00 2001 From: Murat Acikgoz Date: Tue, 1 Dec 2020 23:29:55 -0800 Subject: [PATCH 2/2] Adding error message in case version file is not read with success. --- gnmi_server/server_test.go | 31 ++++++++++++++++++++++++++---- sonic_data_client/non_db_client.go | 4 ++++ 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/gnmi_server/server_test.go b/gnmi_server/server_test.go index 6b7a4eb0e..f6b429428 100644 --- a/gnmi_server/server_test.go +++ b/gnmi_server/server_test.go @@ -736,10 +736,33 @@ func TestGnmiGet(t *testing.T) { wantRetCode: codes.OK, wantRespVal: countersEthernetWildcardPfcwdByte, }, - createBuildVersionTestCase("get osversion/build", `{"build_version": "sonic.12345678.90"}`, "build_version: '12345678.90'\ndebian_version: '9.13'", nil), - createBuildVersionTestCase("get osversion/build file load error", `{"build_version": "sonic.NA"}`, "", fmt.Errorf("Cannot access '%v' ", sdc.SonicVersionFilePath)), - createBuildVersionTestCase("get osversion/build file parse error", `{"build_version": "sonic.NA"}`, "no a valid YAML content", nil), - createBuildVersionTestCase("get osversion/build different value", `{"build_version": "sonic.23456789.01"}`, "build_version: '23456789.01'\ndebian_version: '9.15'", nil), + // Happy path + createBuildVersionTestCase( + "get osversion/build", // query path + `{"build_version": "sonic.12345678.90", "error":""}`, // expected response + "build_version: '12345678.90'\ndebian_version: '9.13'", // YAML file content + nil), // mock file reading error + + // File reading error + createBuildVersionTestCase( + "get osversion/build file load error", + `{"build_version": "sonic.NA", "error":"Cannot access '/etc/sonic/sonic_version.yml'"}`, + "", + fmt.Errorf("Cannot access '%v'", sdc.SonicVersionFilePath)), + + // File content is not valid YAML + createBuildVersionTestCase( + "get osversion/build file parse error", + `{"build_version": "sonic.NA", "error":"yaml: unmarshal errors:\n line 1: cannot unmarshal !!str `+"`not a v...`"+` into client.SonicVersionInfo"}`, + "not a valid YAML content", + nil), + + // Happy path with different value + createBuildVersionTestCase( + "get osversion/build different value", + `{"build_version": "sonic.23456789.01", "error":""}`, + "build_version: '23456789.01'\ndebian_version: '9.15'", + nil), } for _, td := range tds { diff --git a/sonic_data_client/non_db_client.go b/sonic_data_client/non_db_client.go index 5a7a40d8c..a29fe354e 100644 --- a/sonic_data_client/non_db_client.go +++ b/sonic_data_client/non_db_client.go @@ -42,6 +42,7 @@ type statsRing struct { // SonicVersionInfo is a data model to serialize '/etc/sonic/sonic_version.yml' type SonicVersionInfo struct { BuildVersion string `yaml:"build_version" json:"build_version"` + Error string `json:"error"` // Applicable only when there is a failure while reading the file. } // sonicVersionYmlStash caches the content of '/etc/sonic/sonic_version.yml' @@ -284,16 +285,19 @@ func getBuildVersion() ([]byte, error) { // Load and parse the content of version file versionFileStash.once.Do(func() { versionFileStash.versionInfo.BuildVersion = "sonic.NA" + versionFileStash.versionInfo.Error = "" // empty string means no error. fileContent, err := ImplIoutilReadFile(SonicVersionFilePath) if err != nil { log.Errorf("Failed to read '%v', %v", SonicVersionFilePath, err) + versionFileStash.versionInfo.Error = err.Error() return } err = yaml.Unmarshal(fileContent, &versionFileStash.versionInfo) if err != nil { log.Errorf("Failed to parse '%v', %v", SonicVersionFilePath, err) + versionFileStash.versionInfo.Error = err.Error() return }