xds resolver: fix edge cases in interactions between LDS and RDS #31668

markdroth · 2022-11-16T00:28:22Z

This fixes a couple of bugs accidentally introduced in #31457. It adds tests for those bugs and for the edge cases discussed in #25528.

Once this gets merged, it will need to be back-ported to the 1.51 branch before the release. CC @gnossen

yashykt · 2022-11-16T02:12:45Z

test/cpp/end2end/xds/xds_routing_end2end_test.cc

+    ::testing::Values(XdsTestType().set_enable_rds_testing()),
+    &XdsTestType::Name);
+
+TEST_P(LdsRdsInteractionTest, SwitchFromRdsToInlineRouteConfig) {


which of these tests were failing before this?

Without the fix, both SwitchFromRdsToInlineRouteConfig and HcmConfigUpdatedWithoutRdsChange fail.

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc

Automated fix for refs/heads/xds_lds_update_fix

yashykt

The sleep bothers me but otherwise looks ok

yashykt · 2022-11-16T18:12:01Z

test/cpp/end2end/xds/xds_routing_end2end_test.cc

  while (true) {
    ASSERT_LT(absl::Now(), deadline) << "timed out waiting for LDS ACK";
    response_state = balancer_->ads_service()->lds_response_state();
    if (response_state.has_value()) break;
+    absl::SleepFor(absl::Seconds(1) * grpc_test_slowdown_factor());


uh oh! Why do we need a sleep?

The sleep just avoids wasting CPU time. Without it, this is just a busy loop, constantly checking the same thing over and over again. With the sleep, we pause to actually give the system time to change state before we check again.

Note that the sleep does not affect the overall 30-second deadline for getting the ACK.

something like using a notification might be better

yashykt · 2022-11-16T18:12:26Z

test/cpp/end2end/xds/xds_routing_end2end_test.cc

+    ASSERT_LT(absl::Now(), deadline) << "timed out waiting for LDS ACK";
+    response_state = balancer_->ads_service()->lds_response_state();
+    if (response_state.has_value()) break;
+    absl::SleepFor(absl::Seconds(1) * grpc_test_slowdown_factor());


here too, why do we need the sleep?

If we need it anyway, can you add a comment as to why?

Same answer as above. It doesn't change the behavior, just avoids wasting CPU time.

yashykt · 2022-11-16T18:12:40Z

test/cpp/end2end/xds/xds_routing_end2end_test.cc

+    ASSERT_LT(absl::Now(), deadline) << "timed out waiting for RDS ACK";
+    response_state = balancer_->ads_service()->rds_response_state();
+    if (response_state.has_value()) break;
+    absl::SleepFor(absl::Seconds(1) * grpc_test_slowdown_factor());


here too, why do we need the sleep?

Same answer as above. It doesn't change the behavior, just avoids wasting CPU time.

…c#31668) * xds resolver: fix edge cases in interactions between LDS and RDS * improve SwitchFromInlineRouteConfigToRds test * clang-tidy * Automated change: Fix sanity tests Co-authored-by: markdroth <markdroth@users.noreply.github.com>

) (#31672) * xds resolver: fix edge cases in interactions between LDS and RDS * improve SwitchFromInlineRouteConfigToRds test * clang-tidy * Automated change: Fix sanity tests Co-authored-by: markdroth <markdroth@users.noreply.github.com> Co-authored-by: markdroth <markdroth@users.noreply.github.com>

* Bump v1.51.x to 1.51.0-pre1 (#31622) * bump version to 1.51.0-pre1 * regenerate projects * Branch 1.51.x: Cherrypick "Adding token url validation cases for psc endpoints #31616" and "Added url validation for aws metadata endpoints in aws external account #31626" (#31646) * Adding token url validation cases for psc endpoints (#31616) * Adding validation case for psc endpoint * formatting fix * Added url validation for aws metadata endpoints in aws external account (#31626) * Added url validation for aws metadata endpoints in aws external account * addressing review comments * fix error message back * Fix broken test * xds resolver: fix edge cases in interactions between LDS and RDS (#31668) (#31672) * xds resolver: fix edge cases in interactions between LDS and RDS * improve SwitchFromInlineRouteConfigToRds test * clang-tidy * Automated change: Fix sanity tests Co-authored-by: markdroth <markdroth@users.noreply.github.com> Co-authored-by: markdroth <markdroth@users.noreply.github.com> * xDS: fix crash on wrong listener type (both client and server side) (#31684) (#31691) * xds resolver: fix crash on wrong listener type * fix same bug on server side * fix clang-tidy and add requested TODO * Bump v1.51.x to v1.51.0 (#31705) * bump version to 1.51.0 * regenerate projects * Revert "Build with System OpenSSL on Mac OS arm64 (#31096)" (#31739) This reverts commit b3d9833. * Bump v1.51.x to 1.51.1 (#31740) * Bump to 1.51.1 * Regenerate projects Co-authored-by: Richard Belleville <rbellevi@google.com> Co-authored-by: aeitzman <12433791+aeitzman@users.noreply.github.com> Co-authored-by: Mark D. Roth <roth@google.com> Co-authored-by: markdroth <markdroth@users.noreply.github.com>

xds resolver: fix edge cases in interactions between LDS and RDS

56610d0

markdroth added priority/P0/RELEASE BLOCKER release notes: no Indicates if PR should not be in release notes labels Nov 16, 2022

markdroth requested a review from yashykt November 16, 2022 00:28

github-actions bot added lang/c++ lang/core labels Nov 16, 2022

grpc-checks bot added per-call-memory/neutral per-channel-memory/neutral bloat/none labels Nov 16, 2022

yashykt reviewed Nov 16, 2022

View reviewed changes

src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc Show resolved Hide resolved

markdroth and others added 4 commits November 16, 2022 16:59

improve SwitchFromInlineRouteConfigToRds test

cf3694f

clang-tidy

665285c

Automated change: Fix sanity tests

525cada

Merge pull request #271 from markdroth/create-pull-request/patch-665285c

f096703

Automated fix for refs/heads/xds_lds_update_fix

yashykt approved these changes Nov 16, 2022

View reviewed changes

markdroth merged commit 64589d7 into grpc:master Nov 16, 2022

markdroth deleted the xds_lds_update_fix branch November 16, 2022 19:04

markdroth mentioned this pull request Nov 16, 2022

Backport: xds resolver: fix edge cases in interactions between LDS and RDS #31672

Merged

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Nov 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds resolver: fix edge cases in interactions between LDS and RDS #31668

xds resolver: fix edge cases in interactions between LDS and RDS #31668

markdroth commented Nov 16, 2022

yashykt Nov 16, 2022

markdroth Nov 16, 2022

yashykt left a comment

yashykt Nov 16, 2022

markdroth Nov 16, 2022

yashykt Nov 17, 2022

yashykt Nov 16, 2022

yashykt Nov 16, 2022

markdroth Nov 16, 2022

yashykt Nov 16, 2022

markdroth Nov 16, 2022

xds resolver: fix edge cases in interactions between LDS and RDS #31668

xds resolver: fix edge cases in interactions between LDS and RDS #31668

Conversation

markdroth commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashykt 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