Adaptive Load FakeStepController doom update by eric846 · Pull Request #492 · envoyproxy/nighthawk

eric846 · 2020-08-27T03:15:56Z

Updates FakeStepController:

Set doomed state when FakeStepController::UpdateAndRecompute() receives a BenchmarkResult with a negative metric score. Previously BenchmarkResult could contain an error status, and we used the error status to trigger simulated doom. Now errors do not get passed to StepController::UpdateAndRecompute(), so we need a new mechanism.
Adds the ability to simulate an InputVariableSetter failure in GetCurrentCommandLineOptions(). The artificial failure is specified in the config proto.
Adds a countdown mechanism so that GetCurrentCommandLineOptions() returns successfully until UpdateAndRecompute() has been called a configured number of times, then starts returning the simulated input variable setter failure mentioned above. This simple autonomous behavior is needed to test error handling in the testing stage without triggering an early exit in the adjusting stage; the adaptive load controller can't be paused in the middle of the test to reconfigure the FakeStepController to change its behavior.

The behavior of FakeStepController after an UpdateAndRecompute() with a BenchmarkResult containing the given metric scores:

Any positive score: converged
All scores zero: neither converged nor doomed
Any negative score: doomed

Part 3 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>

dubious90

Mostly one big question that I want more clarity on.

dubious90 · 2020-08-27T19:46:51Z

test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller.cc

    nighthawk::client::CommandLineOptions command_line_options_template)
-    : is_converged_{false}, is_doomed_{false}, fixed_rps_value_{config.fixed_rps_value()},
+    : input_setting_failure_countdown_{config.artificial_input_setting_failure_countdown()},
+      config_{std::move(config)}, is_converged_{false}, is_doomed_{false},


not a complaint, but what is std::move accomplishing here, given that this isn't a smart pointer?

Funny story, this was actually required by clang-tidy. If the function implementation is making a copy of a const reference parameter, clang-tidy tells you to change the parameter to be pass by value so it's obvious to the caller that a copy is happening. The argument gets copied at the call site into an unnamed temporary value. Then in the constructor we can actually move this temporary value into the field without incurring the cost of a second copy.

dubious90 · 2020-08-27T21:18:08Z

test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller.proto

  int32 fixed_rps_value = 1;
  // Artificial error that the plugin factory should return during validation. Optional.
  google.rpc.Status artificial_validation_failure = 2;
+  // Artificial error that should be returned from GetCurrentCommandLineOptions(). Optional.


Can we enhance this comment to indicate the relation to the countdown?

dubious90 · 2020-08-27T21:21:31Z

test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller.proto

  // Artificial error that the plugin factory should return during validation. Optional.
  google.rpc.Status artificial_validation_failure = 2;
+  // Artificial error that should be returned from GetCurrentCommandLineOptions(). Optional.
+  google.rpc.Status artificial_input_setting_failure = 3;


I think I'm having trouble understanding the full use case here. If this is 3, then we are going to succeed 3 times, then fail on the 4th attempt. Makes sense, but I'm not sure I understand why that's useful. What is the test that you're supporting by creating it in this way?

GetCurrentCommandLine() can return an error status that should be handled cleanly by the main controller loop. The controller calls GetCurrentCommandLine() repeatedly during the adjusting stage until the step controller says it's converged, and then in the testing stage we call GetCurrentCommandLine() one last time, reusing the last converged value.

In order to test handling of these errors, we need the FakeStepController to return successfully from GetCurrentCommandLine() during the adjusting stage, but then start returning errors just in time for the testing stage. We don't have any way to update the FakeStepController during the run, so it has to somehow be programmed up front to behave differently at different times.

An alternative would be for magic values in UpdateAndRecompute() to trigger GetCurrentCommandLine() error behavior. We already use magic values to control convergence and doom. But there's only so much information we can encode in metric score doubles without having it get out of hand. Currently the UpdateAndRecompute() behavior is: zero scores=non-converged non-doomed, any positive score=converged, any negative score=doomed.

This trick wouldn't be necessary if the step controller were aware of what stage it was operating in.

dubious90 · 2020-08-27T21:23:30Z

@mum4k Can you give this a review and assign back to me when done?

mum4k · 2020-08-27T22:56:11Z

@eric846 please address / respond to the comments from @dubious90 as that can help me by providing additional context for the review.

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

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

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

oschaaf

Just a const nit, LGTM otherwise. Left a question that popped up over at #466 (comment) as that's out of scope here.

test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller_test.cc

mum4k

Looks good with just a few nits.

mum4k · 2020-08-28T16:53:10Z

test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller.h

+ *
+ * @param fixed_rps_value Value for RPS to set in the FakeStepControllerConfig proto until the
+ * countdown reaches zero.
+ * @param artificial_input_setting_failure An error status.


Can we expand this comment, explaining what is the meaning of the error status, i.e. what it is used for?

mum4k · 2020-08-28T16:56:15Z

test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller.h

@@ -22,7 +22,7 @@ class FakeStepController : public StepController {
   * @param config FakeStepControllerConfig proto for setting the fixed RPS value.


(pre-existing, optional) We should probably mention in the class comment that this class isn't thread-safe. At least I am assuming it isn't meant to be.

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

oschaaf

Thanks for resolving the const nit. LGTM for my part of the review.

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 August 27, 2020 16:59

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

6306b4e

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

merge from upstream

4525923

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

dubious90 suggested changes Aug 27, 2020

View reviewed changes

dubious90 assigned mum4k and unassigned dubious90 Aug 27, 2020

mum4k requested a review from oschaaf August 27, 2020 22:56

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 Aug 27, 2020

eric846 added 3 commits August 27, 2020 22:01

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

1ece783

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

Merge branch 'master2' into adaptive-rps-fake-step-controller-doom-up…

0a9c0a5

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

fix merge conflict

59a8cc2

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

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 Aug 28, 2020

oschaaf mentioned this pull request Aug 28, 2020

Consider consolidating IsConverged and IsDoomed in Adaptive Load StepController #466

Open

oschaaf previously approved these changes Aug 28, 2020

View reviewed changes

test/adaptive_load/fake_plugins/fake_step_controller/fake_step_controller_test.cc Outdated Show resolved Hide resolved

oschaaf 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 Aug 28, 2020

mum4k previously approved these changes Aug 28, 2020

View reviewed changes

mum4k assigned dubious90 and unassigned mum4k Aug 28, 2020

add missing const, improve comments

049baed

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

eric846 dismissed stale reviews from mum4k and oschaaf via 049baed August 28, 2020 17:13

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 Aug 28, 2020

oschaaf approved these changes Aug 28, 2020

View reviewed changes

dubious90 approved these changes Aug 29, 2020

View reviewed changes

dubious90 merged commit 886702f into envoyproxy:master Aug 29, 2020

		@@ -22,7 +22,7 @@ class FakeStepController : public StepController {
		* @param config FakeStepControllerConfig proto for setting the fixed RPS value.

Conversation

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

Uh oh!

Uh oh!

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

dubious90 commented Aug 27, 2020

Uh oh!

mum4k commented Aug 27, 2020

Uh oh!

oschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

oschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eric846 commented Aug 27, 2020 •

edited

Loading