ringhash: port e2e tests from c-core

[atollena]

Fixes #6072.

RELEASE NOTES: none

[easwars]

I think the overall approach looks good. So, I will wait for you to move it out of draft before I give any further comments. Thanks.

[atollena]

I implemented 8 out of the 23 tests in https://github.com/grpc/grpc/blob/master/test/cpp/end2end/xds/xds_ring_hash_end2end_test.cc.

I'd suggest we try to get those in and add more tests in a separate commit.

Regarding potential flakyness: I ran those tests with -count 1000 which didn't yield any failure.

[easwars]

I see that this is still assigned to you. Please assign it to me when you think it is ready for me to take a pass. Thanks.

[atollena]

I see that this is still assigned to you. Please assign it to me when you think it is ready for me to take a pass. Thanks.

This is ready for review.

[easwars]

Initial pass.

Many of these comments inside of the test apply to multiple tests instead of just the one where I've made the comment.

Thanks.

[easwars]

Initial pass.

Many of these comments inside of the test apply to multiple tests instead of just the one where I've made the comment.

Thanks.

[atollena]

Looks like one of the tests added in this PR is flaky here: https://github.com/grpc/grpc-go/actions/runs/9398809643/job/25884944530?pr=7271.

I looked into this, and I think there is a race which is not obvious.
Here is what may be happening:

1. We create a stubserver and close it immediately, to simulate a server that is unreachable
2. The stub server calls Serve in a separate goroutine spawned at

   grpc-go/internal/stubserver/stubserver.go

   Line 181 in e40eb2e

   go ss.S.Serve(lis)

   -- if this goroutine runs immediately, then the gRPC server is started properly. Calling Stop() on it will make sure the listener is closed synchronously at

   grpc-go/server.go

   Lines 1886 to 1892 in e40eb2e

   	func (s *Server) stop(graceful bool) {
   	s.quit.Fire()
   	defer s.done.Fire()
   	s.channelzRemoveOnce.Do(func() { channelz.RemoveEntry(s.channelz.ID) })
   	s.mu.Lock()
   	s.closeListenersLocked()

   .
3. However, if server.Stop() is called before server.Serve() is scheduled, then the listener is not yet registered and the listener is not closed. This is what happened in this test run, and in my tests in most cases if you do:

lis, err := net.Listen
server := grpc.NewServer()
go server.Serve(lis)
server.Stop()

then Stop will run before Serve.
Even when this happens, in most cases the listener is then closed by Serve (behavior implemented at

grpc-go/server.go

Lines 843 to 848 in e40eb2e

	if s.lis == nil {
	// Serve called after Stop or GracefulStop.
	s.mu.Unlock()
	lis.Close()
	return ErrServerStopped
	}

).

However, since that happens in a separate goroutine, there's no guarantee that closing the listener will run before the first connections, later on in a test.

I think the best way to work around this is to directly create listeners instead of creating stubservers. This avoids the asynchronous close behavior described above, while still reserving an available port and making sure it's not used by another server. I did that in the last changes.

[easwars]

Thanks for taking care of all the comments.

[easwars]

Ah, sorry, should have caught this much earlier. Could we please use consts here, and in other tests as well. Thanks.

[atollena]

I fixed that in the last commit.

[easwars]

LGTM

Yay!!!

[arvindbr8]

Thanks @atollena for taking care of this!