internal: Wire up new snapshots to DAG rebuilds #2850

stevesloka · 2020-08-28T20:57:43Z

Adds the snapshotHandler to react to DAG rebuilds so that new
snapshots can be created.

Follows #2845
Updates #2134

Signed-off-by: Steve Sloka [email protected]

codecov · 2020-08-28T21:01:44Z

Codecov Report

Merging #2850 into main will decrease coverage by 0.23%.
The diff coverage is 9.75%.

@@            Coverage Diff             @@
##             main    #2850      +/-   ##
==========================================
- Coverage   76.81%   76.57%   -0.24%     
==========================================
  Files          77       79       +2     
  Lines        5848     5866      +18     
==========================================
  Hits         4492     4492              
- Misses       1265     1283      +18     
  Partials       91       91

Impacted Files	Coverage Δ
cmd/contour/serve.go	`2.37% <0.00%> (-0.07%)`	⬇️
internal/contour/cluster.go	`100.00% <ø> (ø)`
internal/contour/snapshot.go	`0.00% <0.00%> (ø)`
internal/xds/hash.go	`0.00% <0.00%> (ø)`
internal/xds/server.go	`100.00% <100.00%> (ø)`

skriss

thanks for the fix @zianke! I'm approving, but it'd be awesome if you could make this same edit in site/docs/v1.8.0/configuration.md, so the latest tagged docs get the fix as well.

zianke · 2020-08-28T21:22:49Z

Thanks @skriss. I guess you're commenting on #2849. Sure I'll fix it.

wrong PR :)

internal/contour/snapshot.go

internal/xds/server.go

cmd/contour/serve.go

jpeach · 2020-08-31T00:29:36Z

cmd/contour/serve.go

@@ -24,6 +24,8 @@ import (
 	"syscall"
 	"time"

+	xds "github.com/envoyproxy/go-control-plane/pkg/cache/types"


If you add an alias for ResourceType to the existing xds package, then you can avoid the import conflict here and in other places. I'd recommend aliasing it as ResourceType, which indicates its usage better.

Something like this?

type ResourceType = xds.ResponseType

How do I get to the enumeration types?

This is the struct:

// ResponseType enumeration of supported response types type ResponseType int const ( Endpoint ResponseType = iota Cluster Route Listener Secret Runtime UnknownType // token to count the total number of supported types )

jpeach · 2020-08-31T00:51:17Z

internal/contour/resources.go

 }

 // ResourcesOf transliterates a slice of ResourceCache into a slice of xds.Resource.
