test/xds: fail only when state changes to something other than READY and IDLE#5463
Conversation
zasweq
left a comment
zasweq
left a comment
There was a problem hiding this comment.
LGTM. Kind of fun to try and break this but I couldn't.
| return | ||
| } | ||
| if !cc.WaitForStateChange(ctx, curr) { | ||
| // Break out of the for loop when the context has been cancelled. |
There was a problem hiding this comment.
Nit: is this comment needed?
| return | ||
| prev := connectivity.Ready // We know we are READY since we just did an RPC. | ||
| for { | ||
| curr := cc.GetState() |
There was a problem hiding this comment.
qq: what happens if the context gets cancelled when you are here. Will the WaitForStateChange call later still return false? It reads the ctx.Done() channel which has already been closed, will this Done channel still send a zero value that signifies it's been closed?
There was a problem hiding this comment.
"Successive calls to Done return the same value."
"A receive operation on a closed channel can always proceed immediately, yielding the element type's zero value after any previously sent values have been received."
I think these statements from godoc mean the answer to my question is yes, but just making sure.
|
Why did you assign me? You can go ahead and merge this. |
2 participants
TestServerSideXDS_RedundantUpdateSuppressionsends a redundant LDS update to an xDS enabled grpc-server, which is currently inSERVINGmode and ensures that client connections are not recycled. To accomplish this, it was blocking on connectivity state changes on theClientConn, and was failing the test if theClientConnmoved away fromREADY. See:grpc-go/test/xds/xds_server_serving_mode_test.go
Line 115 in 5cdb09f
The last few flakes have been because the
ClientConnis transitioning toIDLE. This is expected behavior since we don't have an ongoing stream of RPCs from the test. Therefore we should not fail the test for this.This PR fixes the logic used to check connectivity state on the
ClientConn, and does not fail the test if theClientConnmoves toIDLE.Fixes #4974
RELEASE NOTES: none