CI: parallize unit tests #5954

flouthoc · 2025-01-29T14:57:54Z

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Luap99

Looks like you are reverting the integration test changes.

tests/source.bats

tests/test_runner.sh

tests/bud.bats

tests/blobcache.bats

flouthoc · 2025-01-29T15:22:58Z

Looks like you are reverting the integration test changes.

Yes did it by mistake fixed it.

flouthoc · 2025-01-29T19:07:19Z

Following tests are not parallel and are racing when using t.Parallel , also they are high contributors to the time of unit test, looks like I will have to do changes in tests so we can make them parallel

TestPutNoChroot: 202.85 seconds
TestStatNoChroot: 166.13 seconds
TestCWConvertImage: 51.84 seconds
TestPutNoChroot/archive=regular,topdir=: 34.64 seconds
TestPutNoChroot/archive=regular,topdir=.: 29.35 seconds
TestPutNoChroot/archive=regular,rename=no-match-dir: 27.94 seconds
TestPutNoChroot/archive=regular,topdir=top: 27.89 seconds
TestPutNoChroot/archive=regular,rename=no-match-file: 27.77 seconds

Luap99 · 2025-01-30T12:46:49Z

They do not seem to benefit much from more cores/memory so you likely need to look at other options or get them to run parallel much better. I guess the early spikes are when we we compile the go code in the ci setup. Maybe also worth looking into. It is not clear to me if we would actually need to compile anything else for the unit test.

The other thing could be is to split the unit-test target in three separate ones and then run them in parallel.
Another thing I see the tests use -cover and -race, I am not sure how value these add for our testing.
race is known to be slow, https://go.dev/doc/articles/race_detector#Runtime_Overheads

The cost of race detection varies by program, but for a typical program, memory usage may increase by 5-10x and execution time by 2-20x.
So maybe drop these flags and see how much faster they run.

flouthoc · 2025-01-30T16:35:37Z

They do not seem to benefit much from more cores/memory so you likely need to look at other options or get them to run parallel much better. I guess the early spikes are when we we compile the go code in the ci setup. Maybe also worth looking into. It is not clear to me if we would actually need to compile anything else for the unit test.

The other thing could be is to split the unit-test target in three separate ones and then run them in parallel. Another thing I see the tests use -cover and -race, I am not sure how value these add for our testing. race is known to be slow, https://go.dev/doc/articles/race_detector#Runtime_Overheads

The cost of race detection varies by program, but for a typical program, memory usage may increase by 5-10x and execution time by 2-20x.
So maybe drop these flags and see how much faster they run.

Wow @Luap99 !!! unit test without -cover and -race completed in 8 minutes, let me try without -race only and including -cover.

flouthoc · 2025-01-30T17:46:30Z

Looks like removing -race does the trick, brings down the all the unit tests from 48m something to 8m

buildah_test.go

Luap99 · 2025-01-30T17:51:36Z

@nalind Any objections to removing -race from the unit tests?

nalind · 2025-01-30T18:38:23Z

@nalind Any objections to removing -race from the unit tests?

Other than that I'll probably forget to run them that way from time to time if it isn't automated, which isn't that important, no.

flouthoc · 2025-01-30T21:22:46Z

@nalind @Luap99 @containers/buildah-maintainers PTAL, ready for a review.

flouthoc · 2025-01-30T21:23:14Z

Also it looks like osh-diff-scan:fedora-rawhide-x86_64:buildah-fedora is the bottleneck now, it is the longest running job.

nalind · 2025-01-30T21:33:44Z

@nalind Any objections to removing -race from the unit tests?

Other than that I'll probably forget to run them that way from time to time if it isn't automated, which isn't that important, no.

Come to think of it, can we conditionally keep that for the scheduled "cron" builds? I don't think anyone actually waits for them to finish.

flouthoc · 2025-01-30T21:42:44Z

@nalind Any objections to removing -race from the unit tests?

Other than that I'll probably forget to run them that way from time to time if it isn't automated, which isn't that important, no.

Come to think of it, can we conditionally keep that for the scheduled "cron" builds? I don't think anyone actually waits for them to finish.