-func ResourcesOf(in []ResourceCache) []xds.Resource {
-	out := make([]xds.Resource, len(in))
+func ResourcesOf(in map[xds.ResponseType]ResourceCache) []contourxds.Resource {


I don't think you'll need to do this if you can make the mapping internal to the snapshot observer.

I was wanting to re-use the ResourceCache, but need to know the types later on of which is the listener, which is endpoints, etc.

Can you explain a bit more on your thought?

What I was thinking was using ResourceCache.TypeURL() to internally map to the control-plane ResponseType. The type URLs are global unique, so you can just map internally and keep the implementation details that need ResponseType private.

I can do that and remove the map, but I'd rather have a quick lookup to the resources than have to loop through them all and figure out what type is which.

What is the downside of switching the type to a map?

If we ditch the map I'd end up with something like this in the snapshot constructor which to me feels bad, I'd rather just lookup the index of the map:

for _, r := range resources { switch r.TypeURL() { case resource.ClusterType: clusterCache = r case resource.RouteType: routeCache = r case resource.ListenerType: listenerCache = r case resource.SecretType: secretCache = r case resource.EndpointType: endpointsCache = r } }

Both the integer types and the typeURLs are well-known constants with a permanently fixed relationship, so you can preinitialize a constant map:

m := map[string]xds.ResponseType { resource.EndpointType: types.Endpoint, ... } snapshot[m[r.TypeURL()]] = resourcesOf(r.Contents())

The benefit of this approach is that knowledge of the go-control-plane snapshot types can be encapsulated by the one component that really needs to know. The rest of Contour can be oblivious.

It's still a map though, so why are strings better than types? I'd prefer the types since they are easier to work with unless I'm not following.

What I'm trying to avoid is the need for the go-control-plane types to propagate over the whole codebase. Only the snapshot handler needs them and I'm suggesting that we can localize their use internally to that. So When you create a new snapshot handler, you just give out the slice of resources, then internally it maps them to the go-control-plane response types. We don't have to pre-set that mapping in the caller.

The only way to hide from the other bits is to remove the map as described in #2850 (comment).

I still prefer the type as implemented keeping the map.

To keep this moving I did what you said @jpeach, I don't want this to block hold up the implementation.

jpeach · 2020-08-31T00:51:38Z

internal/contour/snapshot.go

+)
+
+// SnapshotHandler implements the xDS snapshot cache
+type SnapshotHandler struct {


I'm not sure that "handler" is the right naming here. The "Handler" name is used for types that deal with Kubernetes events directly, but this is a DAG observer which is a different role.

It "handles' snapshots? I dunno, please suggest I think the name is fine. =)

It "handles' snapshots? I dunno, please suggest I think the name is fine. =)

ROFL, it's just that it's a handler in a different sense than the other types that we name "*Handler". I will get by with "handler" if we can't think of a evocative alternative :)

jpeach · 2020-08-31T01:01:06Z

Can you please link the PR and commit to #2134? thanks :)

internal/contour/snapshot.go

internal/xds/server.go

skriss

no further comments, LGTM pending resolution of open items

skriss · 2020-09-01T19:33:31Z

cmd/contour/serve.go

@@ -35,9 +37,10 @@ import (
 	"github.com/projectcontour/contour/internal/metrics"
 	"github.com/projectcontour/contour/internal/timeout"
 	"github.com/projectcontour/contour/internal/workgroup"
-	"github.com/projectcontour/contour/internal/xds"
+	contourxds "github.com/projectcontour/contour/internal/xds"


I think you could un-alias this now if you wanted (applies to other files as well)

skriss · 2020-09-01T20:30:05Z

internal/contour/snapshot.go

+	clusterCache   contourxds.Resource
+	listenerCache  contourxds.Resource
+	secretCache    contourxds.Resource
+	endpointsCache contourxds.Resource
+	routeCache     contourxds.Resource


You could still store all these caches in a map keyed on ResourceType if you wanted, but I'm not exactly sure how they'll eventually be used. Easy to change later though if you do want to do that, so not a big deal.

I prefer that approach and not have to map each type in a for loop, but since that's where we are, it's nice to have each cache addressable directly. If that doesn't pan out can refactor later, but I think this will work out nice.

jpeach

Just a few nits around cleaning up the import alias. I think that we can still simplify the way the snapshot handler works, but we could do that as we land the implementation too.

jpeach · 2020-09-02T00:06:44Z

internal/contour/resources.go

@@ -18,20 +18,20 @@ package contour

 import (
 	"github.com/projectcontour/contour/internal/dag"
-	"github.com/projectcontour/contour/internal/xds"
+	contourxds "github.com/projectcontour/contour/internal/xds"


Nit: looks like we don't need to change this file at all?

jpeach · 2020-09-02T00:07:05Z

internal/contour/server_test.go

@@ -25,7 +25,7 @@ import (
 	resource "github.com/envoyproxy/go-control-plane/pkg/resource/v2"
 	"github.com/projectcontour/contour/internal/dag"
 	"github.com/projectcontour/contour/internal/fixture"
-	"github.com/projectcontour/contour/internal/xds"
+	contourxds "github.com/projectcontour/contour/internal/xds"


Nit: looks like we don't need to change this file at all?

jpeach · 2020-09-02T00:08:31Z

internal/featuretests/featuretests.go

@@ -36,7 +36,7 @@ import (
 	"github.com/projectcontour/contour/internal/protobuf"
 	"github.com/projectcontour/contour/internal/sorter"
 	"github.com/projectcontour/contour/internal/workgroup"
-	"github.com/projectcontour/contour/internal/xds"
+	contourxds "github.com/projectcontour/contour/internal/xds"


Nit: looks like we don't need to change this file at all

jpeach · 2020-09-02T00:21:57Z

internal/xds/util.go

 	"k8s.io/apimachinery/pkg/types"
 )

+type ResourceType = xds.ResponseType


Nit: now that the resource types are encapsulated by the snapshot handler, I don't think you need this?

jpeach · 2020-09-02T00:27:39Z

internal/contour/snapshot.go

+		endpointsCache: endpointsCache,
+		routeCache:     routeCache,
+		FieldLogger:    logger,
+	}


Nit:

What I had in mind was something along the lines of the pseudocode below. We can treat the resources generically, and control how we forward the DAG changes.

type SnapshotHandler struct { snapshot types.Snapshot resources []ResourceCache } func NewSnapshotHandler(c cache.SnapshotCache, resources []ResourceCache, logger logrus.FieldLogger) *SnapshotHandler { } func (s *SnapshotHandler) OnChange(root *dag.DAG) { // Forward the DAG change to each resource cache. for _, r := range s.resources { r.OnChange(root) } for _, r := range s.resources { contents := r.Contents() responseType := responseTypeOf(r.TypeURL()) version := nextVersionForType() s.snapshot.Resources[responseType] = cache.NewResources(version, responseSlice(contents) } }

Adds the snapshotHandler to react to DAG rebuilds so that new snapshots can be created. Updates projectcontour#2134 Signed-off-by: Steve Sloka <[email protected]>

skriss previously approved these changes Aug 28, 2020

View reviewed changes

skriss self-requested a review August 28, 2020 21:25

jpeach mentioned this pull request Aug 31, 2020

internal: Implement Envoy go-control-plane xDS Server #2845

Closed

jpeach suggested changes Aug 31, 2020

View reviewed changes

stevesloka force-pushed the gcpServerSnapshot branch 2 times, most recently from 948868b to 9b5519c Compare August 31, 2020 01:57

skriss requested changes Aug 31, 2020

View reviewed changes

internal/contour/snapshot.go Outdated Show resolved Hide resolved

internal/xds/server.go Outdated Show resolved Hide resolved

internal/xds/server.go Outdated Show resolved Hide resolved

stevesloka force-pushed the gcpServerSnapshot branch from 9b5519c to 9bc9ba0 Compare August 31, 2020 17:50

stevesloka requested review from skriss and jpeach August 31, 2020 17:51

skriss approved these changes Aug 31, 2020

View reviewed changes

stevesloka force-pushed the gcpServerSnapshot branch from 9bc9ba0 to e72a33d Compare September 1, 2020 18:11

skriss reviewed Sep 1, 2020

View reviewed changes

jpeach approved these changes Sep 2, 2020

View reviewed changes

stevesloka force-pushed the gcpServerSnapshot branch from e72a33d to e2cb7c3 Compare September 2, 2020 21:05

internal: Wire up new snapshots to DAG rebuilds

0a20edc

Adds the snapshotHandler to react to DAG rebuilds so that new snapshots can be created. Updates projectcontour#2134 Signed-off-by: Steve Sloka <[email protected]>

stevesloka force-pushed the gcpServerSnapshot branch from e2cb7c3 to 0a20edc Compare September 3, 2020 19:47

stevesloka merged commit 7027e1b into projectcontour:main Sep 3, 2020

stevesloka deleted the gcpServerSnapshot branch September 3, 2020 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal: Wire up new snapshots to DAG rebuilds #2850

internal: Wire up new snapshots to DAG rebuilds #2850

stevesloka commented Aug 28, 2020 •

edited

Loading

codecov bot commented Aug 28, 2020 •

edited

Loading

skriss left a comment

zianke commented Aug 28, 2020

jpeach Aug 31, 2020

stevesloka Aug 31, 2020

jpeach Aug 31, 2020

stevesloka Aug 31, 2020

jpeach Aug 31, 2020

stevesloka Aug 31, 2020

jpeach Sep 1, 2020

stevesloka Sep 1, 2020

jpeach Sep 1, 2020 •

edited

Loading

stevesloka Sep 1, 2020

stevesloka Sep 1, 2020

jpeach Aug 31, 2020

stevesloka Aug 31, 2020

jpeach Aug 31, 2020

jpeach commented Aug 31, 2020

skriss left a comment

skriss Sep 1, 2020

skriss Sep 1, 2020

stevesloka Sep 2, 2020 •

edited

Loading

jpeach left a comment

jpeach Sep 2, 2020

jpeach Sep 2, 2020

jpeach Sep 2, 2020

jpeach Sep 2, 2020

jpeach Sep 2, 2020

internal: Wire up new snapshots to DAG rebuilds #2850

internal: Wire up new snapshots to DAG rebuilds #2850

Conversation

stevesloka commented Aug 28, 2020 • edited Loading

codecov bot commented Aug 28, 2020 • edited Loading

Codecov Report

skriss left a comment

Choose a reason for hiding this comment

zianke commented Aug 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeach Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeach commented Aug 31, 2020

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevesloka Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

jpeach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevesloka commented Aug 28, 2020 •

edited

Loading

codecov bot commented Aug 28, 2020 •

edited

Loading

jpeach Sep 1, 2020 •

edited

Loading

stevesloka Sep 2, 2020 •

edited

Loading