-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adaptive Load FakeStepController doom update #492
Changes from all commits
8ea442d
5ac755a
b8c25a5
1c19c68
7050686
0776563
16fd8f6
c383010
6e1a483
4ef1140
4111bf4
871a959
1fd77c1
edc36b2
4d0364e
aed6d94
d9ae87d
a05a6f5
8cd4d21
d814a96
5f5a885
7e20a78
9048267
306c0ec
d33f543
442cca9
677b783
cefb366
f3684df
5463051
46e0e25
f634642
3c39faa
b9c8f2b
5fc4db4
64e7852
12807f1
e8e960f
3d97c2f
6306b4e
4525923
1ece783
0a9c0a5
59a8cc2
049baed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,8 @@ namespace Nighthawk { | |
|
||
/** | ||
* StepController for testing: Configurable convergence and doom countdowns, fixed RPS value. | ||
* | ||
* This class is not thread-safe. | ||
*/ | ||
class FakeStepController : public StepController { | ||
public: | ||
|
@@ -22,7 +24,7 @@ class FakeStepController : public StepController { | |
* @param config FakeStepControllerConfig proto for setting the fixed RPS value. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
* @param command_line_options_template A template for producing Nighthawk input. | ||
*/ | ||
FakeStepController(const nighthawk::adaptive_load::FakeStepControllerConfig& config, | ||
FakeStepController(nighthawk::adaptive_load::FakeStepControllerConfig config, | ||
nighthawk::client::CommandLineOptions command_line_options_template); | ||
/** | ||
* @return bool The current value of |is_converged_|. | ||
|
@@ -42,20 +44,23 @@ class FakeStepController : public StepController { | |
absl::StatusOr<nighthawk::client::CommandLineOptions> | ||
GetCurrentCommandLineOptions() const override; | ||
/** | ||
* Updates |is_converged_| to reflect whether |benchmark_result| contains any score >0. Sets | ||
* |is_doomed_| based whether the status in |benchmark_result| is OK; copies the status message | ||
* into |doomed_reason_| only when the status is not OK. | ||
* Updates |is_converged_| to reflect whether |benchmark_result| contains any score >0. Updates | ||
* |is_doomed_| to reflect whether |benchmark_result| contains any score <0. A non-converged, | ||
* non-doomed input has scores all equal to 0. | ||
* | ||
* @param benchmark_result A Nighthawk benchmark result proto. | ||
*/ | ||
void | ||
UpdateAndRecompute(const nighthawk::adaptive_load::BenchmarkResult& benchmark_result) override; | ||
|
||
private: | ||
// Counts down UpdateAndRecompute() calls. When this reaches zero, GetCurrentCommandLineOptions() | ||
// starts to return an artificial input value setting failure if one is specified in the config. | ||
int input_setting_failure_countdown_; | ||
const nighthawk::adaptive_load::FakeStepControllerConfig config_; | ||
bool is_converged_; | ||
bool is_doomed_; | ||
std::string doomed_reason_; | ||
const int fixed_rps_value_; | ||
const nighthawk::client::CommandLineOptions command_line_options_template_; | ||
}; | ||
|
||
|
@@ -91,7 +96,8 @@ MakeFakeStepControllerPluginConfig(int fixed_rps_value); | |
* Creates a valid TypedExtensionConfig proto that activates a FakeStepController with a | ||
* FakeInputVariableSetterConfig that fails validation. | ||
* | ||
* @param artificial_validation_error An error status. | ||
* @param artificial_validation_error An artificial error status to be returned by | ||
* FakeStepControllerConfigFactory::ValidateConfig() when attempting LoadStepControllerPlugin(). | ||
* | ||
* @return TypedExtensionConfig A proto that activates FakeStepController by name and includes | ||
* a FakeStepControllerConfig proto wrapped in an Any. This proto will fail validation when | ||
|
@@ -100,4 +106,21 @@ MakeFakeStepControllerPluginConfig(int fixed_rps_value); | |
envoy::config::core::v3::TypedExtensionConfig MakeFakeStepControllerPluginConfigWithValidationError( | ||
const absl::Status& artificial_validation_error); | ||
|
||
/** | ||
* Creates a valid TypedExtensionConfig proto that activates a FakeStepController with a | ||
* FakeInputVariableSetterConfig that returns an error from GetCurrentCommandLineOptions(). | ||
* | ||
* @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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we expand this comment, explaining what is the meaning of the error status, i.e. what it is used for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
* @param countdown Number of times UpdateAndRecompute() must be called before | ||
* GetCurrentCommandLineOptions() starts to return the input error status. | ||
* | ||
* @return TypedExtensionConfig A proto that activates FakeStepController by name and includes | ||
* a FakeStepControllerConfig proto wrapped in an Any. | ||
*/ | ||
envoy::config::core::v3::TypedExtensionConfig | ||
MakeFakeStepControllerPluginConfigWithInputSettingError( | ||
int fixed_rps_value, const absl::Status& artificial_input_setting_failure, int countdown); | ||
|
||
} // namespace Nighthawk |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,12 +5,25 @@ package nighthawk.adaptive_load; | |
import "envoy/config/core/v3/extension.proto"; | ||
import "google/rpc/status.proto"; | ||
|
||
// Configuration for FakeStepController (plugin name: "nighthawk.fake_step_controller") that always | ||
// returns a fixed RPS value and changes converged and doomed states based on the latest reported | ||
// BenchmarkResult. | ||
// Configuration for FakeStepController (plugin name: "nighthawk.fake_step_controller") that returns | ||
// a fixed RPS value and changes converged and doomed states based on the latest reported | ||
// BenchmarkResult. Can also be programmed to return a proto validation failure, return an error | ||
// from input value setting every time, or return an error after some number of UpdateAndRecompute() | ||
// iterations. | ||
message FakeStepControllerConfig { | ||
// RPS that should always be returned. Optional, default 0. | ||
// RPS that should always be returned, except when artificial errors are configured. Optional, | ||
// default 0. | ||
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. May be | ||
// used in conjunction with |artificial_input_setting_failure_countdown| to activate error | ||
// behavior after a delay. | ||
google.rpc.Status artificial_input_setting_failure = 3; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In order to test handling of these errors, we need the An alternative would be for magic values in This trick wouldn't be necessary if the step controller were aware of what stage it was operating in. |
||
// Relevant only when |artificial_input_setting_failure| is set. Number of calls to | ||
// UpdateAndRecompute() the controller must receive before it starts to return | ||
// |artificial_input_setting_failure|. Before this total is reached, |fixed_rps_value| is | ||
// returned. Optional, default 0, meaning the failure is returned regardless of calls to | ||
// UpdateAndRecompute(). | ||
int32 artificial_input_setting_failure_countdown = 4; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a complaint, but what is std::move accomplishing here, given that this isn't a smart pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.