CLI - cancel execution upon signal reception #367

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

@@ -73,7 +74,12 @@ bool Main::run() {
  }
  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

@@ -54,5 +54,11 @@ bool RemoteProcessImpl::run(OutputCollector& collector) {
  return false;
 }

+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

@@ -99,7 +101,7 @@ class ProcessImpl : public Process, public Envoy::Logger::Loggable<Envoy::Logger
  void maybeCreateTracingDriver(const envoy::config::trace::v3::Tracing& configuration);

  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

@@ -654,6 +658,34 @@ def test_http_request_release_timing(http_test_server_fixture, qps_parameterizat
    assertCounterEqual(counters, "benchmark.http_2xx", (total_requests))


+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

@@ -654,6 +658,34 @@ def test_http_request_release_timing(http_test_server_fixture, qps_parameterizat
    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI - cancel execution upon signal reception #367

CLI - cancel execution upon signal reception #367

oschaaf commented Jun 17, 2020 •

edited

Loading

mum4k commented Jun 19, 2020

oschaaf Jun 20, 2020

oschaaf commented Jun 22, 2020

mum4k Jun 22, 2020

mum4k Jun 22, 2020

oschaaf Jun 22, 2020

mum4k Jun 22, 2020

mum4k Jun 22, 2020

mum4k Jun 22, 2020

mum4k Jun 22, 2020

mum4k Jun 22, 2020

oschaaf commented Jun 22, 2020

		@@ -654,6 +658,34 @@ def test_http_request_release_timing(http_test_server_fixture, qps_parameterizat
		assertCounterEqual(counters, "benchmark.http_2xx", (total_requests))


		def send_sigterm(p):

CLI - cancel execution upon signal reception #367

CLI - cancel execution upon signal reception #367

Conversation

oschaaf commented Jun 17, 2020 • edited Loading

mum4k commented Jun 19, 2020

Choose a reason for hiding this comment

oschaaf commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oschaaf commented Jun 22, 2020

oschaaf commented Jun 17, 2020 •

edited

Loading