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

exec: fix error code when conmon fails #5396

Merged

Conversation

haircommander
Copy link
Collaborator

@haircommander haircommander commented Mar 4, 2020

this is a cosmetic change that makes sure podman returns a sane error code when conmon dies underneath it

fixes the weirdness in this comment: #5339 (comment)

but unfortunately not the underlying problem.

Signed-off-by: Peter Hunt [email protected]

Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan
Copy link
Member

rhatdan commented Mar 4, 2020

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, 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 Mar 4, 2020
@@ -341,6 +341,10 @@ func (c *Container) Exec(tty, privileged bool, env map[string]string, cmd []stri
logrus.Errorf(lastErr.Error())
}
lastErr = errors.Wrapf(define.ErrOCIRuntime, "non zero exit code: %d", exitCodeData.data)
// if we failed in podman, set the exit code to a generic error
Copy link
Member

Choose a reason for hiding this comment

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

"if we failed in podman"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the comment to be more informative

@edsantiago
Copy link
Member

LGTM but I'm not qualified to understand the whole subcontext here; nor do I have a reliable way to reproduce this.

@edsantiago
Copy link
Member

I spoke too soon: we do have a way to test this: CI:

[+0223s] # time="2020-03-04T16:53:39-05:00" level=error msg="container create failed (no logs from conmon): EOF"
[+0223s] # Error: non zero exit code: -2147483649: OCI runtime error

I was expecting this PR to change this message?

@haircommander
Copy link
Collaborator Author

I spoke too soon: we do have a way to test this: CI:

[+0223s] # time="2020-03-04T16:53:39-05:00" level=error msg="container create failed (no logs from conmon): EOF"
[+0223s] # Error: non zero exit code: -2147483649: OCI runtime error

I was expecting this PR to change this message?

uh yeah I would expect the message to change too, but I clobbered CI with a force push. Time to wait for another failure 😓

@edsantiago
Copy link
Member

Did you mean to check in the .lock file?

And, I misspoke, I wouldn't expect the message to change since the assignment happens after the message. I'm not sure I understand how this helps...

this is a cosmetic change that makes sure podman returns a sane error code when conmon dies underneath it

Signed-off-by: Peter Hunt <[email protected]>
@haircommander
Copy link
Collaborator Author

Did you mean to check in the .lock file?

And, I misspoke, I wouldn't expect the message to change since the assignment happens after the message. I'm not sure I understand how this helps...

jeez it must be the end of the day....

thanks for your hand holding, I dropped the .lock file and updated the location to make it actually work..

@jwhonce
Copy link
Member

jwhonce commented Mar 4, 2020

/hold (pending tests working)
/lgtm

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 4, 2020
@edsantiago
Copy link
Member

Looks better:

[+0228s] # time="2020-03-04T17:47:55-05:00" level=error msg="container create failed (no logs from conmon): EOF"
[+0228s] # Error: non zero exit code: 125: OCI runtime error

Failures restarted, even though one of them is new to me. I'll just treat it as a flake and worry about it later if it recurs.

Running: (env: [PODMAN_VARLINK_ADDRESS=unix:/run/user/18722/io.podman-4cdaee893ade05e694bf42b3a156aa50a069802cf4bd86a35acf7e6859ce65a5]) podman [options]-remote start -l
         Error: executable file not found in $PATH: No such file or directory: OCI runtime command not found error

@edsantiago
Copy link
Member

/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 Mar 4, 2020
@edsantiago
Copy link
Member

Why isn't this automerging?

@openshift-merge-robot openshift-merge-robot merged commit 8a8c2fe into containers:master Mar 5, 2020
@mheon mheon mentioned this pull request Mar 9, 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 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 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