Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Adaptive Load command line client #616

Merged
merged 79 commits into from
Feb 11, 2021
Merged

Adaptive Load command line client #616

merged 79 commits into from
Feb 11, 2021

Conversation

eric846
Copy link
Contributor

@eric846 eric846 commented Feb 3, 2021 •
edited
Loading

An adaptive load command line executable that drives a Nighthawk Service with a series of benchmarks, varying the RPS or another input variable dynamically.

bazel build -c opt :nighthawk

bazel-bin/nighthawk_adaptive_load_client

  • --spec-file pathname
    • Pathname of an AdaptiveLoadSessionSpec textproto defining the session
    • Required
    • Example:test/adaptive_load/test_data/valid_session_spec.textproto
  • --output-file pathname
    • Pathname to write the AdaptiveLoadSessionOutput textproto session output
    • Required
  • --nighthawk-service-address host:port
    • Address of the Nighthawk Service to be driven by the adaptive load controller over gRPC
    • Default localhost:8443
  • --use-tls
    • Use TLS for the gRPC connection when sending commands to the Nighthawk Service
    • Default false (insecure connection)

Note that to run an adaptive load session, you must also have a Nighthawk Service running, e.g.:
bazel-bin/nighthawk_service & (to run at localhost:8443)

Part of #416.

eric846 added 30 commits June 1, 2020 17:23
@eric846
Merge pull request #5 from envoyproxy/master
8ea442d 
update from master
@eric846
Merge pull request #6 from envoyproxy/master
5ac755a 
merge from upstream
@eric846
Merge pull request #7 from envoyproxy/master
b8c25a5 
merge from upstream
@eric846
initial commit
1c19c68 
Signed-off-by: eric846 <[email protected]>
@eric846
fix comments
7050686 
Signed-off-by: eric846 <[email protected]>
@eric846
fix format
0776563 
Signed-off-by: eric846 <[email protected]>
@eric846
rename adaptive_rps to adaptive_load
16fd8f6 
Signed-off-by: eric846 <[email protected]>
@eric846
add field_selector in example
c383010 
Signed-off-by: eric846 <[email protected]>
@eric846
fix example comment
6e1a483 
Signed-off-by: eric846 <[email protected]>
@eric846
fix format
4ef1140 
Signed-off-by: eric846 <[email protected]>
@eric846
add support for fault injection headers
4111bf4 
Signed-off-by: eric846 <[email protected]>
@eric846
replace linear and binary search with exponential search
871a959 
Signed-off-by: eric846 <[email protected]>
@eric846
add InputVariableSetter mechanism
1fd77c1 
Signed-off-by: eric846 <[email protected]>
@eric846
add input variable setter to build file
edc36b2 
Signed-off-by: eric846 <[email protected]>
@eric846
fix syntax errors
4d0364e 
Signed-off-by: eric846 <[email protected]>
@eric846
rename samples/adaptive_rps
aed6d94 
Signed-off-by: eric846 <[email protected]>
@eric846
improve comments, change step controller initial value from int64 to …
d9ae87d 
…double

Signed-off-by: eric846 <[email protected]>
@eric846
add proto validation rules, fix comments, make rps the default input_…
a05a6f5 
…variable_setter

Signed-off-by: eric846 <[email protected]>
@eric846
fix comment wording
8cd4d21 
Signed-off-by: eric846 <[email protected]>
@eric846
simplify protos, add defaults, specify required or optional
d814a96 
Signed-off-by: eric846 <[email protected]>
@eric846
add missing newline
5f5a885 
Signed-off-by: eric846 <[email protected]>
@eric846
Kick CI
7e20a78 
Signed-off-by: eric846 <[email protected]>
@eric846
simplify protos
9048267 
Signed-off-by: eric846 <[email protected]>
@eric846
fix format
306c0ec 
Signed-off-by: eric846 <[email protected]>
@eric846
fix some optional field comments and rules
d33f543 
Signed-off-by: eric846 <[email protected]>
@eric846
Merge pull request #10 from envoyproxy/master
442cca9 
merge from upstream
@eric846
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
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]>
@eric846
Merge remote-tracking branch 'upstream/master' into adaptive-rps-protos2
f3684df 
Signed-off-by: eric846 <[email protected]>
@eric846
create headers
5463051 
Signed-off-by: eric846 <[email protected]>
@eric846
Copy link
Contributor Author

eric846 commented Feb 4, 2021

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: docker (failed build)
🔨 rebuilding ci/circleci: integration_test_coverage (failed build)

🐱

Caused by: a #616 (comment) was created by @eric846.

see: more, trace.

@eric846 eric846 marked this pull request as ready for review February 5, 2021 17:27
@eric846 eric846 added the waiting-for-review A PR waiting for a review. label Feb 5, 2021
@dubious90 dubious90 assigned wjuan-AFK and unassigned wjuan-AFK Feb 5, 2021
@dubious90 dubious90 requested a review from wjuan-AFK February 5, 2021 19:06
@dubious90
Copy link
Contributor

@wjuan-AFK Please review and assign to me when done. Also note that we believe the CI failure to be unrelated to these changes.

@oschaaf
Copy link
Member

oschaaf commented Feb 5, 2021

@wjuan-AFK Please review and assign to me when done. Also note that we believe the CI failure to be unrelated to these changes.

@dubious90 Created #620 to address the ci trouble with respect to docker.

wjuan-AFK
wjuan-AFK reviewed Feb 9, 2021
View reviewed changes
source/adaptive_load/adaptive_load_client_main.h
* writes the output proto to a file. File paths are taken from class members initialized in the
* constructor.
*
* @return Exit code for this process.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we state that 0 is good or is that implicit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a standard in linux, so we can rely on that understanding. https://linuxize.com/post/bash-exit/#exit-status

wjuan-AFK
wjuan-AFK reviewed Feb 9, 2021
View reviewed changes
source/adaptive_load/adaptive_load_client_main.cc Outdated
}
const Envoy::Api::IoCallSizeResult write_result = file->write(contents);
if (!write_result.ok()) {
throw Nighthawk::NighthawkException(absl::StrCat("Unable to write output file \"", path,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: write to output file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

wjuan-AFK
wjuan-AFK reviewed Feb 9, 2021
View reviewed changes
Copy link
Contributor

@wjuan-AFK wjuan-AFK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

dubious90
dubious90 previously approved these changes Feb 11, 2021
View reviewed changes
source/adaptive_load/adaptive_load_client_main.cc
output_filename_ = output_filename.getValue();
}

uint32_t AdaptiveLoadClientMain::Run() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we file a future bug to enable some dependency injection of the channel here? in the future, people might want to enable other ways of connecting to the nighthawk service besides the two provided here. but we don't need to prematurely allow for that case until we have a specific usecase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #626.

source/adaptive_load/adaptive_load_client_main.h
* writes the output proto to a file. File paths are taken from class members initialized in the
* constructor.
*
* @return Exit code for this process.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a standard in linux, so we can rely on that understanding. https://linuxize.com/post/bash-exit/#exit-status

@eric846
Copy link
Contributor Author

eric846 commented Feb 11, 2021

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: integration_test_coverage (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #616 (comment) was created by @eric846.

see: more, trace.

@dubious90 dubious90 merged commit ff42695 into envoyproxy:main Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjuan-AFK wjuan-AFK wjuan-AFK left review comments

@dubious90 dubious90 dubious90 approved these changes

Assignees

@dubious90 dubious90

Labels
waiting-for-review A PR waiting for a review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eric846 @dubious90 @oschaaf @wjuan-AFK