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

Remove final v2remotefail failures #7731

Merged
merged 1 commit into from
Sep 24, 2020

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Sep 22, 2020

Most have been fixed, others I replaced with SkipIfRemote

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2020
@mheon
Copy link
Member

mheon commented Sep 22, 2020

LGTM

@rhatdan
Copy link
Member Author

rhatdan commented Sep 22, 2020

@edsantiago More tests for you...
@jwhonce @baude @QiWang19 @ashley-cui @TomSweeneyRedHat @vrothberg

The end of the v2remotefail, may it Rest In Peace.

@edsantiago
Copy link
Member

This is probably beyond the scope of this PR, but I would love to see SkipIfRemote() extended to require a message, and use that message judiciously such as "mount" is meaningless over remote and FIXME: issue #12345. Otherwise LGTM (pending CI). Thank you for tackling this.

@edsantiago
Copy link
Member

It("podman stop all containers with one stopped", func() {
Skip(v2remotefail)

@TomSweeneyRedHat
Copy link
Member

LGTM

@edsantiago
Copy link
Member

oooh, race condition! Reproducible! (one out of three attempts):

$ ./bin/podman-remote create --name test1 -it --rm alpine foo
91057d106cbbaecb67ad946dfa5c5b7357f732827679377c840bd192a0f393e7
$ ./bin/podman-remote start test1;./bin/podman-remote ps -aq
Error: executable file not found in $PATH: No such file or directory: OCI runtime command not found error
Error: container 91057d106cbbaecb67ad946dfa5c5b7357f732827679377c840bd192a0f393e7 does not exist in database: no such container

@rhatdan
Copy link
Member Author

rhatdan commented Sep 22, 2020

@edsantiago I just fixed a race condition on start, did you test with this latest PR?

@rhatdan rhatdan closed this Sep 22, 2020
@rhatdan rhatdan reopened this Sep 22, 2020
@edsantiago
Copy link
Member

@rhatdan I'm testing against your PR. All I'm doing is reproducing the failure I see in the CI logs.

@edsantiago
Copy link
Member

These are some really unpleasant failures. (lots of hangs).

@edsantiago
Copy link
Member

Bindings flake is #6518, restarted.

@rhatdan rhatdan force-pushed the v2remotefail branch 2 times, most recently from bffb9ca to 1beb73a Compare September 23, 2020 13:11
@rhatdan
Copy link
Member Author

rhatdan commented Sep 23, 2020

@edsantiago This is a lot closer to the way the podman start works, although there is a new FIXME, I had to add to get this in.

BTW I could see the current podman-remote start causing lots of weird flakes.

@edsantiago
Copy link
Member

Errors are #7746 -- I thought you had re-added a SkipIfRemote() in that test?

Comment on lines 133 to -134
It("podman failed to start with --rm should delete the container", func() {
Skip(v2remotefail)
Copy link
Member

Choose a reason for hiding this comment

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

If you rebase and decide to skip this again, please include `SkipIfRemote("FIXME: #7746") to make it easier for the 7746-fixer-person to find this. TIA.

Most have been fixed, others I replaced with SkipIfRemote

Fix ContainerStart on tunnel, it needs to wait for the exit status
before returning.

Signed-off-by: Daniel J Walsh <[email protected]>
@edsantiago
Copy link
Member

docs looks like a Cirrus flake (connection something something). The other failure concerns me: podman-remote search alpine hung for 90 seconds. This is consistent with earlier "Expected process to exit. It did not." failures in previous iterations.

@edsantiago
Copy link
Member

OMG tests are green after countless flakes!
/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2020
@rhatdan
Copy link
Member Author

rhatdan commented Sep 24, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2020
@openshift-merge-robot openshift-merge-robot merged commit 505f312 into containers:master Sep 24, 2020
@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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 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. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants