Added watchdog support for a Multi-Kill threshold. by KBaichoo · Pull Request #12108 · envoyproxy/envoy

KBaichoo · 2020-07-15T17:53:49Z

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

Commit Message: MultiKill threshold support in WatchDog.
Additional Description: WatchDog will now kill if max(2, registered_threads * multi_kill_threshold) threads have gone above the multikill_timeout.
Risk Level: Low, backwards compatible by default
Testing: Implemented Unit tests
Docs Changes: No
Release Notes: None (this is fully backward compatible by default.)
Fixes #11389

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

repokitteh-read-only · 2020-07-15T17:53:54Z

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: #12108 was opened by KBaichoo.

see: more, trace.

KBaichoo · 2020-07-15T17:54:14Z

/ review @antoniovicente

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

antoniovicente · 2020-07-15T20:24:02Z

source/server/guarddog_impl.cc

      time_source_(api.timeSource()), miss_timeout_(config.wdMissTimeout()),
      megamiss_timeout_(config.wdMegaMissTimeout()), kill_timeout_(config.wdKillTimeout()),
      multi_kill_timeout_(config.wdMultiKillTimeout()),
+      multi_kill_threshold_(config.wdMultiKillThreshold() / 100.0),


It's unclear why you need to divide by 100 when converting one kind of threshold to the other. Could be clarified a bit by using a name like multi_kill_fraction_ or multi_kill_ratio_ for the member variable, implying it is a value in the range [0.0,1.0] instead of a percentage.

antoniovicente · 2020-07-15T20:30:50Z

source/server/guarddog_impl.cc

    Thread::LockGuard guard(wd_lock_);
+
+    // Compute the multikill threshold
+    const ssize_t multikill_threshold =


multi_kill_threshold and multi_kill_threshold_ mean very different things in this statement.

Consider something like: required_for_multi_kill

antoniovicente · 2020-07-15T20:31:38Z

source/server/guarddog_impl.cc

+
+    // Compute the multikill threshold
+    const ssize_t multikill_threshold =
+        std::max(2, static_cast<int>(multi_kill_threshold_ * watched_dogs_.size()));


Should you round up?

Also, I think you may want to use size_t instead of ssize_t, also use size_t in the static_cast.

Yep will round up. Done.

antoniovicente · 2020-07-15T21:32:20Z

include/envoy/server/configuration.h

+  /**
+   * @return double the percentage of threads that needs to meet the MultiKillTimeout before we
+   *         kill the process. If it is zero, then it'll fallback to the default behavior of
+   *         killing the process if two threads hit the multikill timeout.


comment nit: The number of threads required for multi-kill is always at least 2. This behavior applies beyond just the 0 default case. You may want to refer to the max(2, registered_threads * percentage) computation.

Tried to clarify it a bit more.

antoniovicente · 2020-07-15T21:32:53Z

api/envoy/config/bootstrap/v3/bootstrap.proto

  google.protobuf.Duration multikill_timeout = 4;
+
+  // Sets threshold for multikill timeout in terms of the percentage of
+  // Watchdogs being nonresponsive for at least the multikill_timeout.


nit: watchdogs -> threads

percentage of nonresponsive threads required for the multikill_timeout.

api/envoy/config/bootstrap/v3/bootstrap.proto

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

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

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

antoniovicente

Thanks for the change, and sorry for the slight delays in review.

source/server/guarddog_impl.h

test/server/guarddog_impl_test.cc

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

KBaichoo · 2020-07-17T21:43:32Z

/ retest

repokitteh-read-only · 2020-07-17T21:43:35Z

🐴 hold your horses - no failures detected, yet.

🐱

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

see: more, trace.

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

KBaichoo · 2020-07-20T18:03:26Z

PTAL @envoyproxy/api-shepherds

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

htuch · 2020-07-20T22:06:11Z

/lgtm api

KBaichoo · 2020-07-21T12:45:10Z

PTAL @envoyproxy/maintainers

mattklein123

Thanks!

WatchDog will now kill if max(2, registered_threads * multi_kill_threshold) threads have gone above the multikill_timeout. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

WatchDog will now kill if max(2, registered_threads * multi_kill_threshold) threads have gone above the multikill_timeout. Signed-off-by: Kevin Baichoo <kbaichoo@google.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

Added watchdog support for a multithreaded kill threshold.

b891eae

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

repokitteh-read-only bot added the api label Jul 15, 2020

repokitteh-read-only bot requested a review from antoniovicente July 15, 2020 17:54

junr03 assigned antoniovicente Jul 15, 2020

Clangtidy fix.

96feecb

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

antoniovicente reviewed Jul 15, 2020

View reviewed changes

KBaichoo added 3 commits July 16, 2020 18:36

minor fixes (naming, comments)

006e4e4

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

Merge remote-tracking branch 'upstream/master' into watchdog-multikil…

3023408

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

Added ceil to spelling dictionary, fixed typo.

01864ee

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

antoniovicente previously approved these changes Jul 17, 2020

View reviewed changes

source/server/guarddog_impl.h Outdated Show resolved Hide resolved

test/server/guarddog_impl_test.cc Outdated Show resolved Hide resolved

test/server/guarddog_impl_test.cc Outdated Show resolved Hide resolved

Minor changes.

39ac18b

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

KBaichoo dismissed antoniovicente’s stale review via 39ac18b July 17, 2020 21:24

antoniovicente previously approved these changes Jul 17, 2020

View reviewed changes

KBaichoo added 2 commits July 20, 2020 13:09

Kick CI

6518049

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

Formatting fix.

0b16370

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

KBaichoo dismissed antoniovicente’s stale review via 0b16370 July 20, 2020 15:25

mattklein123 self-assigned this Jul 20, 2020

protobuf nearby field references formatting.

297d4c8

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

repokitteh-read-only bot removed the api label Jul 20, 2020

mattklein123 approved these changes Jul 21, 2020

View reviewed changes

mattklein123 merged commit 7f78581 into envoyproxy:master Jul 21, 2020

rgs1 mentioned this pull request Jul 22, 2020

http: add removeIf() header map function #12160

Merged

Conversation

KBaichoo commented Jul 15, 2020

Uh oh!

repokitteh-read-only bot commented Jul 15, 2020

Uh oh!

KBaichoo commented Jul 15, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KBaichoo commented Jul 17, 2020

Uh oh!

repokitteh-read-only bot commented Jul 17, 2020

Uh oh!

KBaichoo commented Jul 20, 2020

Uh oh!

htuch commented Jul 20, 2020

Uh oh!

KBaichoo commented Jul 21, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants