Watchdog: use abort action as a default if killing is enabled. #13523

KBaichoo · 2020-10-12T21:13:10Z

Signed-off-by: Kevin Baichoo kbaichoo@google.com

Commit Message: Watchdog: use abort action as a default if killing is enabled and we're on a supported platform.
Additional Description:
Risk Level: low
Testing: unit tests
Docs Changes: Included
Release Notes: Included

See PR #13208 for context as the reason it's part of core envoy and not an extension.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only · 2020-10-12T21:13:17Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13523 was opened by KBaichoo.

see: more, trace.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only · 2020-10-13T12:19:16Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt).

🐱

Caused by: #13523 was synchronize by KBaichoo.

see: more, trace.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente

Thanks for generalizing this action and moving it to envoy core. Change looks good, just a few nits.

source/common/thread/terminate_thread.h

antoniovicente · 2020-10-13T06:42:56Z

source/common/watchdog/abort_action/BUILD

    ],
 )

-envoy_cc_extension(
+# TODO(kbaichoo): is there a more appropriate build unit for this?


This seems about right. I see some similar examples outside the extensions directory like udp_default_writer_config.cc which contain the extension name as part of the filename since the extensions don't have their own directories like in your example. you could consider making this action live in the watchdog directory instead and possibly have the config object link in directly to the watchdog library.

I moved them into the watchdog directory, and linked it directly into the guarddog_lib. Given this, I removed the AbortAction namespace since it originally was for being consistent with directory structure.

source/common/watchdog/abort_action/abort_action.cc

antoniovicente · 2020-10-13T23:39:20Z

source/common/watchdog/abort_action/abort_action.h


 namespace Envoy {
-namespace Extensions {
 namespace Watchdog {
 namespace AbortAction {


The AbortAction namespace doesn't seem necessary. I assume it exists for consistency with the directory structure.

Yep it was for consistency with directory structure. Given my response to your comment about removing the abort_action directory, I removed the AbortAction namespace since it originally was for being consistent with directory structure.

Done.

antoniovicente · 2020-10-14T00:45:10Z

source/common/watchdog/abort_action/abort_action.cc

+    // Successfully signaled to thread to terminate, sleep for wait_duration.
+    absl::SleepFor(absl::Milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(config_, wait_duration, 0)));
+  } else {
+    // Failed to send the signal, abort?


Odd comment since we effectively panic in the next line.

Good point. I removed it. The LOG along with the comment on the PANIC seem to supply enough context there.

antoniovicente · 2020-10-14T00:52:08Z

source/server/configuration_impl.cc

+  // Add abort_action if killing is enabled.
+  envoy::watchdog::abort_action::v3alpha::AbortActionConfig abort_config;
+  // Wait one second for the aborted thread to abort.
+  abort_config.mutable_wait_duration()->set_seconds(1);


Can we rely on the default wait_duration?

antoniovicente · 2020-10-14T00:53:11Z

source/common/watchdog/abort_action/abort_action.cc

+
+  if (Thread::terminateThread(thread_id)) {
+    // Successfully signaled to thread to terminate, sleep for wait_duration.
+    absl::SleepFor(absl::Milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(config_, wait_duration, 0)));


Should the default sleep duration be non-zero?

Yep it's now the default that wait_duration is (5s).

test/server/configuration_impl_test.cc

antoniovicente · 2020-10-14T00:58:07Z

test/server/configuration_impl_test.cc

+
+  // We should have the abort action added to both KILL and MULTIKILL events.
+  EXPECT_EQ(config.workerWatchdogConfig().actions().size(), 2);
+  EXPECT_EQ(config.mainThreadWatchdogConfig().actions().size(), 2);


Worth checking the contents of these action configs?

antoniovicente · 2020-10-14T00:59:53Z

source/common/watchdog/abort_action/abort_action.cc

+  // Abort from the action since the signaled thread hasn't yet crashed the process.
+  // Panicing in the action gives flexibility since it doesn't depend on
+  // external code to kill the process if the signal fails.
+  PANIC(fmt::format(


The PANICs in guarddog_impl.cc are no longer reachable now that we are adding these actions by default. Should they be removed?

They're still reachable in the guarddog unit tests, which doesn't install this action, but perhaps they should. Thoughts?

It may make sense to install these default actions in the guarddog itself instead of doing so by modifying config earlier in the process. Doing so would allow us to get more consistent behavior in smaller tests and avoid potentially showing the config modifications when someone accesses the proxy config via the admin handler.

That was a great suggestion -- having this logic captured within just the guarddog simplifies a lot of the design.

antoniovicente · 2020-10-14T01:01:32Z

api/envoy/watchdog/abort_action/v3alpha/abort_action.proto

+// MULTIKILL events if those are enabled.
+message AbortActionConfig {
+  // How long to wait for the thread to respond to the thread kill function
+  // before killing the process from this action. This is a blocking action.


Should there be a default wait interval?

Yes, that makes sense. Will go with 5 seconds by default -- this is somewhat arbitrary, but it should be sufficient time for the failure handlers to have finished running and the process exiting.

…er core extensions, set a default wait time of 5 second, minor changes. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

source/server/configuration_impl.cc

antoniovicente · 2020-10-14T18:14:28Z

source/server/configuration_impl.cc

+  auto actions = watchdog.actions();
+
+  // Add abort_action if killing is enabled.
+  envoy::watchdog::v3alpha::AbortActionConfig abort_config;


nit: Move just before the 2 PackFrom calls below.

Although, would it work if you don't specify typed config since the proto in question has no fields set?

It still works as we end up setting the type url which IIUC is necessary for finding the right factory for the typed config.

… guarddog. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

source/server/guarddog_impl.cc

antoniovicente · 2020-10-14T20:06:36Z

source/common/thread/terminate_thread.h

+ * only works on platforms that support SIGABRT.
+ *
+ * Returns the result from the platform specific function (i.e. kill) to terminate
+ * the thread. If the platform is currently unsupported, this will return false.


This does not return the result of the platform specific function, it returns true if the platform specific function succeeded. See implementation, return is: kill() == 0;

good catch I've made it:

Returns true if the platform specific function to terminate the thread succeeded (i.e. kill() == 0). If the platform is currently unsupported, this will return false.

antoniovicente · 2020-10-14T20:12:21Z

test/server/test_data/server/watchdogs_bootstrap_with_deprecated_field.yaml

@@ -1,7 +0,0 @@
-watchdogs:
-  main_thread_watchdog:


I guess this config file was never used?

Yep, it was added in an intermediate commit, when we were splitting watchdog into multiple watchdogs. But it didn't get used in the end, and I missed removing it (until now).

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2020-10-14T20:44:56Z

@envoyproxy/windows-dev

docs/root/version_history/v1.16.0.rst

test/per_file_coverage.sh

api/envoy/watchdog/v3alpha/abort_action.proto

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

…ult-cross-platform Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2020-10-19T12:23:37Z

/retest

repokitteh-read-only · 2020-10-19T12:23:42Z

Retrying Azure Pipelines.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13523 (comment) was created by @KBaichoo.

see: more, trace.

KBaichoo · 2020-10-19T15:22:59Z

PTAL @envoyproxy/api-shepherds @envoyproxy/dependency-shepherds

mattklein123

Thanks this LGTM. @htuch any more concerns on this one?

api/envoy/watchdog/v3alpha/abort_action.proto

htuch · 2020-10-19T20:29:48Z

/lgtm api

* master: (22 commits) ci: various improvements (envoyproxy#13660) dns: fix defunct fd bug in apple resolver (envoyproxy#13641) build: support ppc64le with wasm (envoyproxy#13657) [fuzz] Added random load balancer fuzz (envoyproxy#13400) dependencies: compute and check release dates via GitHub API. (envoyproxy#13582) mac ci: try ignoring update failure (envoyproxy#13658) watchdog: Optimize WatchdogImpl::touch in preparation to more frequent petting of the watchdog. (envoyproxy#13103) typos: fix a couple 'enovy' mispellings (envoyproxy#13645) lua: Expose stream info downstreamLocalAddress and downstreamDirectRemoteAddress for Lua filter (envoyproxy#13536) tap: fix upstream streamed transport socket taps (envoyproxy#13638) Revert "delay health checks until transport socket secrets are ready. (envoyproxy#13516)" (envoyproxy#13639) Watchdog: use abort action as a default if killing is enabled. (envoyproxy#13523) [fuzz] Fixed divide by zero bug (envoyproxy#13545) wasm: flip the meaning of the "repository" in envoy_wasm_cc_binary(). (envoyproxy#13621) fix: record recovered local address (envoyproxy#13581) docs: fix incorrect compressor filter doc (envoyproxy#13611) docs: clean up docs for azp migration (envoyproxy#13558) wasm: fix building Wasm example. (envoyproxy#13619) test: Refactor flood tests into a separate test file (envoyproxy#13556) wasm: re-enable tests with precompiled modules. (envoyproxy#13583) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Watchdog: use abort action as a default if killing is enabled.

3b8841c

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo requested a review from antoniovicente as a code owner October 12, 2020 21:13

repokitteh-read-only bot added the api label Oct 12, 2020

antoniovicente self-assigned this Oct 12, 2020

Minor fixes.

931b0e7

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Oct 13, 2020

Added release notes.

8eeca92

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente reviewed Oct 14, 2020

View reviewed changes

Removed AbortAction namespace and directory to be consistent with oth…

ab416ac

…er core extensions, set a default wait time of 5 second, minor changes. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente reviewed Oct 14, 2020

View reviewed changes

Moved logic to add abort_action to kill/multikill if enabled into the…

8a27924

… guarddog. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente previously approved these changes Oct 14, 2020

View reviewed changes

minor fixes.

11b0e04

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo dismissed antoniovicente’s stale review via 11b0e04 October 14, 2020 20:39

antoniovicente previously approved these changes Oct 14, 2020

View reviewed changes

docs/root/version_history/v1.16.0.rst Show resolved Hide resolved

test/per_file_coverage.sh Outdated Show resolved Hide resolved

htuch reviewed Oct 15, 2020

View reviewed changes

api/envoy/watchdog/v3alpha/abort_action.proto Show resolved Hide resolved

KBaichoo added 2 commits October 16, 2020 13:20

comment change.

0e69454

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Merge remote-tracking branch 'upstream/master' into abort-action-defa…

64e7a62

…ult-cross-platform Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo dismissed antoniovicente’s stale review via 64e7a62 October 16, 2020 13:21

mattklein123 self-assigned this Oct 19, 2020

mattklein123 approved these changes Oct 19, 2020

View reviewed changes

api/envoy/watchdog/v3alpha/abort_action.proto Show resolved Hide resolved

repokitteh-read-only bot removed api deps Approval required for changes to Envoy's external dependencies labels Oct 19, 2020

antoniovicente approved these changes Oct 19, 2020

View reviewed changes

htuch merged commit 415af04 into envoyproxy:master Oct 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Watchdog: use abort action as a default if killing is enabled. #13523

Watchdog: use abort action as a default if killing is enabled. #13523

KBaichoo commented Oct 12, 2020 •

edited

Loading

repokitteh-read-only bot commented Oct 12, 2020

repokitteh-read-only bot commented Oct 13, 2020

antoniovicente left a comment

antoniovicente Oct 13, 2020

KBaichoo Oct 14, 2020 •

edited

Loading

antoniovicente Oct 13, 2020

KBaichoo Oct 14, 2020

antoniovicente Oct 14, 2020

KBaichoo Oct 14, 2020

antoniovicente Oct 14, 2020

KBaichoo Oct 14, 2020

antoniovicente Oct 14, 2020

KBaichoo Oct 14, 2020

antoniovicente Oct 14, 2020

KBaichoo Oct 14, 2020

antoniovicente Oct 14, 2020

KBaichoo Oct 14, 2020

antoniovicente Oct 14, 2020

KBaichoo Oct 14, 2020

antoniovicente Oct 14, 2020

KBaichoo Oct 14, 2020

antoniovicente Oct 14, 2020

KBaichoo Oct 14, 2020

antoniovicente Oct 14, 2020

KBaichoo Oct 14, 2020

antoniovicente Oct 14, 2020

KBaichoo Oct 14, 2020

KBaichoo commented Oct 14, 2020

KBaichoo commented Oct 19, 2020

repokitteh-read-only bot commented Oct 19, 2020

KBaichoo commented Oct 19, 2020

mattklein123 left a comment

htuch commented Oct 19, 2020

Watchdog: use abort action as a default if killing is enabled. #13523

Watchdog: use abort action as a default if killing is enabled. #13523

Conversation

KBaichoo commented Oct 12, 2020 • edited Loading

repokitteh-read-only bot commented Oct 12, 2020

repokitteh-read-only bot commented Oct 13, 2020

antoniovicente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KBaichoo Oct 14, 2020 • edited Loading

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

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

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

KBaichoo commented Oct 14, 2020

KBaichoo commented Oct 19, 2020

repokitteh-read-only bot commented Oct 19, 2020

KBaichoo commented Oct 19, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

htuch commented Oct 19, 2020

KBaichoo commented Oct 12, 2020 •

edited

Loading

KBaichoo Oct 14, 2020 •

edited

Loading