xdsclient: fixed a bug to not report connectivity error from new stream recreation if a response was received before

[purnesh42H]

Fixes: #8351

As per gRFC 57 we do not consider it an error if the ADS stream was closed after having received a response on the stream. This should hold true even if we get an error during first attempt of recreation of the stream.

Successful forge test

RELEASE NOTES:

• xdsclient: fixed a bug to not report connectivity error from new stream recreation if a response was received before

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.15%. Comparing base (ec4810c) to head (feb42ad).
Report is 13 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8373      +/-   ##
==========================================
- Coverage   82.22%   82.15%   -0.08%     
==========================================
  Files         419      419              
  Lines       42047    42066      +19     
==========================================
- Hits        34574    34560      -14     
- Misses       6003     6035      +32     
- Partials     1470     1471       +1     

Files with missing lines	Coverage Δ
xds/internal/clients/xdsclient/ads_stream.go	80.08% <100.00%> (-0.14%)	⬇️
xds/internal/xdsclient/transport/ads/ads_stream.go	83.87% <100.00%> (+0.24%)	⬆️

... and 21 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[purnesh42H]

Looks like its failing some fallack tests. Will check them. Unassigning the reviewers until then.

[purnesh42H]

Looks like its failing some fallack tests. Will check them. Unassigning the reviewers until then.

Fixed by resetting after first attempt of recreation

[arjan-bal]

Can you please make the following fixes in the test logic and re-verify on forge?

• Validate that we get a second update metric emitted.
• Use a short context and verify that we don't receive a server failure metric.

grpc-go/xds/internal/xdsclient/metrics_test.go

Lines 320 to 329 in ec4810c

	mdWant = stats.MetricsData{
	Handle: xdsClientServerFailureMetric.Descriptor(),
	IntIncr: 1,
	LabelKeys: []string{"grpc.target", "grpc.xds.server"},
	LabelVals: []string{"Test/ServerFailureMetrics_AfterResponseRecv", mgmtServer.Address},
	}
	// Server failure should still have no recording point.
	if err := tmr.WaitForInt64Count(ctx, mdWant); err == nil {
	t.Fatal("tmr.WaitForInt64Count(ctx, mdWant) succeeded when expected to timeout.")
	}

[purnesh42H]

Can you please make the following fixes in the test logic and re-verify on forge?

• Validate that we get a second update metric emitted.
• Use a short context and verify that we don't receive a server failure metric.

grpc-go/xds/internal/xdsclient/metrics_test.go

Lines 320 to 329 in ec4810c

	mdWant = stats.MetricsData{
	Handle: xdsClientServerFailureMetric.Descriptor(),
	IntIncr: 1,
	LabelKeys: []string{"grpc.target", "grpc.xds.server"},
	LabelVals: []string{"Test/ServerFailureMetrics_AfterResponseRecv", mgmtServer.Address},
	}
	// Server failure should still have no recording point.
	if err := tmr.WaitForInt64Count(ctx, mdWant); err == nil {
	t.Fatal("tmr.WaitForInt64Count(ctx, mdWant) succeeded when expected to timeout.")
	}

Validate that we get a second update metric emitted.

I don't think we can deterministically verify that with the current setup of test metrics recorder as only one metric is kept at a time. We already have tests that verify the behavior of re-requesting resources in case of stream restarts. I have removed the restart operation in metrics test so that we don't have possibility of re-request and resource update metrics.

• Use a short context and verify that we don't receive a server failure metric.

Done

[purnesh42H]

Looks like its failing some fallack tests. Will check them. Unassigning the reviewers until then.

Fixed by resetting messageReceived to false after first attempt of recreation because subsequent attempts should backoff as usual.

[easwars]

Made a comment on the bug. In my opinion, the test should be changed instead of the ADS stream implementation.

[arjan-bal]

Raised a PR with the test changes: #8399.