xds-failover: plumbing the API and adding integration test #34761

adisuissa · 2024-06-14T20:11:27Z

Commit Message: xds-failover: plumbing the API and adding integration test
Additional Description:
Adding the ability to use xDS-Failover into the API (ADS config).
Introducing the xds-failover integration tests.
This is all guarded behind the envoy.restart_features.xds_failover_support guard.
In the future, once we test xDS-Failover out and ensure it works as expected, the runtime guard will be removed.

Risk Level: low if not configured, medium if it is.
Testing: Added unit tests and integration tests
Docs Changes: N/A (once xDS-failover behavior will be finalized).
Release Notes: N/A (once xDS-failover behavior will be finalized).
Platform Specific Features: N/A.

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

repokitteh-read-only · 2024-06-14T20:11:32Z

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: #34761 was opened by adisuissa.

see: more, trace.

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

adisuissa · 2024-06-24T15:31:12Z

Also assigning Kateryna to expedite the review cycle.
/assign @nezdolik

nezdolik · 2024-06-25T17:31:09Z

source/common/config/utility.cc

-  RETURN_IF_NOT_OK(checkApiConfigSourceNames(api_config_source));
+  RETURN_IF_NOT_OK(checkApiConfigSourceNames(
+      api_config_source,
+      Runtime::runtimeFeatureEnabled("envoy.restart_features.xds_failover_support") ? 2 : 1));


can this be computed in the `checkApiConfigSourceNames function instead of being passed as function param?

It will require the checkApiConfigSourceNames() function to become a method (rather than a function), or the runtime passed to it, and I'm not sure if it's worth it as this runtime-flag will disappear in the future.
If you feel strongly about this, then I'll pass the runtime as a second param.

nezdolik · 2024-06-25T17:54:14Z

source/common/config/utility.cc

+    bool skip_cluster_check, int grpc_service_idx) {
+  RETURN_IF_NOT_OK(checkApiConfigSourceNames(
+      api_config_source,
+      Runtime::runtimeFeatureEnabled("envoy.restart_features.xds_failover_support") ? 2 : 1));


nezdolik · 2024-06-25T18:03:27Z

source/extensions/config_subscription/grpc/new_grpc_mux_impl.cc

+                        grpc_mux_context.scope_,
+                        // TODO(adisuissa): the backoff strategy for the failover should
+                        // be the same as the primary source.
+                        std::make_unique<FixedBackOffStrategy>(500),


nit could factor out this magic number 500 into constant

Done, although IMHO it makes the code less readable (because of the template).

nezdolik · 2024-06-25T18:04:44Z

source/common/upstream/cluster_manager_impl.cc

+      auto factory_primary_or_error = Config::Utility::factoryForGrpcApiConfigSource(
+          *async_client_manager_, dyn_resources.ads_config(), *stats_.rootScope(), false, 0);
+      THROW_IF_STATUS_NOT_OK(factory_primary_or_error, throw);
+      Grpc::AsyncClientFactoryPtr factory_failover = nullptr;
+      if (Runtime::runtimeFeatureEnabled("envoy.restart_features.xds_failover_support")) {
+        auto factory_failover_or_error = Config::Utility::factoryForGrpcApiConfigSource(
+            *async_client_manager_, dyn_resources.ads_config(), *stats_.rootScope(), false, 1);
+        THROW_IF_STATUS_NOT_OK(factory_failover_or_error, throw);
+        factory_failover = std::move(factory_failover_or_error.value());
+      }
+      ads_mux_ = factory->create(
+          factory_primary_or_error.value()->createUncachedRawAsyncClient(),
+          factory_failover ? factory_failover->createUncachedRawAsyncClient() : nullptr,
+          dispatcher_, random_, *stats_.rootScope(), dyn_resources.ads_config(), local_info_,
+          std::move(custom_config_validators), std::move(backoff_strategy),
+          makeOptRefFromPtr(xds_config_tracker_.get()), {}, use_eds_cache);


this large code block is is identical to one below, could be factored out into a dedicated function.

There are a few differences between the initializations of SotW and delta, and in this PR I don't want to make major changes - I'm keeping the same pattern as was prior to this PR.
We are planning to work more on this code base, so we will definitely change it later.

test/extensions/config_subscription/grpc/xds_failover_integration_test.cc

nezdolik · 2024-06-25T18:15:45Z

test/extensions/config_subscription/grpc/xds_failover_integration_test.cc

+}
+
+// Validate that once failover answers, primary will not be used, even after disconnecting.
+TEST_P(XdsFailoverAdsIntegrationTest, NoPrimaryUseAfterFailoverResponse) {


what would be happening if primary is not responding and failover as well, would it try to again connect to primary?

Envoy will alternate between the primary and failover over and over until one of them respondes. We've got a unit test that covers this in

envoy/test/extensions/config_subscription/grpc/grpc_mux_failover_test.cc

Line 344 in 46f86da

TEST_F(GrpcMuxFailoverTest, AlternatingBetweenFailoverAndPrimary) {

great, just wanted make sure this is covered by tests.

nezdolik · 2024-06-25T18:16:18Z

test/integration/base_integration_test.h

@@ -167,7 +167,7 @@ class BaseIntegrationTest : protected Logger::Loggable<Logger::Id::testing> {
  envoy::config::core::v3::Node last_node_;

  // Functions for testing reloadable config (xDS)
-  void createXdsUpstream();
+  virtual void createXdsUpstream();


will this break other tests?

No. Until now other tests just called this method. The new test is the only one overriding it. In c++ when a method is not declared virtual it cannot be overridden.

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

adisuissa

Thanks for the comments @nezdolik. This is ready for another round.

adisuissa · 2024-06-26T03:26:01Z

source/common/config/utility.cc

-  RETURN_IF_NOT_OK(checkApiConfigSourceNames(api_config_source));
+  RETURN_IF_NOT_OK(checkApiConfigSourceNames(
+      api_config_source,
+      Runtime::runtimeFeatureEnabled("envoy.restart_features.xds_failover_support") ? 2 : 1));


It will require the checkApiConfigSourceNames() function to become a method (rather than a function), or the runtime passed to it, and I'm not sure if it's worth it as this runtime-flag will disappear in the future.
If you feel strongly about this, then I'll pass the runtime as a second param.

adisuissa · 2024-06-26T03:26:13Z

source/common/config/utility.cc

+    bool skip_cluster_check, int grpc_service_idx) {
+  RETURN_IF_NOT_OK(checkApiConfigSourceNames(
+      api_config_source,
+      Runtime::runtimeFeatureEnabled("envoy.restart_features.xds_failover_support") ? 2 : 1));


adisuissa · 2024-06-26T03:28:40Z

source/common/upstream/cluster_manager_impl.cc

+      auto factory_primary_or_error = Config::Utility::factoryForGrpcApiConfigSource(
+          *async_client_manager_, dyn_resources.ads_config(), *stats_.rootScope(), false, 0);
+      THROW_IF_STATUS_NOT_OK(factory_primary_or_error, throw);
+      Grpc::AsyncClientFactoryPtr factory_failover = nullptr;
+      if (Runtime::runtimeFeatureEnabled("envoy.restart_features.xds_failover_support")) {
+        auto factory_failover_or_error = Config::Utility::factoryForGrpcApiConfigSource(
+            *async_client_manager_, dyn_resources.ads_config(), *stats_.rootScope(), false, 1);
+        THROW_IF_STATUS_NOT_OK(factory_failover_or_error, throw);
+        factory_failover = std::move(factory_failover_or_error.value());
+      }
+      ads_mux_ = factory->create(
+          factory_primary_or_error.value()->createUncachedRawAsyncClient(),
+          factory_failover ? factory_failover->createUncachedRawAsyncClient() : nullptr,
+          dispatcher_, random_, *stats_.rootScope(), dyn_resources.ads_config(), local_info_,
+          std::move(custom_config_validators), std::move(backoff_strategy),
+          makeOptRefFromPtr(xds_config_tracker_.get()), {}, use_eds_cache);


There are a few differences between the initializations of SotW and delta, and in this PR I don't want to make major changes - I'm keeping the same pattern as was prior to this PR.
We are planning to work more on this code base, so we will definitely change it later.

adisuissa · 2024-06-26T03:30:30Z

test/extensions/config_subscription/grpc/xds_failover_integration_test.cc

+}
+
+// Validate that once failover answers, primary will not be used, even after disconnecting.
+TEST_P(XdsFailoverAdsIntegrationTest, NoPrimaryUseAfterFailoverResponse) {


Envoy will alternate between the primary and failover over and over until one of them respondes. We've got a unit test that covers this in

envoy/test/extensions/config_subscription/grpc/grpc_mux_failover_test.cc

Line 344 in 46f86da

TEST_F(GrpcMuxFailoverTest, AlternatingBetweenFailoverAndPrimary) {

adisuissa · 2024-06-26T03:31:50Z

test/integration/base_integration_test.h

@@ -167,7 +167,7 @@ class BaseIntegrationTest : protected Logger::Loggable<Logger::Id::testing> {
  envoy::config::core::v3::Node last_node_;

  // Functions for testing reloadable config (xDS)
-  void createXdsUpstream();
+  virtual void createXdsUpstream();


No. Until now other tests just called this method. The new test is the only one overriding it. In c++ when a method is not declared virtual it cannot be overridden.

adisuissa · 2024-06-26T03:35:58Z

@htuch this is ready for another round of reviews.
RE our discussion about simulated-time, I've attempted using it, but some connections that are expected are not being attempted by Envoy. I'll continue debugging this, but I trust the real-time over the simulated-time for now.

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

htuch · 2024-06-27T03:56:11Z

test/extensions/config_subscription/grpc/xds_failover_integration_test.cc

+          auto* failover_google_grpc = failover_grpc_service->mutable_google_grpc();
+          auto* failover_ssl_creds =
+              failover_google_grpc->mutable_channel_credentials()->mutable_ssl_credentials();
+          failover_ssl_creds->mutable_root_certs()->set_filename(


Nit: I think there is a fair bit of setup boiler plate here that could be nicer to refactor a bit.

Ack, I agree.
I'll follow up on this in a subsequent PR.

htuch · 2024-06-27T03:59:07Z

Would be good to see a coverage report, for whatever reason it's not in the checks (maybe because of failures?)

LGTM otherwise!

adisuissa · 2024-06-27T11:59:36Z

/retest

adisuissa · 2024-06-27T14:43:08Z

Would be good to see a coverage report, for whatever reason it's not in the checks (maybe because of failures?)

LGTM otherwise!

It was a non-related flaky test.
Coverage should be available at: https://storage.googleapis.com/envoy-pr/295784d/coverage/index.html

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

htuch

LGTM, thanks!

xds-failover: plumbing the API and adding integration test

2b60cab

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

adisuissa added 3 commits June 17, 2024 14:37

updating test

061fa52

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

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

ac098f3

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

fixing uhv

896f921

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

adisuissa marked this pull request as ready for review June 20, 2024 03:30

adisuissa assigned htuch Jun 20, 2024

repokitteh-read-only bot assigned nezdolik Jun 24, 2024

nezdolik reviewed Jun 25, 2024

View reviewed changes

adisuissa added 2 commits June 26, 2024 03:16

addressing comments

6ad52b7

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

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

a5dc27a

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

adisuissa commented Jun 26, 2024

View reviewed changes

nezdolik previously approved these changes Jun 26, 2024

View reviewed changes

adisuissa added 2 commits June 27, 2024 02:05

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

df3eced

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

update tls construction

5fdf0f5

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

adisuissa dismissed nezdolik’s stale review via 5fdf0f5 June 27, 2024 02:20

htuch reviewed Jun 27, 2024

View reviewed changes

adisuissa added 2 commits June 27, 2024 20:58

integration tests covering unified xds mux

5de301a

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

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

be6e38f

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

htuch approved these changes Jun 27, 2024

View reviewed changes

htuch enabled auto-merge (squash) June 27, 2024 21:48

htuch merged commit 3feff04 into envoyproxy:main Jun 28, 2024
49 of 51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds-failover: plumbing the API and adding integration test #34761

xds-failover: plumbing the API and adding integration test #34761

adisuissa commented Jun 14, 2024

repokitteh-read-only bot commented Jun 14, 2024

adisuissa commented Jun 24, 2024

nezdolik Jun 25, 2024

adisuissa Jun 26, 2024

nezdolik Jun 25, 2024

adisuissa Jun 26, 2024

nezdolik Jun 25, 2024

adisuissa Jun 26, 2024

nezdolik Jun 25, 2024

adisuissa Jun 26, 2024

nezdolik Jun 25, 2024

adisuissa Jun 26, 2024

nezdolik Jun 26, 2024

nezdolik Jun 25, 2024

adisuissa Jun 26, 2024

adisuissa left a comment

adisuissa Jun 26, 2024

adisuissa Jun 26, 2024

adisuissa Jun 26, 2024

adisuissa Jun 26, 2024

adisuissa Jun 26, 2024

adisuissa commented Jun 26, 2024

htuch Jun 27, 2024

adisuissa Jun 27, 2024

htuch commented Jun 27, 2024

adisuissa commented Jun 27, 2024

adisuissa commented Jun 27, 2024

htuch left a comment

xds-failover: plumbing the API and adding integration test #34761

xds-failover: plumbing the API and adding integration test #34761

Conversation

adisuissa commented Jun 14, 2024

repokitteh-read-only bot commented Jun 14, 2024

adisuissa commented Jun 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

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

adisuissa commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Jun 27, 2024

adisuissa commented Jun 27, 2024

adisuissa commented Jun 27, 2024

htuch left a comment

Choose a reason for hiding this comment