xds/resolver_test: fix flaky test ResolverBadServiceUpdate_NACKedWithoutCache

[hugehoo]

Fixes: #8435

root cause of issue:

• I think there was a race condition when channel communicates between the xDS resolver and test infrastructure
  • insufficient buffer size: original channels (stateCh and errCh) had only buffer size of 1
  • blocking sends: When buffer is full, the resolver would block trying to send the next update
  • test deadlock: test infra might be waiting for a specific update while the resolver was blocked trying to send a different update, creating a deadlock

Changes

1. Increased buffer size (1 → 10):

  stateCh := make(chan resolver.State, 10)
  errCh := make(chan error, 10)

2. Non-blocking send pattern:

 select {
 case stateCh <- s:  // the resolver try to send updates
 default:            // If channel is full, drain old message and retry
     select {
     case <-stateCh:
         stateCh <- s
     default:
     }
 }

• make it drain old messages preventing the resolver from blocking and just keeping the most latest updates.

3. Cleanup with draining goroutines:

  go func() {
      for range stateCh { }  // Drain any remaining messages
  }()

• it ensures the resolver never blocks on sends and prevents goroutine leaks during test cleanup.

RELEASE NOTES: N/A

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.70%. Comparing base (8420f3f) to head (657c28a).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8521      +/-   ##
==========================================
- Coverage   80.91%   80.70%   -0.21%     
==========================================
  Files         413      413              
  Lines       40751    40773      +22     
==========================================
- Hits        32972    32907      -65     
- Misses       6155     6224      +69     
- Partials     1624     1642      +18     

see 17 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[arjan-bal]

Hi @hugehoo I have a few questions/requests to help me reviewing this fix:

1. Can you describe the root cause in the linked issue?
2. Can you also explain the fix in the PR description?
3. Were you able to repro the flakiness? If yes, can you mention the go test command that was used?

[hugehoo]

@arjan-bal i updated PR comment as you mentioned for 1, 2. but can't reproduce the flakiness yet.

[arjan-bal]

I was able to repro the failure by running the test around 1000 times and adding logs. The problem is indeed a write to the stateCh blocking indefinitely.

grpc-go/internal/xds/resolver/helpers_test.go

Lines 122 to 125 in 6524c7b

	updateStateF := func(s resolver.State) error {
	stateCh <- s
	return nil
	}

Once the xDS client receives an invalid LDS resource, it informs the resolver. This causes 1 write to the errCh and stateCh as the xDS resolver sends an empty service config before reporting an error here:

grpc-go/internal/xds/resolver/xds_resolver.go

Lines 294 to 308 in 6524c7b

	if ok && len(r.activeClusters) == 0 {
	// There are no clusters and we are sending a failing configSelector.
	// Send an empty config, which picks pick-first, with no address, and
	// puts the ClientConn into transient failure.
	//
	// This call to UpdateState is expected to return ErrBadResolverState
	// since pick_first doesn't like an update with no addresses.
	r.cc.UpdateState(resolver.State{ServiceConfig: r.cc.ParseServiceConfig("{}")})
	// Send a resolver error to pick_first so that RPCs will fail with a
	// more meaningful error, as opposed to one that says that pick_first
	// received no addresses.
	r.cc.ReportError(errCS.err)
	return true
	}

Then the xDS client sends a NACK for the invalid LDS resource and the xDS management server re-sends the same resource again, causing a second write to both the channels. The write to the stateCh blocks the xds resolver forever.

I think increasing the buffer capacity will just make the deadlock more unlikely, but not impossible. Dropping state updates if the buffered channel is full may cause races if a test wants to verify every state update. I'm trying to think of better ways to resolve this, potentially by allowing the test functions to pass callbacks for handling the error and state updates, instead of using a channel to pass them.

[arjan-bal]

There are 19 usages of buildResolverForTarget which may need to be updated.

grpc-go/internal/xds/resolver/helpers_test.go

Line 100 in 6524c7b

func buildResolverForTarget(t *testing.T, target resolver.Target, bootstrapContents []byte) (chan resolver.State, chan error, resolver.Resolver) {

Solution 1 (Simplest)

Don't use buildResolverForTarget in the flaking test. Duplicate its internals in the flaking test and get rid of the stateCh. This would result in around 25 lines of duplicated code.

Solution 2

We could change buildResolverForTarget to accept callbacks for error and state updates, making the signature similar to the following:

type resolverOpts struct {
	target            resolver.Target
	bootstrapContents []byte
	onError           func()
	onResolverState   func()
}

func buildResolverForTarget(t *testing.T, opts resolverOpts) resolver.Resolver

This would allow callers to get the previous behaviour by calling the function similar to the following:

stateCh := make(chan resolver.State, 1)
updateStateF := func(s resolver.State) error {
    stateCh <- s
    return nil
}

errCh := make(chan error, 1)
reportErrorF := func(err error) {
    select {
    case errCh <- err:
    default:
    }
}

opts := resolverOpts{
    target:            target,
    bootstrapContents: bootstrapContents,
    onError:           reportErrorF,
    onResolverState:   updateStateF,
}
r := buildResolverForTarget(t, opts)

This would also allow callers to ignore state updates if they want.

Solution 3

To avoid updating 19 tests and adding redundant code, we can make changes in option 2, but make a new function with this signature, say buildResolverForTargetWithOpts. Have the existing buildResolverForTarget create the channels and delegate to the new function. This way the existing callers don't need to be changed and only the flaking test can call buildResolverForTargetWithOpts directly.

I'm leaning towards option 1, @easwars wanted to get a second opinion on this.

[easwars]

Scenarios that involve NACKs aren't great with the go-control-plane management server because it will continuously keep re-sending the same resource that was NACKed. This means that if we are writing the resource/update/error to channel based on information from these responses, this will always lead to blocked writes on them.

I'm fine with option 1, since it seems to be the least invasive.

Another thing that could be added is the use of the testutils.Channel type instead of a vanilla channel. The former provides an API to replace the most recent value written to the channel, and we use this approach in some tests that expect NACKs from the management server. See: https://github.com/grpc/grpc-go/blob/master/internal/testutils/channel.go#L100

[arjan-bal]

Hi @hugehoo are you still working on this fix? Based on the last two comments above, we can avoid using the buildResolverForTarget and copy its internals to remove the stateCh.

[hugehoo]

Hi @hugehoo are you still working on this fix? Based on the last two comments above, we can avoid using the buildResolverForTarget and copy its internals to remove the stateCh.

yes, i will update as to copy the buildResolverForTarget logic for the flaky tests.

[hugehoo]

@arjan-bal I updated flaky tests following option1.

• Implemented buildResolverForTarget method logic inside TestResolverBadServiceUpdate_NACKedWithoutCache.

• Instead of calling waitForErrorFromResolver as in buildResolverForTarget, i also implemented waitForErrorFromResolver logic directly because of type incompatiblity.
  Because waitForErrorFromResolverexpects chan error, but testutils.Channel has .C filed of type chan any.

	if err := waitForErrorFromResolver(ctx, errCh, "no RouteSpecifier", nodeID); err != nil {

[arjan-bal]

Looks mostly good, left a couple of minor comments.

[arjan-bal]

LGTM, thanks for the fix!

[easwars]

LGTM