Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add conditional check for split #55

Merged
merged 9 commits into from
Nov 22, 2022
Merged

Add conditional check for split #55

merged 9 commits into from
Nov 22, 2022

Conversation

zbud-msft
Copy link
Contributor

@zbud-msft zbud-msft commented Nov 3, 2022 •
edited by qiluo-msft
Loading

Why I did it

Can crash if no delimitor is present and accessing second part of split

Crash stack

telemetry#/supervisord: telemetry #011/sonic/src/sonic-telemetry/sonic_data_client/db_client.go:
telemetry#/supervisord: telemetry #011/sonic/src/sonic-telemetry/sonic_data_client/db_client.go:315 +0x14f
telemetry#/supervisord: telemetry #011/sonic/src/sonic-telemetry/sonic_data_client/db_client.go:653 +0xcb2
telemetry#/supervisord: telemetry #011/sonic/src/sonic-telemetry/gnmi_server/server.go:317 +0x4d3
telemetry#/supervisord: telemetry #011/sonic/src/sonic-telemetry/vendor/google.golang.org/grpc/server.go:1533 +0xcfd
telemetry#/supervisord: telemetry google.golang.org/grpc.(*Server).serveStreams.func1.2(0xc0013e2640, 0xc00094ba40, 0x11758e0, 0xc002222480, 0xc000e6b100)
telemetry#/supervisord: telemetry github.com/Azure/sonic-telemetry/sonic_data_client.(*DbClient).Get(0xc000182e70, 0x0, 0x1, 0xc0000108d0, 0x1171b60, 0xc000182e70, 0x0)
telemetry#/supervisord: telemetry #011/sonic/src/sonic-telemetry/vendor/github.com/openconfig/gnmi/proto/gnmi/gnmi.pb.go:3381 +0x217
telemetry#/supervisord: telemetry #011/sonic/src/sonic-telemetry/vendor/google.golang.org/grpc/server.go:871 +0xa1

How I did it

Added conditional check

How to verify it

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@ganglyu
Copy link
Contributor

ganglyu commented Nov 3, 2022

Can you add unit test for this case?

@qiluo-msft
Copy link
Collaborator

Please check if we can backport this PR to release branches.

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Nov 3, 2022 •
edited
Loading

I notice something wrong with coverage checker "No lines with coverage information in this diff."
If it is working, it will block the PR for missing unit test.

@ganglyu
Copy link
Contributor

ganglyu commented Nov 4, 2022

I notice something wrong with coverage checker "No lines with coverage information in this diff." If it is working, it will block the PR for missing unit test.

https://github.com/sonic-net/sonic-gnmi/blob/master/Makefile#L74-L76
Current unit test does not count the coverage for sonic_data_client.

@zbud-msft zbud-msft merged commit 6b0253a into sonic-net:master Nov 22, 2022
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Nov 28, 2022
@dprital
[submodule] Advance sonic-gnmi pointer
ef1693b 
Update sonic-gnmi submodule pointer to include the following:
* 99bfa8f Remove LOGLEVEL DB since is no longer used ([sonic-net#56](sonic-net/sonic-gnmi#56))
* 6b0253a Add conditional check for split ([sonic-net#55](sonic-net/sonic-gnmi#55))
* ae72767 Add gnmi_dump tool for debug and unit test ([sonic-net#60](sonic-net/sonic-gnmi#60))
* 8226e46 Upgrade pipeline to use bullseye. ([sonic-net#58](sonic-net/sonic-gnmi#58))

Signed-off-by: dprital <drorp@nvidia.com>
@dprital dprital mentioned this pull request Nov 28, 2022
Closed
7 tasks
This was referenced Nov 29, 2022
Merged
Merged
ganglyu added a commit to sonic-net/sonic-buildimage that referenced this pull request Nov 29, 2022
@ganglyu
Update submodule for sonic-gnmi (#12860)
62698c7 
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.
StormLiangMS pushed a commit to StormLiangMS/sonic-buildimage that referenced this pull request Dec 8, 2022
@ganglyu @StormLiangMS
Update submodule for sonic-gnmi (sonic-net#12860)
3d6a9fd 
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.
StormLiangMS pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Dec 10, 2022
@ganglyu @StormLiangMS
Update submodule for sonic-gnmi (#12860)
93ae2b3 
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.
@zbud-msft zbud-msft deleted the BugFixInvalidDBKey branch February 28, 2023 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomek-US tomek-US tomek-US approved these changes

@ganglyu ganglyu ganglyu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zbud-msft @ganglyu @qiluo-msft @tomek-US