Skip to content

[CI]: rewrite some container and compose tests #4122

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

Merged
merged 1 commit into from
Apr 21, 2025

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Apr 17, 2025

I have been delaying engaging with compose and container tests so far because of the daunting amount of work...

But at this point, they are the number one contributor to overall slowness (and most of remaining flakyness).
Moving these will cut through the total runtime and make a dent in the bucket of "tests with retries".

Note that we should merge first, before this:
- #4111
- #4081
- #4116

and preferably #4114.

... as this PR does leverage some of the recent changes in Tigron (specifically temp files manipulation).

I will obviously take care of rebasing once they are in, though the last commit of this PR could be reviewed already at this point.

Note that this PR also adopt / cleans-up / remove deprecated helpers (some of them wrongly located under ./cmd, some under ./pkg/testutil).

I know it is painful to review large PRs like that, but I believe there is really no other way here - and at the end of the day, this is only tests - the CI being green is proof in the pudding.

@apostasie apostasie force-pushed the ci-rewrite-more-tests branch 6 times, most recently from 3a345b4 to 2a88d6a Compare April 17, 2025 23:48
@apostasie
Copy link
Contributor Author

@apostasie apostasie force-pushed the ci-rewrite-more-tests branch from 2a88d6a to d9546ec Compare April 18, 2025 00:29
@apostasie apostasie marked this pull request as ready for review April 18, 2025 01:27
@apostasie apostasie changed the title [WIP] [CI]: rewrite some container and compose tests [CI]: rewrite some container and compose tests Apr 18, 2025
@AkihiroSuda AkihiroSuda added this to the v2.0.5 milestone Apr 18, 2025
@AkihiroSuda AkihiroSuda added the area/ci e.g., CI failure label Apr 18, 2025
@AkihiroSuda
Copy link
Member

Needs rebase

@apostasie apostasie force-pushed the ci-rewrite-more-tests branch from d9546ec to 6bcdc42 Compare April 18, 2025 15:23
@apostasie
Copy link
Contributor Author

Rebased.

Thanks for all the merges @AkihiroSuda

I'll do another proof-reading of this right now.

@AkihiroSuda AkihiroSuda requested a review from a team April 18, 2025 15:30
@apostasie
Copy link
Contributor Author

apostasie commented Apr 18, 2025

Failures are a bunch of existing EL8 specific fails.

@apostasie apostasie force-pushed the ci-rewrite-more-tests branch from 6bcdc42 to ff8b599 Compare April 18, 2025 20:35
@apostasie
Copy link
Contributor Author

Rebasing for good measure.

@apostasie
Copy link
Contributor Author

Failure is again buildg timeout.

I do not have a good explanation for why we are seeing this so much now.
Intuition would suggest:

  • recent update in buildkitd that trips this (also buildg is linking a quite old buildkit fork, so, compat may be an issue)
  • recent changes in test.Command which may slightly alter behavior when the streams are attached to buildg (seems unlikely, but then?)
  • changes in tests (or otherwise github runner sizing change) that would increase the pressure on the github instance, making for a slower response, that would (now) trip over the timeout (rather short, at 3 secs)

#4103 should get to the root cause eventually, but it is an excruciating process to nail flaky behavior in third party binaries...

Copy link
Member

@djdongjin djdongjin left a comment

Choose a reason for hiding this comment

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

I hope we can try to reduce PR sizes in future PRs.

I know it is painful to review large PRs like that, but I believe there is really no other way here - and at the end of the day, this is only tests - the CI being green is proof in the pudding.

I would suggest at least split the PR by areas, e.g., one for compose, one for image, one for container.

@apostasie
Copy link
Contributor Author

I hope we can try to reduce PR sizes in future PRs.

I know it is painful to review large PRs like that, but I believe there is really no other way here - and at the end of the day, this is only tests - the CI being green is proof in the pudding.

I would suggest at least split the PR by areas, e.g., one for compose, one for image, one for container.

That is fair - will do.

Thanks @djdongjin

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 75e182c into containerd:main Apr 21, 2025
49 of 50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci e.g., CI failure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants