Adaptive Load Controller main loop by eric846 · Pull Request #535 · envoyproxy/nighthawk

eric846 · 2020-09-14T22:46:57Z

This is the main function of the Adaptive Load Controller library:

Check the input proto for errors
Apply default input values
Adjusting Stage loop:
- Get the latest dynamically generated CommandLineOptions from the StepController
- Run a short benchmark with the Nighthawk Service
- Obtain values from MetricsPlugins
- Score metrics using ScoringFunction plugins
- Report scores back to the StepController, which recalculates the load for the next iteration
- Check for convergence deadline exceeded
- Check for StepController convergence
- Check for StepController doom
Testing Stage: Run one long benchmark on the Nighthawk Service at the converged load

Fixes #485.

Part 9 of splitting PR #483.

update from master

merge from upstream

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

…double Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

…variable_setter Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

merge from upstream

…ent.Output turns out not to include the status Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

…plugin names, log thresholds only once per session Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

oschaaf

Generally LGTM, it's quite cool to see everything come together. I feel it reads like poetry :-) except for the special way we treat duration, which still feels a tiny bit off; maybe a code level comment can help here. Somehow I'm also wondering if duration isn't something we should allow implementing as a parameter, like qps. E.g. to allow one to research a question like "how does test duration influence convergence speed at a steady rps".
Left a few small other remarks.

source/adaptive_load/adaptive_load_controller_impl.cc

dubious90 · 2020-09-15T15:40:48Z

@pamorgan Please review and assign back to me once done.

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

oschaaf

lgtm

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

pamorgan

some nits but LGTM.

pamorgan · 2020-09-18T14:07:58Z

include/nighthawk/adaptive_load/adaptive_load_controller.h

