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

[Never Merge] Verify parallel build race fix and new storage API #13339

Closed
wants to merge 2 commits into from

Conversation

flouthoc
Copy link
Collaborator

DO NOT MERGE

Verify race condition fix for parallel builds changes in deps.
Just to verify: https://github.com/containers/image/pull/1480/files & containers/storage#1153

Fixes issue for following reproducer

Reproducer

#!/bin/bash
x=1
sudo ./podman rmi -af
rm -f log*.*
sudo ./podman build -t first . &> logfirst.log
while [ $x -le 30 ]
do
  echo "$x times"
  sudo ./podman build --log-level debug -t $x . &> log$x.log &
  x=$(( $x + 1 ))
#  sleep 1
done

Dockerfile

FROM quay.io/jitesoft/alpine

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

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 Feb 24, 2022
@flouthoc flouthoc marked this pull request as draft February 24, 2022 15:28
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 24, 2022
@flouthoc flouthoc force-pushed the verify-fix-race branch 3 times, most recently from 9d29ab4 to 99a934d Compare February 24, 2022 18:42
Use race-free c/storage and c/image api

Signed-off-by: Aditya R <arajan@redhat.com>
@TomSweeneyRedHat
Copy link
Member

Looks like happy tests @flouthoc ! For the final, it would be nice to add a test similar to the reproducer.

Signed-off-by: Aditya R <arajan@redhat.com>
@flouthoc
Copy link
Collaborator Author

Looks like happy tests @flouthoc ! For the final, it would be nice to add a test similar to the reproducer.

Thanks @TomSweeneyRedHat added a test for this as well.

@flouthoc
Copy link
Collaborator Author

@containers/storage-maintainers PTAL but following PR is just to verify not to merge

@vrothberg
Copy link
Member

Once the c/storage PR is merged, we also need to open a PR against c/common and to migrate Tag/Untag.

@flouthoc
Copy link
Collaborator Author

@vrothberg Yes I agree. For parallel build issue we only need containers/image#1480 and containers/storage#1153 so should we drive this change slowly.

@vrothberg
Copy link
Member

Since we know that SetNames is broken (and is soon deprecated), we should update all callers rather sooner than later. It's easy to forget about things on the TODO list.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 2, 2022

@flouthoc: 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/test-infra repository.

flouthoc added a commit to flouthoc/podman that referenced this pull request Mar 3, 2022
Invoking parallel/concurrent builds from podman race against each other
following behviour was fixed in
containers/storage#1153 and containers/image#1480

Test verifies if following bug is fixed in new race-free API or not.
Read more about this issue, see bz 2055487 for more details.

More details here: containers/buildah#3794 and containers#13339

Co-authored-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Aditya R <arajan@redhat.com>
@vrothberg
Copy link
Member

vrothberg commented Mar 11, 2022

@flouthoc is this PR still needed?

@flouthoc
Copy link
Collaborator Author

@vrothberg Nope. Thanks for reminding.

@flouthoc flouthoc closed this Mar 11, 2022
flouthoc added a commit to flouthoc/podman that referenced this pull request Apr 6, 2022
Invoking parallel/concurrent builds from podman race against each other
following behviour was fixed in
containers/storage#1153 and containers/image#1480

Test verifies if following bug is fixed in new race-free API or not.
Read more about this issue, see bz 2055487 for more details.

More details here: containers/buildah#3794 and containers#13339

Co-authored-by: Ed Santiago <santiago@redhat.com>
Signed-off-by: Aditya R <arajan@redhat.com>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. 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.

3 participants