Add IPv6 availability check to skip tests when unavailable

[diego-ciciani01]

Problem e test suite fails when IPv6 is not available on the system with the error:

The test suite fails when IPv6 is not available on the system

Solution

• Add is_ipv6_available() to detect IPv6 support
• Add VALLKEY_REQUIRE_IPV6 env var (inside a 'if' condition) to force IPv6 tests in CI, implement in the pipeline the export and volorization of env var
• Skip IPv6 tests automatically when IPv6 is not available
• Wrapped IPv6 tests in tests/unit/introspection.tcl to skip when IPv6 is unavailable

Testing

• Without IPv6: test are skipped automatically

• With IPv6: test run normally

• With VALKEY_REQUIRE_IPV6=1: tests always run (fails if IPv6 unavailable)

• Fixes [BUG] make test should detect ipv6 support #2643

[zuiderkwast]

Good job. However, I think we should do it using the existing mechanism to skip tests based on the existing tagging.

The tests/test_helper.tcl is the entry point that checks command line arguments. Here, we can add "ipv6" to ::denytags if IPv6 is not available and not required.

[zuiderkwast]

Why do we need three ways? Can we use only the 3rd way to detect this and save the result in some global variable to avoid doing it multiple times?

[diego-ciciani01]

I agree with you, multiple checks can seem a bit repetitive, but during development, the third test on my macbook never worked (I'll leave a test I did in the comments, but I didn't add it to the PR). I also made sure that IPv6 was enabled, so as a workaround I also did the first two checks.

I currently have version 9 of tcl.

here the the did

proc is_ipv6_available {} { if {[catch { set sock [socket -async ::1 0] close $sock } err]} { puts "ipv6 check failed: $err" return 0 } return 1 }

and i leave the result of the running test

[zuiderkwast]

Ah, you're testing to connect to port 22 = the SSH port. It is not always opened. We can not even assume that SSH is installed.

I think we can't assume that any specific port is opened. What we need to do is to create a server socket on a free port chosen by the OS. To do this, use port 0, a command like socket -server ::1 0. Read details under SERVER SOCKETS on the page https://www.tcl-lang.org/man/tcl/TclCmd/socket.html#M9

This is what I get on my system where IPv6 is available:

$ tclsh
% set s [socket -server ::1 0]
sockaaab2529cdd0
% chan configure $s -sockname
0.0.0.0 0.0.0.0 33953 :: :: 33953

Do the tests work with TCL 9? Do you get some error when running all the tests?

From what I remember, TCL 9 didn't work and I have opened #1673 to fix that.

[zuiderkwast]

Try this:

Suggested change

	if {[catch {exec ifconfig lo0} result] == 0} {
	if {[string match "*inet6*::1*" $result]} {
	return 1
	}
	}
	if {[catch {exec netstat -an} result] == 0} {
	if {[string match "*::1*" $result]} {
	return 1
	}
	}
	if {[catch {
	set sock [socket -async ::1 22]
	close $sock
	return 1
	}]} {
	return 0
	}
	if {[catch {
	set sock [socket -server ::1 0]
	close $sock
	return 1
	}]} {
	return 0
	}

[zuiderkwast]

I don't know how TCL behaves, but if set sock [socket -server ::1 0] never fails, even if IPv6 is not available, then we maybe also need to try to connect as a client to this server socket.

Something like this:

    if {[catch {
        set server [socket -server ::1 0]
        set port [lindex [chan configure $server -sockname] 5]
        set client [socket ::1 $port]
        close $server
        close $client
        return 1
    }]} {
        return 0
    }

[diego-ciciani01]

I solved the problem. As you said, version 9 of TCL doesn't work very well. I switched to 8.6, and now the socket opens correctly.

I will test also the client - server connect

[diego-ciciani01]

why did you set set port [lindex [chan configure $server -sockname] 5] ?

i thinks that the correct value should be 2, because this position returns the port

[zuiderkwast]

I didn't find the docs for this but I tested locally in tclsh and I got the port on two places:

% chan configure $sock -sockname           
0.0.0.0 0.0.0.0 35783 :: :: 35783

It looks like IPv4 IPv4 port IPv6 IPv6 port. The last port maybe is for IPv6. That's what I was thinking. I don't know if it's correct. It's better to find the proper documentation for this.

[diego-ciciani01]

aah it make sense, so i found this doc ->https://core.tcl-lang.org/tips/doc/trunk/tip/162.md
which explains exactly your test.

So i also added few commits with the changes that you told me

[diego-ciciani01]

aah it make sense, so i found this doc ->https://core.tcl-lang.org/tips/doc/trunk/tip/162.md
which explains exactly your test.