+  /**
+   * Performs an adaptive load session consisting of the Adjusting Stage and the
+   * Testing Stage. Adjusting Stage: Runs a series of short benchmarks, checks metrics according to
+   * MetricSpecs, and adjusts load up or down based on the result; returns an error if convergence


nit: extra quote.

Reformatted this

pamorgan · 2020-09-18T14:11:55Z

include/nighthawk/adaptive_load/adaptive_load_controller.h

+   * computed by a StepController plugin. Metric values are obtained through MetricsPlugins. Testing
+   * Stage: When the optimal load is found, runs one long benchmark to validate it.
+   *
+   * @param nighthawk_service_stub A Nighthawk Service gRPC stub.


nit: Maybe stub is more appropriate as a member in the class than an argument to the method. I don't think we expect it to change on a multiple call scenario.

This class is intended to be stateless helpers and it's only a class to make mocking easier. We will have a chance to design a more convenient client that might be stateful when we merge RemoteProcessImpl and NighthawkServiceClientImpl.

pamorgan · 2020-09-18T14:13:06Z

include/nighthawk/adaptive_load/adaptive_load_controller.h

-absl::StatusOr<nighthawk::adaptive_load::AdaptiveLoadSessionOutput> PerformAdaptiveLoadSession(
-    nighthawk::client::NighthawkService::StubInterface* nighthawk_service_stub,
-    const nighthawk::adaptive_load::AdaptiveLoadSessionSpec& spec, Envoy::TimeSource& time_source);
+class AdaptiveLoadController {


should this be renamed to interface now, since we now a pure methods.

The convention for Nighthawk seems to be that everything under include should be pure but no existing interfaces have Interface in their name.

pamorgan · 2020-09-18T14:21:31Z

source/adaptive_load/adaptive_load_controller_impl.h

+   *       controller.PerformAdaptiveLoadSession(&nighthawk_service_stub, spec);
+   *
+   * @param nighthawk_service_client A helper that executes Nighthawk Service benchmarks given a
+   * gRPC stub.


What is the reason behing seperating the client from the stub?

This is for historical reasons: We had a helper that made it easier to call the stub, then moved the helper into its own class because it's only easy to mock out methods when they are in a class.

I'm not sure what will happen with this, but we may add a more convenient Nighthawk Service client wrapper object that contains the stub, and then take this wrapper instead. Or the stub might be the best thing for the public API to take.

pamorgan · 2020-09-18T14:23:57Z

source/adaptive_load/adaptive_load_controller_impl.h

+   * metrics from MetricsPlugins, scores the results, and consults a StepController plugin to
+   * determine the next load and detect convergence. All plugins are specified through the
+   * AdaptiveLoadSessionSpec proto.
+   *


can you add a comment on the thread safety? Can you run concurrent sessions?

pamorgan · 2020-09-18T14:25:28Z

source/adaptive_load/adaptive_load_controller_impl.cc

+  AdaptiveLoadSessionSpec spec = session_spec_proto_helper_.SetSessionSpecDefaults(input_spec);
+  absl::Status validation_status = session_spec_proto_helper_.CheckSessionSpec(spec);
+  if (!validation_status.ok()) {
+    return validation_status;


Should we log reasons of failures.

pamorgan · 2020-09-18T14:29:27Z

source/adaptive_load/adaptive_load_controller_impl.cc

+        absl::StrCat("Step controller determined that it can never converge: ", doom_reason));
+  }
+
+  absl::StatusOr<BenchmarkResult> result_or = PerformAndAnalyzeNighthawkBenchmark(


Can you comment on why calling this again after complete.

Done (this is the testing stage)

BTW we have #539 to consider making the testing stage an optional thing, decided by the StepController, so this special call might go away.

pamorgan · 2020-09-18T14:30:25Z

source/adaptive_load/adaptive_load_controller_impl.cc

+      return absl::DeadlineExceededError(absl::StrFormat(
+          "Failed to converge before deadline of %.2f seconds.", time_limit_ns.count() / 1e9));
+    }
+  } while (!step_controller->IsConverged() && !step_controller->IsDoomed(doom_reason));


Usually while loops are more readable than do/while. Is there a reason not to use a simple while loop here?

We had discussed this, I think the motivation for do/while is that IsConverged and IsDoomed don't have meaningful answers until the loop body has executed.

pamorgan · 2020-09-18T14:37:13Z

source/adaptive_load/adaptive_load_controller_impl.cc

+    }
+    BenchmarkResult result = result_or.value();
+    *output.mutable_adjusting_stage_results()->Add() = result;
+    step_controller->UpdateAndRecompute(result);


It took me a second to follow the logic because step_controller->GetCurrentCommandLineOptions and step_controller->UpdateAndRecompute(result); are called in 2 different methods.
I think it would be easier to follow if you moved step_controller->GetCurrentCommandLineOptions to this method.

Just moved UpdateAndRecompute into the helper, which hopefully makes the same amount of sense. I had moved GetCurrentCommandLineOptions to the helper to reduce duplication in the main loop.

pamorgan · 2020-09-18T14:45:05Z

source/adaptive_load/adaptive_load_controller_impl.cc

+  absl::flat_hash_map<std::string, MetricsPluginPtr> name_to_custom_metrics_plugin_map;
+  for (const envoy::config::core::v3::TypedExtensionConfig& config :
+       spec.metrics_plugin_configs()) {
+    absl::StatusOr<MetricsPluginPtr> metrics_plugin_or = LoadMetricsPlugin(config);


nit: usually it's better to perform the load right after validations and pass in the metric plugins to avoid the assert. No action needed if it's too much of code refactoring.

Agreed, it's a bit awkward for historical reasons -- filed #547

…o helper Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

mum4k

Looks great, it is obvious that a lot of work was done here to improve readability. This PR only contains the business logic of the adaptive controller. The main functions and test cases are easy to follow. Thank you for taking all the additional steps.

eric846 added 30 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 <56563761+eric846@users.noreply.github.com>

fix comments

7050686

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix format

0776563

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

rename adaptive_rps to adaptive_load

16fd8f6

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add field_selector in example

c383010

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix example comment

6e1a483

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix format

4ef1140

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add support for fault injection headers

4111bf4

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

replace linear and binary search with exponential search

871a959

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add InputVariableSetter mechanism

1fd77c1

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add input variable setter to build file

edc36b2

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix syntax errors

4d0364e

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

rename samples/adaptive_rps

aed6d94

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

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

d9ae87d

…double Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add proto validation rules, fix comments, make rps the default input_…

a05a6f5

…variable_setter Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix comment wording

8cd4d21

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

simplify protos, add defaults, specify required or optional

d814a96

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add missing newline

5f5a885

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Kick CI

7e20a78

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

simplify protos

9048267

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix format

306c0ec

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix some optional field comments and rules

d33f543

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Merge pull request #10 from envoyproxy/master

442cca9

merge from upstream

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

677b783

…ent.Output turns out not to include the status Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

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

cefb366

…plugin names, log thresholds only once per session Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

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

f3684df

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

create headers

5463051

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

eric846 added 2 commits September 14, 2020 18:39

Merge branch 'master2' into adaptive-rps-main-loop-short

27dbf6f

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

fix comments

d22ca48

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

eric846 marked this pull request as ready for review September 14, 2020 23:09

eric846 added the waiting-for-review A PR waiting for a review. label Sep 14, 2020

oschaaf requested changes Sep 15, 2020

View reviewed changes

dubious90 requested a review from pamorgan September 15, 2020 15:40

handle chrono-duration in more standard way, remove using namespace

5918b25

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

oschaaf previously approved these changes Sep 15, 2020

View reviewed changes

eric846 mentioned this pull request Sep 15, 2020

Consider generalizing the adaptive load "testing stage" #539

Open

fix asan

1882f2d

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

eric846 dismissed oschaaf’s stale review via 1882f2d September 15, 2020 22:06

eric846 added 2 commits September 15, 2020 18:37

Merge remote-tracking branch 'upstream/master' into master2

9d1cf53

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Merge branch 'master2' into adaptive-rps-main-loop-short

63f5c5e

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

pamorgan previously approved these changes Sep 18, 2020

View reviewed changes

mum4k added 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 Sep 18, 2020

improve comments, simplify main loop by moving UpdateAndRecompute int…

170909f

…o helper Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

eric846 dismissed pamorgan’s stale review via 170909f September 19, 2020 02:25

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 Sep 19, 2020

eric846 added 3 commits September 18, 2020 22:49

Merge remote-tracking branch 'upstream/master' into master2

cfad844

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

Merge branch 'master2' into adaptive-rps-main-loop-short

cf669b2

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

add more error logging

4d009ae

Signed-off-by: eric846 <56563761+eric846@users.noreply.github.com>

eric846 requested a review from pamorgan September 21, 2020 17:18

eric846 assigned dubious90 Sep 22, 2020

mum4k assigned mum4k and unassigned dubious90 Sep 22, 2020

mum4k approved these changes Sep 22, 2020

View reviewed changes

mum4k merged commit 401f09e into envoyproxy:master Sep 22, 2020

Conversation

eric846 commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dubious90 commented Sep 15, 2020

Uh oh!

oschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

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

eric846 Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

eric846 Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

eric846 Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eric846 commented Sep 14, 2020 •

edited

Loading

eric846 Sep 18, 2020 •

edited

Loading

eric846 Sep 18, 2020 •

edited

Loading

eric846 Sep 18, 2020 •

edited

Loading