cleanup: replace dial with newclient #7920

janardhanvissa · 2024-12-11T05:08:22Z

Partially address: #7049

RELEASE NOTES: None

arjan-bal · 2024-12-11T05:26:29Z

The tests are failing, please fix the failures. In the future, please ensure that tests pass locally before raising a PR.

go test ./... -count=1 -failfast

janardhanvissa · 2024-12-11T09:41:23Z

The tests are failing, please fix the failures. In the future, please ensure that tests pass locally before raising a PR.
go test ./... -count=1 -failfast                                                         

I have tested locally before pushing the code, nothing is failing. Let me re-trigger the PR.

test/creds_test.go

resolver_balancer_ext_test.go

test/creds_test.go

arjan-bal · 2024-12-13T13:08:50Z

The test failure for the new pickfirst policy seem to be because the DNS resolver is resolving localhost to both the IPv4 and IPv6 addresses, while the server listens only on the IPv4 address. Can you change the following line to la := "[::]:0" and see if the test passes?

grpc-go/test/end2end_test.go

Line 612 in 38a8b9a

la := "localhost:0"

This should make the server listen on both the IPv4 and IPv6 localhost addresses.

codecov · 2024-12-15T06:41:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.29%. Comparing base (e4d084a) to head (93a17b6).
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7920      +/-   ##
==========================================
+ Coverage   82.05%   82.29%   +0.23%     
==========================================
  Files         377      379       +2     
  Lines       38180    38261      +81     
==========================================
+ Hits        31330    31487     +157     
+ Misses       5549     5491      -58     
+ Partials     1301     1283      -18

see 37 files with indirect coverage changes

janardhanvissa · 2024-12-15T06:43:27Z

The test failure for the new pickfirst policy seem to be because the DNS resolver is resolving localhost to both the IPv4 and IPv6 addresses, while the server listens only on the IPv4 address. Can you change the following line to la := "[::]:0" and see if the test passes?

grpc-go/test/end2end_test.go

Line 612 in 38a8b9a

la := "localhost:0"

This should make the server listen on both the IPv4 and IPv6 localhost addresses.

Yeah, it's working, thanks

balancer/grpclb/grpclb_test.go

arjan-bal · 2024-12-16T04:09:06Z

test/end2end_test.go

@@ -609,7 +609,7 @@ func (te *test) listenAndServe(ts testgrpc.TestServiceServer, listen func(networ
 	if te.serverInitialConnWindowSize > 0 {
 		sopts = append(sopts, grpc.InitialConnWindowSize(te.serverInitialConnWindowSize))
 	}
-	la := "localhost:0"
+	la := "[::]:0"


@dfawley this is required to fix test failures with the new pickfirst. localhost is being resolved to both IPv4 and IPv6 addresses on Github, while net.Listen("tcp", "localhost") is listening only on the IPv4 address. The resolver list has the IPv4 address followed by the IPv6 one. The old pickfirst reports the first connectivity error while the new pickfirst reports the latest error.

Using [::] makes the listener bind to all (possibly non-local) interfaces. Is this a concern? We could instead update the test to send "127.0.0.1" through the name resolver.

We should avoid net.Listen("localhost"), and use net.Listen(":port") instead. That will make it listen on both ipv4 and ipv6 addresses.

@janardhanvissa can you please change this to ":0"?

…lState

balancer/grpclb/grpclb_test.go

dfawley · 2024-12-16T21:57:53Z

balancer/grpclb/grpclb_test.go

@@ -460,7 +460,7 @@ func (s) TestGRPCLB_Basic(t *testing.T) {
 	}
 	cc, err := grpc.NewClient(r.Scheme()+":///"+beServerName, dopts...)
 	if err != nil {
-		t.Fatalf("Failed to dial to the backend %v", err)
+		t.Fatalf("Failed to create a newclient to the backend %v", err)


Nit: "... create a client for the backend" everywhere, please.

OR, ("grpc.NewClient(%q) failed unexpectedly: %v", target, err)

dfawley · 2024-12-16T22:00:27Z

test/end2end_test.go

@@ -609,7 +609,7 @@ func (te *test) listenAndServe(ts testgrpc.TestServiceServer, listen func(networ
 	if te.serverInitialConnWindowSize > 0 {
 		sopts = append(sopts, grpc.InitialConnWindowSize(te.serverInitialConnWindowSize))
 	}
-	la := "localhost:0"
+	la := "[::]:0"


We should avoid net.Listen("localhost"), and use net.Listen(":port") instead. That will make it listen on both ipv4 and ipv6 addresses.

This reverts commit a21e192.

arjan-bal self-requested a review December 11, 2024 05:26

arjan-bal assigned janardhanvissa Dec 11, 2024

arjan-bal added the Type: Internal Cleanup Refactors, etc label Dec 11, 2024

arjan-bal added this to the 1.70 Release milestone Dec 11, 2024

janardhankrishna-sai added 3 commits December 11, 2024 10:17

replace grpc.Dial with grpc.NewClient

2648448

re-trigger

b3f2f5e

re-trigger

76915bd

janardhanvissa force-pushed the newclient-insteadof-dial branch from c5b8178 to 76915bd Compare December 11, 2024 10:20

arjan-bal reviewed Dec 11, 2024

View reviewed changes

test/creds_test.go Outdated Show resolved Hide resolved

arjan-bal reviewed Dec 11, 2024

View reviewed changes

test/creds_test.go Outdated Show resolved Hide resolved

janardhankrishna-sai added 4 commits December 11, 2024 15:24

using r.InitialState instead of r.UpdateState

1cba7c2

using rb.InitialState instead of cc.UpdateState

6cebed0

reverting stubserver changes

1c11e23

reverting stubserver changes

6e64672

arjan-bal reviewed Dec 13, 2024

View reviewed changes

resolver_balancer_ext_test.go Outdated Show resolved Hide resolved

test/creds_test.go Outdated Show resolved Hide resolved

test/creds_test.go Outdated Show resolved Hide resolved

janardhankrishna-sai added 3 commits December 13, 2024 08:10

reverting rb.InitialState from cc.UpdateState

0cb8e39

removing empty lines

46449b7

replacing dial with newclient

17366d0

updating default listener address

f2c6c00

arjan-bal reviewed Dec 16, 2024

View reviewed changes

arjan-bal assigned dfawley Dec 16, 2024

janardhankrishna-sai added 3 commits December 16, 2024 11:30

removing explicit cc.Connect() and updating r.UpdateState to r.Initia…

e3e6810

…lState

removing explicit cc.Connect() and updating r.UpdateState to r.Initia…

98a3d9c

…lState

reverting updated err msg

9855d08

arjan-bal reviewed Dec 16, 2024

View reviewed changes

balancer/grpclb/grpclb_test.go Show resolved Hide resolved

arjan-bal reviewed Dec 16, 2024

View reviewed changes

balancer/grpclb/grpclb_test.go Outdated Show resolved Hide resolved

reverting TestGRPCLB_FallBackWithNoServerAddress updates

58eb4a3

dfawley reviewed Dec 16, 2024

View reviewed changes

dfawley removed their assignment Dec 16, 2024

janardhankrishna-sai added 2 commits December 17, 2024 01:59

updating error msg

c71b79d

updating default listener address

647c7d2

janardhanvissa force-pushed the newclient-insteadof-dial branch from cc0aa2e to 647c7d2 Compare December 17, 2024 18:34

re-trigger

93a17b6

arjan-bal approved these changes Dec 17, 2024

View reviewed changes

arjan-bal assigned dfawley and unassigned janardhanvissa Dec 17, 2024

dfawley approved these changes Dec 18, 2024

View reviewed changes

dfawley merged commit 56a14ba into grpc:master Dec 18, 2024
15 checks passed

arjan-bal mentioned this pull request Dec 20, 2024

test: Workaround slow SRV lookups in flaking test #7957

Merged

vinothkumarr227 pushed a commit to vinothkumarr227/grpc-go that referenced this pull request Dec 23, 2024

cleanup: replace dial with newclient (grpc#7920)

a21e192

vinothkumarr227 added a commit to vinothkumarr227/grpc-go that referenced this pull request Dec 23, 2024

Revert "cleanup: replace dial with newclient (grpc#7920)"

cf12ace

This reverts commit a21e192.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup: replace dial with newclient #7920

cleanup: replace dial with newclient #7920

janardhanvissa commented Dec 11, 2024 •

edited

Loading

arjan-bal commented Dec 11, 2024

janardhanvissa commented Dec 11, 2024

arjan-bal commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 15, 2024 •

edited

Loading

janardhanvissa commented Dec 15, 2024

arjan-bal Dec 16, 2024 •

edited

Loading

dfawley Dec 16, 2024

arjan-bal Dec 17, 2024

janardhanvissa Dec 17, 2024

dfawley Dec 16, 2024

janardhanvissa Dec 17, 2024

dfawley Dec 16, 2024

cleanup: replace dial with newclient #7920

cleanup: replace dial with newclient #7920

Conversation

janardhanvissa commented Dec 11, 2024 • edited Loading

arjan-bal commented Dec 11, 2024

janardhanvissa commented Dec 11, 2024

arjan-bal commented Dec 13, 2024 • edited Loading

codecov bot commented Dec 15, 2024 • edited Loading

Codecov Report

janardhanvissa commented Dec 15, 2024

arjan-bal Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

dfawley Dec 16, 2024

Choose a reason for hiding this comment

arjan-bal Dec 17, 2024

Choose a reason for hiding this comment

janardhanvissa Dec 17, 2024

Choose a reason for hiding this comment

dfawley Dec 16, 2024

Choose a reason for hiding this comment

janardhanvissa Dec 17, 2024

Choose a reason for hiding this comment

dfawley Dec 16, 2024

Choose a reason for hiding this comment

janardhanvissa commented Dec 11, 2024 •

edited

Loading

arjan-bal commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 15, 2024 •

edited

Loading

arjan-bal Dec 16, 2024 •

edited

Loading