Add support for drop category policy and reporting by vishalpowar · Pull Request #3894 · envoyproxy/envoy

vishalpowar · 2018-07-19T00:03:24Z

This PR contains changes to implement feature requested in issue #3823

Adding DropOverload in eds policy which can be used to specify
drop_percentage per category.
Adding DroppedRequests in load_report which can report deliberately
dropped requests for each category.

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level:
Testing: None
Docs Changes: None
Release Notes:
Fixes #3823

zuercher

Thanks for taking this on. I have a few questions and found a couple typos, but I think it's headed in the right direction.

zuercher · 2018-07-23T16:25:45Z

api/envoy/api/v2/eds.proto

I gather this was never implemented, but do we need to keep this in place (but marked deprecated) until 1.8.0 on the off chance that someone is setting it?

I think it's reasonable to skip it. If someone is setting a hidden/unimplemented field, I don't think we need to be applying the policy. Maybe we should clarify it to make this explicit if it isn't already.

zuercher · 2018-07-23T16:28:39Z

api/envoy/api/v2/eds.proto

typo: rom; also drop the first or

zuercher · 2018-07-23T16:29:15Z

api/envoy/api/v2/endpoint/load_report.proto

typo: request should be requests

zuercher · 2018-07-23T16:29:33Z

api/envoy/api/v2/endpoint/load_report.proto

typo: request should be requests

zuercher · 2018-07-23T16:35:21Z

api/envoy/api/v2/eds.proto

I'm not sure about this validation. If there are other, unrelated policy settings in the future we won't want to require this one.

Removed validation.

zuercher · 2018-07-23T16:36:03Z

api/envoy/api/v2/eds.proto

Should this have a validation rule to prevent it from being an empty string?

htuch

Yep, thanks for making this happen.

htuch · 2018-07-24T03:25:33Z

DEPRECATED.md

I would just drop this, it's a hidden field.

htuch · 2018-07-24T03:26:26Z

api/envoy/api/v2/eds.proto

Identifier for the policy specifying the drop.`

htuch · 2018-07-24T03:26:40Z

api/envoy/api/v2/eds.proto

Please use envoy.type.Percent here.

htuch · 2018-07-24T03:27:58Z

api/envoy/api/v2/eds.proto

Can you provide the full explanation in the comments here for how drop policy is applied, including the worked example from your doc? These comments get rendered into docs.

htuch

LGTM modulo remaining comments.

htuch · 2018-07-25T04:40:29Z

api/envoy/api/v2/eds.proto

Nit: Please use JSON or YAML examples, even for pseudo-code. There is no XML in the Envoy code base :)

htuch · 2018-07-25T04:41:22Z

api/envoy/api/v2/endpoint/load_report.proto

I think this can also be deprecated, since it can be inferred from dropped_requests now. Better to be concise in APIs.

Yes, I did not want to do this as it is implemented.
I am open to doing those changes in subsequent PRs

htuch · 2018-07-25T04:42:07Z

Also, looks like build is broken @vishalpowar

This PR contains changes to implement feature requested in issue envoyproxy#3823 - Adding DropOverload in eds policy which can be used to specify drop_percentage per category. - Adding DroppedRequests in load_report which can report deliberately dropped requests for each category. Signed-off-by: vishalpowar <vishal.powar@gmail.com>

htuch

Awesome, thanks.

mattklein123 · 2018-07-26T00:14:56Z

api/envoy/api/v2/eds.proto

+      string category = 1 [(validate.rules).string.min_bytes = 1];
+
+      // Percentage of traffic that should be dropped for the category.
+      envoy.type.Percent drop_percentage = 2;


@vishalpowar I'm a little late here, but I would recommend using FractionalPercent here. It will be higher performance in the data path. cc @zuercher @htuch

I can do that in another PR (this one is already merged) unless there is any restriction on making such changes in two different PRs.

@vishalpowar I would just do a follow up PR, it's fine to change it now since it just merged and no one is using it, unless there are objections from others, but we can discuss that in the follow up.

Yeah, sure, FractionalPercent makes sense here.

vishalpowar force-pushed the vishalpowar-drop-category branch 4 times, most recently from 5a5bc32 to d449441 Compare July 20, 2018 17:13

junr03 assigned zuercher Jul 20, 2018

zuercher reviewed Jul 23, 2018

View reviewed changes

htuch self-assigned this Jul 24, 2018

htuch suggested changes Jul 24, 2018

View reviewed changes

vishalpowar force-pushed the vishalpowar-drop-category branch 4 times, most recently from 443b195 to 9cc3f3b Compare July 25, 2018 01:24

htuch suggested changes Jul 25, 2018

View reviewed changes

vishalpowar force-pushed the vishalpowar-drop-category branch 3 times, most recently from b5e8b34 to 4ddabc1 Compare July 25, 2018 19:37

vishalpowar force-pushed the vishalpowar-drop-category branch from 4ddabc1 to 6839b73 Compare July 25, 2018 20:57

htuch approved these changes Jul 25, 2018

View reviewed changes

zuercher approved these changes Jul 25, 2018

View reviewed changes

zuercher merged commit 3ee3aa3 into envoyproxy:master Jul 25, 2018

mattklein123 reviewed Jul 26, 2018

View reviewed changes

vishalpowar deleted the vishalpowar-drop-category branch July 27, 2018 21:43

yanjunxiang-google mentioned this pull request Dec 1, 2023

Support drop_overload API #30827

Merged

Conversation

vishalpowar commented Jul 19, 2018

Uh oh!

zuercher 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!

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!

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!

Choose a reason for hiding this comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

htuch commented Jul 25, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone