Skip to content
This repository has been archived by the owner on Aug 31, 2022. It is now read-only.

[multi-DB] Part 5: Golang API changes and replacement #24

Merged
merged 7 commits into from
Nov 22, 2019

Conversation

dzhangalibaba
Copy link
Contributor

  • add new package to parse database_config.json file and provide get APIs

  • replace hardcoded TCP/SOCK Addr with new Get Func()

  • fix telemetry test cases which is not working before, when make deb pkg, test cases run will be triggered

  • all current testcases passed and on DUT, telemetry and dialout_client_cli are running
    /usr/local/go/bin/go test -v /tmp/go/src/github.com/Azure/sonic-telemetry/gnmi_server
    === RUN TestGnmiGet
    === RUN TestGnmiGet/Test_non-existing_path_Target
    === RUN TestGnmiGet/Test_empty_path_target
    === RUN TestGnmiGet/Get_valid_but_non-existing_node
    === RUN TestGnmiGet/Get_COUNTERS_PORT_NAME_MAP
    === RUN TestGnmiGet/get_COUNTERS:Ethernet68
    === RUN TestGnmiGet/get_COUNTERS:Ethernet68_SAI_PORT_STAT_PFC_7_RX_PKTS
    === RUN TestGnmiGet/get_COUNTERS:Ethernet68_Pfcwd
    === RUN TestGnmiGet/get_COUNTERS_(use_vendor_alias):Ethernet68/1
    === RUN TestGnmiGet/get_COUNTERS_(use_vendor_alias):Ethernet68/1_SAI_PORT_STAT_PFC_7_RX_PKTS
    === RUN TestGnmiGet/get_COUNTERS_(use_vendor_alias):Ethernet68/1_Pfcwd
    === RUN TestGnmiGet/get_COUNTERS:Ethernet*
    === RUN TestGnmiGet/get_COUNTERS:Ethernet*SAI_PORT_STAT_PFC_7_RX_PKTS
    === RUN TestGnmiGet/get_COUNTERS:Ethernet*Pfcwd
    --- PASS: TestGnmiGet (4.40s)
    --- PASS: TestGnmiGet/Test_non-existing_path_Target (0.01s)
    --- PASS: TestGnmiGet/Test_empty_path_target (0.00s)
    --- PASS: TestGnmiGet/Get_valid_but_non-existing_node (0.00s)
    --- PASS: TestGnmiGet/Get_COUNTERS_PORT_NAME_MAP (0.00s)
    --- PASS: TestGnmiGet/get_COUNTERS:Ethernet68 (0.00s)
    --- PASS: TestGnmiGet/get_COUNTERS:Ethernet68_SAI_PORT_STAT_PFC_7_RX_PKTS (0.00s)
    --- PASS: TestGnmiGet/get_COUNTERS:Ethernet68_Pfcwd (0.00s)
    --- PASS: TestGnmiGet/get_COUNTERS
    (use_vendor_alias):Ethernet68/1 (0.00s)
    --- PASS: TestGnmiGet/get_COUNTERS
    (use_vendor_alias):Ethernet68/1_SAI_PORT_STAT_PFC_7_RX_PKTS (0.00s)
    --- PASS: TestGnmiGet/get_COUNTERS_(use_vendor_alias):Ethernet68/1_Pfcwd (0.00s)
    --- PASS: TestGnmiGet/get_COUNTERS:Ethernet* (0.00s)
    --- PASS: TestGnmiGet/get_COUNTERS:Ethernet*_SAI_PORT_STAT_PFC_7_RX_PKTS (0.00s)
    --- PASS: TestGnmiGet/get_COUNTERS:Ethernet*_Pfcwd (0.00s)
    === RUN TestGnmiSubscribe
    === RUN TestGnmiSubscribe/stream_query_for_table_COUNTERS_PORT_NAME_MAP_with_new_test_field_field
    === RUN TestGnmiSubscribe/stream_query_for_table_key_Ethernet68_with_new_test_field_field
    === RUN TestGnmiSubscribe/(use_vendor_alias)_stream_query_for_table_key_Ethernet68/1_with_new_test_field_field
    === RUN TestGnmiSubscribe/stream_query_for_COUNTERS/Ethernet68/SAI_PORT_STAT_PFC_7_RX_PKTS_with_update_of_field_value
    === RUN TestGnmiSubscribe/(use_vendor_alias)_stream_query_for_COUNTERS/[Ethernet68/1]/SAI_PORT_STAT_PFC_7_RX_PKTS_with_update_of_field_value
    === RUN TestGnmiSubscribe/stream_query_for_COUNTERS/Ethernet68/Pfcwd_with_update_of_field_value
    === RUN TestGnmiSubscribe/(use_vendor_alias)_stream_query_for_COUNTERS/[Ethernet68/1]/Pfcwd_with_update_of_field_value
    === RUN TestGnmiSubscribe/stream_query_for_table_key_Ethernet*_with_new_test_field_field_on_Ethernet68
    === RUN TestGnmiSubscribe/stream_query_for_table_key_Ethernet*/SAI_PORT_STAT_PFC_7_RX_PKTS_with_field_value_update
    === RUN TestGnmiSubscribe/stream_query_for_table_key_Ethernet*/Pfcwd_with_field_value_update
    === RUN TestGnmiSubscribe/poll_query_for_table_COUNTERS_PORT_NAME_MAP_with_new_field_test_field
    === RUN TestGnmiSubscribe/poll_query_for_table_COUNTERS_PORT_NAME_MAP_with_test_field_delete
    === RUN TestGnmiSubscribe/poll_query_for_COUNTERS/Ethernet68/SAI_PORT_STAT_PFC_7_RX_PKTS_with_field_value_change
    === RUN TestGnmiSubscribe/(use_vendor_alias)_poll_query_for_COUNTERS/[Ethernet68/1]/SAI_PORT_STAT_PFC_7_RX_PKTS_with_field_value_change
    === RUN TestGnmiSubscribe/poll_query_for_COUNTERS/Ethernet68/Pfcwd_with_field_value_change
    === RUN TestGnmiSubscribe/(use_vendor_alias)_poll_query_for_COUNTERS/[Ethernet68/1]/Pfcwd_with_field_value_change
    === RUN TestGnmiSubscribe/poll_query_for_table_key_Ethernet*_with_Ethernet68/SAI_PORT_STAT_PFC_7_RX_PKTS_field_value_change
    === RUN TestGnmiSubscribe/poll_query_for_table_key_field_Ethernet*/SAI_PORT_STAT_PFC_7_RX_PKTS_with_Ethernet68/SAI_PORT_STAT_PFC_7_RX_PKTS_field_value_change
    === RUN TestGnmiSubscribe/poll_query_for_table_key_field_Etherenet*/Pfcwd_with_Ethernet68:3/PFC_WD_QUEUE_STATS_DEADLOCK_DETECTED_field_value_change
    === RUN TestGnmiSubscribe/poll_query_for_COUNTERS/Ethernet*/Queues
    === RUN TestGnmiSubscribe/poll_query_for_COUNTERS/Ethernet68/Queues_with_field_value_change
    === RUN TestGnmiSubscribe/(use_vendor_alias)_poll_query_for_COUNTERS/Ethernet68/Queues_with_field_value_change
    --- PASS: TestGnmiSubscribe (76.84s)
    --- PASS: TestGnmiSubscribe/stream_query_for_table_COUNTERS_PORT_NAME_MAP_with_new_test_field_field (2.00s)
    --- PASS: TestGnmiSubscribe/stream_query_for_table_key_Ethernet68_with_new_test_field_field (3.00s)
    --- PASS: TestGnmiSubscribe/(use_vendor_alias)_stream_query_for_table_key_Ethernet68/1_with_new_test_field_field (3.00s)
    --- PASS: TestGnmiSubscribe/stream_query_for_COUNTERS/Ethernet68/SAI_PORT_STAT_PFC_7_RX_PKTS_with_update_of_field_value (3.00s)
    --- PASS: TestGnmiSubscribe/(use_vendor_alias)_stream_query_for_COUNTERS/[Ethernet68/1]/SAI_PORT_STAT_PFC_7_RX_PKTS_with_update_of_field_value (3.00s)
    --- PASS: TestGnmiSubscribe/stream_query_for_COUNTERS/Ethernet68/Pfcwd_with_update_of_field_value (3.00s)
    --- PASS: TestGnmiSubscribe/(use_vendor_alias)_stream_query_for_COUNTERS/[Ethernet68/1]/Pfcwd_with_update_of_field_value (3.00s)
    --- PASS: TestGnmiSubscribe/stream_query_for_table_key_Ethernet*_with_new_test_field_field_on_Ethernet68 (3.00s)
    --- PASS: TestGnmiSubscribe/stream_query_for_table_key_Ethernet*/SAI_PORT_STAT_PFC_7_RX_PKTS_with_field_value_update (2.00s)
    --- PASS: TestGnmiSubscribe/stream_query_for_table_key_Ethernet*/Pfcwd_with_field_value_update (2.00s)
    --- PASS: TestGnmiSubscribe/poll_query_for_table_COUNTERS_PORT_NAME_MAP_with_new_field_test_field (2.00s)
    --- PASS: TestGnmiSubscribe/poll_query_for_table_COUNTERS_PORT_NAME_MAP_with_test_field_delete (2.00s)
    --- PASS: TestGnmiSubscribe/poll_query_for_COUNTERS/Ethernet68/SAI_PORT_STAT_PFC_7_RX_PKTS_with_field_value_change (2.00s)
    --- PASS: TestGnmiSubscribe/(use_vendor_alias)_poll_query_for_COUNTERS/[Ethernet68/1]/SAI_PORT_STAT_PFC_7_RX_PKTS_with_field_value_change (2.00s)
    --- PASS: TestGnmiSubscribe/poll_query_for_COUNTERS/Ethernet68/Pfcwd_with_field_value_change (2.00s)
    --- PASS: TestGnmiSubscribe/(use_vendor_alias)_poll_query_for_COUNTERS/[Ethernet68/1]/Pfcwd_with_field_value_change (2.00s)
    --- PASS: TestGnmiSubscribe/poll_query_for_table_key_Ethernet*_with_Ethernet68/SAI_PORT_STAT_PFC_7_RX_PKTS_field_value_change (2.01s)
    --- PASS: TestGnmiSubscribe/poll_query_for_table_key_field_Ethernet*/SAI_PORT_STAT_PFC_7_RX_PKTS_with_Ethernet68/SAI_PORT_STAT_PFC_7_RX_PKTS_field_value_change (2.01s)
    --- PASS: TestGnmiSubscribe/poll_query_for_table_key_field_Etherenet*/Pfcwd_with_Ethernet68:3/PFC_WD_QUEUE_STATS_DEADLOCK_DETECTED_field_value_change (2.02s)
    --- PASS: TestGnmiSubscribe/poll_query_for_COUNTERS/Ethernet*/Queues (1.05s)
    --- PASS: TestGnmiSubscribe/poll_query_for_COUNTERS/Ethernet68/Queues_with_field_value_change (2.01s)
    --- PASS: TestGnmiSubscribe/(use_vendor_alias)_poll_query_for_COUNTERS/Ethernet68/Queues_with_field_value_change (2.01s)
    PASS
    ok github.com/Azure/sonic-telemetry/gnmi_server (cached)
    /usr/local/go/bin/go get -v -t github.com/Azure/sonic-telemetry/dialout/dialout_client/...
    github.com/google/gnxi (download)
    /usr/local/go/bin/go test -v /tmp/go/src/github.com/Azure/sonic-telemetry/dialout/dialout_client
    === RUN TestGNMIDialOutPublish
    === RUN TestGNMIDialOutPublish/DialOut_to_first_collector_in_stream_mode_and_synced
    === RUN TestGNMIDialOutPublish/DialOut_to_second_collector_in_stream_mode_upon_failure_of_first_collector
    --- PASS: TestGNMIDialOutPublish (19.58s)
    --- PASS: TestGNMIDialOutPublish/DialOut_to_first_collector_in_stream_mode_and_synced (0.52s)
    --- PASS: TestGNMIDialOutPublish/DialOut_to_second_collector_in_stream_mode_upon_failure_of_first_collector (7.52s)
    PASS
    ok github.com/Azure/sonic-telemetry/dialout/dialout_client (cached)

Signed-off-by: Dong Zhang [email protected]

@dzhangalibaba
Copy link
Contributor Author

dzhangalibaba commented Nov 11, 2019

@qiluo-msft #Resolved

@@ -158,7 +159,7 @@ func getRedisClient(t *testing.T) *redis.Client {
dbn := spb.Target_value["COUNTERS_DB"]
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Target_value [](start = 12, length = 12)

Should we remove enum Target? Everything should come from json config. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't remove it. Instead, updating the map values with the data read from database_config.json file when do init() in proto pkg. After that the we register use the same name and struct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest follow the design of c++ or python. something like sdcfg.GetDbId("COUNTERS_DB")


In reply to: 344860708 [](ancestors = 344860708)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added GetDbId()

Makefile Outdated

all: sonic-telemetry

sonic-telemetry:
# copy sonic-telemetry source code into ${GOPATH}/src directory for building, otherwise it is not using committed codes
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent the comment line? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

Makefile Outdated
mkdir -p ${GOPATH}/src/github.com/Azure
rsync -a ../sonic-telemetry ${GOPATH}/src/github.com/Azure/
cd ${GOPATH}/src/github.com/Azure/sonic-telemetry
# go get won't overwrite exsisting ${GOPATH}/src/sonic-telemetry directory and download other package
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent the comment line? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

Makefile Outdated

all: sonic-telemetry

sonic-telemetry:
# copy sonic-telemetry source code into ${GOPATH}/src directory for building, otherwise it is not using committed codes
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise it is not using committed codes [](start = 78, length = 41)

what is it using? #Closed

Copy link
Contributor Author

@dzhangalibaba dzhangalibaba Nov 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go will use ${GOPATH}/src as the building dir, we need to copy the modified codes there. Before the changes, it downloaded the source codes from the latest at github.com/Azure/sonic-telemetry/XXXX instead of using the local PR committed codes.

Makefile Outdated
/usr/local/go/bin/go get -v github.com/Azure/sonic-telemetry/telemetry
/usr/local/go/bin/go get -v github.com/Azure/sonic-telemetry/dialout/dialout_client_cli

check:
sudo mkdir -p ${DBDIR}
sudo rsync -a ./testdata/database_config.json ${DBDIR}
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[](start = 47, length = 1)

Remove extra blank #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

Makefile Outdated
/usr/local/go/bin/go get -v github.com/Azure/sonic-telemetry/telemetry
/usr/local/go/bin/go get -v github.com/Azure/sonic-telemetry/dialout/dialout_client_cli

check:
sudo mkdir -p ${DBDIR}
sudo rsync -a ./testdata/database_config.json ${DBDIR}
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rsync [](start = 6, length = 5)

Why not use cp ? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally I like to use rsync, cp is also fine. Updated

Makefile Outdated

all: sonic-telemetry

sonic-telemetry:
# copy sonic-telemetry source code into ${GOPATH}/src directory for building, otherwise it is not using committed codes
mkdir -p ${GOPATH}/src/github.com/Azure
rsync -a ../sonic-telemetry ${GOPATH}/src/github.com/Azure/
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rsync [](start = 1, length = 5)

Why not use cp ? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally I like to use rsync, cp is also fine. Updated

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As comments

@@ -3,16 +3,25 @@ export GOPATH=/tmp/go
endif

INSTALL := /usr/bin/install
DBDIR := /var/run/redis/sonic-db/

all: sonic-telemetry

sonic-telemetry:
Copy link
Contributor

@qiluo-msft qiluo-msft Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sonic-telemetry [](start = 0, length = 15)

I am not familiar with golang.
Seems this is a build target, why not use go build directly, instead of go get. Then we don't need mkdir and cp #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go build still require ${GOPATH}/src as source codes workspace, we still need mkdir and cp

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that you can run go build without a target, it just compile *.go in current directory.


In reply to: 344968232 [](ancestors = 344968232)

Copy link
Contributor Author

@dzhangalibaba dzhangalibaba Nov 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some search online, looks after go version 1.11, go modules are introduced to solve the $GOPATH problem. We can use go mod init and go install to remove the dependency on GOPATH. Also looks our go version in docker is 1.11.5 which is updated this July. Then we are able to build using local source files without cp. Makefile is updated.

@dzhangalibaba
Copy link
Contributor Author

dzhangalibaba commented Nov 13, 2019

retest this please

1 similar comment
@dzhangalibaba
Copy link
Contributor Author

retest this please

@dzhangalibaba
Copy link
Contributor Author

unit test failed . looks docker environment has problem , it cannot install redis related debs. Please help checking.

@dzhangalibaba
Copy link
Contributor Author

retest this please

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Please fix PR test.

@dzhangalibaba
Copy link
Contributor Author

retest this please

@dzhangalibaba
Copy link
Contributor Author

Looks good to me. Please fix PR test.

Thanks for reviewing. Looks the PR test is failed due to slave docker environment issue. which is not caused by this PR. it is a general issue for all testing. I think Ying or someone else is looking at that. Probabaly we need to wait slave docker to be fixed and then have a try.

@dzhangalibaba
Copy link
Contributor Author

retest this please

1 similar comment
@dzhangalibaba
Copy link
Contributor Author

retest this please

@dzhangalibaba
Copy link
Contributor Author

dzhangalibaba commented Nov 14, 2019

I am still see the build issue on Jenkis when installing redis-tools.
This works before at least last week. I see now many pr build has the same issue. e.g. sonic-sairedis-build-pr, sonic-telemetry-build-pr, sonic-swss-common-build-pr. All of them have the same issue.

Looks the docker environment still has some issues. Could you confirm it is fixed or you guys are still working on it ? @lguohan @yxieca @qiluo-msft

Below is the build error:

+ sudo dpkg -i buildimage/target/debs/stretch/redis-tools_5.0.3-3~bpo9+2_amd64.deb
Selecting previously unselected package redis-tools.
(Reading database ... 138346 files and directories currently installed.)
Preparing to unpack .../redis-tools_5.0.3-3~bpo9+2_amd64.deb ...
Unpacking redis-tools (5:5.0.3-3~bpo9+2) ...
dpkg: dependency problems prevent configuration of redis-tools:
 redis-tools depends on liblua5.1-0; however:
  Package liblua5.1-0 is not installed.
 redis-tools depends on lua-bitop; however:
  Package lua-bitop is not installed.
 redis-tools depends on lua-cjson; however:
  Package lua-cjson is not installed.

dpkg: error processing package redis-tools (--install):
 dependency problems - leaving unconfigured
Processing triggers for man-db (2.7.6.1-2) ...
Errors were encountered while processing:
 redis-tools
[Pipeline] }
[Pipeline] // stage
[Pipeline] }
[Pipeline] // withEnv
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline

@dzhangalibaba
Copy link
Contributor Author

@qiluo-msft one question, I am hitting below error:

/usr/local/go/bin/go mod init github.com/Azure/sonic-telemetry
go: unknown subcommand "mod"
Run 'go help' for usage.

I tried on local slave-docker, the go version is 1.11.5 and go mod subcmd is available.

I checked the pr docker, I am not sure which sonic-docker it is using, looks the go version is not 1.11.5 as what we have in slave docker. Could you help comfirm ?

"Running on docker-001oaz053w8db on sonic-docker-stretch in /var/johnar/workspace/common/sonic-telemetry-build-pr"

where does sonic-docker-stretch come from
is it the same as sonic-slave docker ?

@sonic-net sonic-net deleted a comment from dzhangalibaba Nov 22, 2019
@qiluo-msft
Copy link
Contributor

@dzhangalibaba Fixed in buildimage and also Jenkins config.

@sonic-net sonic-net deleted a comment from dzhangalibaba Nov 22, 2019
@sonic-net sonic-net deleted a comment from yxieca Nov 22, 2019
@qiluo-msft qiluo-msft merged commit 4bb02f5 into sonic-net:master Nov 22, 2019
@dzhangalibaba dzhangalibaba deleted the multidb_go branch November 22, 2019 08:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants