redis: add support for hostnames returned in cluster_slots response by huntsman90 · Pull Request #19541 · envoyproxy/envoy

huntsman90 · 2022-01-13T18:33:27Z

Commit Message:

Currently, CLUSTER SLOTS response is expected to contain the IP address of each redis host in the response.
However, response from AWS Elasticache contains the hostnames instead. This PR adds support to DNS resolve the hostnames asynchronously before cluster slot updates are applied.

Co-authored-by: Konstantin Belyalov arsenicus@mail.ru

Additional Description:

This PR builds directly on top of #19192 and fixes the bugs in it.

To read this PR, the best starting point may be the onResponse method in redis_cluster.cc and read from there. Almost all of the logic change is confined to redis_cluster.cc.

The failure scenarios while resolving the hostnames are listed below:

If the DNS resolution for any of the primary redis entries fails, we stop the entire resolution, log an error, and reset the resolve timer.
If the DNS resolution for any of the replicas fails, we still continue on, skipping that replica. A warning is logged instead.
In both scenarios, counter update_failure_ is incremented.

Testing:

Unit tests
We also tested this manually in an AWS setup, and it worked great

Fixes #8440

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru> Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

- refactor code to make more readable, and simplify recursion logic - do not fail entire resolution if a replica resolution fails - add test, fix existing one Co-authored-by: Konstantin Belyalov <arsenicus@mail.ru> Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

repokitteh-read-only · 2022-01-13T18:33:30Z

Hi @huntsman90, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #19541 was opened by huntsman90.

see: more, trace.

repokitteh-read-only · 2022-01-13T18:33:31Z

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: #19541 was opened by huntsman90.

see: more, trace.

…ponse using addrInfo() - ran code formatter, which changed some unrelated code, but it mostly looks fine. Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

alyssawilk · 2022-01-14T12:54:48Z

Hey @snowp redis is owned by Matt, but could you field reviews until he's back?

snowp

Thanks for working on this! Some comments to get you started

/wait

source/extensions/clusters/redis/redis_cluster.cc

- refactor `resolveReplicas` to resolve all the replicas in a loop, in parallel - add docstrings for `resolveReplicas` and `resolveHostname` Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

huntsman90 · 2022-01-19T07:05:50Z

Thanks for working on this! Some comments to get you started

/wait

Thanks for the review @snowp . I have incorporated all the suggestions. I've left a few comments/questions for the ones left unresolved.

NOTE: I'm not sure of the review-comment-resolve process here; so, apologies if me resolving the comments slows the review process for you.

Additionally, I have a question: It says that the branch has conflicts that need resolution. However, would rebasing on main branch cause problems with the review? (I read in the contributing guide to avoid rebasing until the review is complete, so unsure of the recommended steps here).

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

huntsman90 · 2022-01-19T19:45:45Z

Additionally, I have a question: It says that the branch has conflicts that need resolution. However, would rebasing on main branch cause problems with the review? (I read in the contributing guide to avoid rebasing until the review is complete, so unsure of the recommended steps here).

I fixed this via a merge of main branch.

snowp

Thanks for working on this! Some high level comments, I would love to be able to simplify this code a little bit

source/extensions/clusters/redis/redis_cluster.cc

snowp · 2022-01-24T21:10:07Z

Also FYI @mattklein123 since you're back now and Redis belongs to you :)

KBaichoo · 2022-02-18T16:26:28Z

PTAL @snowp to either take another pass, or hand off to codeowner

snowp · 2022-02-18T16:35:51Z

We've been talking over DM, I believe this one is still waiting for an integration test?

/wait

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

snowp · 2022-02-18T22:38:53Z

@mattklein123 Can you keep an eye on this as I'll be out starting next week?

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

source/extensions/clusters/redis/redis_cluster.cc

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

huntsman90 · 2022-02-22T16:56:45Z

Hello! 👋 I've added the integrations test, and I believe the PR is ready for a review now. Please take a look when possible @mattklein123 . Thanks!

taking over review, integration test added

mattklein123

Thanks generally this LGTM. In addition to my comment in the code, can you please also add a release note as well as update https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/other_protocols/redis.html to make it more clear that we now support this.

Thank you.

/wait

source/extensions/clusters/redis/redis_cluster.cc

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

huntsman90 · 2022-02-25T19:33:39Z

@mattklein123 the PR is updated. Please recommend updates to the documentation added, if required.

mattklein123

Thanks!

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

…voyproxy#20217) Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com> Signed-off-by: kuochunghsu <kuochunghsu@pinterest.com>

belyalov and others added 7 commits January 13, 2022 10:11

WIP: adding support for redis cluster dns resolving

fc8bdff

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru> Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

Format code

b0d77c5

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru> Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

Add ElastiCache into spellcheck

f8720ee

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru> Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

Fix style

8fc72be

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru> Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

copy element from replicas to resolve vector

b9d8d23

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru> Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

run fix_format

063681e

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

huntsman90 marked this pull request as ready for review January 13, 2022 18:39

huntsman90 requested a review from mattklein123 as a code owner January 13, 2022 18:39

alyssawilk assigned HenryYYang and msukalski Jan 13, 2022

huntsman90 added 3 commits January 13, 2022 11:56

- fix syntax error introduced due to rebase; fetch address from DnsRe…

0f5b9d4

…ponse using addrInfo() - ran code formatter, which changed some unrelated code, but it mostly looks fine. Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

revert the formatting change introduced by tool

af08aed

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

fix check_format failure

0ed133f

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

alyssawilk assigned snowp Jan 14, 2022

snowp suggested changes Jan 18, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 18, 2022

- review comments

455e620

- refactor `resolveReplicas` to resolve all the replicas in a loop, in parallel - add docstrings for `resolveReplicas` and `resolveHostname` Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

repokitteh-read-only bot removed the waiting label Jan 19, 2022

huntsman90 requested a review from snowp January 19, 2022 07:05

huntsman90 added 2 commits January 18, 2022 23:09

Merge branch 'main' into elasticache_redis_hostnames

a91cc1a

fix bugprone-use-after-move error

491d544

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

snowp previously requested changes Jan 24, 2022

View reviewed changes

snowp added the waiting label Jan 28, 2022

repokitteh-read-only bot removed the waiting label Feb 14, 2022

huntsman90 requested a review from snowp February 14, 2022 17:52

rkarthick mentioned this pull request Feb 18, 2022

support for envoyproxy StackExchange/StackExchange.Redis#1989

Merged

repokitteh-read-only bot added the waiting label Feb 18, 2022

add integration test

9bab9bd

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

repokitteh-read-only bot removed the waiting label Feb 18, 2022

change the dns lookup family

9aa8772

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

snowp reviewed Feb 18, 2022

View reviewed changes

source/extensions/clusters/redis/redis_cluster.cc Outdated Show resolved Hide resolved

mattklein123 self-assigned this Feb 19, 2022

make the integer a shared pointer

5e2f4f1

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

huntsman90 requested a review from snowp February 19, 2022 01:01

mattklein123 requested changes Feb 22, 2022

View reviewed changes

source/extensions/clusters/redis/redis_cluster.cc Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Feb 22, 2022

update documentation

7087783

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

repokitteh-read-only bot removed the waiting label Feb 25, 2022

huntsman90 requested a review from mattklein123 February 25, 2022 19:31

Merge branch 'main' into elasticache_redis_hostnames

1fadbc6

mattklein123 approved these changes Feb 28, 2022

View reviewed changes

mattklein123 merged commit 4ce48c4 into envoyproxy:main Feb 28, 2022

huntsman90 mentioned this pull request Mar 4, 2022

redis: fix hostname resolution bug introduced in #19541 #20217

Merged

mattklein123 pushed a commit that referenced this pull request Mar 8, 2022

redis: fix hostname resolution bug introduced in #19541 (#20217)

153c825

Signed-off-by: Sai Teja Duthuluri <dsaiteja1908@gmail.com>

weisisea mentioned this pull request Jun 6, 2022

Unexpected response to cluster slot command 'Redis Elatsicache Cluster' #21587

Closed

ramaraochavali mentioned this pull request Sep 6, 2022

redis cluster stuck in warming state when there are "two" redis clusters #22995

Closed

Conversation

huntsman90 commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jan 13, 2022

Uh oh!

repokitteh-read-only bot commented Jan 13, 2022

Uh oh!

alyssawilk commented Jan 14, 2022

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

huntsman90 commented Jan 19, 2022

Uh oh!

huntsman90 commented Jan 19, 2022

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snowp commented Jan 24, 2022

Uh oh!

KBaichoo commented Feb 18, 2022

Uh oh!

snowp commented Feb 18, 2022

Uh oh!

snowp commented Feb 18, 2022

Uh oh!

Uh oh!

huntsman90 commented Feb 22, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

huntsman90 commented Feb 25, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

huntsman90 commented Jan 13, 2022 •

edited

Loading