route match: Add runtime_fraction field for more granular routing by tonya11en · Pull Request #4217 · envoyproxy/envoy

tonya11en · 2018-08-21T19:58:19Z

Description:
This patch introduces the runtime_fraction field to RouteMatch and deprecates the runtime field. Prior to this field, runtime only allowed for splitting of traffic amongst clusters with a granularity of 1%, but this new field uses a FractionalPercent structure in the configuration to allow for much smaller amounts to be specified.

Using the runtime_fraction field will cause the runtime field to be ignored in a route configuration if they are both specified. If a numerator or denominator is not present in the runtime config for the provided key, or if a key is not provided, the default value provided will be used.

Risk Level:
Low

Testing:
Unit tests.

Docs Changes:
Protobuf descriptions.

Release Notes:
n/a

Fixes #4192

danielhochman · 2018-08-21T21:13:39Z

source/common/json/config_schemas.cc

remove this, backport to v1 is not required

Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en · 2018-08-22T23:41:52Z

Just FYI- looks like my api check has fallen victim to #4218. Everything else is in good working order though.

danielhochman · 2018-08-22T23:50:29Z

api/envoy/type/percent.proto

+
+// Runtime derived FractionalPercent with defaults for when the numerator or denominator is not
+// specified via a runtime key.
+message RuntimeFractionalPercent {


can we move this back into route.proto or create a runtime proto? i don't think percent.proto should contain this runtime concept.

danielhochman · 2018-08-22T23:55:59Z

source/common/router/config_impl.cc

+    data.numerator_key_ = route_match.runtime_fraction().numerator_runtime_key();
+    data.numerator_default_ = route_match.runtime_fraction().default_value().numerator();
+
+    const std::string& d_key = route_match.runtime_fraction().denominator_runtime_key();


nit: spell out d_ prefix

danielhochman · 2018-08-22T23:58:44Z

api/envoy/type/percent.proto

+  FractionalPercent default_value = 1 [(validate.rules).message.required = true];
+
+  // Runtime key to get numerator value for comparison. This value is used if defined.
+  string numerator_runtime_key = 2;


i think you could safely shorten these to numerator_key, denominator_key since they're already under runtime_fraction. up to you.

danielhochman · 2018-08-22T23:59:19Z

looking good. minor comments.

Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en · 2018-08-28T19:05:14Z

@ggreenway just FYI, that circleci API check failure has nothing to do with my PR- it's due to #4218. In case you were holding off on the review due to the check failure.

zuercher

Looks pretty good. I have one question about the behavior when the denominator key lookup fails to produce a valid denominator.

Unfortunately, I'm headed out of town until Tuesday, so another maintainer will have to pick this up if you want to get it merged before then.

source/common/router/config_impl.cc

zuercher · 2018-08-30T00:20:30Z

source/common/router/config_impl.cc

+    FractionalPercent::DenominatorType denominator_type;
+    if (!FractionalPercent::DenominatorType_Parse(denominator_data, &denominator_type)) {
+      denominator_type = route_match.runtime_fraction().default_value().denominator();
+    }


If I read this right, failure to convert the denominator key into a valid denominator value causes us to use the numerator key's value with the default value's denominator. That seems like it might cause some pretty unexpected behavior. Is it reasonable to just switch using the default value altogether in this case?

I think what you propose is the sane thing to do. I'll change the logic.

Actually- scratch that. A user would have unexpected behavior either way if they misspell a denominator type and it switches to a default value. I'm not sure one way is better than the other. @htuch what do you think?

Not sure what the best practice to alert when runtime is wrong; how do folks operationally determine when a runtime change has taken effect? Via /runtime?

One thought that occurred to me just now is that instead of having separate values for numerator/denominator, we have a single values, which is a textual representation of a FractionalPercent proto. That way, we can just use PGV validation on this to check sanity. It also solves an update atomicity issue.

api/envoy/api/v2/route/route.proto

Signed-off-by: Tony Allen <tallen@lyft.com>

api/envoy/api/v2/route/route.proto

htuch · 2018-08-31T20:47:35Z

api/envoy/type/runtime_percent.proto

+  // Default value if the runtime value's for the numerator/denominator keys are not available.
+  envoy.type.FractionalPercent default_value = 1 [(validate.rules).message.required = true];
+
+  // Runtime key for a textual representation of a :ref:`fractional percent


Nice to see this approach. I actually think we should use the JSON/YAML encoding on second thoughts, since text proto is not a standard. If you go this route, we have some helpers to make it easy to parse, see https://github.com/envoyproxy/envoy/blob/master/source/common/protobuf/utility.h#L210. This will also validate the proto for you for constraints, which is missing in the parsing below.

Signed-off-by: Tony Allen <tallen@lyft.com>

htuch

Looks great, two final minor comments (and the api build looks still broken).

htuch · 2018-09-04T23:20:20Z

api/envoy/type/runtime_percent.proto

+
+// Runtime derived FractionalPercent with defaults for when the numerator or denominator is not
+// specified via a runtime key.
+message RuntimeFractionalPercent {


Should this be in api/envoy/api/v2/core/base.proto so as to be next to RuntimeUint32?

I didn't do that because I didn't want to introduce a dependency on percent.proto. It was originally in percent.proto, but an earlier comment from @danielhochman suggested I move it out into its own file.

It's fine for base to depend on percent.proto (just not the other way around).

api/envoy/type/runtime_percent.proto

Signed-off-by: Tony Allen <tallen@lyft.com>

htuch

Looks great, thanks!

htuch · 2018-09-06T04:00:04Z

Oh crap, we're hitting the PGV CLI issue in the Mac build (CC @zuercher). I think this is solved due to some nice work done by @sesmith177 in fixing this in PGV, but we're still blocked on importing this into Envoy due to PG* issues. I think @rodaine suggested we might see this progress this week.

It might be possible to trick the build into passing in the meanwhile by doing something hacky to make the PGV dependencies have a shorter CLI, but if you can wait a few days maybe this problem sorts itself out...

tonya11en · 2018-09-06T04:16:08Z

That's fine, it's not a super high-priority for us at the moment. I'm assuming it'll be fixed in the next week or two?

Let me know if I can help in any way.

tonya11en · 2018-09-12T20:24:45Z

@htuch are you able to re-run the ci/circleci: mac test now that #4413 is merged? I don't have privileges to do so.

htuch · 2018-09-12T21:04:42Z

@tonya11en you'll need to merge master to pick up that change; if you do that and push the CI will rerun.

Signed-off-by: Tony Allen <tallen@lyft.com>

…ting (envoyproxy#4217)" This reverts commit 75e54d0. This was breaking Mac CI due to known PGV CLI limits. Signed-off-by: Harvey Tuch <htuch@google.com>

…ting (#4217)" (#4482) This reverts commit 75e54d0. This was breaking Mac CI due to known PGV CLI limits. Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2018-09-28T19:38:10Z

@tonya11en do you want to try rolling this forward now that #4551 has merged? Sorry about all this futz, if we hit PGV issues still I can now go and fix them.

…voyproxy#4217) Signed-off-by: Tony Allen <tallen@lyft.com>

) This patch reintroduces PR #4217. Signed-off-by: Tony Allen <tallen@lyft.com>

…voyproxy#4560) This patch reintroduces PR envoyproxy#4217. Signed-off-by: Tony Allen <tallen@lyft.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>

tonya11en force-pushed the fractional_percent branch from 64d1488 to 2f063e1 Compare August 21, 2018 21:07

danielhochman reviewed Aug 21, 2018

View reviewed changes

source/common/json/config_schemas.cc Outdated

Copy link

Contributor

danielhochman Aug 21, 2018

Choose a reason for hiding this comment

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

remove this, backport to v1 is not required

danielhochman self-assigned this Aug 21, 2018

route match: Add runtime_fraction field for more granularity

9490c37

Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en force-pushed the fractional_percent branch from 2f063e1 to 9490c37 Compare August 21, 2018 21:31

danielhochman reviewed Aug 22, 2018

View reviewed changes

Tony Allen added 2 commits August 23, 2018 11:31

Address Daniel's comments and move RuntimeFractionalPercent.

9f7fc09

Signed-off-by: Tony Allen <tallen@lyft.com>

Fix docs.

dad5be2

Signed-off-by: Tony Allen <tallen@lyft.com>

danielhochman previously approved these changes Aug 24, 2018

View reviewed changes

danielhochman assigned ggreenway Aug 24, 2018

zuercher reviewed Aug 30, 2018

View reviewed changes

htuch reviewed Aug 30, 2018

View reviewed changes

api/envoy/api/v2/route/route.proto Outdated Show resolved Hide resolved

htuch assigned htuch and unassigned ggreenway Aug 30, 2018

Tony Allen added 2 commits August 30, 2018 18:36

Address Harvey's comments.

7ee808d

Signed-off-by: Tony Allen <tallen@lyft.com>

Fix format.

7e92dca

Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en dismissed danielhochman’s stale review via 7e92dca August 31, 2018 01:39

htuch reviewed Aug 31, 2018

View reviewed changes

Tony Allen added 2 commits September 4, 2018 13:48

Use JSON/YAML encoding and validation.

e40132c

Signed-off-by: Tony Allen <tallen@lyft.com>

Fix docs and format.

fca465f

Signed-off-by: Tony Allen <tallen@lyft.com>

htuch suggested changes Sep 4, 2018

View reviewed changes

Tony Allen added 2 commits September 5, 2018 11:01

Move RuntimeFraction into base.proto.

6d5c6bc

Signed-off-by: Tony Allen <tallen@lyft.com>

Merge branch 'master' into fractional_percent

1ff4323

htuch approved these changes Sep 5, 2018

View reviewed changes

zuercher added the no stalebot Disables stalebot from closing an issue label Sep 10, 2018

Tony Allen added 6 commits September 12, 2018 14:07

Merge remote-tracking branch 'origin/master' into fractional_percent

4df9106

Signed-off-by: Tony Allen <tallen@lyft.com>

Merge remote-tracking branch 'origin/master' into fractional_percent

336122e

Signed-off-by: Tony Allen <tallen@lyft.com>

Merge remote-tracking branch 'origin/master' into fractional_percent

4121f9d

Kick CI

aee0a34

Signed-off-by: Tony Allen <tallen@lyft.com>

Kick CI

87bc01a

Signed-off-by: Tony Allen <tallen@lyft.com>

Kick CI

cb67993

Signed-off-by: Tony Allen <tallen@lyft.com>

mattklein123 merged commit 75e54d0 into envoyproxy:master Sep 19, 2018

tonya11en deleted the fractional_percent branch September 19, 2018 21:29

lizan mentioned this pull request Sep 20, 2018

mac: install latest Bazel #4472

Merged

htuch mentioned this pull request Sep 20, 2018

Revert "route match: Add runtime_fraction field for more granular rou… #4482

Merged

tonya11en pushed a commit to tonya11en/envoy that referenced this pull request Sep 28, 2018

route match: Add runtime_fraction field for more granular routing (en…

4f4c7be

…voyproxy#4217) Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en mentioned this pull request Sep 28, 2018

route match: Add runtime_fraction field for more granular routing #4560

Merged

htuch pushed a commit that referenced this pull request Sep 30, 2018

route match: Add runtime_fraction field for more granular routing (#4560

bcc7a96

) This patch reintroduces PR #4217. Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en mentioned this pull request Oct 5, 2018

routes: RouteMatch runtime keys do not take effect after snapshot updates #4607

Closed

Conversation

tonya11en commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en commented Aug 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhochman commented Aug 22, 2018

Uh oh!

tonya11en commented Aug 28, 2018

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Sep 6, 2018

Uh oh!

tonya11en commented Sep 6, 2018

Uh oh!

tonya11en commented Sep 12, 2018

Uh oh!

htuch commented Sep 12, 2018

Uh oh!

htuch commented Sep 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants