It may have been a merge conflict which wasn't resolved properly.

yes, i guess so, sorry for the mistake, i'm must say that I didn't take a deep look in your DNS lookup PR during the rebase/merge conflict, this is a mistake that shouldn't be

@enjoy-binbin Don't worry! Thanks for fixing it!

@enjoy-binbin Can you add a brief summary of the problem that we can add to the release notes.

I'm wondering if we should add some tests for this, I checked it out locally to validate the fix.

Can you add a brief summary of the problem that we can add to the release notes.

i added it in the top comment, hope i made it clear

I'm wondering if we should add some tests for this, I checked it out locally to validate the fix.

thanks for the check, i also did it locally to validate this. And yes, i think we should, but in #10436, i see @zuiderkwast dropping the tests for some reasons, do you think we can add the tests?

@enjoy-binbin I didn't drop any test, did I? Do you mean I skipped adding a test? That's true, probably because I was lazy or I didn't have time. A test would be better than no test of course, but I think this can be merged without a test too.

i mean this 2ae3c59, using "localhost", do you think i can bring it (localhost part) back? i can take a look in it when i am free.

but I think this can be merged without a test too.

yes, i think this one is safe to merge, but it would be greet if we have some tests to cover it, i will try "localhost" when i have times

i am not able to add the localhost test without the Prefer IPv4 over IPv6 change in 2ae3c59

I tried these confs:

• bind ::1 [err]: Can't start the Redis server
• bind "127.0.0.1 ::1" [exception]: Executing test client: couldn't open socket: Name or service not known. this one work in local, but failed in Github CI
• bind "127.0.0.1" hang
• bind "*" hang
• bind "0.0.0.0 ::" [exception]: Executing test client: couldn't open socket: Name or service not known.
• bind "127.0.0.1 -::1": Executing test client: couldn't open socket: Name or service not known.

This sounds familiar. @yossigo didn't like "prefer IPv4 over IPv6". That's why I dropped the tests with "localhost" before. The discussion is here: #10436 (comment). He mentioned that /etc/gai.conf can be used for that, but I don't know how.

@enjoy-binbin Did you try bind 127.0.0.1 -::1, to allow the server to start on systems with no IPv6?
Also, what does localhost resolve to on your different tests? This is tricky because some systems map localhost to ::1 as well, and others use a dedicated ipv6-localhost.

The default, as described by RFC 3484 is to prefer IPv6. To change that, you can add the following line to the end of your /etc/gai.conf:

precedence ::ffff:0:0/96  100

Reconsidering this, I agree we can't assume users go around changing system files for this purpose. Perhaps we should explicitly support a -4/-6 flag that controls the "prefer IPv4 over IPv6" behavior? Many tools include such a flag for this purpose.

Did you try bind 127.0.0.1 -::1, to allow the server to start on systems with no IPv6?

the test was hang in somewhere

Also, what does localhost resolve to on your different tests? This is tricky because some systems map localhost to ::1 as well, and others use a dedicated ipv6-localhost.

i see in ours test-ubantu-latest and test-sanitizer-address, both resolve to ::1, like both are sending CLUSTER MEET ::1.
it is odd that in my machine, bind "127.0.0.1 ::1" will work (localhost resolve to ::1 too), but failed in GH CI

and i see the /etc/hosts in test-ubantu-latest is:

127.0.0.1 localhost

# The following lines are desirable for IPv6 capable hosts
::1     localhost ip6-localhost ip6-loopback
fe00::0 ip6-localnet
ff00::0 ip6-mcastprefix
ff02::1 ip6-allnodes
ff02::2 ip6-allrouters
ff02::3 ip6-allhosts
10.1.0.126 fv-az203-278.hq41me5fykbulac445kfrvsskg.dx.internal.cloudapp.net fv-az203-278

Reconsidering this, I agree we can't assume users go around changing system files for this purpose. Perhaps we should explicitly support a -4/-6 flag that controls the "prefer IPv4 over IPv6" behavior? Many tools include such a flag for this purpose.

the -4 / -6 flag looks like a good idea, i will give it a try next day

i am adding the -4 / -6 options for redis-cli, can see if we need it... it can also be a separate PR (don't want the fix blocked by it)

i see hiredis add this option as well in: redis/hiredis#1096
so maybe we can someday update the hiredis, and use redisConnectWithOptions instead of redisConnect in redis-cli, with -4 / -6 options

@yossigo @madolson any reason this wasn't merged (i didn't read the correspondence)

@enjoy-binbin i understand this fixes a bug that was introduced in 7.0.3 and adds some new CLI options, does it have a chance to break anything?
(sorry, too many other tasks, can't afford to really read the code or discussion right now)

I think we can just fix the bug, it's very safe to merge. And postpone the clI options, no need to rush it now.

I reverted it back, let's merge it first and see if cli options are needed later

@oranagra AFAICT the added cli options can't break anything since the default behaviour is unchanged. The options made it possible to reliably test dns lookup in CI though.

I suppose they could be done in a separate PR but otoh they enable testing of this PR.

I'm gonna assume Yossi and Madelyn are too busy at the moment, and i don't like to leave this bugfix out of the next release, so i'll merge the fix as is, and we can open another PR to add the test and the new options.

@enjoy-binbin Are you going to open a separate PR with the -4 and -6 options?

@zuiderkwast yean, on my way :)

Ok, done, #11315