Add container error message to ContainerState #16806

jakecorrenti · 2022-12-11T01:15:54Z

This change aims to store an error message to the ContainerState struct with the last known error from the Start, StartAndAttach, and Stop OCI Runtime functions.

The goal was to act in accordance with Docker's behavior.

Example:

$ podman create -t --name demo alpine efcho Hello World
$ podman start demo
$ podman inspect -f '{{ .State.Error }}' demo
crun: executable file `efcho` not found in $PATH: No such file or directory: OCI runtime attempted to invoke a command that was not found

Fixes: #13729

Signed-off-by: Jake Correnti [email protected]

Does this PR introduce a user-facing change?

Adds a feature for the user to see why a container failed when inspecting it.

jakecorrenti · 2022-12-11T01:18:54Z

I still need to figure out the best way to test all of the different ways an error could be saved to the ContainerState and make sure the message is correct in podman inspect.

I also need to verify that everywhere I have saveContainerError is correct and there aren't any that are unnecessary/missing (I continue to go back and forth with myself on where this should be added).

vrothberg

It's probably easier to return a named error and use a defer to save the error. For instance

defer func() {
if finalErr != nil {
   saveContainerError(...)
}
}()

Any chance we move saving the error further up the call stack? The higher we are, the less locations we have to update (and will probably miss in the future).

@mheon PTAL

jakecorrenti · 2022-12-12T14:32:22Z

I definitely agree the named error would be easier.

The issue I was having with the location of the error being saved came down to the container API. To the best of my knowledge, I don't actually get access to the container state until this point in the call stack, unless I was to modify the container API (which I tried to avoid). I accept the fact there's a good chance I'm wrong, though.

mheon · 2022-12-12T15:28:06Z

I'm OK with leaving the error where it is... Moving out of Libpod is just going to start introducing substantial complexity (having the ability to update the DB outside of libpod is problematic).

libpod/container_api.go

vrothberg

Changes LGTM.

Note that the diff would be even smaller when calling the named error finalError or something that is not yet used in the function bodies. Than, they can continue to return err.

@mheon PTAL

test/e2e/inspect_test.go

TomSweeneyRedHat · 2022-12-28T20:54:35Z

All kinds of red unhappy tests

jakecorrenti · 2023-01-03T17:02:08Z

Is it possible to restart the tests? Looks like some unrelated failure with a missing catatonit executable

rhatdan · 2023-01-03T17:55:12Z

libpod/container_api.go

+	defer func() {
+		if finalErr != nil {
+			_ = saveContainerError(c, finalErr)
+		}


Can you logrus.Debug this error if not nil?

rhatdan · 2023-01-03T17:55:18Z

libpod/container_api.go

+func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachStreams, keys string, resize <-chan resize.TerminalSize, recursive bool) (retChan <-chan error, finalErr error) {
+	defer func() {
+		if finalErr != nil {
+			_ = saveContainerError(c, finalErr)


Can you logrus.Debug this error if not nil?

rhatdan · 2023-01-03T17:55:25Z

libpod/container_api.go

+func (c *Container) StopWithTimeout(timeout uint) (finalErr error) {
+	defer func() {
+		if finalErr != nil {
+			_ = saveContainerError(c, finalErr)


Can you logrus.Debug this error if not nil?

This change aims to store an error message to the ContainerState struct with the last known error from the Start, StartAndAttach, and Stop OCI Runtime functions. The goal was to act in accordance with Docker's behavior. Fixes: containers#13729 Signed-off-by: Jake Correnti <[email protected]>

rhatdan · 2023-01-03T19:03:42Z

/lgtm
/hold

rhatdan · 2023-01-05T18:51:14Z

/approve

openshift-ci · 2023-01-05T18:51:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jakecorrenti, 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:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jakecorrenti · 2023-01-05T20:58:55Z

/hold cancel

openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Dec 11, 2022

jakecorrenti force-pushed the podman-inspect-add-error-info branch 2 times, most recently from 9920290 to 80887fc Compare December 11, 2022 15:16

vrothberg reviewed Dec 12, 2022

View reviewed changes

mheon reviewed Dec 12, 2022

View reviewed changes

libpod/container_api.go Outdated Show resolved Hide resolved

jakecorrenti force-pushed the podman-inspect-add-error-info branch 2 times, most recently from 775fc9c to c5a5d09 Compare December 13, 2022 00:08

jakecorrenti changed the title ~~[WIP] Add container error message to ContainerState~~ Add container error message to ContainerState Dec 13, 2022

jakecorrenti marked this pull request as ready for review December 13, 2022 04:29

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2022

vrothberg reviewed Dec 13, 2022

View reviewed changes

test/e2e/inspect_test.go Show resolved Hide resolved

jakecorrenti force-pushed the podman-inspect-add-error-info branch 3 times, most recently from d195045 to e9209c9 Compare December 13, 2022 22:16

jakecorrenti force-pushed the podman-inspect-add-error-info branch from e9209c9 to 03bc0c8 Compare December 31, 2022 04:47

rhatdan reviewed Jan 3, 2023

View reviewed changes

jakecorrenti force-pushed the podman-inspect-add-error-info branch from 03bc0c8 to df02cb5 Compare January 3, 2023 18:21

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 3, 2023

openshift-ci bot assigned rhatdan Jan 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 5, 2023

openshift-merge-robot merged commit b7314bd into containers:main Jan 5, 2023

This was referenced Jan 9, 2023

executable file /catatonit [et al] not found in $PATH: No such file or directory #17042

Closed

[email protected] template: expected inactive, got active #17093

Closed

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 16, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add container error message to ContainerState #16806

Add container error message to ContainerState #16806

jakecorrenti commented Dec 11, 2022 •

edited

Loading

jakecorrenti commented Dec 11, 2022

vrothberg left a comment

jakecorrenti commented Dec 12, 2022

mheon commented Dec 12, 2022

vrothberg left a comment

TomSweeneyRedHat commented Dec 28, 2022

jakecorrenti commented Jan 3, 2023

rhatdan Jan 3, 2023

rhatdan Jan 3, 2023

rhatdan Jan 3, 2023

rhatdan commented Jan 3, 2023

rhatdan commented Jan 5, 2023

openshift-ci bot commented Jan 5, 2023

jakecorrenti commented Jan 5, 2023

Add container error message to ContainerState #16806

Add container error message to ContainerState #16806

Conversation

jakecorrenti commented Dec 11, 2022 • edited Loading

Does this PR introduce a user-facing change?

jakecorrenti commented Dec 11, 2022

vrothberg left a comment

Choose a reason for hiding this comment

jakecorrenti commented Dec 12, 2022

mheon commented Dec 12, 2022

vrothberg left a comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Dec 28, 2022

jakecorrenti commented Jan 3, 2023

rhatdan Jan 3, 2023

Choose a reason for hiding this comment

rhatdan Jan 3, 2023

Choose a reason for hiding this comment

rhatdan Jan 3, 2023

Choose a reason for hiding this comment

rhatdan commented Jan 3, 2023

rhatdan commented Jan 5, 2023

openshift-ci bot commented Jan 5, 2023

jakecorrenti commented Jan 5, 2023

jakecorrenti commented Dec 11, 2022 •

edited

Loading