-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
WIP: system test parallelization: two-pass approach #23275
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
THIS IS NOT EVEN CLOSE TO DREAMING ABOUT MERGING! @Luap99 I think this approach holds promise. I would like to spend some time pursuing it. Before I do so, WDYT? |
Just some quick thoughts, will add more once I am back. Syntax wise this seems to be better as there were many files I could not run parallel because one or two tests. Assuming this now runs things parallel across files as well it should utilize the cpu better. However I do not see how this addresses the functional issues from my PR. I think there are nice gains here but honestly I am no longer sure that the ongoing maintenance will not cause to much work on all maintainers. |
f456c95
to
e79fca4
Compare
Okay..... I'm really favorably impressed with this approach. The two-pass requirement sucks, and debugging failures is really hard, but I think the benefits (so far) are outweighing those negatives. Running lots and lots of different tests in parallel, not just from one file, is finding a lot of bugs. CI is likely to fail because of #23282. This is still very much a WIP. My plan is to break out much of the safename work, commit that separately in individual reviewable PRs, in order to minimize the changes in this one. |
Cockpit tests failed for commit e79fca4. @martinpitt, @jelly, @mvollmer please check. |
e79fca4
to
8cf6051
Compare
8cf6051
to
0c5bb35
Compare
@edsantiago Ok let's do this then. I will try to fix all the related podman bugs which you reported in the next days. |
0c5bb35
to
f6b6178
Compare
dc66bb4
to
477cbe0
Compare
re tag name: |
Full name: my concern is typos. I know that we'll get occasional "parralel" or "parrallel" misspellings and those are hard to catch in review. I've been letting my brain think about this in the background and still haven't come up with any ideas. The other consideration is a string that's easily greppable in source code and command-line history. |
ci:parallel SGTM. Another reason for something like codespell to be part of the actual CI checks. |
In general I would good to get some docs in test/system/README.md that descripe how this parallel mode works and what test can/cannot run in parallel (--all,--latest, output checks like podman ps empty output, etc... ) |
Also another flake I saw locally.
I know what is wrong with that and will do another PR to fix that. |
Fix in #23326 |
477cbe0
to
b48e602
Compare
Cockpit tests failed for commit c313488. @martinpitt, @jelly, @mvollmer please check. |
5d3139c
to
3b6a5c2
Compare
Cockpit tests failed for commit 3b6a5c2. @martinpitt, @jelly, @mvollmer please check. |
69eb127
to
8163cf0
Compare
...try to trace them back to the culprit tests Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
In theory when syslog is set the cleanup process should log its errors to syslog (journald) so we can have a look at the errors in CI. Without it podman container cleanup errors will never be logged anywhere. In order to rey to debug containers#21569 Signed-off-by: Paul Holzinger <pholzing@redhat.com>
All we care about in this PR is system tests. Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Luap99@df865c8 Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Previous version was badly broken: it relied on 'make' rebuilding a file under cwd, which is a no-no; and, in the case where we don't have a source directory, just blindly hoped that there'd be a system-installed .service file with the correct path to podman. Solution: . if running in source directory, run sed directly into destination service file in $UNIT_DIR. This is ugly duplication of a line in Makefile. . if NOT running in a source directory, check $PODMAN: . if it's /usr/bin/podman, continue . otherwise skip, because we don't know what we're testing Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
...for debugging containers#24147, because "md5sum mismatch" is not the best way to troubleshoot bytestream differences. Because socat is run on the container, we need to build a new testimage (20241009). Signed-off-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Ed Santiago <santiago@redhat.com>
8163cf0
to
9d8e3b0
Compare
Split system tests into two: those that can be run in
parallel, and those that can't. Run tests in two passes.
This requires eliminating the per-test leak check and
teardown. I think that's okay.
Tests that can run in parallel:
so the leak test at end can be useful
Signed-off-by: Ed Santiago santiago@redhat.com