Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

xds/client: cleanup Dump to remove unnecessary version field #4978

Merged
merged 2 commits into from
Nov 16, 2021
Merged

xds/client: cleanup Dump to remove unnecessary version field #4978

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
080101e
[xdsclient_dump_cleanup] [xds_client_federation_multi_controller_to_s…
menghanl Nov 12, 2021
bca22cb
[xdsclient_dump_cleanup] move back to _test
menghanl Nov 15, 2021
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
4 changes: 2 additions & 2 deletions xds/csds/csds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,8 @@ func nodeProtoToV3(n proto.Message) *v3corepb.Node {
return node
}

func dumpToGenericXdsConfig(typeURL string, dumpF func() (string, map[string]xdsresource.UpdateWithMD)) []*v3statuspb.ClientConfig_GenericXdsConfig {
_, dump := dumpF()
func dumpToGenericXdsConfig(typeURL string, dumpF func() map[string]xdsresource.UpdateWithMD) []*v3statuspb.ClientConfig_GenericXdsConfig {
dump := dumpF()
ret := make([]*v3statuspb.ClientConfig_GenericXdsConfig, 0, len(dump))
for name, d := range dump {
config := &v3statuspb.ClientConfig_GenericXdsConfig{
Expand Down
8 changes: 4 additions & 4 deletions xds/internal/xdsclient/attributes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,10 @@ type XDSClient interface {
WatchEndpoints(clusterName string, edsCb func(xdsresource.EndpointsUpdate, error)) (cancel func())
ReportLoad(server string) (*load.Store, func())

DumpLDS() (string, map[string]xdsresource.UpdateWithMD)
DumpRDS() (string, map[string]xdsresource.UpdateWithMD)
DumpCDS() (string, map[string]xdsresource.UpdateWithMD)
DumpEDS() (string, map[string]xdsresource.UpdateWithMD)
DumpLDS() map[string]xdsresource.UpdateWithMD
DumpRDS() map[string]xdsresource.UpdateWithMD
DumpCDS() map[string]xdsresource.UpdateWithMD
DumpEDS() map[string]xdsresource.UpdateWithMD

BootstrapConfig() *bootstrap.Config
Close()
Expand Down
8 changes: 4 additions & 4 deletions xds/internal/xdsclient/dump.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,21 @@ import (
)

// DumpLDS returns the status and contents of LDS.
func (c *clientImpl) DumpLDS() (string, map[string]xdsresource.UpdateWithMD) {
func (c *clientImpl) DumpLDS() map[string]xdsresource.UpdateWithMD {
return c.pubsub.Dump(xdsresource.ListenerResource)
}

// DumpRDS returns the status and contents of RDS.
func (c *clientImpl) DumpRDS() (string, map[string]xdsresource.UpdateWithMD) {
func (c *clientImpl) DumpRDS() map[string]xdsresource.UpdateWithMD {
return c.pubsub.Dump(xdsresource.RouteConfigResource)
}

// DumpCDS returns the status and contents of CDS.
func (c *clientImpl) DumpCDS() (string, map[string]xdsresource.UpdateWithMD) {
func (c *clientImpl) DumpCDS() map[string]xdsresource.UpdateWithMD {
return c.pubsub.Dump(xdsresource.ClusterResource)
}

// DumpEDS returns the status and contents of EDS.
func (c *clientImpl) DumpEDS() (string, map[string]xdsresource.UpdateWithMD) {
func (c *clientImpl) DumpEDS() map[string]xdsresource.UpdateWithMD {
return c.pubsub.Dump(xdsresource.EndpointsResource)
}
52 changes: 23 additions & 29 deletions xds/internal/xdsclient/dump_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*
*/

package xdsclient_test
package xdsclient
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleanup.
I believe it was put in a different package because of a circular dependency. That's resolved by the xdsclient refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, and, this will need to access some unexported testing helper functions, in a future change.

Copy link
Member

Choose a reason for hiding this comment

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

I believe it was put in a different package because of a circular dependency

Maybe so, but IMO all our tests should be in a separate package unless they are unit tests of unexported functions or need to access internals of the package being tested (which we should be trying to minimize). This forces the tests to be written at the API layer, which is more representative of actual usage.

Can we leave this as xdsclient_test, and add the things that access unexported fields to a different test file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all our tests should be in a separate package

I think this is an overkill. There's a trade-off between unit tests and e2e tests. The complexity added by this requirement is not worth the effort.

Accessing unexported fields doesn't mean to check the field values. It can be to use the unexport update handler to send an update.

For example, in xdsclient, the way to receive an update is to have a real control plane, and use that to send an xDS resp. (There are currently exported functions on the xdsclient to trigger updates, but I think that's not in the right place, and will be unexported)
Requiring a control plane for all the watcher tests is too much work. Or you are arguing that we should move most (all?) the xdsclient tests to e2e.

I changed this back to _test. It's not required in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is an overkill.

It's overkill to say it must be done that way, but not that we should try to do it that way as much as is reasonable.

Obviously if you need to mock out something to test it more easily (e.g. time things), then you'll put your tests in the same package.

you are arguing that we should move most (all?) the xdsclient tests to e2e.

I didn't mean all our tests need to be at the gRPC API level, but at the API level of the package being tested. A fake management server might be too difficult to be worth doing (maybe?), but we definitely don't want every test of "update XYZ happened, what did the xdsclient see?" to be done all the way up at the E2E/application level.


import (
"fmt"
Expand All @@ -40,12 +40,9 @@ import (
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/internal/testutils"
xdstestutils "google.golang.org/grpc/xds/internal/testutils"
"google.golang.org/grpc/xds/internal/xdsclient"
"google.golang.org/grpc/xds/internal/xdsclient/bootstrap"
)

const defaultTestWatchExpiryTimeout = 500 * time.Millisecond

func (s) TestLDSConfigDump(t *testing.T) {
const testVersion = "test-version-lds"
var (
Expand Down Expand Up @@ -76,7 +73,7 @@ func (s) TestLDSConfigDump(t *testing.T) {
listenerRaws[ldsTargets[i]] = testutils.MarshalAny(listenersT)
}

client, err := xdsclient.NewWithConfigForTesting(&bootstrap.Config{
client, err := NewWithConfigForTesting(&bootstrap.Config{
XDSServer: &bootstrap.ServerConfig{
ServerURI: testXDSServer,
Creds: grpc.WithTransportCredentials(insecure.NewCredentials()),
Expand All @@ -90,7 +87,7 @@ func (s) TestLDSConfigDump(t *testing.T) {
updateHandler := client.(pubsub.UpdateHandler)

// Expected unknown.
if err := compareDump(client.DumpLDS, "", map[string]xdsresource.UpdateWithMD{}); err != nil {
if err := compareDump(client.DumpLDS, map[string]xdsresource.UpdateWithMD{}); err != nil {
t.Fatalf(err.Error())
}

Expand All @@ -101,7 +98,7 @@ func (s) TestLDSConfigDump(t *testing.T) {
wantRequested[n] = xdsresource.UpdateWithMD{MD: xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusRequested}}
}
// Expected requested.
if err := compareDump(client.DumpLDS, "", wantRequested); err != nil {
if err := compareDump(client.DumpLDS, wantRequested); err != nil {
t.Fatalf(err.Error())
}

Expand All @@ -117,7 +114,7 @@ func (s) TestLDSConfigDump(t *testing.T) {
updateHandler.NewListeners(update0, xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusACKed, Version: testVersion})

// Expect ACK.
if err := compareDump(client.DumpLDS, testVersion, want0); err != nil {
if err := compareDump(client.DumpLDS, want0); err != nil {
t.Fatalf(err.Error())
}

Expand Down Expand Up @@ -157,7 +154,7 @@ func (s) TestLDSConfigDump(t *testing.T) {
MD: xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusACKed, Version: nackVersion},
Raw: listenerRaws[ldsTargets[1]],
}
if err := compareDump(client.DumpLDS, nackVersion, wantDump); err != nil {
if err := compareDump(client.DumpLDS, wantDump); err != nil {
t.Fatalf(err.Error())
}
}
Expand Down Expand Up @@ -192,7 +189,7 @@ func (s) TestRDSConfigDump(t *testing.T) {
routeRaws[rdsTargets[i]] = testutils.MarshalAny(routeConfigT)
}

client, err := xdsclient.NewWithConfigForTesting(&bootstrap.Config{
client, err := NewWithConfigForTesting(&bootstrap.Config{
XDSServer: &bootstrap.ServerConfig{
ServerURI: testXDSServer,
Creds: grpc.WithTransportCredentials(insecure.NewCredentials()),
Expand All @@ -206,7 +203,7 @@ func (s) TestRDSConfigDump(t *testing.T) {
updateHandler := client.(pubsub.UpdateHandler)

// Expected unknown.
if err := compareDump(client.DumpRDS, "", map[string]xdsresource.UpdateWithMD{}); err != nil {
if err := compareDump(client.DumpRDS, map[string]xdsresource.UpdateWithMD{}); err != nil {
t.Fatalf(err.Error())
}

Expand All @@ -217,7 +214,7 @@ func (s) TestRDSConfigDump(t *testing.T) {
wantRequested[n] = xdsresource.UpdateWithMD{MD: xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusRequested}}
}
// Expected requested.
if err := compareDump(client.DumpRDS, "", wantRequested); err != nil {
if err := compareDump(client.DumpRDS, wantRequested); err != nil {
t.Fatalf(err.Error())
}

Expand All @@ -233,7 +230,7 @@ func (s) TestRDSConfigDump(t *testing.T) {
updateHandler.NewRouteConfigs(update0, xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusACKed, Version: testVersion})

// Expect ACK.
if err := compareDump(client.DumpRDS, testVersion, want0); err != nil {
if err := compareDump(client.DumpRDS, want0); err != nil {
t.Fatalf(err.Error())
}

Expand Down Expand Up @@ -272,7 +269,7 @@ func (s) TestRDSConfigDump(t *testing.T) {
MD: xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusACKed, Version: nackVersion},
Raw: routeRaws[rdsTargets[1]],
}
if err := compareDump(client.DumpRDS, nackVersion, wantDump); err != nil {
if err := compareDump(client.DumpRDS, wantDump); err != nil {
t.Fatalf(err.Error())
}
}
Expand Down Expand Up @@ -308,7 +305,7 @@ func (s) TestCDSConfigDump(t *testing.T) {
clusterRaws[cdsTargets[i]] = testutils.MarshalAny(clusterT)
}

client, err := xdsclient.NewWithConfigForTesting(&bootstrap.Config{
client, err := NewWithConfigForTesting(&bootstrap.Config{
XDSServer: &bootstrap.ServerConfig{
ServerURI: testXDSServer,
Creds: grpc.WithTransportCredentials(insecure.NewCredentials()),
Expand All @@ -322,7 +319,7 @@ func (s) TestCDSConfigDump(t *testing.T) {
updateHandler := client.(pubsub.UpdateHandler)

// Expected unknown.
if err := compareDump(client.DumpCDS, "", map[string]xdsresource.UpdateWithMD{}); err != nil {
if err := compareDump(client.DumpCDS, map[string]xdsresource.UpdateWithMD{}); err != nil {
t.Fatalf(err.Error())
}

Expand All @@ -333,7 +330,7 @@ func (s) TestCDSConfigDump(t *testing.T) {
wantRequested[n] = xdsresource.UpdateWithMD{MD: xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusRequested}}
}
// Expected requested.
if err := compareDump(client.DumpCDS, "", wantRequested); err != nil {
if err := compareDump(client.DumpCDS, wantRequested); err != nil {
t.Fatalf(err.Error())
}

Expand All @@ -349,7 +346,7 @@ func (s) TestCDSConfigDump(t *testing.T) {
updateHandler.NewClusters(update0, xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusACKed, Version: testVersion})

// Expect ACK.
if err := compareDump(client.DumpCDS, testVersion, want0); err != nil {
if err := compareDump(client.DumpCDS, want0); err != nil {
t.Fatalf(err.Error())
}

Expand Down Expand Up @@ -388,7 +385,7 @@ func (s) TestCDSConfigDump(t *testing.T) {
MD: xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusACKed, Version: nackVersion},
Raw: clusterRaws[cdsTargets[1]],
}
if err := compareDump(client.DumpCDS, nackVersion, wantDump); err != nil {
if err := compareDump(client.DumpCDS, wantDump); err != nil {
t.Fatalf(err.Error())
}
}
Expand All @@ -410,7 +407,7 @@ func (s) TestEDSConfigDump(t *testing.T) {
endpointRaws[edsTargets[i]] = testutils.MarshalAny(claT)
}

client, err := xdsclient.NewWithConfigForTesting(&bootstrap.Config{
client, err := NewWithConfigForTesting(&bootstrap.Config{
XDSServer: &bootstrap.ServerConfig{
ServerURI: testXDSServer,
Creds: grpc.WithTransportCredentials(insecure.NewCredentials()),
Expand All @@ -424,7 +421,7 @@ func (s) TestEDSConfigDump(t *testing.T) {
updateHandler := client.(pubsub.UpdateHandler)

// Expected unknown.
if err := compareDump(client.DumpEDS, "", map[string]xdsresource.UpdateWithMD{}); err != nil {
if err := compareDump(client.DumpEDS, map[string]xdsresource.UpdateWithMD{}); err != nil {
t.Fatalf(err.Error())
}

Expand All @@ -435,7 +432,7 @@ func (s) TestEDSConfigDump(t *testing.T) {
wantRequested[n] = xdsresource.UpdateWithMD{MD: xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusRequested}}
}
// Expected requested.
if err := compareDump(client.DumpEDS, "", wantRequested); err != nil {
if err := compareDump(client.DumpEDS, wantRequested); err != nil {
t.Fatalf(err.Error())
}

Expand All @@ -451,7 +448,7 @@ func (s) TestEDSConfigDump(t *testing.T) {
updateHandler.NewEndpoints(update0, xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusACKed, Version: testVersion})

// Expect ACK.
if err := compareDump(client.DumpEDS, testVersion, want0); err != nil {
if err := compareDump(client.DumpEDS, want0); err != nil {
t.Fatalf(err.Error())
}

Expand Down Expand Up @@ -490,16 +487,13 @@ func (s) TestEDSConfigDump(t *testing.T) {
MD: xdsresource.UpdateMetadata{Status: xdsresource.ServiceStatusACKed, Version: nackVersion},
Raw: endpointRaws[edsTargets[1]],
}
if err := compareDump(client.DumpEDS, nackVersion, wantDump); err != nil {
if err := compareDump(client.DumpEDS, wantDump); err != nil {
t.Fatalf(err.Error())
}
}

func compareDump(dumpFunc func() (string, map[string]xdsresource.UpdateWithMD), wantVersion string, wantDump interface{}) error {
v, dump := dumpFunc()
if v != wantVersion {
return fmt.Errorf("Dump() returned version %q, want %q", v, wantVersion)
}
func compareDump(dumpFunc func() map[string]xdsresource.UpdateWithMD, wantDump interface{}) error {
dump := dumpFunc()
cmpOpts := cmp.Options{
cmpopts.EquateEmpty(),
cmp.Comparer(func(a, b time.Time) bool { return true }),
Expand Down
15 changes: 5 additions & 10 deletions xds/internal/xdsclient/pubsub/dump.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,35 +50,30 @@ func rawFromCache(s string, cache interface{}) *anypb.Any {
}

// Dump dumps the resource for the given type.
func (pb *Pubsub) Dump(t xdsresource.ResourceType) (string, map[string]xdsresource.UpdateWithMD) {
func (pb *Pubsub) Dump(t xdsresource.ResourceType) map[string]xdsresource.UpdateWithMD {
pb.mu.Lock()
defer pb.mu.Unlock()

var (
version string
md map[string]xdsresource.UpdateMetadata
cache interface{}
md map[string]xdsresource.UpdateMetadata
cache interface{}
)
switch t {
case xdsresource.ListenerResource:
version = pb.ldsVersion
md = pb.ldsMD
cache = pb.ldsCache
case xdsresource.RouteConfigResource:
version = pb.rdsVersion
md = pb.rdsMD
cache = pb.rdsCache
case xdsresource.ClusterResource:
version = pb.cdsVersion
md = pb.cdsMD
cache = pb.cdsCache
case xdsresource.EndpointsResource:
version = pb.edsVersion
md = pb.edsMD
cache = pb.edsCache
default:
pb.logger.Errorf("dumping resource of unknown type: %v", t)
return "", nil
return nil
}

ret := make(map[string]xdsresource.UpdateWithMD, len(md))
Expand All @@ -88,5 +83,5 @@ func (pb *Pubsub) Dump(t xdsresource.ResourceType) (string, map[string]xdsresour
Raw: rawFromCache(s, cache),
}
}
return version, ret
return ret
}
4 changes: 0 additions & 4 deletions xds/internal/xdsclient/pubsub/pubsub.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,15 @@ type Pubsub struct {
// All the following maps are to keep the updates/metadata in a cache.
mu sync.Mutex
ldsWatchers map[string]map[*watchInfo]bool
ldsVersion string // Only used in CSDS.
ldsCache map[string]xdsresource.ListenerUpdate
ldsMD map[string]xdsresource.UpdateMetadata
rdsWatchers map[string]map[*watchInfo]bool
rdsVersion string // Only used in CSDS.
rdsCache map[string]xdsresource.RouteConfigUpdate
rdsMD map[string]xdsresource.UpdateMetadata
cdsWatchers map[string]map[*watchInfo]bool
cdsVersion string // Only used in CSDS.
cdsCache map[string]xdsresource.ClusterUpdate
cdsMD map[string]xdsresource.UpdateMetadata
edsWatchers map[string]map[*watchInfo]bool
edsVersion string // Only used in CSDS.
edsCache map[string]xdsresource.EndpointsUpdate
edsMD map[string]xdsresource.UpdateMetadata
}
Expand Down
16 changes: 0 additions & 16 deletions xds/internal/xdsclient/pubsub/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,6 @@ func (pb *Pubsub) NewListeners(updates map[string]xdsresource.ListenerUpdateErrT
pb.mu.Lock()
defer pb.mu.Unlock()

pb.ldsVersion = metadata.Version
if metadata.ErrState != nil {
pb.ldsVersion = metadata.ErrState.Version
}
for name, uErr := range updates {
if s, ok := pb.ldsWatchers[name]; ok {
if uErr.Err != nil {
Expand Down Expand Up @@ -145,10 +141,6 @@ func (pb *Pubsub) NewRouteConfigs(updates map[string]xdsresource.RouteConfigUpda
defer pb.mu.Unlock()

// If no error received, the status is ACK.
pb.rdsVersion = metadata.Version
if metadata.ErrState != nil {
pb.rdsVersion = metadata.ErrState.Version
}
for name, uErr := range updates {
if s, ok := pb.rdsWatchers[name]; ok {
if uErr.Err != nil {
Expand Down Expand Up @@ -193,10 +185,6 @@ func (pb *Pubsub) NewClusters(updates map[string]xdsresource.ClusterUpdateErrTup
pb.mu.Lock()
defer pb.mu.Unlock()

pb.cdsVersion = metadata.Version
if metadata.ErrState != nil {
pb.cdsVersion = metadata.ErrState.Version
}
for name, uErr := range updates {
if s, ok := pb.cdsWatchers[name]; ok {
if uErr.Err != nil {
Expand Down Expand Up @@ -260,10 +248,6 @@ func (pb *Pubsub) NewEndpoints(updates map[string]xdsresource.EndpointsUpdateErr
pb.mu.Lock()
defer pb.mu.Unlock()

pb.edsVersion = metadata.Version
if metadata.ErrState != nil {
pb.edsVersion = metadata.ErrState.Version
}
for name, uErr := range updates {
if s, ok := pb.edsWatchers[name]; ok {
if uErr.Err != nil {
Expand Down