-
Notifications
You must be signed in to change notification settings - Fork 597
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.
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
CI adjustments #3535
base: main
Are you sure you want to change the base?
CI adjustments #3535
Conversation
127556a
to
4650e5d
Compare
fdbf2ac
to
c32df15
Compare
Signed-off-by: apostasie <[email protected]>
strategy: | ||
fail-fast: false | ||
matrix: | ||
# ubuntu-20.04: cgroup v1, ubuntu-22.04 and later: cgroup v2 | ||
include: | ||
- ubuntu: 24.04 | ||
containerd: v1.7.23 | ||
containerd: v2.0.0-rc.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was discussed before: we run ipv6 tests only on the latest.
@AkihiroSuda this has been stripped down to the bones and is finally ready. Obviously, disabling retries on a fraction of tests is still surfacing weird issues - oddly, docker - so, I will keep sending separate PRs until this thing is green, but otherwise, the CI code changes themselves are ready for review. |
@@ -44,11 +44,13 @@ jobs: | |||
- name: "Run unit tests" | |||
run: go test -v ./pkg/... | |||
- name: "Run integration tests" | |||
run: docker run -t --rm --privileged test-integration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split the two steps.
@@ -91,5 +94,6 @@ jobs: | |||
ctrdVersion: ${{ env.CONTAINERD_VERSION }} | |||
run: powershell hack/configure-windows-ci.ps1 | |||
- name: "Run integration tests" | |||
# See https://github.com/containerd/nerdctl/blob/main/docs/testing/README.md#about-parallelization | |||
run: go test -p 1 -v ./cmd/nerdctl/... | |||
run: ./hack/test-integration.sh -test.only-flaky=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split the two steps and use gotestsum for better summary
|
||
jobs: | ||
test-unit: | ||
# Supposed to work: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#example-returning-a-json-data-type | ||
# Apparently does not | ||
# timeout-minutes: ${{ fromJSON(env.SHORT_TIMEOUT) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish that worked. Anyone has insight?
max_attempts: 2 | ||
retry_on: error | ||
command: docker run -t --rm --privileged test-integration | ||
run: docker run -t --rm --privileged test-integration ./hack/test-integration.sh -test.only-flaky=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split the two steps.
@@ -129,7 +133,7 @@ jobs: | |||
echo '{"ipv6": true, "fixed-cidr-v6": "2001:db8:1::/64", "experimental": true, "ip6tables": true}' | sudo tee /etc/docker/daemon.json | |||
sudo systemctl restart docker | |||
- name: "Prepare integration test environment" | |||
run: docker build -t test-integration-ipv6 --target test-integration-ipv6 --build-arg UBUNTU_VERSION=${UBUNTU_VERSION} --build-arg CONTAINERD_VERSION=${CONTAINERD_VERSION} . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the extra stage that has nothing specific. It is just an extra arg.
max_attempts: 2 | ||
retry_on: error | ||
command: docker run --network host -t --rm --privileged test-integration-ipv6 | ||
run: docker run --network host -t --rm --privileged test-integration ./hack/test-integration.sh -test.only-ipv6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nick should go
@@ -248,12 +246,13 @@ jobs: | |||
go-version: ${{ matrix.go-version }} | |||
cache: true | |||
check-latest: true | |||
- name: "Cross" | |||
- name: "build" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better display. This is not "cross-compilation" it is building for different versions of go.
@@ -787,6 +787,9 @@ func newBase(t *testing.T, ns string, ipv6Compatible bool, kubernetesCompatible | |||
} else if !base.EnableKubernetes && base.KubernetesCompatible { | |||
t.Skip("runner skips Kubernetes compatible tests in the non-Kubernetes environment") | |||
} | |||
if !GetFlakyEnvironment() && !GetEnableKubernetes() && !GetEnableIPv6() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the part that keep the legacy tests into the "retry" part.
Oh docker, I... |
33e89d5
to
fd7528d
Compare
dc52901
to
f4954f0
Compare
Signed-off-by: apostasie <[email protected]>
That was quite a trek. @AkihiroSuda @djdongjin at your convenience. |
NOTE: this is pending #3572, #3573, #3574, #3575This is the next part of the test rework effort.
The most important change here
Is that we will now have two separate steps for every integration test run:
Note that the above split does NOT apply to ipv6, nor kube, tests. These are considered safe.
Right now, the balance is about 286 (rootful, no retry) vs. 500 (with retry).
Hopefully, as we fix and migrate more tests, and address bugs in nerdctl, it will shift.
Note that running legacy tests without retries is currently very painful and does surface a lot of conditions that have been previously unseen / ignored for a long time. As a lot of bugs have been recently fixed with regards to concurrency, underlying issues are also starting to crop up, and these very likely run deep / are involved to fix.
These are chiefly:
This is the reason why we are - conservatively - leaving the pre-existing, legacy tests in the "with retries" bucket.
There is now a helper script,
test-integration.sh
.When ran without any argument, if will run both steps in order:
If you want to run ONLY the first step (aka "safe tests"), call:
test-integration.sh -test.only-flaky=false
If you want to run ONLY the second step, call:
test-integration.sh -test.only-flaky=true
That works as well with gotestsum, or go test, evidently:
Note though that the default in that case is
-test.only-flaky=false
:Other CI changes