Sounds good to me, is there any way to test that when I open a PR or it will only run on merge ? I will open a separate PR to add env variable for -race in .github workflow.

Luap99 · 2025-01-31T09:45:36Z

Also it looks like osh-diff-scan:fedora-rawhide-x86_64:buildah-fedora is the bottleneck now, it is the longest running job.

None of the packit tests enforce anything, just ignore them for now.

Come to think of it, can we conditionally keep that for the scheduled "cron" builds? I don't think anyone actually waits for them to finish.

That is easy to do sure except nobody looks at these results so that is pretty pointless.

How imporant is -race really, did you ever catch actual issues with it?

Sounds good to me, is there any way to test that when I open a PR or it will only run on merge ? I will open a separate PR to add env variable for -race in .github workflow.

Well anything post merge cannot be tested on a PR and this is not a github workflow. This must be done in cirrus.yml and would be easy, just check if the CIRRUS_PR env is empty then we run on a branch.

Luap99

LGTM

Luap99 · 2025-01-31T09:52:11Z

Total CI time reported by cirrus 38:01

type	user	fs	d13	f40	f41
Unit	root	vfs		08:17
Unit	root	overlay		08:02
Conformance	root	vfs	23:49
Conformance	root	overlay	14:33
Integration	root	vfs	23:53	29:30	30:12
Integration	root	overlay	19:50		23:37
Integration	rootless	overlay	26:06		26:34

That seems quite good, vfs is noticeably slower though which I guess is expected to the higher amount of io required for it.

flouthoc · 2025-01-31T16:35:23Z

@nalind @TomSweeneyRedHat @rhatdan PTAL, if consensus is made for cron then I'll open a new PR for that.

nalind · 2025-02-03T20:45:19Z

buildah_test.go

+	// which eventually and indirectly accesses a global variable
+	// defined in `go-selinux`, this must be fixed at `go-selinux`
+	// or builder must enable sometime of locking mechanism i.e if
+	// routine is creating Builder other's must wait for it.


How will we know when it's safe? Is there an open issue?

I just created an issue #5967 and added in comment and commit message.

Signed-off-by: flouthoc <[email protected]>

nalind · 2025-02-03T21:12:42Z

@nalind @TomSweeneyRedHat @rhatdan PTAL, if consensus is made for cron then I'll open a new PR for that.

Why wait?

Luap99

I still don't think there is a point in having race enabled post merge.
AFAICT nobody ever looks at any of these failures anyway. ci-monitor emails for the cron jobs are ignored and for post merge tests on nobody ever looked at them.

But maybe I am just wrong here? Who is looking at that?

Makefile

contrib/cirrus/test.sh

nalind · 2025-02-04T14:43:55Z

But maybe I am just wrong here? Who is looking at that?

Anecdata: I do read the ones for buildah.

Luap99 · 2025-02-04T14:48:56Z

But maybe I am just wrong here? Who is looking at that?

Anecdata: I do read the ones for buildah.

Ok if you actual can make use of the data then sure let's do that. I just have never seen anyone respond to these emails or post PRs to fix the issues from there. Maybe that just wasn't necessary on buildah so far and/or I don't look at buildah PRs often enough.

flouthoc · 2025-02-04T15:59:21Z

@nalind @Luap99 PTAL again.

rhatdan · 2025-02-04T19:04:45Z

Makefile

@@ -25,8 +25,7 @@ GO_GCFLAGS := $(shell if $(GO) version|grep -q gccgo; then echo "-gccgoflags"; e
 NPROCS := $(shell nproc)
 export GO_BUILD=$(GO) build
 export GO_TEST=$(GO) test -parallel=$(NPROCS)
-#RACEFLAGS := $(shell $(GO_TEST) -race ./pkg/dummy > /dev/null 2>&1 && echo -race)
-RACEFLAGS :=
+RACEFLAGS ?= $(shell $(GO_TEST) -race ./pkg/dummy > /dev/null 2>&1 && echo -race)


Can you merge this one with previous

rhatdan · 2025-02-04T19:05:20Z

LGTM

Add `t.Parallel()` to unit tests whereever its possible without race. Signed-off-by: flouthoc <[email protected]>

Signed-off-by: flouthoc <[email protected]>

See issue: containers#5967 Signed-off-by: flouthoc <[email protected]>

rhatdan · 2025-02-04T19:40:32Z

@nalind @Luap99 PTAL

Luap99 · 2025-02-05T13:08:28Z

contrib/cirrus/test.sh

@@ -67,7 +67,13 @@ else
            showrun make validate
            ;;
        unit)
-            showrun make test-unit
+	    race=


nit, line indentation still seems wrong, looks like you use one tab and spaces on that line.

Luap99

rest LGTM

openshift-ci · 2025-02-05T13:10:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: flouthoc, Luap99
Once this PR has been reviewed and has the lgtm label, please assign rhatdan for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: flouthoc <[email protected]>

packit-as-a-service · 2025-02-05T14:59:51Z

Ephemeral COPR build failed. @containers/packit-build please check.

flouthoc force-pushed the parallel-unit-test branch from 47f4ea4 to 7dd8114 Compare January 29, 2025 15:07

Luap99 requested changes Jan 29, 2025

View reviewed changes

tests/source.bats Outdated Show resolved Hide resolved

tests/test_runner.sh Outdated Show resolved Hide resolved

tests/bud.bats Outdated Show resolved Hide resolved

tests/blobcache.bats Outdated Show resolved Hide resolved

flouthoc force-pushed the parallel-unit-test branch from 7dd8114 to 36f2888 Compare January 29, 2025 15:21

flouthoc force-pushed the parallel-unit-test branch 2 times, most recently from 5693588 to 8d1e570 Compare January 29, 2025 18:06

flouthoc force-pushed the parallel-unit-test branch 2 times, most recently from 4d4cb56 to 845f109 Compare January 30, 2025 16:12

flouthoc force-pushed the parallel-unit-test branch from 845f109 to 6aea70d Compare January 30, 2025 17:10

Luap99 reviewed Jan 30, 2025

View reviewed changes

buildah_test.go Outdated Show resolved Hide resolved

flouthoc force-pushed the parallel-unit-test branch from 6aea70d to 6ba924d Compare January 30, 2025 20:43

flouthoc requested a review from Luap99 January 30, 2025 21:30

Luap99 approved these changes Jan 31, 2025

View reviewed changes

flouthoc mentioned this pull request Feb 3, 2025

time integration tests #5959

Draft

flouthoc requested a review from nalind February 3, 2025 16:27

nalind reviewed Feb 3, 2025

View reviewed changes

flouthoc mentioned this pull request Feb 3, 2025

Parallelize unit tests which cannot be parallelized because of NewBuilder() #5967

Open

flouthoc force-pushed the parallel-unit-test branch from 6ba924d to 25dce01 Compare February 3, 2025 20:55

flouthoc requested a review from nalind February 3, 2025 20:56

flouthoc added a commit to flouthoc/buildah that referenced this pull request Feb 3, 2025

pull changes from PR containers#5954

4f3c219

Signed-off-by: flouthoc <[email protected]>

flouthoc force-pushed the parallel-unit-test branch from b582c5e to c56c842 Compare February 4, 2025 05:37

Luap99 reviewed Feb 4, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

contrib/cirrus/test.sh Outdated Show resolved Hide resolved

flouthoc force-pushed the parallel-unit-test branch 4 times, most recently from 6000bd1 to ae100a5 Compare February 4, 2025 15:42

flouthoc requested a review from Luap99 February 4, 2025 15:59

flouthoc force-pushed the parallel-unit-test branch from ae100a5 to 752df31 Compare February 4, 2025 16:38

rhatdan reviewed Feb 4, 2025

View reviewed changes

flouthoc added 4 commits February 4, 2025 11:07

unit_test: use Parallel test where possible

aadfc5c

Add `t.Parallel()` to unit tests whereever its possible without race. Signed-off-by: flouthoc <[email protected]>

Makefile: use -parallel for go test

1879f3d

Signed-off-by: flouthoc <[email protected]>

.cirrus: use multiple cpu for unit tests

81479b2

Signed-off-by: flouthoc <[email protected]>

unit: deparallize some tests

de3c3ba

See issue: containers#5967 Signed-off-by: flouthoc <[email protected]>

flouthoc force-pushed the parallel-unit-test branch from 752df31 to 7102793 Compare February 4, 2025 19:08

flouthoc requested a review from rhatdan February 4, 2025 19:12

Luap99 reviewed Feb 5, 2025

View reviewed changes

Luap99 approved these changes Feb 5, 2025

View reviewed changes

.cirrus: run -race only on non-PR branch

147a3ca

Signed-off-by: flouthoc <[email protected]>

flouthoc force-pushed the parallel-unit-test branch from 7102793 to 147a3ca Compare February 5, 2025 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI: parallize unit tests #5954

CI: parallize unit tests #5954

flouthoc commented Jan 29, 2025

Luap99 left a comment

flouthoc commented Jan 29, 2025

flouthoc commented Jan 29, 2025 •

edited

Loading

Luap99 commented Jan 30, 2025

flouthoc commented Jan 30, 2025

flouthoc commented Jan 30, 2025 •

edited

Loading

Luap99 commented Jan 30, 2025

nalind commented Jan 30, 2025

flouthoc commented Jan 30, 2025 •

edited

Loading

flouthoc commented Jan 30, 2025

nalind commented Jan 30, 2025

flouthoc commented Jan 30, 2025 •

edited

Loading

Luap99 commented Jan 31, 2025

Luap99 left a comment

Luap99 commented Jan 31, 2025

flouthoc commented Jan 31, 2025

nalind Feb 3, 2025

flouthoc Feb 3, 2025

nalind commented Feb 3, 2025

Luap99 left a comment

nalind commented Feb 4, 2025

Luap99 commented Feb 4, 2025

flouthoc commented Feb 4, 2025

rhatdan Feb 4, 2025

flouthoc Feb 4, 2025

rhatdan commented Feb 4, 2025

rhatdan commented Feb 4, 2025

Luap99 Feb 5, 2025

flouthoc Feb 5, 2025

Luap99 left a comment

openshift-ci bot commented Feb 5, 2025

packit-as-a-service bot commented Feb 5, 2025

CI: parallize unit tests #5954

Are you sure you want to change the base?

CI: parallize unit tests #5954

Conversation

flouthoc commented Jan 29, 2025

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Luap99 left a comment

Choose a reason for hiding this comment

flouthoc commented Jan 29, 2025

flouthoc commented Jan 29, 2025 • edited Loading

Luap99 commented Jan 30, 2025

flouthoc commented Jan 30, 2025

flouthoc commented Jan 30, 2025 • edited Loading

Luap99 commented Jan 30, 2025

nalind commented Jan 30, 2025

flouthoc commented Jan 30, 2025 • edited Loading

flouthoc commented Jan 30, 2025

nalind commented Jan 30, 2025

flouthoc commented Jan 30, 2025 • edited Loading

Luap99 commented Jan 31, 2025

Luap99 left a comment

Choose a reason for hiding this comment

Luap99 commented Jan 31, 2025

flouthoc commented Jan 31, 2025

nalind Feb 3, 2025

Choose a reason for hiding this comment

flouthoc Feb 3, 2025

Choose a reason for hiding this comment

nalind commented Feb 3, 2025

Luap99 left a comment

Choose a reason for hiding this comment

nalind commented Feb 4, 2025

Luap99 commented Feb 4, 2025

flouthoc commented Feb 4, 2025

rhatdan Feb 4, 2025

Choose a reason for hiding this comment

flouthoc Feb 4, 2025

Choose a reason for hiding this comment

rhatdan commented Feb 4, 2025

rhatdan commented Feb 4, 2025

Luap99 Feb 5, 2025

Choose a reason for hiding this comment

flouthoc Feb 5, 2025

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 5, 2025

packit-as-a-service bot commented Feb 5, 2025

flouthoc commented Jan 29, 2025 •

edited

Loading

flouthoc commented Jan 30, 2025 •

edited

Loading

flouthoc commented Jan 30, 2025 •

edited

Loading

flouthoc commented Jan 30, 2025 •

edited

Loading