Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion internal/xds/clients/xdsclient/authority.go
Original file line number Diff line number Diff line change
Expand Up @@ -376,8 +376,20 @@ func (a *authority) handleADSResourceUpdate(serverConfig *ServerConfig, rType Re
ServerURI: serverConfig.ServerIdentifier.ServerURI, ResourceType: rType.TypeName,
})
}
state.md.ErrState = md.ErrState
isDuplicateErr := state.md.ErrState != nil && state.md.ErrState.Err != nil && state.md.ErrState.Err.Error() == uErr.Err.Error()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also instead of evaluating this as a variable and then checking can we instead do the assignment first(which I assume needs to be done for either case) and then directly in an if block we can evaluate the condition and call continue on true. WDYT ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state works as an cache of the previously received error for the resource, so trying to find out if there was an error duplication after updating state would always return true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! but we can do the check first and if true call continue, because if we are getting the same error, we might not need to update the state? Or can the status be different ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stored metadata ErrState may differ, this is why we need to update it even if a duplicate error for a resource is detected. This is also the reason why the duplicate check has to be done before updating it. Please, look at the comment with an example that I made: #9185 (comment)

var errState *xdsresource.UpdateErrorMetadata
if md.ErrState != nil {
errState = &xdsresource.UpdateErrorMetadata{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to do this ? Can't we directly do state.md.ErrState = md.ErrState like earlier ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

md.ErrState contains concatenated errors for all resources in the received update, so it's impossible to detect an error duplication right, if there was more than one error. It might be worth it to keep uErr.Err (which is a resource specific error) as a separate member in the state to avoid confusion? WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think that is the right way to go. We should not change the error stored. We can instead compare the error in state with the ms error like :
isDuplicateErr := state.md.ErrState != nil && state.md.ErrState.Err != nil && state.md.ErrState.Err.Error() == md.ErrState.Err.Error()
WDYT ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot compare state.md.ErrState.Err.Error() == md.ErrState.Err.Error() in order to detect a duplication, e.g. Let's assume that we got an update for three resources, where two of them got errors:

  • Resource A (Ok)
  • Resource B (Error B)
  • Resource C (Error C)

md.ErrState.Err.Error() returns a concatenated error "Error B; Error C". This concatenated error is stored in state.md.ErrState.

Then the next update brings a duplicate error for the Resource C only, because Resource B is fine, we did not get an update for that resource or the error is different:

  • Resource A (Ok)
  • Resource B (Ok)
  • Resource C (Error C)

md.ErrState.Err.Error() returns a concatenated error "Error C", which is different from the previously stored one "Error B; Error C". Even though the duplication is there a simple comparison will not work.

As an alternative I can try to check if the concatenated error in the md.ErrState contains a resource specific error to seek for a duplication.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the logic to look for a duplicate resource error in the concatenated error (md.ErrState)

Version: md.ErrState.Version,
Err: uErr.Err,
Timestamp: md.ErrState.Timestamp,
}
}
Comment thread
eshitachandwani marked this conversation as resolved.
Outdated
state.md.ErrState = errState
state.md.Status = md.Status
if isDuplicateErr {
continue
}
for watcher := range state.watchers {
watcher := watcher
err := uErr.Err
Expand Down
97 changes: 97 additions & 0 deletions internal/xds/xdsclient/tests/eds_watchers_test.go
Comment thread
eshitachandwani marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"encoding/json"
"fmt"
"strings"
"sync/atomic"
"testing"
"time"

Expand All @@ -36,6 +37,7 @@ import (
"google.golang.org/grpc/internal/xds/clients"
"google.golang.org/grpc/internal/xds/xdsclient"
"google.golang.org/grpc/internal/xds/xdsclient/xdsresource"
"google.golang.org/grpc/internal/xds/xdsclient/xdsresource/version"
"google.golang.org/grpc/resolver"
"google.golang.org/protobuf/types/known/wrapperspb"

Expand Down Expand Up @@ -1043,3 +1045,98 @@ func (s) TestEDSWatch_PartialValid(t *testing.T) {
t.Fatal(err)
}
}

// TestEDSWatch_NACKError_DuplicateSuppression verifies that when a duplicate
// invalid EDS resource is sent by the server, the client suppresses the
// duplicate error notification to the watcher.
func (s) TestEDSWatch_NACKError_DuplicateSuppression(t *testing.T) {
nackReceivedCh := testutils.NewChannel()
var nackCount atomic.Int32
mgmtServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{
OnStreamRequest: func(_ int64, req *v3discoverypb.DiscoveryRequest) error {
if req.GetTypeUrl() != version.V3EndpointsURL {
return nil
}
if req.GetErrorDetail() != nil {
if count := nackCount.Add(1); count >= 3 {
nackReceivedCh.Send(nil)
}
}
return nil
},
})

nodeID := uuid.New().String()
bc := e2e.DefaultBootstrapContents(t, nodeID, mgmtServer.Address)

config, err := bootstrap.NewConfigFromContents(bc)
if err != nil {
t.Fatalf("Failed to parse bootstrap contents: %s, %v", string(bc), err)
}
pool := xdsclient.NewPool(config)
client, close, err := pool.NewClientForTesting(xdsclient.OptionsForTesting{
Name: t.Name(),
})
if err != nil {
t.Fatalf("Failed to create xDS client: %v", err)
}
defer close()

// Use a custom watcher that counts invocations.
errCh := testutils.NewChannel()
watcher := &countingEndpointsWatcher{errCh: errCh}
edsCancel := xdsresource.WatchEndpoints(client, edsName, watcher)
defer edsCancel()

resources := e2e.UpdateOptions{
NodeID: nodeID,
Endpoints: []*v3endpointpb.ClusterLoadAssignment{badEndpointsResource(edsName, edsHost1, []uint32{edsPort1})},
SkipValidation: true,
}
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
if err := mgmtServer.Update(ctx, resources); err != nil {
t.Fatalf("Failed to update management server with resources: %v, err: %v", resources, err)
}

// Verify that the watcher receives the initial error.
v, err := errCh.Receive(ctx)
if err != nil {
t.Fatalf("timeout when waiting for an endpoints resource error from the management server: %v", err)
}
gotErr := v.(error)
if !strings.Contains(gotErr.Error(), wantEndpointsNACKErr) {
t.Fatalf("update received with error: %v, want %q", gotErr, wantEndpointsNACKErr)
}

// Wait for the management server to receive at least 3 NACKs, indicating that
// the NACK resend loop has run multiple times.
if _, err := nackReceivedCh.Receive(ctx); err != nil {
t.Fatalf("timeout waiting for 3 NACKs to be received by the server: %v", err)
}

// Verify that the count of error callbacks is still exactly 1, proving that
// subsequent duplicate error updates were suppressed.
if count := watcher.errCount.Load(); count != 1 {
t.Fatalf("Error callback invoked %d times, want 1", count)
}
}

type countingEndpointsWatcher struct {
errCh *testutils.Channel
errCount atomic.Int32
}

func (*countingEndpointsWatcher) ResourceChanged(_ *xdsresource.EndpointsUpdate, onDone func()) {
onDone()
}
func (c *countingEndpointsWatcher) ResourceError(err error, onDone func()) {
c.errCount.Add(1)
c.errCh.Send(err)
onDone()
}
func (c *countingEndpointsWatcher) AmbientError(err error, onDone func()) {
c.errCount.Add(1)
c.errCh.Send(err)
onDone()
}
Loading