Skip to content
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

Parallelize buildah bud tests #24656

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

edsantiago
Copy link
Member

Buildah bats tests have been made (mostly) parallel-safe
in the past few months. One test is flaking, but it's
not a test that needs to be run under podman: that
functionality is almost entirely buildah-manifest-push
so it uses the buildah binary, and doesn't exercise
anything under podman.

Therefore:

  1. run bud tests with -j$(nproc) on fastvm (was: standardvm)

  2. desperate scramble to parallelize podman system service.

May not be quite 100% perfect, but I think this is in good
enough shape for someone to adopt and push through.

Signed-off-by: Ed Santiago [email protected]

No release-note, because this should not merge as-is

Buildah bats tests have been made (mostly) parallel-safe
in the past few months. One test is flaking, but it's
not a test that needs to be run under podman: that
functionality is almost entirely buildah-manifest-push
so it uses the buildah binary, and doesn't exercise
anything under podman.

Therefore:

  1) run bud tests with -j$(nproc) on fastvm (was: standardvm)

  2) desperate scramble to parallelize podman system service.

May not be quite 100% perfect, but I think this is in good
enough shape for someone to adopt and push through.

Signed-off-by: Ed Santiago <[email protected]>
Copy link
Contributor

openshift-ci bot commented Nov 22, 2024

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Nov 22, 2024
@edsantiago edsantiago added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. do-not-merge/needs-unit-tests do-not-merge/needs-integration-tests do-not-merge/needs-confirmation Waiting for feedback from stakeholder before merging and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Nov 22, 2024
Copy link
Contributor

openshift-ci bot commented Nov 22, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2024
Comment on lines +78 to +89
+ # FIXME! 2024-11-22: I have no idea if this is still relevant, but it
+ # certainly does not play well with parallel bats tests. Let's see
+ # what happens if we disable it.
+ # FIXME! 2023-04-11: under remote + rootless, on the very first test,
+ # we somehow end up with two podman-system-service jobs. The second one
+ # lingers, and prevents BATS from completing, manifesting as a test hang.
+ if is_rootless; then
+ ps auxww | grep "system service" | grep -v grep | while read user pid rest; do
+ echo "# teardown: killing stray server: $user $pid $rest" >&3
+ kill $pid
+ done
+ fi
+# if is_rootless; then
+# ps auxww | grep "system service" | grep -v grep | while read user pid rest; do
+# echo "# teardown: killing stray server: $user $pid $rest" >&3
+# kill $pid
+# done
+# fi
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm choosing to leave this in place, instead of deleting it, because I want someone else to practice the bud-tests dance.

You will need to:

  1. Check out this PR
  2. invoke test/buildah-bud/run-buildah-tests --no-test. This will check out the buildah source tree into test-buildah-* and apply some patches
  3. cd there
  4. You will need to git reset --hard, to get rid of the applied skips and error message changes, but before you do so, why not run git diff just so you can get a feel for what things look like in a bud-under-podman scenario?
  5. git reset --hard
  6. edit tests/helpers.bash and remove this block
  7. git commit --amend. Don't worry, this isn't podman, this is a scratch git repo for buildah. Commit message is irrelevant, don't bother changing it, just commit your new changes
  8. ./make-new-buildah-diffs
  9. cd .., back to your podman source tree
  10. git diff, and this is hard to grok because you're diffing a diff patch, but basically it should look like changes in commit hash, line numbers, and one block removed
  11. git commit that, and push your PR, and party on

@openshift-ci openshift-ci bot removed the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Nov 22, 2024
@edsantiago
Copy link
Member Author

Obviously still bugs left to squash

         not ok 372 bud-with-mount-cache-like-buildkit
...
         # #|     FAIL: podman-remote build --force-rm=false --layers=false -t testbud2 --signature-policy /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.38.1-0.20241122025341-998312277d88/tests/policy.json -f /var/tmp/buildah_tests.x68cyr/buildkit-mount/Dockerfilecacheread
         # #| expected: =~ 'hello'
         # #|   actual:    'STEP 1/3: FROM alpine'
         # #|         >    'STEP 2/3: RUN mkdir /test'
         # #|         >    'STEP 3/3: RUN --mount=type=cache,target=/test,z cat /test/world'
         # #|         >    'world'
...

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2024
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/needs-confirmation Waiting for feedback from stakeholder before merging do-not-merge/needs-integration-tests do-not-merge/needs-unit-tests do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants