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

[bugfix] add target length validation for each grpc client exection method #1939

Merged
merged 1 commit into from
Feb 7, 2023
Merged

[bugfix] add target length validation for each grpc client exection method #1939

merged 1 commit into from
Feb 7, 2023

Conversation

kpango
Copy link
Collaborator

@kpango kpango commented Feb 2, 2023 •
edited
Loading

Signed-off-by: kpango [email protected]

Description:

Vald's gRPC Client is implemented to remove Unhealthy Targets from the connection list and not query the Target in question until the new connection becomes Healthy.

In the event of network total loss, all targets become unhealthy, so Gateways that perform Broadcast and DoMulti instructions do not enter the processing loop and the error is not recognized, making it appear as if the client has succeeded.

As a solution, gRPC Client changed its policy to return the ErrClientConnectionNotFound error when the Target of gRPC Client is zero or the number of times the processing loop is executed is zero, judging that there is no Target.

Versions:

  • Go Version: 1.20
  • Docker Version: 20.10.8
  • Kubernetes Version: 1.22.0
  • NGT Version: 2.0.9

Checklist:

Special notes for your reviewer:

@vdaas-ci
Copy link
Collaborator

vdaas-ci commented Feb 2, 2023

[CHATOPS:HELP] ChatOps commands.

  • 🙆‍♀️ /approve - approve
  • 💌 /changelog - replace the PR body by changelog details
  • 🍱 /format - format codes and add licenses
  • ✅ /gen-test - generate test codes
  • 🏷️ /label - add labels
  • ⏪ /rebase - rebase main
  • 🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

@codecov
Copy link

codecov bot commented Feb 2, 2023 •
edited
Loading

Codecov Report

Base: 29.98% // Head: 32.73% // Increases project coverage by +2.75% 🎉

Coverage data is based on head (9d16336) compared to base (f529cbd).
Patch coverage: 14.54% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1939      +/-   ##
==========================================
+ Coverage   29.98%   32.73%   +2.75%     
==========================================
  Files         366      362       -4     
  Lines       33704    30840    -2864     
==========================================
- Hits        10107    10097      -10     
+ Misses      23188    20341    -2847     
+ Partials      409      402       -7
Impacted Files Coverage Δ
internal/errors/ngt.go 70.00% <ø> (-3.92%) ⬇️
internal/net/grpc/client.go 0.00% <0.00%> (ø)
internal/net/grpc/grpcconns.go 0.00% <0.00%> (ø)
internal/observability/exporter/otlp/otlp.go 0.00% <ø> (ø)
internal/observability/trace/status.go 0.00% <ø> (ø)
internal/errors/agent.go 100.00% <100.00%> (ø)
internal/net/net.go 85.71% <100.00%> (ø)
internal/backoff/backoff.go 71.28% <0.00%> (-1.99%) ⬇️
pkg/gateway/lb/handler/grpc/handler.go
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kpango kpango force-pushed the bugfix/internal-grpc/grpc-connectin-not-return-error-when-no-target-exists branch from 447edbf to 95c41e3 Compare February 2, 2023 06:42
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Feb 2, 2023 •
edited
Loading

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9d16336
Status: ✅  Deploy successful!
Preview URL: https://a8a6bf6e.vald.pages.dev
Branch Preview URL: https://bugfix-internal-grpc-grpc-co.vald.pages.dev

View logs

@kpango kpango force-pushed the bugfix/internal-grpc/grpc-connectin-not-return-error-when-no-target-exists branch 2 times, most recently from 899ef2d to bbc30de Compare February 2, 2023 06:58
@kpango kpango force-pushed the bugfix/internal-grpc/grpc-connectin-not-return-error-when-no-target-exists branch from bbc30de to 995d3b5 Compare February 2, 2023 07:57
@kpango kpango requested review from hlts2 and datelier February 2, 2023 08:03
@kpango kpango force-pushed the bugfix/internal-grpc/grpc-connectin-not-return-error-when-no-target-exists branch from 995d3b5 to 3f0cbc0 Compare February 2, 2023 08:16
hlts2
hlts2 previously approved these changes Feb 2, 2023
View reviewed changes
Copy link
Collaborator

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

LGTM

@kpango kpango force-pushed the bugfix/internal-grpc/grpc-connectin-not-return-error-when-no-target-exists branch from 3f0cbc0 to e004aae Compare February 2, 2023 08:25
@github-actions github-actions bot added team/set SET team size/XXL and removed size/L labels Feb 2, 2023
@kpango kpango force-pushed the bugfix/internal-grpc/grpc-connectin-not-return-error-when-no-target-exists branch from 46025c0 to ee62c81 Compare February 2, 2023 09:12
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

[FOSSA] The scan result will be available at https://app.fossa.com/projects/custom%2B21465%2Fvald/refs/branch/bugfix%2Finternal-grpc%2Fgrpc-connectin-not-return-error-when-no-target-exists/d076617c094a1a18b1d4d403283eee8c4d4e96dc

@kpango kpango force-pushed the bugfix/internal-grpc/grpc-connectin-not-return-error-when-no-target-exists branch from ee62c81 to 94cd020 Compare February 2, 2023 09:24
github-actions[bot]
github-actions bot reviewed Feb 6, 2023
View reviewed changes
pkg/gateway/lb/handler/grpc/handler.go
}, info.Get())
id, err := s.exists(ctx, uuid)
var attrs trace.Attributes
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
1529-1546 lines are duplicate of pkg/gateway/lb/handler/grpc/handler.go:1784-1801 (dupl)

pkg/gateway/lb/handler/grpc/handler.go
}, info.Get())
id, err := s.exists(ctx, uuid)
var attrs trace.Attributes
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
1784-1801 lines are duplicate of pkg/gateway/lb/handler/grpc/handler.go:1529-1546 (dupl)

pkg/gateway/lb/handler/grpc/handler.go
if err == nil {
err = errors.ErrObjectIDNotFound(uuid)
vec, err := s.getObject(ctx, uuid)
if err != nil || vec == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
2004-2049 lines are duplicate of pkg/gateway/lb/handler/grpc/handler.go:2272-2317 (dupl)

pkg/gateway/lb/handler/grpc/handler.go
if err == nil {
err = errors.ErrObjectIDNotFound(uuid)
vec, err := s.getObject(ctx, uuid)
if err != nil || vec == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
2272-2317 lines are duplicate of pkg/gateway/lb/handler/grpc/handler.go:2004-2049 (dupl)

pkg/gateway/lb/handler/grpc/handler.go
if err == nil {
err = errors.ErrObjectIDNotFound(id.GetId())
_, err := s.exists(ctx, uuid)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
2900-2947 lines are duplicate of pkg/gateway/lb/handler/grpc/handler.go:3094-3141 (dupl)

pkg/gateway/lb/handler/grpc/handler.go
if err == nil {
err = errors.ErrObjectIDNotFound(id.GetId())
_, err = s.exists(ctx, uuid)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
3094-3141 lines are duplicate of pkg/gateway/lb/handler/grpc/handler.go:2900-2947 (dupl)

pkg/gateway/lb/handler/grpc/handler_test.go
UnimplementedValdServer: test.fields.UnimplementedValdServer,
}

gotId, err := s.exists(test.args.ctx, test.args.uuid)
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [golangci] reported by reviewdog 🐶
ST1003: var gotId should be gotID (stylecheck)

datelier
datelier previously approved these changes Feb 6, 2023
View reviewed changes
@kpango kpango force-pushed the bugfix/internal-grpc/grpc-connectin-not-return-error-when-no-target-exists branch from 0d91032 to 12b1af3 Compare February 6, 2023 08:05
@kpango kpango force-pushed the bugfix/internal-grpc/grpc-connectin-not-return-error-when-no-target-exists branch from 12b1af3 to d95226b Compare February 6, 2023 08:31
@github-actions github-actions bot added size/XXL and removed size/XXL labels Feb 6, 2023
@vdaas-ci
Copy link
Collaborator

vdaas-ci commented Feb 6, 2023

Profile Report

typevald-agent-ngtvald-lb-gatewayvald-discoverervald-manager-index
cpu
heap
 other images

hlts2
hlts2 previously approved these changes Feb 6, 2023
View reviewed changes
Copy link
Collaborator

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

LGTM

@kpango kpango force-pushed the bugfix/internal-grpc/grpc-connectin-not-return-error-when-no-target-exists branch from d95226b to 9d16336 Compare February 7, 2023 02:16
hlts2
hlts2 approved these changes Feb 7, 2023
View reviewed changes
Copy link
Collaborator

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

LGTM

@kpango kpango merged commit c83a852 into main Feb 7, 2023
@kpango kpango deleted the bugfix/internal-grpc/grpc-connectin-not-return-error-when-no-target-exists branch February 7, 2023 03:04
This was referenced Feb 8, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@sonatype-lift sonatype-lift[bot] sonatype-lift[bot] left review comments

@datelier datelier datelier approved these changes

@hlts2 hlts2 hlts2 approved these changes

@vankichi vankichi Awaiting requested review from vankichi

Assignees
No one assigned
Labels
actions/e2e-chaos actions/e2e-deploy actions/e2e-max-dim actions/e2e-profiling actions/fossa area/gateway/lb area/internal priority/medium size/XXXL team/set SET team type/bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kpango @vdaas-ci @hlts2 @datelier