xds/xdsclient: ignore resource deletion as per gRFC A53 #6035
Conversation
As per gRFC A53, the gRPC xDSClient can be configured to ignore resource deletions sent by the xDS server. [A53](https://github.com/grpc/proposal/blob/master/A53-xds-ignore-resource-deletion.md)
arvindbr8
force-pushed
the
ignore-resource-deletion-real
branch
from
26eb7da
to
9b2ddae
Compare
|
|
||
| var ( | ||
| resolverBuilder = internal.NewXDSResolverWithConfigForTesting.(func([]byte) (resolver.Builder, error)) | ||
| nodeID = uuid.New().String() |
There was a problem hiding this comment.
I'm a little concerned about using the same nodeID across tests. Would prefer if we can keep the tests as hermetic as possible.
There was a problem hiding this comment.
The reason for this being global is to avoid passing it along to all the helper function and since this was a string constant. I think maybe i should have not used many nested helpers then.
Do you think this is a problem now?
There was a problem hiding this comment.
I've run into problems in the past when the same nodeID was reused across tests, but maybe at that point we were also using a single management server instance across tests. But since this is simple cleanup of declaring the nodeID inside the test and passing it whatever helper needs it, I think we should do it. Because if it ever leads to some test flakes, it will be hard to debug.
There was a problem hiding this comment.
Ill update this one in a bit
| return bootstrapContents | ||
| } | ||
|
|
||
| // This helper generates a custom bootstrap config for the test. |
There was a problem hiding this comment.
Comment needs to be updated.
There was a problem hiding this comment.
🤦 sorry.. i missed this. updating
| // This test creates a xds-enabled gRPC server with a listener resource and this | ||
| // server features "ignore_resource_deletion". The test then verifies successful | ||
| // RPCs to server. The test then removes the listener resource from the server | ||
| // and verifies that server continues to server requests. |
There was a problem hiding this comment.
In this test-level comment and the next, you need to disambiguate between the xDS-enabled gRPC server and the management server. When you say this server features "ignore_resource_deletion", this is actually a property of the management server. And there are too many servers in this line The test then removes the listener resource from the server and verifies that server continues to server requests.
There was a problem hiding this comment.
sorry lol. I see what has happened here. updated.
| } | ||
| t.Cleanup(func() { lis.Close() }) | ||
|
|
||
| // Setup the management server to respond with the listener resources. |
As per gRFC A53, the gRPC xDSClient can be configured to ignore resource deletions sent by the xDS server. A53 This diff adds that feature to the xDS client. Also adds units to test this out.
RELEASE NOTES: