Adaptive Load Controller protos #398

eric846 · 2020-07-09T02:01:43Z

All protos, including plugin-specific config protos for all plugins we plan to ship initially.

Works on #416

update from master

merge from upstream

Signed-off-by: eric846 <[email protected]>

mum4k · 2020-07-09T19:46:41Z

@oschaaf please assign to me once you are done reviewing this PR (after Eric implements the latest round of changes).

Signed-off-by: eric846 <[email protected]>

eric846 · 2020-07-10T04:01:29Z

Hi Otto,
This should be ready to review now. Renamed Adaptive RPS Controller to Adaptive Load Controller to reflect the general capability.

The step_controller_impl.proto configs now take a NighthawkFieldSelector (defined in step_controller.proto) enum input that tells the controller what single field within CommandLineOptions it should adjust dynamically. I included every numeric field in the enum that I didn't know to rule out. It's possible that some of the fields I included in NighthawkFieldSelector don't actually make sense and should be removed.

Future numerical fields added to CommandLineOptions would need a corresponding value added to NighthawkFieldSelector.

We will ship only basic single-variable StepControllers at first, but they will already work equally well for any field, not just RPS.

Advanced multivariable optimization StepControllers could still use NighthawkFieldSelector fields to specify the multiple variables.

Thanks!

Signed-off-by: eric846 <[email protected]>

eric846 · 2020-07-11T00:44:42Z

Updated again to remove some of the hard-coded stuff for setting multiple field and instead use plugins, which actually simplifies the design.

Signed-off-by: eric846 <[email protected]>

…double Signed-off-by: eric846 <[email protected]>

oschaaf

Generally looks great to me, just a couple of toughts and a possible proto3/validation nit.

api/adaptive_load/metric_spec.proto

api/adaptive_load/adaptive_load.proto

samples/adaptive_load/example_session_spec.textproto

oschaaf

Thanks for the explanations, LGTM!

eric846 · 2020-07-15T19:07:46Z

@mum4k PTAL, simplified protos as discussed

Signed-off-by: eric846 <[email protected]>

merge from upstream

mum4k

Still looks good, let's wait to see what Harvey thinks about this.

dubious90 · 2020-07-17T19:37:41Z

/retest

repokitteh-read-only · 2020-07-17T19:37:43Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #398 (comment) was created by @dubious90.

see: more, trace.

…ent.Output turns out not to include the status Signed-off-by: eric846 <[email protected]>

htuch

Generally looks great, a few questions/comments.

api/adaptive_load/config.proto

api/adaptive_load/adaptive_load.proto

api/adaptive_load/benchmark_result.proto

api/adaptive_load/metric_spec.proto

…plugin names, log thresholds only once per session Signed-off-by: eric846 <[email protected]>

Signed-off-by: eric846 <[email protected]>

mum4k · 2020-07-22T13:59:48Z

@pamorgan please review and assign back to me when done.

htuch

API LGTM modulo comment.

htuch · 2020-07-22T22:42:26Z

api/adaptive_load/metric_spec.proto

+message MetricSpec {
+  // Name of the metric to evaluate. For the set of built-in metric names, see
+  // source/adaptive_load/metrics_plugin_impl.cc. Required.
+  string metric_name = 1 [(validate.rules).string.min_len = 1];


How do plugins and metric names relate? Does each plugin only export out a fixed number of metric names? Or is metric name an opaque ID?

Each plugin will support its own fixed set of metric names. We will query each plugin for its metric names at startup in order to validate the MetricSpec in the adaptive load session input proto.

For nighthawk.builtin:

latency-min

latency-mean

latency-mean-plus-1stdev

latency-mean-plus-2stdev

latency-mean-plus-3stdev

latency-max

achieved-rps

attempted-rps

send-rate

success-rate

SGTM. I'm going to be OOTO next two weeks, so please go ahead with this set of protos. I think as long as we're the only consumers, which is highly likely for the near future, it's fine to iterate as needed.

Signed-off-by: eric846 <[email protected]>

mum4k · 2020-07-23T19:41:21Z

fyi pending review by @pamorgan. Please assign to me once done.

pamorgan · 2020-07-24T16:21:14Z

api/adaptive_load/adaptive_load.proto

+  BenchmarkResult testing_stage_result = 3;
+  // Metrics and thresholds that were used to determine load adjustments, as referenced in the
+  // BenchmarkResults.
+  repeated MetricSpecWithThreshold metric_thresholds = 4;


Can this be different than the AdaptiveLoadSessionSpec metric_thresholds. If not, then maybe this is redundant.

It's copied directly from metric_thresholds in the input. I included it in the output to preserve the context when dumping the output proto to an archive. Otherwise the archive would only have the value and the score, but not the threshold. If somebody just archived the full input proto alongside the output proto, this field would be redundant.

pamorgan · 2020-07-24T16:22:29Z

api/adaptive_load/adaptive_load.proto

+  // The duration of the single benchmark session of the testing stage to
+  // confirm the performance at the level of load found in the adjusting stage.
+  // Required.
+  google.protobuf.Duration testing_stage_duration = 7


Can we add a default reasonable value for this?

pamorgan · 2020-07-24T16:36:04Z

api/adaptive_load/benchmark_result.proto

+  // Service input spec.
+  nighthawk.client.Output nighthawk_service_output = 1;
+  // Status of this Nighthawk Service benchmark session.
+  google.rpc.Status status = 2;


can we add more comments on what the status should we get for special errors. For example what is the error if we never converge?

Clarified the comment on this field, which applies narrowly to failures to call the Nighthawk Service and internal errors returned by Nighthawk Service.

Also extended the comment in AdaptiveLoadSessionOutput where things like convergence status are recorded.

pamorgan · 2020-07-24T16:42:16Z

api/adaptive_load/metric_spec.proto

+  string metric_name = 1 [(validate.rules).string.min_len = 1];
+  // Name of the MetricsPlugin providing the metric ("nighthawk.builtin" for built-in).
+  // Required.
+  string metrics_plugin_name = 2 [(validate.rules).string.min_len = 1];


can we add default for "nighthawk.builtin"?

pamorgan · 2020-07-24T17:01:40Z

api/adaptive_load/BUILD

@@ -0,0 +1,21 @@
+load("@envoy_api//bazel:api_build_system.bzl", "api_cc_py_proto_library")


I have some nits but LGTM

Signed-off-by: eric846 <[email protected]>

eric846 added 6 commits June 1, 2020 17:23

Merge pull request #5 from envoyproxy/master

8ea442d

update from master

Merge pull request #6 from envoyproxy/master

5ac755a

merge from upstream

Merge pull request #7 from envoyproxy/master

b8c25a5

merge from upstream

initial commit

1c19c68

Signed-off-by: eric846 <[email protected]>

fix comments

7050686

Signed-off-by: eric846 <[email protected]>

fix format

0776563

Signed-off-by: eric846 <[email protected]>

eric846 requested review from oschaaf and mum4k July 9, 2020 02:02

eric846 added waiting-for-review A PR waiting for a review. waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Jul 9, 2020

eric846 added 4 commits July 9, 2020 23:27

rename adaptive_rps to adaptive_load

16fd8f6

Signed-off-by: eric846 <[email protected]>

add field_selector in example

c383010

Signed-off-by: eric846 <[email protected]>

fix example comment

6e1a483

Signed-off-by: eric846 <[email protected]>

fix format

4ef1140

Signed-off-by: eric846 <[email protected]>

eric846 changed the title ~~Adaptive RPS protos~~ Adaptive Load Controller protos Jul 10, 2020

eric846 added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Jul 10, 2020

eric846 added 5 commits July 10, 2020 12:09

add support for fault injection headers

4111bf4

Signed-off-by: eric846 <[email protected]>

replace linear and binary search with exponential search

871a959

Signed-off-by: eric846 <[email protected]>

add InputVariableSetter mechanism

1fd77c1

Signed-off-by: eric846 <[email protected]>

add input variable setter to build file

edc36b2

Signed-off-by: eric846 <[email protected]>

fix syntax errors

4d0364e

Signed-off-by: eric846 <[email protected]>

eric846 added 2 commits July 10, 2020 22:49

rename samples/adaptive_rps

aed6d94

Signed-off-by: eric846 <[email protected]>

improve comments, change step controller initial value from int64 to …

d9ae87d

…double Signed-off-by: eric846 <[email protected]>

oschaaf reviewed Jul 13, 2020

View reviewed changes

oschaaf previously approved these changes Jul 13, 2020

View reviewed changes

eric846 added 2 commits July 15, 2020 15:45

fix some optional field comments and rules

d33f543

Signed-off-by: eric846 <[email protected]>

Merge pull request #10 from envoyproxy/master

442cca9

merge from upstream

mum4k previously approved these changes Jul 17, 2020

View reviewed changes

eric846 dismissed mum4k’s stale review via 070e404 July 19, 2020 23:40

add Nighthawk status field in BenchmarkResult as nested nighthawk.cli…

677b783

…ent.Output turns out not to include the status Signed-off-by: eric846 <[email protected]>

eric846 force-pushed the adaptive-rps-protos2 branch from 070e404 to 677b783 Compare July 19, 2020 23:42

htuch reviewed Jul 20, 2020

View reviewed changes

api/adaptive_load/config.proto Outdated Show resolved Hide resolved

api/adaptive_load/adaptive_load.proto Show resolved Hide resolved

api/adaptive_load/benchmark_result.proto Outdated Show resolved Hide resolved

api/adaptive_load/metric_spec.proto Outdated Show resolved Hide resolved

eric846 added 2 commits July 21, 2020 20:33

switch to standard Envoy plugin config proto, add prefix to internal …

cefb366

…plugin names, log thresholds only once per session Signed-off-by: eric846 <[email protected]>

Merge remote-tracking branch 'upstream/master' into adaptive-rps-protos2

f3684df

Signed-off-by: eric846 <[email protected]>

eric846 mentioned this pull request Jul 22, 2020

Adaptive Load Controller headers #417

Merged

eric846 requested a review from htuch July 22, 2020 02:05

mum4k requested a review from pamorgan July 22, 2020 13:59

mum4k removed their assignment Jul 22, 2020

htuch reviewed Jul 22, 2020

View reviewed changes

eric846 added 2 commits July 22, 2020 19:08

Kick CI

b71f060

Signed-off-by: eric846 <[email protected]>

Kick CI

eb495b8

Signed-off-by: eric846 <[email protected]>

mum4k self-assigned this Jul 23, 2020

mum4k removed their assignment Jul 23, 2020

pamorgan reviewed Jul 24, 2020

View reviewed changes

eric846 added 2 commits July 24, 2020 20:58

add more proto defaults and comments

3659817

Signed-off-by: eric846 <[email protected]>

fix nighthawk.rps comment

bbc8d32

Signed-off-by: eric846 <[email protected]>

mum4k self-assigned this Jul 28, 2020

pamorgan approved these changes Jul 28, 2020

View reviewed changes

mum4k approved these changes Jul 29, 2020

View reviewed changes

mum4k merged commit 67d0f96 into envoyproxy:master Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adaptive Load Controller protos #398

Adaptive Load Controller protos #398

eric846 commented Jul 9, 2020 •

edited by mum4k

Loading

mum4k commented Jul 9, 2020

eric846 commented Jul 10, 2020

eric846 commented Jul 11, 2020

oschaaf left a comment

oschaaf left a comment

eric846 commented Jul 15, 2020

mum4k left a comment

dubious90 commented Jul 17, 2020

repokitteh-read-only bot commented Jul 17, 2020

htuch left a comment

mum4k commented Jul 22, 2020

htuch left a comment

htuch Jul 22, 2020

eric846 Jul 22, 2020

htuch Jul 23, 2020

mum4k commented Jul 23, 2020

pamorgan Jul 24, 2020

eric846 Jul 24, 2020

pamorgan Jul 24, 2020

eric846 Jul 25, 2020

pamorgan Jul 24, 2020

eric846 Jul 25, 2020

pamorgan Jul 24, 2020

eric846 Jul 25, 2020

pamorgan Jul 24, 2020

		@@ -0,0 +1,21 @@
		load("@envoy_api//bazel:api_build_system.bzl", "api_cc_py_proto_library")

Adaptive Load Controller protos #398

Adaptive Load Controller protos #398

Conversation

eric846 commented Jul 9, 2020 • edited by mum4k Loading

mum4k commented Jul 9, 2020

eric846 commented Jul 10, 2020

eric846 commented Jul 11, 2020

oschaaf left a comment

Choose a reason for hiding this comment

oschaaf left a comment

Choose a reason for hiding this comment

eric846 commented Jul 15, 2020

mum4k left a comment

Choose a reason for hiding this comment

dubious90 commented Jul 17, 2020

repokitteh-read-only bot commented Jul 17, 2020

htuch left a comment

Choose a reason for hiding this comment

mum4k commented Jul 22, 2020

htuch 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

mum4k commented Jul 23, 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

Choose a reason for hiding this comment

eric846 commented Jul 9, 2020 •

edited by mum4k

Loading