Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
45 changes: 40 additions & 5 deletions internal/grpctest/tlogger.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,11 @@ type tLogger struct {
v int
initialized bool

mu sync.Mutex // guards t, start, and errors
t *testing.T
start time.Time
errors map[*regexp.Regexp]int
mu sync.Mutex // guards t, start, and errors
t *testing.T
start time.Time
errors map[*regexp.Regexp]int
warnings map[*regexp.Regexp]int
Comment on lines +69 to +73
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of relying on comments, we should group the fields protected by the mutex using empty lines. See the following as an example:

mu sync.Mutex
config *iringhash.LBConfig
inhibitChildUpdates bool
shouldRegenerateRing bool
endpointStates *resolver.EndpointMap[*endpointState]

}

func init() {
Expand All @@ -87,7 +88,7 @@ func init() {
}
}
// Initialize tLogr with the determined verbosity level.
tLogr = &tLogger{errors: make(map[*regexp.Regexp]int), v: vLevel}
tLogr = &tLogger{errors: make(map[*regexp.Regexp]int), warnings: make(map[*regexp.Regexp]int), v: vLevel}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of duplicating the code for error and warning logs, can we change the errors map to be slice of maps instead, i.e. []map[*regexp.Regexp]int. We can use the logType enum to index into thls slice of maps.

const (
infoLog logType = iota
warningLog
errorLog
fatalLog
)

If we log an error, we can update logs[errorLog] and if we log a warning we can update logs[warningLog]. This should help make the code DRY.

}

// getCallingPrefix returns the <file:line> at the given depth from the stack.
Expand Down Expand Up @@ -120,6 +121,9 @@ func (tl *tLogger) log(ltype logType, depth int, format string, args ...any) {
} else {
tl.t.Error(args...)
}
case warningLog:
tl.expectedWarning(fmt.Sprintln(args...))
tl.t.Log(args...)
case fatalLog:
panic(fmt.Sprint(args...))
default:
Expand Down Expand Up @@ -155,6 +159,7 @@ func (tl *tLogger) update(t *testing.T) {
tl.t = t
tl.start = time.Now()
tl.errors = map[*regexp.Regexp]int{}
tl.warnings = map[*regexp.Regexp]int{}
}

// ExpectError declares an error to be expected. For the next test, the first
Expand All @@ -178,6 +183,18 @@ func ExpectErrorN(expr string, n int) {
tLogr.errors[re] += n
}

// ExpectWarning declares a warning to be expected.
func ExpectWarning(expr string) {
tLogr.mu.Lock()
defer tLogr.mu.Unlock()
re, err := regexp.Compile(expr)
if err != nil {
tLogr.t.Error(err)
return
}
tLogr.warnings[re]++
}

// endTest checks if expected errors were not encountered.
func (tl *tLogger) endTest(t *testing.T) {
tl.mu.Lock()
Expand All @@ -188,6 +205,12 @@ func (tl *tLogger) endTest(t *testing.T) {
}
}
tl.errors = map[*regexp.Regexp]int{}
for re, count := range tl.warnings {
if count > 0 {
t.Errorf("Expected warning '%v' not encountered", re.String())
}
}
tl.warnings = map[*regexp.Regexp]int{}
}

// expected determines if the error string is protected or not.
Expand All @@ -204,6 +227,18 @@ func (tl *tLogger) expected(s string) bool {
return false
}

// expectedWarning determines if the warning string is protected or not.
func (tl *tLogger) expectedWarning(s string) {
for re, count := range tl.warnings {
if re.FindStringIndex(s) != nil {
tl.warnings[re]--
if count <= 1 {
delete(tl.warnings, re)
}
}
}
}

func (tl *tLogger) Info(args ...any) {
tl.log(infoLog, 0, "", args...)
}
Expand Down
95 changes: 95 additions & 0 deletions internal/xds/xdsclient/xdsresource/xdsconfig.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/*
*
* Copyright 2025 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package xdsresource

import "google.golang.org/grpc/resolver"

// XDSConfig holds the complete gRPC client-side xDS configuration containing
// all necessary resources.
type XDSConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, we are using pointer fields to avoid copies. If so, are any of these fields guaranteed to be non-nil at runtime? If so, we should mention this in the godoc of those fields to help consumers of this struct.

// Listener holds the listener configuration.
Listener *ListenerUpdate

// RouteConfig is the route configuration. It will be populated even if
// RouteConfig is inlined into the Listener resource.
RouteConfig RouteConfigUpdate
Copy link
Member Author

Choose a reason for hiding this comment

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

Will change other resources to pointer after #8652 is merged

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please clarify why we are blocked on #8652? It should be possible to get a ref value if necessary by dereferencing a pointer (*RouteConfig).


Also, can you please add a TODO for this?


// VirtualHost selected from the route configuration whose domain field
// offers the best match against the provided dataplane authority.
VirtualHost *VirtualHost

// Clusters is a map from cluster name to its configuration.
Clusters map[string]*ClusterResult
}

// ClusterResult contains a cluster's configuration when we receive a
// valid resource from the management server. It contains an error when:
// - we receive an invalid resource from the management server and
// we did not already have a valid resource or
// - the cluster resource does not exist on the management server
type ClusterResult struct {
Config ClusterConfig
Err error
}

// ClusterConfig contains configuration for a single cluster.
type ClusterConfig struct {
Cluster ClusterUpdate // Cluster configuration. Always present.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add comments at the top of each field and use complete sentences?

Adding good comments makes it easier to use these fields correctly. For example, instead of Always present., we should use This field is always set to a non-zero value.

EndpointConfig EndpointConfig // Endpoint configuration for leaf clusters.
AggregateConfig AggregateConfig // List of children for aggregate clusters.
Comment on lines +53 to +54
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these fields are optional, should they be pointers?

}

// AggregateConfig contains a list of leaf cluster names for .
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please complete this sentence.

type AggregateConfig struct {
LeafClusters []string
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment for this exported field. Here, it will be good to describe what the strings in the slice represent.

Please consider this change elsewhere in this PR.

}

// EndpointConfig contains configuration corresponding to the endpoints in a
// cluster. Only one of EDSUpdate or DNSEndpoints will be populated based on the
// cluster type.
type EndpointConfig struct {
EDSUpdate EndpointsUpdate // Configuration for EDS clusters.
DNSEndpoints DNSUpdate // Configuration for LOGICAL_DNS clusters.
ResolutionNote error // Error obtaining endpoints data.
}

// DNSUpdate contains the update from DNS resolver.
type DNSUpdate struct {
Endpoints []resolver.Endpoint
}

// xdsConfigkey is the type used as the key to store XDSConfig in
// the Attributes field of resolver.states.
type xdsConfigkey struct{}

// SetXDSConfig returns a copy of state in which the Attributes field
// is updated with the XDSConfig.
func SetXDSConfig(state resolver.State, config *XDSConfig) resolver.State {
state.Attributes = state.Attributes.WithValue(xdsConfigkey{}, config)
return state
}

// XDSConfigFromResolverState returns XDSConfig stored in attribute in resolver
// state.
func XDSConfigFromResolverState(state resolver.State) *XDSConfig {
state.Attributes.Value(xdsConfigkey{})
if v := state.Attributes.Value(xdsConfigkey{}); v != nil {
return v.(*XDSConfig)
}
return nil
}
34 changes: 34 additions & 0 deletions internal/xds/xdsdependencymanager/logging.go
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do we need a separate file for declaring the logger? In my opinion this partition is too granular and the file can be combined with xds_dependency_manager.go.

Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
*
* Copyright 2025 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package xdsdepmgr

import (
"fmt"

"google.golang.org/grpc/grpclog"
internalgrpclog "google.golang.org/grpc/internal/grpclog"
)

const prefix = "[xds-dependency-manager %p] "

var logger = grpclog.Component("xds")

func prefixLogger(p *DependencyManager) *internalgrpclog.PrefixLogger {
return internalgrpclog.NewPrefixLogger(logger, fmt.Sprintf(prefix, p))
}
134 changes: 134 additions & 0 deletions internal/xds/xdsdependencymanager/watch_service.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
/*
*
* Copyright 2020 gRPC authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The date is off by 5 years.

*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package xdsdepmgr

import (
"context"

"google.golang.org/grpc/internal/xds/xdsclient/xdsresource"
)

type listenerWatcher struct {
resourceName string
cancel func()
isCancelled bool
parent *DependencyManager
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, parent is a very general name and it doesn't give many clues about what this field does. Can we rename it to depMgr or something more useful?

}

func newListenerWatcher(resourceName string, parent *DependencyManager) *listenerWatcher {
lw := &listenerWatcher{resourceName: resourceName, parent: parent}
lw.cancel = xdsresource.WatchListener(parent.xdsClient, resourceName, lw)
return lw
}

func (l *listenerWatcher) ResourceChanged(update *xdsresource.ListenerUpdate, onDone func()) {
if l.isCancelled {
return
}
handleUpdate := func(context.Context) {
l.parent.onListenerResourceUpdate(update)
onDone()
}
l.parent.serializer.ScheduleOr(handleUpdate, onDone)
Comment on lines +44 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

The listenerWatcher is directly accessing fields of the DependencyManager here which is coupling the implementation of these two structs. This would make refactoring these two structs harder.


Can we instead do the following?

  • Have parent.onListenerResourceUpdate accept an onDone func() param.
  • Have the DependencyManager schedule the update and the call to onDone in its serializer.
    This would simplify this method on the listenerWatcher:
func (l *listenerWatcher) ResourceChanged(update *xdsresource.ListenerUpdate, onDone func()) {
	if l.isCancelled {
		return
	}
    l.parent.onListenerResourceUpdate(update, onDone)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar for other listenerWatcher methods that use the parent's serializeer.

}

func (l *listenerWatcher) ResourceError(err error, onDone func()) {
if l.isCancelled {
return
}
handleError := func(context.Context) {
l.parent.onListenerResourceError(err)
onDone()
}

l.parent.serializer.ScheduleOr(handleError, onDone)
}

func (l *listenerWatcher) AmbientError(err error, onDone func()) {
if l.isCancelled {
return
}
handleError := func(context.Context) {
l.parent.onListenerResourceAmbientError(err)
onDone()
}
l.parent.serializer.ScheduleOr(handleError, onDone)
}

func (l *listenerWatcher) stop() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a guarantee that stop is not called concurrently with ResourceChanged, ResourceError or other methods? If so, we should document why this is the case so that this assumption is not broken in the future.

l.isCancelled = true
l.cancel()
if logger.V(2) {
l.parent.logger.Infof("Canceling watch on Listener resource %q", l.resourceName)
}
}

type routeConfigWatcher struct {
resourceName string
cancel func()
parent *DependencyManager
isCancelled bool
}

func newRouteConfigWatcher(resourceName string, parent *DependencyManager) *routeConfigWatcher {
rw := &routeConfigWatcher{resourceName: resourceName, parent: parent}
rw.cancel = xdsresource.WatchRouteConfig(parent.xdsClient, resourceName, rw)
return rw
}

func (r *routeConfigWatcher) ResourceChanged(u *xdsresource.RouteConfigResourceData, onDone func()) {
if r.isCancelled {
return
}
handleUpdate := func(context.Context) {
r.parent.onRouteConfigResourceUpdate(r.resourceName, u.Resource)
onDone()
}
r.parent.serializer.ScheduleOr(handleUpdate, onDone)
}

func (r *routeConfigWatcher) ResourceError(err error, onDone func()) {
if r.isCancelled {
return
}
handleError := func(context.Context) {
r.parent.onRouteConfigResourceError(r.resourceName, err)
onDone()
}
r.parent.serializer.ScheduleOr(handleError, onDone)
}

func (r *routeConfigWatcher) AmbientError(err error, onDone func()) {
if r.isCancelled {
return
}
handleError := func(context.Context) {
r.parent.onRouteConfigResourceAmbientError(r.resourceName, err)
onDone()
}
r.parent.serializer.ScheduleOr(handleError, onDone)
}

func (r *routeConfigWatcher) stop() {
r.isCancelled = true
r.cancel()
if logger.V(2) {
r.parent.logger.Infof("Canceling watch on RouteConfiguration resource %q", r.resourceName)
}
}
Loading