sonic-telemetry icon indicating copy to clipboard operation
sonic-telemetry copied to clipboard

Add Virtual Database Client + Unit Tests

Open randygaulmsft opened this issue 5 years ago • 12 comments

This commit adds virtual database support for the SONiC target, built by an intern Chang Lui. I added some integration tests over the top of the virtual path + virtual database work by Chang. Here is a diagram describing the high level organization of most of Chang's contributions. Some important pieces are numbered, and described below.

image

Information about the Numbered Pieces of the Above Diagram

  1. virtual_database_client/db_client.go
    • NewDbClient - Instance created to deal with all RPC logic
    • Get
    • PollRun
  2. virtual_database_client/handler_*.go
    • Injects data into the search trie. The trie maps gNMI paths to Redis keys.
  3. virtual_database_client/handler_<YOUR_DATA_TYPE>.go
    • path2HdlrFuncTbl - Table to populate the search trie. Each entry represents a supported data type. Each handler references a function in handler_<YOUR_DATA_TYPE>.go file.
    • TriePopulate - Called when module is initialized, and loops over path2HdlrFuncTbl.
  4. virtual_database_client/path.go
    • populateAlltablePaths - Translate gNMI path to Redis keys
    • msi2TypedValue - Transform redis value to proto stream for RPC return value

Important Callstacks

Get

The below callstack reaches into the handlers to construct gNMI to Redis mappings.

  • Get - db_client.go line 188
  • populateAlltablePaths - path.go line 31
  • populateNewtablePath - path.go line 44
  • searchPathTrie - handler_func.go line 53
  • handler - handler_func.go line 60

The final line calls into the "virtual to real" conversion functions as defined in the handler_<YOUR_DATA_TYPE>.go files. Once a value is retrieved it is handed to tableData2TypedValue and converted to JSON form ready for gNMI proto stream return.

This callstack converts a Redis key value to an appropriate proto stream return value.

  • Get - db_client.go line 188
  • tableData2TypedValue - path.go line 99
  • tableData2Msi - path.go line 111
  • msi2TypedValue - path.go line 73

tableData2Msi calls into the redis API with redis.HGetAll, passing in redis keys for actual SONiC values. The redis return value is converted to gNMI proto stream form via msi2TypedValue.

Subscribe (StreamRun)

  • StreamRun - db_client.go line 126
  • dbPathSubscribe - path.go line 214 - Runs as a goroutine, performs actual susbscription logic. The calling function will wait until the goroutine syncs.
  • tableData2TypedValue - path.go line 99
  • tableData2Msi - path.go line 111
  • msi2TypedValue - path.go line 73
  • dbSingleTableKeySubscribe - path.go line 158

Subscribe (PollRun)

PollRun is not implemented!

NewDbClient

Called when an RPC is recieved by the gNMI service running on the SONiC device (gnmi_server/*.go). These functions are called to initialize gNMI to Redis mappings:

  • initCountersPortNameMap
  • initCountersQueueNameMap
  • initAliasMap
  • initCountersPfcwdNameMap

These functions load the various Redis targets into suitable run-time formats, used in gNMI <-> SONiC key transformations via the database client's trie data structure. These functions are using redis.HGetAll, from the Go redis API to load out key-value pairs.

Running Tests

Use go test -v ./package_folder. Make sure the folder points to a particular package you want to test.

randygaulmsft avatar Mar 25 '19 21:03 randygaulmsft

The flowchart and callstack is very helpful to new developers and reviewers. Thanks for adding them. Could you please also add some for PollRun callstack?

hui-ma avatar Apr 10 '19 16:04 hui-ma

Add Wenda to verify PFC WD logic

hui-ma avatar Apr 10 '19 16:04 hui-ma

@hui-ma PollRun is not implemented. StreamRun is implemented though.

Many of Jipan's tests from gnmi_server are not possible with the virtual database client, because it does not support some of the query features such as lookup up a single field under PfcCounter. However, it is possible to write some tests for StreamRun via Subscribe.

Not implemented in virtual database:

  • Path to a specific field under PfcCounter.
  • PollRun.
  • Set.
  • Probably more, but would require more time to all missing pieces.

Can have additional testing to closer match Jipan's tests:

  • StreamRun.

I will work on StreamRun tests.

randygaulmsft avatar Apr 14 '19 22:04 randygaulmsft

Could you use gofmt to format your changes?

There are some existing virtual path implementations integrated with the StreamRun() and PollRun() as of today. It looks you duplicated part of the code, what are the major differences?

jipanyang avatar Apr 15 '19 03:04 jipanyang

Could you share the command line to unit test? We plan to add it into PR checker and automate it, even for this PR. #Closed

qiluo-msft avatar Apr 19 '19 19:04 qiluo-msft

@qiluo-msft: @randygaulmsft is working on adding the PR check build.

jleveque avatar Apr 19 '19 19:04 jleveque

Could you use gofmt to format your changes?

There are some existing virtual path implementations integrated with the StreamRun() and PollRun() as of today. It looks you duplicated part of the code, what are the major differences?

@jipanyang Ran go format (push pending). Also I noticed a lot of code duplication in virtual_client_database as well. This was written by Chang Lui the previous intern. The idea here was to augment the telemetry container with the new virtual database to add new query features.

Your code was duplicated to avoid potentially breaking the existing implementation. As far as I know there are no significant changes, other than string translations from vendor-specific form through the new generic query form.

randygaulmsft avatar May 15 '19 19:05 randygaulmsft

@qiluo-msft

I've already setup the PR builder. Here's the current version of the script as-is in Jenkins.

#!/bin/bash -xe

# Install golang.
go_version=go1.12.4.linux-amd64

if [ ! -e $go_version.tar.gz ]
then
    wget -q -o /dev/null https://storage.googleapis.com/golang/$go_version.tar.gz
    tar xf $go_version.tar.gz
fi

export GOROOT=$WORKSPACE/go
export PATH=$PATH:$GOROOT/bin
export GOPATH=$HOME/go

# Setup and install redis, which requires some minor conf settings for the unit tests.
sudo apt -qq -y install redis-server

sudo sed -i 's/^supervised no/supervised systemd/' /etc/redis/redis.conf
sudo sed -i '$ a unixsocket /var/run/redis/redis.sock' /etc/redis/redis.conf
sudo sed -i '$ a unixsocketperm 766' /etc/redis/redis.conf

sudo service redis stop
sudo service redis start

# Install and test specific packages.
go get -v -t github.com/Azure/sonic-telemetry/gnmi_server/...
go test -v $GOPATH/src/github.com/Azure/sonic-telemetry/gnmi_server

go get -v -t github.com/Azure/sonic-telemetry/virtual_database_client/...
go test -v $GOPATH/src/github.com/Azure/sonic-telemetry/virtual_database_client

NOTE: I just now added Azure/sonic-telemetry/virtual_database_client (the last two lines of the script), so the PR builder will fail until this PR is actually merged in, since the paths do not quite yet match.

randygaulmsft avatar May 15 '19 19:05 randygaulmsft

Get and run code:

go get -u github.com/Azure/sonic-telemetry/gnmi_server
cd ~/go/src/github.com/Azure
delete all files in here
git clone -b new-pfcwd --single-branch https://github.com/randygaulmsft/sonic-telemetry.git
cd sonic-telemetry
go test -v ./virtual_database_client

randygaulmsft avatar May 16 '19 23:05 randygaulmsft

Please note the Jenkins script is failing now since I added the new package path. This is expected. The tests are running properly as expected, and besides any more feedback the pull request is ready for merging.

randygaulmsft avatar May 17 '19 05:05 randygaulmsft

The new unittest depends on unixsocket while the legacy on doesn't need. Ideally, the new one should not either. Please check what the new dependence is introduced in the new test case. unixsocket is for better performance. Therefore, unit test doesn't necessary depends on it.

hui-ma avatar May 17 '19 17:05 hui-ma

Even with unixsocket, I saw failure case. e.g, FAIL: TestVirtualDatabaseGNMISubscribe/Stream_query_for_Interfaces/Port[name=Ethernet68]/Queue[name=Queue3]/Pfcwd,_updating_PFC_WD_QUEUE_STATS_DEADLOCK_DETECTED_from_0_to_1. (2.53s)

Please verify and fix if there is any

hui-ma avatar May 17 '19 17:05 hui-ma