Skip to content

hack/test/unit: run libnetwork tests sequentially #42594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
samuelkarp merged 1 commit into from
Aug 3, 2021
Merged

hack/test/unit: run libnetwork tests sequentially #42594

samuelkarp merged 1 commit into from
Aug 3, 2021

Conversation

@rvolosatovs
Copy link
Contributor

@rvolosatovs rvolosatovs commented Jul 2, 2021 •
edited
Loading

- What I did
Run libnetwork tests sequentially. See #42458 (comment) for explanation.
Closes #42458 maybe more

Note, that many tests in libnetwork/... already depend on the assumption that they are run sequentially and do not "easily" support things like -run due to that fact. E.g. this test is a prime example of such behavior:

moby/libnetwork/iptables/iptables_test.go

Lines 1 to 327 in 45b45ad

// +build linux
package iptables
import (
"net"
"os/exec"
"strconv"
"strings"
"testing"
_ "github.com/docker/docker/libnetwork/testutils"
"golang.org/x/sync/errgroup"
)
const chainName = "DOCKEREST"
var natChain *ChainInfo
var filterChain *ChainInfo
var bridgeName string
func TestNewChain(t *testing.T) {
var err error
bridgeName = "lo"
iptable := GetIptable(IPv4)
natChain, err = iptable.NewChain(chainName, Nat, false)
if err != nil {
t.Fatal(err)
}
err = iptable.ProgramChain(natChain, bridgeName, false, true)
if err != nil {
t.Fatal(err)
}
filterChain, err = iptable.NewChain(chainName, Filter, false)
if err != nil {
t.Fatal(err)
}
err = iptable.ProgramChain(filterChain, bridgeName, false, true)
if err != nil {
t.Fatal(err)
}
}
func TestForward(t *testing.T) {
ip := net.ParseIP("192.168.1.1")
port := 1234
dstAddr := "172.17.0.1"
dstPort := 4321
proto := "tcp"
bridgeName := "lo"
iptable := GetIptable(IPv4)
err := natChain.Forward(Insert, ip, port, proto, dstAddr, dstPort, bridgeName)
if err != nil {
t.Fatal(err)
}
dnatRule := []string{
"-d", ip.String(),
"-p", proto,
"--dport", strconv.Itoa(port),
"-j", "DNAT",
"--to-destination", dstAddr + ":" + strconv.Itoa(dstPort),
"!", "-i", bridgeName,
}
if !iptable.Exists(natChain.Table, natChain.Name, dnatRule...) {
t.Fatal("DNAT rule does not exist")
}
filterRule := []string{
"!", "-i", bridgeName,
"-o", bridgeName,
"-d", dstAddr,
"-p", proto,
"--dport", strconv.Itoa(dstPort),
"-j", "ACCEPT",
}
if !iptable.Exists(filterChain.Table, filterChain.Name, filterRule...) {
t.Fatal("filter rule does not exist")
}
masqRule := []string{
"-d", dstAddr,
"-s", dstAddr,
"-p", proto,
"--dport", strconv.Itoa(dstPort),
"-j", "MASQUERADE",
}
if !iptable.Exists(natChain.Table, "POSTROUTING", masqRule...) {
t.Fatal("MASQUERADE rule does not exist")
}
}
func TestLink(t *testing.T) {
var err error
bridgeName := "lo"
iptable := GetIptable(IPv4)
ip1 := net.ParseIP("192.168.1.1")
ip2 := net.ParseIP("192.168.1.2")
port := 1234
proto := "tcp"
err = filterChain.Link(Append, ip1, ip2, port, proto, bridgeName)
if err != nil {
t.Fatal(err)
}
rule1 := []string{
"-i", bridgeName,
"-o", bridgeName,
"-p", proto,
"-s", ip1.String(),
"-d", ip2.String(),
"--dport", strconv.Itoa(port),
"-j", "ACCEPT"}
if !iptable.Exists(filterChain.Table, filterChain.Name, rule1...) {
t.Fatal("rule1 does not exist")
}
rule2 := []string{
"-i", bridgeName,
"-o", bridgeName,
"-p", proto,
"-s", ip2.String(),
"-d", ip1.String(),
"--sport", strconv.Itoa(port),
"-j", "ACCEPT"}
if !iptable.Exists(filterChain.Table, filterChain.Name, rule2...) {
t.Fatal("rule2 does not exist")
}
}
func TestPrerouting(t *testing.T) {
args := []string{
"-i", "lo",
"-d", "192.168.1.1"}
iptable := GetIptable(IPv4)
err := natChain.Prerouting(Insert, args...)
if err != nil {
t.Fatal(err)
}
if !iptable.Exists(natChain.Table, "PREROUTING", args...) {
t.Fatal("rule does not exist")
}
delRule := append([]string{"-D", "PREROUTING", "-t", string(Nat)}, args...)
if _, err = iptable.Raw(delRule...); err != nil {
t.Fatal(err)
}
}
func TestOutput(t *testing.T) {
args := []string{
"-o", "lo",
"-d", "192.168.1.1"}
iptable := GetIptable(IPv4)
err := natChain.Output(Insert, args...)
if err != nil {
t.Fatal(err)
}
if !iptable.Exists(natChain.Table, "OUTPUT", args...) {
t.Fatal("rule does not exist")
}
delRule := append([]string{"-D", "OUTPUT", "-t",
string(natChain.Table)}, args...)
if _, err = iptable.Raw(delRule...); err != nil {
t.Fatal(err)
}
}
func TestConcurrencyWithWait(t *testing.T) {
RunConcurrencyTest(t, true)
}
func TestConcurrencyNoWait(t *testing.T) {
RunConcurrencyTest(t, false)
}
// Runs 10 concurrent rule additions. This will fail if iptables
// is actually invoked simultaneously without --wait.
// Note that if iptables does not support the xtable lock on this
// system, then allowXlock has no effect -- it will always be off.
func RunConcurrencyTest(t *testing.T, allowXlock bool) {
if !allowXlock && supportsXlock {
supportsXlock = false
defer func() { supportsXlock = true }()
}
ip := net.ParseIP("192.168.1.1")
port := 1234
dstAddr := "172.17.0.1"
dstPort := 4321
proto := "tcp"
group := new(errgroup.Group)
for i := 0; i < 10; i++ {
group.Go(func() error {
return natChain.Forward(Append, ip, port, proto, dstAddr, dstPort, "lo")
})
}
if err := group.Wait(); err != nil {
t.Fatal(err)
}
}
func TestCleanup(t *testing.T) {
var err error
var rules []byte
// Cleanup filter/FORWARD first otherwise output of iptables-save is dirty
link := []string{"-t", string(filterChain.Table),
string(Delete), "FORWARD",
"-o", bridgeName,
"-j", filterChain.Name}
iptable := GetIptable(IPv4)
if _, err = iptable.Raw(link...); err != nil {
t.Fatal(err)
}
filterChain.Remove()
err = iptable.RemoveExistingChain(chainName, Nat)
if err != nil {
t.Fatal(err)
}
rules, err = exec.Command("iptables-save").Output()
if err != nil {
t.Fatal(err)
}
if strings.Contains(string(rules), chainName) {
t.Fatalf("Removing chain failed. %s found in iptables-save", chainName)
}
}
func TestExistsRaw(t *testing.T) {
testChain1 := "ABCD"
testChain2 := "EFGH"
iptable := GetIptable(IPv4)
_, err := iptable.NewChain(testChain1, Filter, false)
if err != nil {
t.Fatal(err)
}
defer func() {
iptable.RemoveExistingChain(testChain1, Filter)
}()
_, err = iptable.NewChain(testChain2, Filter, false)
if err != nil {
t.Fatal(err)
}
defer func() {
iptable.RemoveExistingChain(testChain2, Filter)
}()
// Test detection over full and truncated rule string
input := []struct{ rule []string }{
{[]string{"-s", "172.8.9.9/32", "-j", "ACCEPT"}},
{[]string{"-d", "172.8.9.0/24", "-j", "DROP"}},
{[]string{"-s", "172.0.3.0/24", "-d", "172.17.0.0/24", "-p", "tcp", "-m", "tcp", "--dport", "80", "-j", testChain2}},
{[]string{"-j", "RETURN"}},
}
for i, r := range input {
ruleAdd := append([]string{"-t", string(Filter), "-A", testChain1}, r.rule...)
err = iptable.RawCombinedOutput(ruleAdd...)
if err != nil {
t.Fatalf("i=%d, err: %v", i, err)
}
if !iptable.existsRaw(Filter, testChain1, r.rule...) {
t.Fatalf("Failed to detect rule. i=%d", i)
}
// Truncate the rule
trg := r.rule[len(r.rule)-1]
trg = trg[:len(trg)-2]
r.rule[len(r.rule)-1] = trg
if iptable.existsRaw(Filter, testChain1, r.rule...) {
t.Fatalf("Invalid detection. i=%d", i)
}
}
}
func TestGetVersion(t *testing.T) {
mj, mn, mc := parseVersionNumbers("iptables v1.4.19.1-alpha")
if mj != 1 || mn != 4 || mc != 19 {
t.Fatal("Failed to parse version numbers")
}
}
func TestSupportsCOption(t *testing.T) {
input := []struct {
mj int
mn int
mc int
ok bool
}{
{1, 4, 11, true},
{1, 4, 12, true},
{1, 5, 0, true},
{0, 4, 11, false},
{0, 5, 12, false},
{1, 3, 12, false},
{1, 4, 10, false},
}
for ind, inp := range input {
if inp.ok != supportsCOption(inp.mj, inp.mn, inp.mc) {
t.Fatalf("Incorrect check: %d", ind)
}
}
}

I, personally, don't think that -p=1 is the "correct" solution here. Although, to do this "right" I think we'd have to rewrite a large portion of libnetwork tests and I do not think that is worth it at this point.

- How I did it
Run gotestsum at most twice and pass -p=1 when running unit tests in /libnetwork namespace

- How to verify it
e.g. TESTDIRS='github.com/docker/docker/libnetwork/iptables github.com/docker/docker/libnetwork/drivers/bridge' make test-unit should pass (and sometimes fail without this change)

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@rvolosatovs rvolosatovs force-pushed the fix_concurrency_test branch from ed38b59 to d2aaae8 Compare July 2, 2021 19:30
@rvolosatovs rvolosatovs mentioned this pull request Jul 2, 2021
Closed
@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Jul 2, 2021

@thaJeztah @fredericdalleau (and anyone else) thoughts on this? Please also see #42458 (comment)

Most probably refs and maybe fixes #42553

@thaJeztah
Copy link
Member

thaJeztah commented Jul 2, 2021

Ah, heh. Nice find. Yes, unit tests that are not-so-unit 😞

@thaJeztah
Copy link
Member

thaJeztah commented Jul 2, 2021

/cc @tianon @cpuguy83

@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Jul 2, 2021

CI failure (so far): #42459

@tianon
Copy link
Member

tianon commented Jul 2, 2021

Ah, -p controls the number of packages that will be tested in parallel (so this doesn't disable parallelism completely, just the default of running tests for more than one package at a time, which normally defaults to the number of CPUs available).

$ go help testflag
...
	-parallel n
	    Allow parallel execution of test functions that call t.Parallel.
	    The value of this flag is the maximum number of tests to run
	    simultaneously; by default, it is set to the value of GOMAXPROCS.
	    Note that -parallel only applies within a single test binary.
	    The 'go test' command may run tests for different packages
	    in parallel as well, according to the setting of the -p flag
	    (see 'go help build').
...
$ go help build
...
	-p n
		the number of programs, such as build commands or
		test binaries, that can be run in parallel.
		The default is the number of CPUs available.
...

I don't love it (and wish we could set it just for libnetwork/* somehow 😞), but it seems like an acceptable workaround IMO.

@cpuguy83
Copy link
Member

cpuguy83 commented Jul 2, 2021

We can absolutely filter libnetwork and run it separately

@rvolosatovs rvolosatovs force-pushed the fix_concurrency_test branch from d2aaae8 to 3c3e055 Compare July 5, 2021 17:59
@rvolosatovs rvolosatovs changed the title hack/test/unit: use -p=1 by default hack/test/unit*: run libnetwork tests sequentially Jul 5, 2021
@rvolosatovs rvolosatovs force-pushed the fix_concurrency_test branch from 3c3e055 to 49bcb66 Compare July 5, 2021 18:10
rvolosatovs
rvolosatovs commented Jul 5, 2021
View reviewed changes
@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Jul 5, 2021 •
edited
Loading

I added a Go test runner, which runs libnetwork tests exclusively with -p=1 and handles merging resulting profiles.
What do you think of such approach?
Note, I will still go through the implementation, for now it is just a prototype, so not asking for a full review - I would like to get some feedback on the overall idea first.
I used Go for clarity and maintainability, but this could also be a shell script, if so desired.

rvolosatovs
rvolosatovs commented Jul 5, 2021
View reviewed changes
@rvolosatovs rvolosatovs force-pushed the fix_concurrency_test branch 5 times, most recently from d6165dd to 147df55 Compare July 8, 2021 09:53
@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Jul 9, 2021

Still did not get any feedback, moving this out of draft

@rvolosatovs rvolosatovs marked this pull request as ready for review July 9, 2021 13:54
@rvolosatovs rvolosatovs requested a review from tianon as a code owner July 9, 2021 13:54
@rvolosatovs rvolosatovs force-pushed the fix_concurrency_test branch from 147df55 to d9eeff5 Compare July 13, 2021 11:49
@thaJeztah thaJeztah mentioned this pull request Jul 13, 2021
Merged
@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Jul 16, 2021

@tianon @cpuguy83 PTAL

cpuguy83
cpuguy83 reviewed Jul 20, 2021
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the added complexity here is worth it.
If we just need to ensure libnetwork tests are exclusive, we can do that in bash.

@rvolosatovs rvolosatovs force-pushed the fix_concurrency_test branch from d9eeff5 to a7e0efe Compare July 23, 2021 11:56
@rvolosatovs rvolosatovs changed the title hack/test/unit*: run libnetwork tests sequentially hack/test/unit: run libnetwork tests sequentially Aug 1, 2021
@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Aug 1, 2021 •
edited
Loading

A few failures that seem unrelated:

[2021-08-01T15:55:33.884Z]  > [rootlesskit 1/3] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg/mod     --mount=type=bind,src=hack/dockerfile/install,target=/tmp/install         PREFIX=/build /tmp/install/install.sh rootlesskit:

[2021-08-01T15:55:33.884Z] ------

[2021-08-01T15:55:33.884Z] executor failed running [/bin/sh -c PREFIX=/build /tmp/install/install.sh rootlesskit]: exit code: 128

script returned exit code 1

[2021-08-01T15:59:54.289Z] #32 [swagger 2/2] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg/mod     --mount=type=tmpfs,target=/go/src/         set -x         && git clone https://github.com/kolyshkin/go-swagger.git .         && git checkout -q "c56166c036004ba7a3a321e5951ba472b9ae298c"         && go build -o /build/swagger github.com/go-swagger/go-swagger/cmd/swagger

[2021-08-01T15:59:54.289Z] #32 sha256:b91ffc0025b99e48e874df39aa649930bfdbb27d79b80cdc6e030f11987f2e15

[2021-08-01T16:06:00.846Z] #32 623.7 error: RPC failed; curl 56 GnuTLS recv error (-54): Error in the pull function.

[2021-08-01T16:06:00.846Z] #32 623.7 fatal: the remote end hung up unexpectedly

[2021-08-01T16:06:00.846Z] #32 623.7 fatal: early EOF

[2021-08-01T16:06:00.846Z] #32 623.7 fatal: index-pack failed

[2021-08-01T16:06:00.846Z] #32 ERROR: executor failed running [/bin/sh -c set -x         && git clone https://github.com/kolyshkin/go-swagger.git .         && git checkout -q "$GO_SWAGGER_COMMIT"         && go build -o /build/swagger github.com/go-swagger/go-swagger/cmd/swagger]: exit code: 128

[2021-08-01T16:06:00.846Z] ------

[2021-08-01T16:06:00.846Z]  > [swagger 2/2] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg/mod     --mount=type=tmpfs,target=/go/src/         set -x         && git clone https://github.com/kolyshkin/go-swagger.git .         && git checkout -q "c56166c036004ba7a3a321e5951ba472b9ae298c"         && go build -o /build/swagger github.com/go-swagger/go-swagger/cmd/swagger:

[2021-08-01T16:06:00.846Z] ------

[2021-08-01T16:06:00.846Z] executor failed running [/bin/sh -c set -x         && git clone https://github.com/kolyshkin/go-swagger.git .         && git checkout -q "$GO_SWAGGER_COMMIT"         && go build -o /build/swagger github.com/go-swagger/go-swagger/cmd/swagger]: exit code: 128

script returned exit code 1

[2021-08-01T16:23:00.228Z] === Failed

[2021-08-01T16:23:00.228Z] === FAIL: github.com/docker/docker/integration/build TestBuildWCOWSandboxSize (651.44s)

[2021-08-01T16:23:00.228Z]     build_test.go:572: assertion failed: value "{\"stream\":\"Step 1/8 : FROM busybox AS intermediate\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e 64a7ef3b89f1\\n\"}\r\n{\"stream\":\"Step 2/8 : WORKDIR C:\\\\\\\\stuff\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e Running in b7a0e518e389\\n\"}\r\n{\"stream\":\"Removing intermediate container b7a0e518e389\\n\"}\r\n{\"stream\":\" ---\\u003e 17c807ffb38c\\n\"}\r\n{\"stream\":\"Step 3/8 : RUN fsutil file createnew C:\\\\\\\\stuff\\\\\\\\bigfile_0.txt 22548578304 \\u0026\\u0026 del bigfile_0.txt\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e Running in 7745fac8e671\\n\"}\r\n{\"stream\":\"File C:\\\\stuff\\\\bigfile_0.txt is created\\r\\n\"}\r\n{\"stream\":\"Removing intermediate container 7745fac8e671\\n\"}\r\n{\"stream\":\" ---\\u003e 9d9556f09ffe\\n\"}\r\n{\"stream\":\"Step 4/8 : RUN fsutil file createnew C:\\\\\\\\stuff\\\\\\\\bigfile_1.txt 7516192768\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e Running in 697045102b5a\\n\"}\r\n{\"stream\":\"File C:\\\\stuff\\\\bigfile_1.txt is created\\r\\n\"}\r\n{\"stream\":\"Removing intermediate container 697045102b5a\\n\"}\r\n{\"stream\":\" ---\\u003e 9b7467f32168\\n\"}\r\n{\"stream\":\"Step 5/8 : RUN fsutil file createnew C:\\\\\\\\stuff\\\\\\\\bigfile_2.txt 7516192768\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e Running in ba733f5ff4f8\\n\"}\r\n{\"stream\":\"File C:\\\\stuff\\\\bigfile_2.txt is created\\r\\n\"}\r\n{\"stream\":\"Removing intermediate container ba733f5ff4f8\\n\"}\r\n{\"stream\":\" ---\\u003e bace7e0e6946\\n\"}\r\n{\"stream\":\"Step 6/8 : RUN fsutil file createnew C:\\\\\\\\stuff\\\\\\\\bigfile_3.txt 7516192768\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e Running in f1588afe20ae\\n\"}\r\n{\"stream\":\"File C:\\\\stuff\\\\bigfile_3.txt is created\\r\\n\"}\r\n{\"stream\":\"Removing intermediate container f1588afe20ae\\n\"}\r\n{\"stream\":\" ---\\u003e 7758de385ecc\\n\"}\r\n{\"aux\":{\"ID\":\"sha256:7758de385ecc0248d5c3f029c805a3eec51e319262330fd1cdab0bd0f5151bf8\"}}\r\n{\"stream\":\"Step 7/8 : FROM busybox\"}\r\n{\"stream\":\"\\n\"}\r\n{\"stream\":\" ---\\u003e 64a7ef3b89f1\\n\"}\r\n{\"stream\":\"Step 8/8 : COPY --from=intermediate C:\\\\\\\\stuff C:\\\\\\\\stuff\"}\r\n{\"stream\":\"\\n\"}\r\n{\"errorDetail\":{\"message\":\"re-exec error: exit status 1: output: hcsshim::ImportLayer - failed failed in Win32: The system cannot find the path specified. (0x3)\"},\"error\":\"re-exec error: exit status 1: output: hcsshim::ImportLayer - failed failed in Win32: The system cannot find the path specified. (0x3)\"}\r\n" does not match regexp "Successfully built|re-exec error: exit status 1: output: write.*daemon\\\\\\\\tmp\\\\\\\\hcs.*bigfile_[1-3].txt: There is not enough space on the disk."

thaJeztah
thaJeztah reviewed Aug 2, 2021
View reviewed changes
@thaJeztah thaJeztah added the area/networking Networking label Aug 2, 2021
@thaJeztah
Copy link
Member

thaJeztah commented Aug 2, 2021

Oh; forgot to leave a comment about that; I think we also will have to fix the Windows counterparts of this. i.e.;

moby/hack/make.ps1

Lines 315 to 325 in 0b39cc2

$goListCommand = "go list -e -f '{{if ne .Name """ + '\"github.com/docker/docker\"' + """}}{{.ImportPath}}{{end}}' $testPath"
$pkgList = $(Invoke-Expression $goListCommand)
if ($LASTEXITCODE -ne 0) { Throw "go list for unit tests failed" }
$pkgList = $pkgList | Select-String -Pattern "github.com/docker/docker"
$pkgList = $pkgList | Select-String -NotMatch "github.com/docker/docker/vendor"
$pkgList = $pkgList | Select-String -NotMatch "github.com/docker/docker/man"
$pkgList = $pkgList | Select-String -NotMatch "github.com/docker/docker/integration"
$pkgList = $pkgList -replace "`r`n", " "
$goTestArg = "--format=standard-verbose --jsonfile=bundles\go-test-report-unit-tests.json --junitfile=bundles\junit-report-unit-tests.xml -- " + $raceParm + " -cover -ldflags -w -a """ + "-test.timeout=10m" + """ $pkgList"
Write-Host "INFO: Invoking unit tests run with $GOTESTSUM_LOCATION\gotestsum.exe $goTestArg"

and

moby/hack/ci/windows.ps1

Lines 772 to 776 in 0b39cc2

if (Test-Path "bundles\junit-report-unit-tests.xml") {
Write-Host -ForegroundColor Magenta "INFO: Unit tests results(bundles\junit-report-unit-tests.xml) exist. pwd=$pwd"
} else {
Write-Host -ForegroundColor Magenta "ERROR: Unit tests results(bundles\junit-report-unit-tests.xml) do not exist. pwd=$pwd"
}

@thaJeztah
Copy link
Member

thaJeztah commented Aug 2, 2021

🤔 although; if it's only the iptables issue that's a problem, I guess that's not needed for Windows 😂

@rvolosatovs rvolosatovs force-pushed the fix_concurrency_test branch from 494c9d7 to 58e9b93 Compare August 2, 2021 08:06
@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Aug 2, 2021 •
edited
Loading

🤔 although; if it's only the iptables issue that's a problem, I guess that's not needed for Windows 😂

I would think the same in this case. Are the tests in question known to be flaky on Windows in particular?
At least on this PR builds I did not see related Windows failures.
Perhaps we can see if this works and revisit if necessary at a later time?

@rvolosatovs rvolosatovs requested a review from thaJeztah August 2, 2021 08:10
@rvolosatovs rvolosatovs force-pushed the fix_concurrency_test branch from 58e9b93 to 6e8466b Compare August 2, 2021 08:29
@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Aug 2, 2021

-p=1 on the whole /libnetwork package tree actually disables some of the tests:

[2021-08-02T08:58:37.153Z] === Skipped

[2021-08-02T08:58:37.153Z] === SKIP: libnetwork TestParallel3 (0.00s)

[2021-08-02T08:58:37.153Z]     libnetwork_linux_test.go:966: Skipped because t.parallel was less than  3

[2021-08-02T08:58:37.153Z] 

[2021-08-02T08:58:37.153Z] === SKIP: libnetwork TestParallel1 (0.00s)

[2021-08-02T08:58:37.153Z]     libnetwork_linux_test.go:966: Skipped because t.parallel was less than  3

[2021-08-02T08:58:37.153Z] 

[2021-08-02T08:58:37.153Z] === SKIP: libnetwork TestParallel2 (0.00s)

[2021-08-02T08:58:37.153Z]     libnetwork_linux_test.go:966: Skipped because t.parallel was less than  3

[2021-08-02T08:58:37.153Z] 

[2021-08-02T08:58:37.154Z] === SKIP: libnetwork/ipam TestParallelPredefinedRequest4 (0.00s)

[2021-08-02T08:58:37.154Z]     allocator_test.go:1423: Skipped because t.parallel was less than  5

[2021-08-02T08:58:37.154Z] 

[2021-08-02T08:58:37.154Z] === SKIP: libnetwork/ipam TestParallelPredefinedRequest5 (0.00s)

[2021-08-02T08:58:37.154Z]     allocator_test.go:1423: Skipped because t.parallel was less than  5

[2021-08-02T08:58:37.154Z] 

[2021-08-02T08:58:37.154Z] === SKIP: libnetwork/ipam TestParallelPredefinedRequest3 (0.00s)

[2021-08-02T08:58:37.154Z]     allocator_test.go:1423: Skipped because t.parallel was less than  5

[2021-08-02T08:58:37.154Z] 

[2021-08-02T08:58:37.154Z] === SKIP: libnetwork/ipam TestParallelPredefinedRequest2 (0.00s)

[2021-08-02T08:58:37.154Z]     allocator_test.go:1423: Skipped because t.parallel was less than  5

[2021-08-02T08:58:37.154Z] 

[2021-08-02T08:58:37.154Z] === SKIP: libnetwork/ipam TestParallelPredefinedRequest1 (0.00s)

[2021-08-02T08:58:37.154Z]     allocator_test.go:1423: Skipped because t.parallel was less than  5

Looks like we actually need to apply this flag to a smaller set of packages, I guess only the ones depending on iptables. libnetwork_linux_test.go then probably will have to be moved, I will investigate...

@rvolosatovs rvolosatovs marked this pull request as draft August 2, 2021 11:22
@thaJeztah
Copy link
Member

thaJeztah commented Aug 2, 2021

-p=1 on the whole /libnetwork package tree actually disables some of the tests:

Wondering; Can we re-enable parallel as part of those tests itself?

@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Aug 2, 2021

-p=1 on the whole /libnetwork package tree actually disables some of the tests:

Wondering; Can we re-enable parallel as part of those tests itself?

I don't think we can do that. We could try to do some trickery in TestMain, but I doubt that would be maintainable.
If we want to handle this within the tests themselves, I think our best bet would be to use an on-disk lock, which would be used by the flaky tests using iptables.

@thaJeztah
Copy link
Member

thaJeztah commented Aug 2, 2021

Yeah, so I was wondering -p=1 disables multiple packages to be run in parallel, but would it still allow t.Parallel() to be used to enable parallel runs within a test (subtests to be run in parallel)

@rvolosatovs rvolosatovs marked this pull request as ready for review August 2, 2021 12:01
@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Aug 2, 2021 •
edited
Loading

Oh, nevermind, that is not caused by this change - it turns out that those tests are never actually run on CI, e.g. on latest master arm64 build (https://ci-next.docker.com/public/blue/organizations/jenkins/moby/detail/master/1006/pipeline/363):

[2021-08-02T08:11:56.627Z] === SKIP: libnetwork/ipam TestParallelPredefinedRequest1 (0.00s)

[2021-08-02T08:11:56.627Z]     allocator_test.go:1423: Skipped because t.parallel was less than  5

[2021-08-02T08:11:56.627Z] 

[2021-08-02T08:11:56.627Z] === SKIP: libnetwork/ipam TestParallelPredefinedRequest5 (0.00s)

[2021-08-02T08:11:56.627Z]     allocator_test.go:1423: Skipped because t.parallel was less than  5

[2021-08-02T08:11:56.627Z] 

[2021-08-02T08:11:56.627Z] === SKIP: libnetwork/ipam TestParallelPredefinedRequest3 (0.00s)

[2021-08-02T08:11:56.627Z]     allocator_test.go:1423: Skipped because t.parallel was less than  5

[2021-08-02T08:11:56.627Z] 

[2021-08-02T08:11:56.627Z] === SKIP: libnetwork/ipam TestParallelPredefinedRequest2 (0.00s)

[2021-08-02T08:11:56.627Z]     allocator_test.go:1423: Skipped because t.parallel was less than  5

[2021-08-02T08:11:56.627Z] 

[2021-08-02T08:11:56.627Z] === SKIP: libnetwork/ipam TestParallelPredefinedRequest4 (0.00s)

[2021-08-02T08:11:56.627Z]     allocator_test.go:1423: Skipped because t.parallel was less than  5

-p does not influence test.parallel value:

cat test_test.go && go test && go test -p=1 && go test -parallel 1
───────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: test_test.go
───────┼───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ package test_test
   2   │ 
   3   │ import (
   4   │     "flag"
   5   │     "fmt"
   6   │     "testing"
   7   │ )
   8   │ 
   9   │ func TestTest(t *testing.T) {
  10   │     fmt.Println(flag.Lookup("test.parallel").Value)
  11   │ }
───────┴───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
     | 8
PASS
PASS | github.com/rvolosatovs/test 
     | 8
PASS
PASS | github.com/rvolosatovs/test 
     | 1
PASS
PASS | github.com/rvolosatovs/test

@rvolosatovs
Copy link
Contributor Author

rvolosatovs commented Aug 2, 2021

cc @samuelkarp

thaJeztah
thaJeztah approved these changes Aug 2, 2021
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

perhaps we should create a tracking issue for the tests that are currently skipped because we don't have -parallel enabled

samuelkarp
samuelkarp reviewed Aug 2, 2021
View reviewed changes
@rvolosatovs rvolosatovs requested a review from samuelkarp August 3, 2021 07:51
hack/test/unit: run libnetwork tests sequentially
3af2217 
Run all tests within `libnetwork` namespace with `-p=1`
in a separate `gotestsum` invocation.

Signed-off-by: Roman Volosatovs <roman.volosatovs@docker.com>
@rvolosatovs rvolosatovs force-pushed the fix_concurrency_test branch from 6e8466b to 3af2217 Compare August 3, 2021 10:20
thaJeztah
thaJeztah approved these changes Aug 3, 2021
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

samuelkarp
samuelkarp approved these changes Aug 3, 2021
View reviewed changes
Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@samuelkarp samuelkarp merged commit 52af466 into moby:master Aug 3, 2021
@rvolosatovs rvolosatovs deleted the fix_concurrency_test branch August 4, 2021 11:18
@thaJeztah thaJeztah added this to the 21.xx milestone Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samuelkarp samuelkarp samuelkarp approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@tianon tianon Awaiting requested review from tianon tianon is a code owner

@cpuguy83 cpuguy83 Awaiting requested review from cpuguy83

Assignees

No one assigned

Labels

area/networking Networking area/testing kind/bugfix PR's that fix bugs status/4-merge

Projects

None yet

Milestone

23.0.0

Development

Successfully merging this pull request may close these issues.

Flaky test: libnetwork/iptables TestConcurrencyNoWait

5 participants

@rvolosatovs @thaJeztah @tianon @cpuguy83 @samuelkarp