xds-failover: adding main logic #34215

adisuissa · 2024-05-17T02:56:17Z

Commit Message: xds-failover: adding main logic
Additional Description:
Adding the main logic for the xDS-failover support.
It currently supports a primary and failover sources. Initially, the manager attempts to connect to the primary twice, and if both fail, alternates between the failover and the primary until one is successfully connected to.
Once a connection is made (a response arrives) from either source, the failover manager will stick to that source.

Risk Level: low - not plumbed into any GrpcMux object yet.
Testing: Added unit test.
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only · 2024-05-17T02:56:22Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #34215 was opened by adisuissa.

see: more, trace.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2024-05-17T03:23:54Z

Harvey PTAL. LMK if you want more context (I've got an internal integration test the exercises this code).
/assign @htuch

htuch

Approach LGTM modulo a few issues commented on.

source/extensions/config_subscription/grpc/grpc_mux_failover.h

test/extensions/config_subscription/grpc/grpc_mux_failover_test.cc

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2024-05-21T17:33:04Z

Assigning @soulxu as !Google maintainer reviewer.
/assign @soulxu

adisuissa · 2024-05-22T02:45:06Z

Unassigning @soulxu as he is OOO this week.
@wbpcode can you please take a ~Google pass at this PR?

source/extensions/config_subscription/grpc/grpc_mux_failover.h

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2024-05-24T21:23:34Z

@nezdolik will you have some bandwidth to review this PR as a !Google reviewer?
Thanks in advance!

nezdolik · 2024-05-27T08:21:10Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+      ENVOY_LOG_MISC(trace, "Attempting to reconnect to the failover gRPC source, as a connection "
+                            "to it was previously established.");
+      if (connected_to_ != ConnectedState::Failover) {
+        ASSERT(connected_to_ == ConnectedState::None);


do we need to explicitly set connecting_to_primary_ = false; here?

Added an assert, as connecting_to_primary_ should always be false after being connected to failover. Added a similar assert for the primary part above.

nezdolik · 2024-05-27T08:21:39Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

  }

  virtual ~GrpcMuxFailover() = default;

+  // Attempts to establish a new stream to the either the primary or failover source.
  void establishNewStream() {


this method has certain degree of code duplication, wonder if it would be possible to refactor.

Added functions that abstract the calls. I think it is still more readable/understandable if the "ever-connected" parts are done before the rest of the code.

nezdolik · 2024-05-27T08:26:51Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+  // determined similar to the primary variants.
+  // Note that while Envoy can only be connected to a single source (mutually
+  // exclusive), it can attempt connecting to more than one source at a time.
+  bool connecting_to_primary_ : 1;


what is the benefit of using bit fields here and not simply initilialise like:

bool connecting_to_primary_ {false};

Force of habit (to keep bools small, which is good when there are many instances), but I agree that it makes little sense here.
Modified as you suggested.

nezdolik · 2024-05-27T08:30:22Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+  ConnectedState connected_to_;
+
+  // A flag that keeps track of whether Envoy successfully connected to either the
+  // primary or failover source.


Should we also clarify that successful connection here means "receiving discovery response from xds server"?

Added a note.

nezdolik · 2024-05-27T08:39:26Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+      return;
+    }
+    // No prior connection was established, prefer the primary over the failover.
+    if (connecting_to_failover_ || (connected_to_ == ConnectedState::Failover)) {


confused how (connected_to_ == ConnectedState::Failover) could evaluate to true here, e.g. the code execution would get here if ever_connected_to_ is None and there is no established stream

You are correct, it cannot. Removed that part.

nezdolik · 2024-05-27T08:42:17Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+      failover_grpc_stream_->sendMessage(request);
+      return;
+    }
+    // Either connecting/connected to the primary, or no connection was attempted.
    primary_grpc_stream_->sendMessage(request);


do we need to check here if we are connected to primary prior to sending? Or would it just produce stream error logs when not connected?

I wanted to keep it the same as GrpcStream. We may want to ensure that in the future, but for now I think we can keep parity.

nezdolik · 2024-05-27T08:49:01Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+      // connection was closed.
+      ASSERT((parent_.connecting_to_primary_ || parent_.connected_to_ == ConnectedState::Primary) &&
+             !parent_.connecting_to_failover_ &&
+             (parent_.connected_to_ != ConnectedState::Failover));


do we need to repeat the parent_.connected_to_ != ConnectedState::Failover assertion here? since it is also being evaluated in the line above

Yes, you are correct. Removed the prior ASSERT (same for the failover part).

nezdolik · 2024-05-27T08:52:51Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

-      // TODO(adisuissa): At the moment this is a pass-through method. Once the
-      // implementation matures, this call will be updated.
+      ASSERT(
+          (parent_.connecting_to_primary_ || (parent_.connected_to_ == ConnectedState::Primary)) &&


nit: this bool expression (and maybe similar ones) appear frequently in the code:
(parent_.connecting_to_primary_ || (parent_.connected_to_ == ConnectedState::Primary), it could benefit from extracting it into small utility method like isConnectingToOrConnectedToPrimary()

Good idea!
I've refactored these and the similar primary counterparts into a function.

source/extensions/config_subscription/grpc/grpc_mux_failover.h

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa

Thanks for the great comments @nezdolik!

source/extensions/config_subscription/grpc/grpc_mux_failover.h

adisuissa · 2024-05-28T14:55:30Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

  }

  virtual ~GrpcMuxFailover() = default;

+  // Attempts to establish a new stream to the either the primary or failover source.
  void establishNewStream() {


Added functions that abstract the calls. I think it is still more readable/understandable if the "ever-connected" parts are done before the rest of the code.

adisuissa · 2024-05-28T14:56:34Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+      ENVOY_LOG_MISC(trace, "Attempting to reconnect to the failover gRPC source, as a connection "
+                            "to it was previously established.");
+      if (connected_to_ != ConnectedState::Failover) {
+        ASSERT(connected_to_ == ConnectedState::None);


Added an assert, as connecting_to_primary_ should always be false after being connected to failover. Added a similar assert for the primary part above.

adisuissa · 2024-05-28T14:57:14Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+      return;
+    }
+    // No prior connection was established, prefer the primary over the failover.
+    if (connecting_to_failover_ || (connected_to_ == ConnectedState::Failover)) {


You are correct, it cannot. Removed that part.

adisuissa · 2024-05-28T14:58:23Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+      failover_grpc_stream_->sendMessage(request);
+      return;
+    }
+    // Either connecting/connected to the primary, or no connection was attempted.
    primary_grpc_stream_->sendMessage(request);


I wanted to keep it the same as GrpcStream. We may want to ensure that in the future, but for now I think we can keep parity.

adisuissa · 2024-05-28T14:59:18Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+      // connection was closed.
+      ASSERT((parent_.connecting_to_primary_ || parent_.connected_to_ == ConnectedState::Primary) &&
+             !parent_.connecting_to_failover_ &&
+             (parent_.connected_to_ != ConnectedState::Failover));


Yes, you are correct. Removed the prior ASSERT (same for the failover part).

adisuissa · 2024-05-28T14:59:41Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

-      // TODO(adisuissa): At the moment this is a pass-through method. Once the
-      // implementation matures, this call will be updated.
+      ASSERT(
+          (parent_.connecting_to_primary_ || (parent_.connected_to_ == ConnectedState::Primary)) &&


Good idea!
I've refactored these and the similar primary counterparts into a function.

adisuissa · 2024-05-28T15:01:57Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+  // determined similar to the primary variants.
+  // Note that while Envoy can only be connected to a single source (mutually
+  // exclusive), it can attempt connecting to more than one source at a time.
+  bool connecting_to_primary_ : 1;


Force of habit (to keep bools small, which is good when there are many instances), but I agree that it makes little sense here.
Modified as you suggested.

adisuissa · 2024-05-28T15:02:08Z

source/extensions/config_subscription/grpc/grpc_mux_failover.h

+  ConnectedState connected_to_;
+
+  // A flag that keeps track of whether Envoy successfully connected to either the
+  // primary or failover source.


Added a note.

nezdolik

Thanks, exciting feature! There is also certain degree of code duplication across FailoverGrpcStreamCallbacks and PrimaryGrpcStreamCallbacks, but is also fine to leave it as it is if refactoring is too much effort.

adisuissa · 2024-05-28T17:54:41Z

Thanks, exciting feature! There is also certain degree of code duplication across FailoverGrpcStreamCallbacks and PrimaryGrpcStreamCallbacks, but is also fine to leave it as it is if refactoring is too much effort.

Thanks!
I think that the primary and fallback will be quite different as we get to test this out and add some different behavior.
We can always refactor the code later, making them extend the same class or something.

adisuissa · 2024-05-28T17:54:58Z

cc @htuch for final approval and merge

htuch

LGTM, thanks!

xds-failover: adding main logic

06e12a9

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa added 2 commits May 17, 2024 03:21

format

10d7daf

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Adding more tests

c5cd726

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa marked this pull request as ready for review May 17, 2024 03:22

repokitteh-read-only bot assigned htuch May 17, 2024

htuch requested changes May 19, 2024

View reviewed changes

adisuissa added 4 commits May 20, 2024 19:12

increase coverage

e0f3975

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

format

5af4dea

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Adding timer to constrain retries

26aa991

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

typos

b446347

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot assigned soulxu May 21, 2024

htuch reviewed May 23, 2024

View reviewed changes

source/extensions/config_subscription/grpc/grpc_mux_failover.h Outdated Show resolved Hide resolved

adisuissa added 3 commits May 24, 2024 17:54

converting connected_to to enum

aeec2ef

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

increasing coverage

3125aa6

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into xds_failover_main

e7aa3bc

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

nezdolik reviewed May 27, 2024

View reviewed changes

adisuissa added 2 commits May 28, 2024 14:52

comments

5c1508e

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Merge remote-tracking branch 'upstream/main' into xds_failover_main

a4db443

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa commented May 28, 2024

View reviewed changes

nezdolik approved these changes May 28, 2024

View reviewed changes

htuch approved these changes May 29, 2024

View reviewed changes

htuch merged commit f7952ef into envoyproxy:main May 29, 2024
52 checks passed

xds-failover: adding main logic #34215

xds-failover: adding main logic #34215

Conversation

adisuissa commented May 17, 2024

repokitteh-read-only bot commented May 17, 2024

adisuissa commented May 17, 2024

htuch left a comment

Choose a reason for hiding this comment

adisuissa commented May 21, 2024

adisuissa commented May 22, 2024

adisuissa commented May 24, 2024

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

adisuissa left a 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

nezdolik left a comment

Choose a reason for hiding this comment

adisuissa commented May 28, 2024

adisuissa commented May 28, 2024

htuch left a comment

Choose a reason for hiding this comment