CLI - cancel execution upon signal reception by oschaaf · Pull Request #367 · envoyproxy/nighthawk

oschaaf · 2020-06-17T10:23:13Z

Teach the CLI to handle SIGTERM/SIGINT, and handle those as a request to
gracefully cancel execution.

Partially resolves #280

Prerequisite: Land Extract SignalHandler class for re-use #282

In preparation of sharing functionality for signal handling, extract what we have right now into SignalHandler. Status: draft For fixing envoyproxy#280 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…ndler Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…ndler Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…ndler

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…ation Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…ation Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k · 2020-06-19T19:02:55Z

@eric846 please review and assign to me once ready.

…ation Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-06-20T21:55:54Z

source/client/process_impl.cc

-  cluster_manager_ = cluster_manager_factory_->clusterManagerFromProto(bootstrap);
-  maybeCreateTracingDriver(bootstrap.tracing());
-  cluster_manager_->setInitializedCb([this]() -> void { init_manager_.initialize(init_watcher_); });
+  {


Note to reviewers: The "hide whitespace" toggle in Github makes this a lot more fun to review.
(The scope introduced for the lock guard introduces a lot of whitespace noise).

test/integration/test_integration_basics.py

test/process_test.cc

…ation

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

test/process_test.cc

oschaaf · 2020-06-22T20:28:08Z

Thanks @eric846 for the review. Just marked this as ready for another pass

mum4k · 2020-06-22T20:49:07Z

source/client/client.cc

  OutputFormatterFactoryImpl output_formatter_factory;
  OutputCollectorImpl output_collector(time_system, *options_);
-  const bool res = process->run(output_collector);
+  bool res;


(nit) If we can, we should try to avoid abbreviations in variable names (res).

mum4k · 2020-06-22T20:50:07Z

source/client/remote_process_impl.cc


+bool RemoteProcessImpl::requestExecutionCancellation() {
+  ENVOY_LOG(error, "Remote process cancellation not supported yet");
+  // TODO(XXX): Send a cancel request to the gRPC service.


Did we want to replace XXX with a value?

I filed #380 (and updated XXX to #380) in the changes I'm about to push.

mum4k · 2020-06-22T20:50:23Z

source/client/process_impl.h


  void configureComponentLogLevels(spdlog::level::level_enum level);
-  const std::vector<ClientWorkerPtr>& createWorkers(const uint32_t concurrency);
+  void createWorkers(const uint32_t concurrency);


(optional) Is this a good time to document the method?

mum4k · 2020-06-22T20:50:57Z

test/integration/test_integration_basics.py



+def send_sigterm(p):
+  # Sleep for a while, under tsan the client needs a lot of time


Can we add a Python style docstring, since the function is public.

mum4k · 2020-06-22T20:51:08Z

test/integration/test_integration_basics.py

    assertCounterEqual(counters, "benchmark.http_2xx", (total_requests))


+def send_sigterm(p):


Can we choose a more readable name than "p".

mum4k · 2020-06-22T20:51:46Z

test/integration/test_integration_basics.py

+  stdout, stderr = client_process.communicate()
+  client_process.wait()
+  output = stdout.decode('utf-8')
+  assert (client_process.returncode == 0)


Can we add a custom error message giving some context?

Or should this be a regular test assertion like below?

mum4k · 2020-06-22T20:52:09Z

test/process_test.cc

+    if (do_cancel) {
+      cancel_thread = std::thread([&process, terminate_right_away] {
+        if (!terminate_right_away) {
+          sleep(5);


Should we add a comment explaining the sleep and the choice of 5?

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-06-22T22:41:58Z

@mum4k thanks for the review. I pushed 3198623 to address your feedback.

Teach the CLI to handle SIGTERM/SIGINT, and handle those as a request to gracefully cancel execution. Partially resolves envoyproxy#280

oschaaf added 11 commits January 15, 2020 16:21

Extract SignalHandler

b973b75

In preparation of sharing functionality for signal handling, extract what we have right now into SignalHandler. Status: draft For fixing envoyproxy#280 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Remove accidentally left in comment

fb5c434

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Fix format

1bd1d31

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into extract-signal-ha…

fcba594

…ndler Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Amend according to clang-tidy's complaints

ce5ead6

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Pass const ref

3f99119

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into extract-signal-ha…

111ba5d

…ndler Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into extract-signal-ha…

89dfdf6

…ndler

Merge remote-tracking branch 'upstream/master' into extract-signal-ha…

c457af6

…ndler

CLI: Handle signals, allow cancellation of executions

04eec56

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Add lock guarding of the cancellation process + tests

a11050f

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf marked this pull request as ready for review June 17, 2020 14:32

This was referenced Jun 17, 2020

Add "no-duration" to facilitate infinite load generation #366

Merged

Fix missing test coverage for --no-duration #370

Closed

oschaaf added 4 commits June 19, 2020 00:33

Merge remote-tracking branch 'upstream/master' into execution-cancell…

781df87

…ation Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

s/cancel_requests/graceful_stop_requested/

2c04d10

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Eliminate the NullTerminationPredicate: dead code

55f694f

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Remove debug print line

d1a696b

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added P1 waiting-for-review A PR waiting for a review. labels Jun 18, 2020

Merge remote-tracking branch 'upstream/master' into execution-cancell…

3c35f86

…ation Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k requested a review from eric846 June 19, 2020 19:02

mum4k self-assigned this Jun 19, 2020

Merge remote-tracking branch 'upstream/master' into execution-cancell…

9e0d9b2

…ation Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf commented Jun 20, 2020

View reviewed changes

eric846 requested changes Jun 22, 2020

View reviewed changes

test/integration/test_integration_basics.py Show resolved Hide resolved

test/process_test.cc Outdated Show resolved Hide resolved

eric846 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 Jun 22, 2020

Merge remote-tracking branch 'upstream/master' into execution-cancell…

57d71f0

…ation

oschaaf added 2 commits June 22, 2020 20:47

Partially address review feedback

1fddbec

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback pt II

20e6108

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

eric846 requested changes Jun 22, 2020

View reviewed changes

test/process_test.cc Outdated Show resolved Hide resolved

oschaaf 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 Jun 22, 2020

mum4k requested changes Jun 22, 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 Jun 22, 2020

This was referenced Jun 22, 2020

Coverage flakes #362

Closed

Support remote execution cancellation #380

Open

Review feedback

3198623

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf 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 Jun 22, 2020

oschaaf mentioned this pull request Jun 24, 2020

Cover --no-duration and restore the CI coverage percentage threshold #384

Merged

1 task

mum4k approved these changes Jun 24, 2020

View reviewed changes

mum4k merged commit 3fe461c into envoyproxy:master Jun 24, 2020



		def send_sigterm(p):
		# Sleep for a while, under tsan the client needs a lot of time

		assertCounterEqual(counters, "benchmark.http_2xx", (total_requests))


		def send_sigterm(p):

Conversation

oschaaf commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mum4k commented Jun 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oschaaf commented Jun 22, 2020

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!

oschaaf commented Jun 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oschaaf commented Jun 17, 2020 •

edited

Loading