upstream group and subsets #601

yuval-k · 2019-03-25T15:57:46Z

The draft for groups and subsets.
adding tests now, but figure you guys should take a look.

ilackarms · 2019-03-25T17:27:28Z

personally i am not in favor of extending our api with new api objects, additions to core fields (upstream spec, destination spec) to support an api which increases the coupling between the route and the upstream (route must now know which subsets are available on the upstream)

my suggestion is to implement a new upstreamType for grouped upstreams which allow specifying a list of other upstreams which can be grouped. this is a far simpler means of achieving the same result without adding new solo-kit resources and modifying core api structs just to support a particular use case. this is the entire reason we have upstreamType - to support extending our upstream model without needing to bloat the core apis

yuval-k · 2019-03-25T20:03:37Z

to support envoy subsets route will have to know the subsets regardless; this has nothing to do with the upstream group.

I see a difference in semantic between an Upstream and an UpstreamGroup
Things that upstream has that upstream group doesn't have are sslConfig, and subset configuration.

Additionally making an upstream group a type of upstream will allow recursive upstream groups, which there is no need to have IMHO

Also unlike Upstream, UpstreamGroup is not translated to a cluster.

I see UpstreamGroup as an abstraction of the MultiDestionation part in the route.

I believe these semantic differences merits a new type of api object.

ilackarms · 2019-03-25T20:31:55Z

to support envoy subsets route will have to know the subsets regardless; this has nothing to do with the upstream group.

this is the purpose for putting the subset only in one place (on the upstream). preserve semantics rather than making things more complex. want to route to a subset? create a route like normal, and select an upstream which represents a subset

Additionally making an upstream group a type of upstream will allow recursive upstream groups, which there is no need to have IMHO

i see this edge case as trivial, it can be easily verified with validation on the cli and translator levels. i prefer it to the complexity your proposal would add to the current api

Also unlike Upstream, UpstreamGroup is not translated to a cluster.

This is up to us to decide. the translator can easily be modified so that it does not create a cluster for every upstream

I see UpstreamGroup as an abstraction of the MultiDestionation part in the route.

I see UpstreamGroup as an Upstream which represents a set of weighted upstreams. It's taking the concept of a MultiDestination and moving it to the upstream itself rather than putting it on the route. i consider the following:

route:
  destination:
    single:
      upstream:
        my-upstream-group

to be an equivalent form for

route:
  destination:
    multi:
    - upstream:
        my-upstream-1
    - upstream:
        my-upstream-2

yuval-k · 2019-03-25T23:33:43Z

this the purpose for putting the subset only in one place (on the upstream). preserve semantics rather than making things more complex. want to route to a subset? create a route like normal, and select an upstream which represents a subset

There are subsets defined by discovery. This PR introduces a new type - subsets defined by the user.
The advantage of this:

we dont need to guess which subset the user needs
having the same endpoint in the multiple clusters will increase the amounts of health - checks. giving envoy the subset information (as this PR does) allows envoy being more efficient

i see this edge case as trivial, it can be easily verified with validation on the cli and translator levels. i prefer it to the complexity your proposal would add to the current api

Why allow configuration to have an invalid state if we can prevent that?

This is up to us to decide. the translator can easily be modified so that it does not create a cluster for every upstream

It is up for us to decide but it does introduce special cases to our code, so I think it nullifies i prefer it to the complexity your proposal would add to the current api as there is added complexity either way.

also consider that things that are general cluster concepts do not work will with an upstream group as an upstream type. things like:

ssl
subset configuration
load balancing algorithm
circuit breakers
health_checks
outlier_detection

ilackarms · 2019-03-25T23:48:08Z

projects/gloo/api/v1/plugins.proto

+    // Sub set configuration. For discovery sources that has labels (like kubernetes). this 
+    // configuration allows you to partition the upstream to a set of subsets.
+    // for each unique set of keys and values, a subset will be created.
+    SubsetConfig subset_config = 7;


i think this functionality is already available through the use of the selector on the kubernetes upstream spec

The idea here is to allow subsets beyond the labels on the service.
In addition the user needs to be aware of what they are so at a minimum we need to write them here.
I'm not sure I 100% understand you here so let's discuss this tomorrow in person

ilackarms · 2019-03-25T23:52:19Z

you make good points. taking a second look at the code, i think it makes sense to have a new crd for upstreams with ustreamgroup as an alternative type of destination. one thing i will say though is that i believe subset config being part of the generic upstream doesn't make sense to me. it only (as far as i can tell) applies to kubernetes, and the kubernetes service spec already has a selector which the user can manually configure for subset selection.

soloio-bot · 2019-03-27T19:49:43Z

Issues linked to changelog:
#601

rickducott · 2019-03-27T17:29:12Z

projects/gloo/api/v1/plugins/kubernetes/kubernetes.proto

    .plugins.gloo.solo.io.ServiceSpec service_spec = 5;
+
+
+    // Sub set configuration. For discovery sources that has labels (like kubernetes). this 


nit: subset

rickducott · 2019-03-27T20:12:37Z

projects/gloo/api/v1/proxy.proto

+    // message UpstreamGroupSpec {
+        repeated WeightedDestination destinations = 1;
+    // }
+    // UpstreamGroupSpec spec = 1;


Remove commented out stuff

rickducott · 2019-03-27T20:14:10Z

projects/gloo/api/v1/subset.proto

+option (gogoproto.equal_all) = true;
+
+message Subset {
+   map<string, string> values =1;


nit: formatting -> "... values = 1;"

rickducott · 2019-03-27T20:35:08Z

projects/gloo/pkg/translator/translator_test.go

+				},
+			}
+			params.Snapshot.Upstreams["gloo-system"] = append(params.Snapshot.Upstreams["gloo-system"], upstream2)
+			params.Snapshot.Upstreamgroups = v1.UpstreamgroupsByNamespace{


Realize this is not related to this PR, but kinda annoying that solo-kit doesn't respect "UpstreamGroupsByNamespace"

yuval-k · 2019-03-27T21:24:58Z

/kick ssl flake

yuval-k added 3 commits March 25, 2019 11:55

upstream group and subsets

220eb8c

discovery leave subsets alone

0c604b6

missing proto

225b2e6

ilackarms reviewed Mar 25, 2019

View reviewed changes

yuval-k added 16 commits March 26, 2019 12:46

move subset around

f83a014

add TODO

5b1b027

updated proto

5863e5d

add subset spec proto

3202e6f

add extensions

947e8c6

make things compile with the new proto

1f92c3e

fix e2e test

b454af0

Merge remote-tracking branch 'origin/master' into upstream-group

74031fa

add TODO

c5c7e64

Merge remote-tracking branch 'origin/master' into upstream-group

dcd4cb1

regenerate code; add definition to json

7d307b0

add tests; fix bugs;

d5ef6c4

update header util

1c27b60

update cli

155da1c

tests and code

190a2ee

minor changes

a091fb4

yuval-k marked this pull request as ready for review March 27, 2019 19:46

yuval-k added 2 commits March 27, 2019 15:48

changelog

3072155

spellcheck

283e971

add upstream group validation

2c8b76b

compilefix

d2778b7

rickducott reviewed Mar 27, 2019

View reviewed changes

add missing file; fix pr comments

3a7d378

rickducott previously approved these changes Mar 27, 2019

View reviewed changes

add crd to helm and cli test

141f229

yuval-k dismissed rickducott’s stale review via 141f229 March 27, 2019 22:16

rickducott approved these changes Mar 27, 2019

View reviewed changes

soloio-bulldozer bot merged commit 89aeddb into master Mar 27, 2019

soloio-bulldozer bot deleted the upstream-group branch March 27, 2019 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upstream group and subsets #601

upstream group and subsets #601

yuval-k commented Mar 25, 2019

ilackarms commented Mar 25, 2019

yuval-k commented Mar 25, 2019

ilackarms commented Mar 25, 2019

yuval-k commented Mar 25, 2019

ilackarms Mar 25, 2019 •

edited

yuval-k Mar 26, 2019

ilackarms commented Mar 25, 2019

soloio-bot commented Mar 27, 2019

rickducott Mar 27, 2019

rickducott Mar 27, 2019

rickducott Mar 27, 2019

rickducott Mar 27, 2019

yuval-k Mar 27, 2019

yuval-k commented Mar 27, 2019

		.plugins.gloo.solo.io.ServiceSpec service_spec = 5;


		// Sub set configuration. For discovery sources that has labels (like kubernetes). this

upstream group and subsets #601

upstream group and subsets #601

Conversation

yuval-k commented Mar 25, 2019

ilackarms commented Mar 25, 2019

yuval-k commented Mar 25, 2019

ilackarms commented Mar 25, 2019

yuval-k commented Mar 25, 2019

ilackarms Mar 25, 2019 • edited

Choose a reason for hiding this comment

yuval-k Mar 26, 2019

Choose a reason for hiding this comment

ilackarms commented Mar 25, 2019

soloio-bot commented Mar 27, 2019

rickducott Mar 27, 2019

Choose a reason for hiding this comment

rickducott Mar 27, 2019

Choose a reason for hiding this comment

rickducott Mar 27, 2019

Choose a reason for hiding this comment

rickducott Mar 27, 2019

Choose a reason for hiding this comment

yuval-k Mar 27, 2019

Choose a reason for hiding this comment

yuval-k commented Mar 27, 2019

ilackarms Mar 25, 2019 •

edited