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

Tests: make _prefetch() parallel-safe #5841

Merged

Conversation

edsantiago
Copy link
Member

The _prefetch helper, introduced in #2036, is not parallel-safe: two
or more parallel jobs fetching the same image can step on each other
and produce garbage images.

Although we still can't run buildah tests in parallel (see #5552),
we can at least set up the scaffolding for that to happen. This
commit reworks _prefetch() such that the image work is wrapped
inside flock. It has been working fine for months in #5552,
and is IMO safe for production. This can then make it much
easier to flip the parallelization switch once the final zstd
bug is squashed.

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

None

@siteshwar
Copy link

This was pointed to you earlier. Changes in this pull request introduce a potentially dangerous statement that shows up in shellcheck:

Error: SHELLCHECK_WARNING (CWE-398): [#def1]
/usr/share/buildah/test/system/helpers.bash:211:24: warning[SC2115]: Use "${var:?}" to ensure this never expands to / .
#  209|               else
#  210|                   # Failed. Clean up, so we don't leave incomplete remnants
#  211|->                 rm -fr $_BUILDAH_IMAGE_CACHEDIR/$fname
#  212|               fi
#  213|               sleep 5

You are assuming both of these variables are always set. It should be fixed here and at other places before merging.

The _prefetch helper, introduced in containers#2036, is not parallel-safe: two
or more parallel jobs fetching the same image can step on each other
and produce garbage images.

Although we still can't run buildah tests in parallel (see containers#5552),
we can at least set up the scaffolding for that to happen. This
commit reworks _prefetch() such that the image work is wrapped
inside flock. It has been working fine for months in containers#5552,
and is IMO safe for production. This can then make it much
easier to flip the parallelization switch once the final zstd
bug is squashed.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago edsantiago force-pushed the parallel-safe-prefetch branch from e371fcf to 75c8852 Compare November 19, 2024 17:11
@edsantiago
Copy link
Member Author

You are assuming both of these variables are always set

Fixed, thank you

@TomSweeneyRedHat
Copy link
Member

Changed LGTM, but a rebase is needed @edsantiago

@edsantiago
Copy link
Member Author

Is there a way to shut up the anti-helpful "branch is out-of-date" message in this repo?

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

/lgtm

@Luap99
Copy link
Member

Luap99 commented Nov 21, 2024

Is there a way to shut up the anti-helpful "branch is out-of-date" message in this repo?

Yes these are settings in the branch protection github settings AFAIK.

@nalind
Copy link
Member

nalind commented Nov 21, 2024

/approve

Copy link
Contributor

openshift-ci bot commented Nov 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, Luap99, nalind

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-merge-bot openshift-merge-bot bot merged commit 9983122 into containers:main Nov 22, 2024
32 checks passed
@edsantiago edsantiago deleted the parallel-safe-prefetch branch November 22, 2024 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants