Removed AbortAction namespace and directory to be consistent with oth…

…er core extensions, set a default wait time of 5 second, minor changes.

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

api/BUILD

@@ -271,7 +271,7 @@ proto_library(
         "//envoy/type/metadata/v3:pkg",
         "//envoy/type/tracing/v3:pkg",
         "//envoy/type/v3:pkg",
-        "//envoy/watchdog/abort_action/v3alpha:pkg",
+        "//envoy/watchdog/v3alpha:pkg",
     ],
 )
 

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

@@ -1,14 +1,14 @@
 syntax = "proto3";
 
-package envoy.watchdog.abort_action.v3alpha;
+package envoy.watchdog.v3alpha;
 
 import "google/protobuf/duration.proto";
 
 import "udpa/annotations/status.proto";
 import "udpa/annotations/versioning.proto";
 import "validate/validate.proto";
 
-option java_package = "io.envoyproxy.envoy.watchdog.abort_action.v3alpha";
+option java_package = "io.envoyproxy.envoy.watchdog.v3alpha";
 option java_outer_classname = "AbortActionProto";
 option java_multiple_files = true;
 option (udpa.annotations.file_status).work_in_progress = true;
@@ -24,5 +24,6 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
 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.
+  // By default this is 5 seconds.
   google.protobuf.Duration wait_duration = 1;
 }

api/versioning/BUILD

@@ -154,7 +154,7 @@ proto_library(
         "//envoy/type/metadata/v3:pkg",
         "//envoy/type/tracing/v3:pkg",
         "//envoy/type/v3:pkg",
-        "//envoy/watchdog/abort_action/v3alpha:pkg",
+        "//envoy/watchdog/v3alpha:pkg",
     ],
 )
 

source/common/thread/terminate_thread.h

@@ -8,6 +8,9 @@ namespace Thread {
  * Tries to terminates the process by killing the thread specified by
  * the ThreadId. The implementation is platform dependent and currently
  * 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.
  */
 bool terminateThread(const ThreadId& tid);
 

source/common/watchdog/abort_action/BUILD → source/common/watchdog/BUILD

@@ -19,21 +19,20 @@ envoy_cc_library(
         "//source/common/common:assert_lib",
         "//source/common/protobuf:utility_lib",
         "//source/common/thread:terminate_thread_lib",
-        "@envoy_api//envoy/watchdog/abort_action/v3alpha:pkg_cc_proto",
+        "@envoy_api//envoy/watchdog/v3alpha:pkg_cc_proto",
     ],
 )
 
-# TODO(kbaichoo): is there a more appropriate build unit for this?
 envoy_cc_library(
-    name = "config",
-    srcs = ["config.cc"],
-    hdrs = ["config.h"],
+    name = "abort_action_config",
+    srcs = ["abort_action_config.cc"],
+    hdrs = ["abort_action_config.h"],
     deps = [
         ":abort_action_lib",
         "//include/envoy/registry",
         "//source/common/config:utility_lib",
         "//source/common/protobuf",
         "//source/common/protobuf:message_validator_lib",
-        "@envoy_api//envoy/watchdog/abort_action/v3alpha:pkg_cc_proto",
+        "@envoy_api//envoy/watchdog/v3alpha:pkg_cc_proto",
     ],
 )

source/common/watchdog/abort_action/abort_action.cc → source/common/watchdog/abort_action.cc

@@ -1,4 +1,4 @@
-#include "common/watchdog/abort_action/abort_action.h"
+#include "common/watchdog/abort_action.h"
 
 #include "envoy/thread/thread.h"
 
@@ -10,11 +10,14 @@
 
 namespace Envoy {
 namespace Watchdog {
-namespace AbortAction {
+namespace {
+constexpr uint64_t DefaultWaitDurationMs = 5000;
+} // end namespace
 
-AbortAction::AbortAction(envoy::watchdog::abort_action::v3alpha::AbortActionConfig& config,
+AbortAction::AbortAction(envoy::watchdog::v3alpha::AbortActionConfig& config,
                          Server::Configuration::GuardDogActionFactoryContext& /*context*/)
-    : config_(config){};
+    : wait_duration_(absl::Milliseconds(
+          PROTOBUF_GET_MS_OR_DEFAULT(config, wait_duration, DefaultWaitDurationMs))) {}
 
 void AbortAction::run(
     envoy::config::bootstrap::v3::Watchdog::WatchdogAction::WatchdogEvent /*event*/,
@@ -34,9 +37,8 @@ void AbortAction::run(
 
   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)));
+    absl::SleepFor(wait_duration_);
   } else {
-    // Failed to send the signal, abort?
     ENVOY_LOG_MISC(error, "Failed to terminate tid {}", tid_string);
   }
 
@@ -48,6 +50,5 @@ void AbortAction::run(
       tid_string));
 }
 
-} // namespace AbortAction
 } // namespace Watchdog
 } // namespace Envoy

source/common/watchdog/abort_action/abort_action.h → source/common/watchdog/abort_action.h

@@ -1,34 +1,29 @@
 #pragma once
 
-#include <chrono>
-
 #include "envoy/server/guarddog_config.h"
 #include "envoy/thread/thread.h"
-#include "envoy/watchdog/abort_action/v3alpha/abort_action.pb.h"
+#include "envoy/watchdog/v3alpha/abort_action.pb.h"
 
 namespace Envoy {
 namespace Watchdog {
-namespace AbortAction {
-
 /**
  * A GuardDogAction that will terminate the process by killing the
  * stuck thread.
  */
 class AbortAction : public Server::Configuration::GuardDogAction {
 public:
-  AbortAction(envoy::watchdog::abort_action::v3alpha::AbortActionConfig& config,
+  AbortAction(envoy::watchdog::v3alpha::AbortActionConfig& config,
               Server::Configuration::GuardDogActionFactoryContext& context);
 
   void run(envoy::config::bootstrap::v3::Watchdog::WatchdogAction::WatchdogEvent event,
            const std::vector<std::pair<Thread::ThreadId, MonotonicTime>>& thread_last_checkin_pairs,
            MonotonicTime now) override;
 
 private:
-  const envoy::watchdog::abort_action::v3alpha::AbortActionConfig config_;
+  const absl::Duration wait_duration_;
 };
 
 using AbortActionPtr = std::unique_ptr<AbortAction>;
 
-} // namespace AbortAction
 } // namespace Watchdog
 } // namespace Envoy

source/common/watchdog/abort_action/config.cc → source/common/watchdog/abort_action_config.cc

@@ -1,14 +1,13 @@
-#include "common/watchdog/abort_action/config.h"
+#include "common/watchdog/abort_action_config.h"
 
 #include "envoy/registry/registry.h"
 
 #include "common/config/utility.h"
 #include "common/protobuf/message_validator_impl.h"
-#include "common/watchdog/abort_action/abort_action.h"
+#include "common/watchdog/abort_action.h"
 
 namespace Envoy {
 namespace Watchdog {
-namespace AbortAction {
 
 Server::Configuration::GuardDogActionPtr AbortActionFactory::createGuardDogActionFromProto(
     const envoy::config::bootstrap::v3::Watchdog::WatchdogAction& config,
@@ -24,6 +23,5 @@ Server::Configuration::GuardDogActionPtr AbortActionFactory::createGuardDogActio
  */
 REGISTER_FACTORY(AbortActionFactory, Server::Configuration::GuardDogActionFactory);
 
-} // namespace AbortAction
 } // namespace Watchdog
 } // namespace Envoy

source/common/watchdog/abort_action/config.h → source/common/watchdog/abort_action_config.h

@@ -1,13 +1,12 @@
 #pragma once
 
 #include "envoy/server/guarddog_config.h"
-#include "envoy/watchdog/abort_action/v3alpha/abort_action.pb.h"
+#include "envoy/watchdog/v3alpha/abort_action.pb.h"
 
 #include "common/protobuf/protobuf.h"
 
 namespace Envoy {
 namespace Watchdog {
-namespace AbortAction {
 
 class AbortActionFactory : public Server::Configuration::GuardDogActionFactory {
 public:
@@ -23,9 +22,8 @@ class AbortActionFactory : public Server::Configuration::GuardDogActionFactory {
 
   std::string name() const override { return "envoy.watchdog.abort_action"; }
 
-  using AbortActionConfig = envoy::watchdog::abort_action::v3alpha::AbortActionConfig;
+  using AbortActionConfig = envoy::watchdog::v3alpha::AbortActionConfig;
 };
 
-} // namespace AbortAction
 } // namespace Watchdog
 } // namespace Envoy

source/server/BUILD

@@ -55,7 +55,7 @@ envoy_cc_library(
         "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/metrics/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/trace/v3:pkg_cc_proto",
-        "@envoy_api//envoy/watchdog/abort_action/v3alpha:pkg_cc_proto",
+        "@envoy_api//envoy/watchdog/v3alpha:pkg_cc_proto",
     ],
 )
 
@@ -128,6 +128,7 @@ envoy_cc_library(
         "//source/common/event:libevent_lib",
         "//source/common/protobuf:utility_lib",
         "//source/common/stats:symbol_table_lib",
+        "//source/common/watchdog:abort_action_config",
         "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
     ],
 )
@@ -446,7 +447,6 @@ envoy_cc_library(
         "//source/common/upstream:cluster_manager_lib",
         "//source/common/upstream:health_discovery_service_lib",
         "//source/common/version:version_lib",
-        "//source/common/watchdog/abort_action:config",
         "//source/server:overload_manager_lib",
         "//source/server/admin:admin_lib",
         "@envoy_api//envoy/admin/v3:pkg_cc_proto",

source/server/configuration_impl.cc

@@ -15,7 +15,7 @@
 #include "envoy/server/instance.h"
 #include "envoy/server/tracer_config.h"
 #include "envoy/ssl/context_manager.h"
-#include "envoy/watchdog/abort_action/v3alpha/abort_action.pb.h"
+#include "envoy/watchdog/v3alpha/abort_action.pb.h"
 
 #include "common/common/assert.h"
 #include "common/common/utility.h"
@@ -175,9 +175,7 @@ WatchdogImpl::WatchdogImpl(const envoy::config::bootstrap::v3::Watchdog& watchdo
   auto actions = watchdog.actions();
 
   // 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);
+  envoy::watchdog::v3alpha::AbortActionConfig abort_config;
 
   if (kill_timeout > 0) {
     envoy::config::bootstrap::v3::Watchdog::WatchdogAction* abort_action_config = actions.Add();

test/common/watchdog/abort_action/BUILD → test/common/watchdog/BUILD

@@ -18,26 +18,26 @@ envoy_cc_test(
         "//include/envoy/common:time_interface",
         "//include/envoy/registry",
         "//include/envoy/server:guarddog_config_interface",
-        "//source/common/watchdog/abort_action:abort_action_lib",
-        "//source/common/watchdog/abort_action:config",
+        "//source/common/watchdog:abort_action_config",
+        "//source/common/watchdog:abort_action_lib",
         "//test/common/stats:stat_test_utility_lib",
         "//test/test_common:utility_lib",
         "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
-        "@envoy_api//envoy/watchdog/abort_action/v3alpha:pkg_cc_proto",
+        "@envoy_api//envoy/watchdog/v3alpha:pkg_cc_proto",
     ],
 )
 
 envoy_cc_test(
-    name = "config_test",
-    srcs = ["config_test.cc"],
+    name = "abort_action_config_test",
+    srcs = ["abort_action_config_test.cc"],
     deps = [
         "//include/envoy/registry",
         "//include/envoy/server:guarddog_config_interface",
-        "//source/common/watchdog/abort_action:abort_action_lib",
-        "//source/common/watchdog/abort_action:config",
+        "//source/common/watchdog:abort_action_config",
+        "//source/common/watchdog:abort_action_lib",
         "//test/common/stats:stat_test_utility_lib",
         "//test/mocks/event:event_mocks",
         "//test/test_common:utility_lib",
-        "@envoy_api//envoy/watchdog/abort_action/v3alpha:pkg_cc_proto",
+        "@envoy_api//envoy/watchdog/v3alpha:pkg_cc_proto",
     ],
 )

test/common/watchdog/abort_action/config_test.cc → test/common/watchdog/abort_action_config_test.cc

@@ -1,8 +1,8 @@
 #include "envoy/registry/registry.h"
 #include "envoy/server/guarddog_config.h"
-#include "envoy/watchdog/abort_action/v3alpha/abort_action.pb.h"
+#include "envoy/watchdog/v3alpha/abort_action.pb.h"
 
-#include "common/watchdog/abort_action/config.h"
+#include "common/watchdog/abort_action_config.h"
 
 #include "test/common/stats/stat_test_utility.h"
 #include "test/mocks/event/mocks.h"
@@ -12,7 +12,6 @@
 
 namespace Envoy {
 namespace Watchdog {
-namespace AbortAction {
 namespace {
 
 TEST(AbortActionFactoryTest, CanCreateAction) {
@@ -49,6 +48,5 @@ TEST(AbortActionFactoryTest, CanCreateAction) {
 }
 
 } // namespace
-} // namespace AbortAction
 } // namespace Watchdog
 } // namespace Envoy

test/common/watchdog/abort_action/abort_action_test.cc → test/common/watchdog/abort_action_test.cc

@@ -6,10 +6,10 @@
 #include "envoy/event/dispatcher.h"
 #include "envoy/server/guarddog_config.h"
 #include "envoy/thread/thread.h"
-#include "envoy/watchdog/abort_action/v3alpha/abort_action.pb.h"
+#include "envoy/watchdog/v3alpha/abort_action.pb.h"
 
-#include "common/watchdog/abort_action/abort_action.h"
-#include "common/watchdog/abort_action/config.h"
+#include "common/watchdog/abort_action.h"
+#include "common/watchdog/abort_action_config.h"
 
 #include "test/common/stats/stat_test_utility.h"
 #include "test/test_common/utility.h"
@@ -19,10 +19,9 @@
 
 namespace Envoy {
 namespace Watchdog {
-namespace AbortAction {
 namespace {
 
-using AbortActionConfig = envoy::watchdog::abort_action::v3alpha::AbortActionConfig;
+using AbortActionConfig = envoy::watchdog::v3alpha::AbortActionConfig;
 
 class AbortActionTest : public testing::Test {
 protected:
@@ -53,8 +52,7 @@ TEST_F(AbortActionTest, ShouldNotAbortIfNoTids) {
   action_->run(envoy::config::bootstrap::v3::Watchdog::WatchdogAction::KILL, tid_ltt_pairs, now);
 }
 
-// insufficient signal support on Windows.
-TEST_F(AbortActionTest, CanKillThread) {
+TEST_F(AbortActionTest, ShouldKillTheProcess) {
   AbortActionConfig config;
   config.mutable_wait_duration()->set_seconds(1);
   action_ = std::make_unique<AbortAction>(config, context_);
@@ -82,6 +80,8 @@ TEST_F(AbortActionTest, CanKillThread) {
   EXPECT_DEATH(die_function(), "");
 }
 
+#ifndef WIN32
+// insufficient signal support on Windows.
 void handler(int sig, siginfo_t* /*siginfo*/, void* /*context*/) {
   std::cout << "Eating signal :" << std::to_string(sig) << ". will ignore it." << std::endl;
   signal(SIGABRT, SIG_IGN);
@@ -121,8 +121,8 @@ TEST_F(AbortActionTest, PanicsIfThreadDoesNotDie) {
 
   EXPECT_DEATH(die_function(), "aborting from Watchdog AbortAction instead");
 }
+#endif
 
 } // namespace
-} // namespace AbortAction
 } // namespace Watchdog
 } // namespace Envoy