So i also added few commits with the changes that you told me

[diego-ciciani01]

hi @zuiderkwast, thank you very much for your feedback. If you need any further clarification, please let me know. Your advice is clear to me, please also let me know what you think about the IPv6 socket issue, after which I will modify the PR with your suggestions.

[codecov]

Codecov Report

❌ Patch coverage is 93.26923% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.62%. Comparing base (8182f4a) to head (fe06ea2).
⚠️ Report is 17 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/replication.c	25.00%	3 Missing ⚠️
src/scripting_engine.c	62.50%	3 Missing ⚠️
src/functions.c	96.87%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2674      +/-   ##
============================================
+ Coverage     72.58%   72.62%   +0.04%     
============================================
  Files           128      128              
  Lines         71277    71303      +26     
============================================
+ Hits          51736    51786      +50     
+ Misses        19541    19517      -24     

Files with missing lines	Coverage Δ
src/blocked.c	91.32% <100.00%> (ø)
src/cluster_migrateslots.c	91.69% <ø> (-0.40%)	⬇️
src/commands.def	100.00% <ø> (ø)
src/db.c	93.24% <100.00%> (ø)
src/eval.c	89.23% <100.00%> (ø)
src/hashtable.c	89.32% <100.00%> (+0.01%)	⬆️
src/kvstore.c	96.01% <100.00%> (ø)
src/lazyfree.c	92.56% <100.00%> (+6.17%)	⬆️
src/lua/engine_lua.c	88.12% <100.00%> (-0.22%)	⬇️
src/lua/function_lua.c	98.26% <100.00%> (-0.45%)	⬇️
... and 6 more

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[zuiderkwast]

Great! This is getting closer. I have some more ideas though after reading code and documentation. (None of us is a Tcl expert.)

[zuiderkwast]

Suggested change

[zuiderkwast]

Suggested change

[zuiderkwast]

Add a space between the # and the text, for better readability.

Suggested change

	#IPv6 detection utilities
	#for this detaction: the socket connection on ::1 address
	# IPv6 detection utilities
	# for this detaction: the socket connection on ::1 address

[zuiderkwast]

I'm reading the documentation (https://www.tcl-lang.org/man/tcl/TclCmd/socket.html) for server sockets again. The server sockets, the syntax is

socket -server command ?options? port

The IP address is not specified here. The command is the code to run when a client connects to it. The documentation says:

For each connection Tcl will create a new channel that may be used to communicate with the client. Tcl then invokes command (properly a command prefix list, see the EXAMPLES below) with three additional arguments: the name of the new channel, the address, in network address notation, of the client's host, and the client's port number.

Anyway, it seems to be OK to not have a correct command here, but I guess we should not write ::1 here because it's not an IP address at all. Better just use dummy or as the command (which will never be called anyway if the server doesn't start an event loop using vwait).

Also, if we use the option -myaddr ::1, then the server will only listen on the IPv6 address and the chan configure will only return 3 elements, where the last one is the port. That's also documented here under CONFIGURATION OPTIONS, -sockname:

For server sockets this option returns a list of a multiple of three elements each group of which have the same meaning as described above. The list contains more than one group when the server socket was created without -myaddr or with the argument to -myaddr being a domain name that resolves multiple IP addresses that are local to the invoking host.

One more thing, the indentation here looks odd. Use 4 spaces for each level.

To sum up:

Suggested change

	if {[catch {
	set server [socket -server ::1 0]
	set port [lindex [chan configure $server -sockname] 5]
	if {[catch {
	set server [socket -server dummy -myaddr ::1 0]
	set port [lindex [chan configure $server -sockname] 2]

[zuiderkwast]

I can see this will work, but comparing with other similar mechanisms to skip tests in various conditions, this logic should better be placed in the function tags_acceptable in the file tests/support/server.tcl. Add it next to these checks, here: https://github.com/valkey-io/valkey/blob/9.0.0-rc3/tests/support/server.tcl#L266

[zuiderkwast]

It appears you didn't get the purpose of this function and how it works.

This function runs for each test (and for start_server where tags can also be specified) and it checks it each test needs to be skipped or not.

For the command-line argument --tags, the ::denytags and ::allowtags are used. When a test is skipped because of ::denytags, it prints "Tag: $tag denied". When a test is skipped for another reason, that specific reason is printed instead, such as "Not supported in cluster mode", etc. The reason I wanted you to move the check to this function is to have a separate check and separate message, and not hijack the command line --tags mechanism.

As you can see for all of the checks in this file, it sets err to an appropriate explanation and then returns 0.

    if {$::debug_defrag && [lsearch $tags "debug_defrag:skip"] >= 0} {
        set err "Not supported on server compiled with DEBUG_FORCE_DEFRAG option"
        return 0
    }

    if {$::singledb && [lsearch $tags "singledb:skip"] >= 0} {
        set err "Not supported on singledb"
        return 0
    }

    if {$::cluster_mode && [lsearch $tags "cluster:skip"] >= 0} {
        set err "Not supported in cluster mode"
        return 0
    }

    if {$::tls && [lsearch $tags "tls:skip"] >= 0} {
        set err "Not supported in tls mode"
        return 0
    }

Each check checks if $tags contains a specific tag and also checks if some condition is met, then sets err and returns 0 (which means skip the this test).

What you did here is to set err to "IPv6 not available on this system, skipping IPv6 tests" but the function still returns 1 below so the first test is not skipped, and as a side-effect, ipv6 is added to ::denytags as if --tags -ipv6 was given on the command line. This means that the next test that has the ipv6 will be skipped as if because ipv6 is in ::denytags and the error message will be "Tag: ipv6 denied". This is what is printed when --tags -ipv6 is used on the command line. That's why I said "hijack" this machanism.

What the check needs to do instead is for check if "ipv6" is among the tags for this test and if it's not required and not available, then set the message and return 0. Something like...

Suggested change

	if {!$::require_ipv6 && ![is_ipv6_available]} {
	lappend ::denytags "ipv6"
	set err "IPv6 not available on this system, skipping IPv6 tests"
	if {!$::require_ipv6 && [lsearch $tags "ipv6"] >= 0 && ![is_ipv6_available]} {
	set err "IPv6 not available on this system"
	return 0

We could also add an optimization to avoid running the auto-detect for every test case with the ipv6 tag (but if that's not too many tests, then it might be OK to run it every time).

[diego-ciciani01]

Now i get what you mean. For optimization of the test, we can save the result of IPv6 check on a variable, to avoid performing the test unnecessarily. I agree to take this into consideration when there are a considerable number of IPv6 tests. At the moment, I only count 5.

[zuiderkwast]

I'm think if we need this flag or not. Comparing to other checks, I can see that we just skip tests that are not possible to run. For example we simply skip all ipv6 tests if we're running TCL 8.5. Similarly, some tests are skipped if TLS is disabled, if IO threads are used, and so on.

None of the other skipped tests have a require flag, so comparing to these other checks, I think we don't need this --require-ipv6 flag. We can just skip them if it's not available.

Sorry for changing my mind! 😄

[diego-ciciani01]

Don't worry, hahah.
If I understand correctly, you just want to remove the print "--require-ipv6 Run tests forcing the ipv6.”
from, "print_help_screen" ? The meccanismo with the require_ipv6 variable remain the same ?

[zuiderkwast]

If I understand correctly, you just want to remove the print "--require-ipv6 Run tests forcing the ipv6.” from, "print_help_screen" ? The meccanismo with the require_ipv6 variable remain the same ?

No, I think we can remove the require_ipv6 variable completely.

There is no variables like that for other things that are only sometimes supported.

[zuiderkwast]

Good catch!

[zuiderkwast]

This change looks unnecessary. Did you review your own diff after fixing the merge conflict?

[diego-ciciani01]

hi @zuiderkwast

While testing, I noticed an important detail about the catch usage in is_ipv6_available().

Initially, we had:

proc is_ipv6_available {} {
    if {[catch {
        set server [socket -server dummy -myaddr ::1 0]
        set port [lindex [chan configure $server -sockname] 2]
        set client [socket ::1 $port]
        close $server
        close $client
        return 1
    }]} {
        return 0
    }
}

However, the return 1 inside the catch block doesn't actually return from the function - it only exits the catch block. This means if the socket operations succeed, the function would fall through without returning a value.

The correct pattern, i think is :

proc is_ipv6_available {} {
    if {[catch {
        set server [socket -server dummy -myaddr ::1 0]
        set port [lindex [chan configure $server -sockname] 2]
        set client [socket ::1 $port]
        close $server
        close $client
    }] == 0} {
        return 1
    }
    return 0
}

According to the Tcl catch documentation, catch returns 0 on success and non-zero on error, but return statements inside the catch block don't propagate out of the procedure.

So let me know what do you think

[zuiderkwast]

However, the return 1 inside the catch block doesn't actually return from the function - it only exits the catch block.

Oh, I didn't know that! TCL is weird. Thank you! 😄

[zuiderkwast]

This PR has now a lot of unrelated commits and changed 33 files. What did you do?

The recommended way is to merge unstable into the branch and do a normal push, not a rebase and force-push. It doesn't matter if there are merge-commits, because when we merge, we will squash all the commits into one anyway.