[multi-asic] Added Support for multi-asic for telemetry/gnmi server #77
Conversation
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
for single and multi namespace/asic. Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
abdosi
changed the title
|
@macikgozwa Somehow cannot add you in reviewers so tagging you. |
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Makefile
Outdated
|
|
||
| clean: | ||
| $(RM) -r build | ||
| $(RM) -r vendor | ||
| $(RM) -r ${DBDIR} |
There was a problem hiding this comment.
yes updated and needed in Azure Pipeline setup. In local docker it was fine as running as root so got missed.
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
|
Hi @reshmaintel - please assign reviewers from Intel team, thanks. |
|
"If the “/ASICX” is not present, then request will map to default namespace (existing behavior will be achieved", does this rule also |
|
Hi @akokhan could you please review this PR. Thanks |
@hui-ma for the counter DB we support V2R mapping dataset for which this PR (as mention in design document) get information from all ASIC's counter DB. As of now this PR don't support passing /ASICx for Counter Db V2R set and log error for this case. However in future this is extendable when Counter DB has any dataset that needs it. |
|
@macikgozwa can you please review/approve again. |
|
@hui-ma and @@macikgozwa is it ok to approve/merge this . Can you please check. |
Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
|
@hui-ma can we review/approve this ? |
| tlsConfig := &tls.Config{InsecureSkipVerify: true} | ||
| opts := []grpc.DialOption{grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig))} | ||
|
|
||
| //targetAddr := "30.57.185.38:8080" |
There was a problem hiding this comment.
if this comment is redundant it can be removed. (Same for other comments)
There was a problem hiding this comment.
thanks, we can do cleanup as part of other PR.
| pfcwdName_map[name] = make(map[string]string) | ||
| } | ||
| for _, key := range resp { | ||
| name := key[13:] |
There was a problem hiding this comment.
could you please add a comment, to explain what is the magic number 13?
I see, that it was done before this PR, but if you can add comment, it will simplify the code reading.
There was a problem hiding this comment.
this represent the name in key. Best place to see different key name used in in SONiC is to use to refer schema.
https://github.com/Azure/sonic-swss-common/blob/master/common/schema.h
| @@ -419,7 +468,7 @@ func v2rEthPortPfcwdStats(paths []string) ([]tablePath, error) { | |||
| // Populate real data paths from paths like | |||
| // [COUNTER_DB COUNTERS Ethernet* Queues] or [COUNTER_DB COUNTERS Ethernet68 Queues] | |||
| func v2rEthPortQueStats(paths []string) ([]tablePath, error) { | |||
| separator, _ := GetTableKeySeparator(paths[DbIdx]) | |||
| separator, _ := GetTableKeySeparator(paths[DbIdx], "") | |||
There was a problem hiding this comment.
if the blank string is intended to get separator for default namespace, it's better to use constant for the default namespace sonic_db_config.db_config.SONIC_DEFAULT_NAMESPACE or GetDbDefaultNamespace() from the same package
There was a problem hiding this comment.
even though value is same the SONIC_DEFAULT_NAMESPACE is used for some other purpose and has different meaning. To replace constant we will need to use other macro. This can be enhanced as part of other pr regarding cleanup the code
|
This is a comment for This code wouldn't run in case target has a asic name segment. I recall Compare to the query without asic suffix (on the same environment): If UPDATE: |
| panic(fmt.Errorf("Global Database config file is not valid(multiple include for same namespace!")) | ||
| } | ||
| //Ref:https://www.geeksforgeeks.org/filepath-join-function-in-golang-with-examples/ | ||
| db_include_file := filepath.Join(dir, entry.(map[string]interface{})["include"].(string)) |
There was a problem hiding this comment.
Since it is taking dir for the first param, the include path has to be a relative path (to the global file path). e.g. It wouldn't accept full paths.
I don't think it is an issue, but something to be aware of.
There was a problem hiding this comment.
@macikgozwa the same file is used in other places also. So we should be fine. Eg: https://github.com/Azure/sonic-swss-common/blob/master/common/dbconnector.cpp#L219
| @@ -62,7 +62,7 @@ type Stream interface { | |||
| var UseRedisLocalTcpPort bool = false | |||
|
|
|||
| // redis client connected to each DB | |||
| var Target2RedisDb = make(map[string]*redis.Client) | |||
| var Target2RedisDb = make(map[string]map[string]*redis.Client) | |||
There was a problem hiding this comment.
I am new to go, but could you explain why this map[string] is repeated here? Does this mean 2D array?
There was a problem hiding this comment.
@roweisch yes. Basically we have {dbname} to {redis client} mapping .. now we have {namespace, dbname} to {redis client} mapping.
path then always return error. Signed-off-by: Abhishek Dosi <abdosi@microsoft.com>
Thanks @macikgozwa I have fixed this in latest commit. Also added unit test case for same. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@lguohan @liushilongbuaa can you plese help check azure pipeline build failure. We are not able to down artifact for sonic-mgmt-common repo. |
|
See failure like below in utest. It is expected failure? @hui-ma this is negative test case I belive added by @macikgozwa |
Yes, we have a negative case to test what happens when the version file is missing. @hui-ma @abdosi |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
As part of PR: #77 i added removal of DB Directory (/var/run/redis) as part of make clean. This is causing issue where other debian package test fails (as they don't see database_config.json) .
What I did:-
This PR handles the changes as mention in the document:
MultiasicTelemetry.docx
Summary of Changes:-
sonic_db_configpackage for multi-asic to read different namespace redis configuration. Added unit-test for same.GetandSubscribetests for multi-namespace. Addedtest_utilspackage providing utility API's needed by test-cases. Also added multi-namespace specificjsonfiles into testdataTargetin gNMI Request to understand namespace if present.go fmt. Please ignore whitespaces diff as part of review.IntervalTimeTickerfunction pointer not re-assigned after test case is done.How I Verify:
GETandSUBSCRIBEwith newNamespacespecific test-cases also.sonic_db_configtestcases passed.