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.

Sign up for GitHub

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

Jump to bottom

Set correct exitcode in remove events #20609

Merged
merged 1 commit into from
Nov 28, 2023
Merged

Set correct exitcode in remove events #20609

merged 1 commit into from
Nov 28, 2023

Conversation

cgiradkar
Copy link
Contributor

Added additional check for event type to be remove and set the correct exitcode by fetching it from runtime state

closes #19124

Does this PR introduce a user-facing change?

Podman remove events now contain correct exit-code.

@openshift-ci 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 Nov 6, 2023
cgiradkar
cgiradkar commented Nov 6, 2023
View reviewed changes
libpod/events.go Outdated
@@ -77,6 +77,11 @@ func (c *Container) newContainerEventWithInspectData(status events.Status, inspe
e.HealthStatus = containerHealthStatus
}

if status == events.Remove {
exitCode, _ := c.runtime.state.GetContainerExitCode(c.ID())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Luap99 what kind of check should be added here to check for error from GetContainerExitCode

Copy link
Member

Choose a reason for hiding this comment

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

I think it is fine to ignore the error here but IMO we should not display the ContainerExitCode in the events when there is no exit code. However because 0 is a valid exit code we cannot use it with the current omitempty json logic.
While this is a pre-existing problem I think we should fix it. Having -1 for no exit code might be fine but I don't particular like this either. IMO it would make much more sense to change ContainerExitCode to *int then we have a proper nil value for no exit code which also means the omitempty json will correctly write it for 0 and only exclude it for nil.

This ensures it will not show up the the events output and thus not change the behaviour because "ContainerExitCode":-1 looks odd.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 8ce566d25d59ab31899febaa3a661c4b8c2056d2. @martinpitt, @jelly, @mvollmer please check.

@cgiradkar cgiradkar force-pushed the 19124_remove_event_fix branch from 8ce566d to 8fd4e40 Compare November 7, 2023 12:04
@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Nov 8, 2023
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 70e7804fc140834ff543f1717604bf83ff0c32b4. @martinpitt, @jelly, @mvollmer please check.

Luap99
Luap99 reviewed Nov 8, 2023
View reviewed changes
pkg/domain/infra/abi/containers.go Outdated
@@ -1188,13 +1188,15 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
return &report, nil
}

func (ic *ContainerEngine) GetContainerExitCode(ctx context.Context, ctr *libpod.Container) int {
func (ic *ContainerEngine) GetContainerExitCode(ctx context.Context, ctr *libpod.Container) *int {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this function changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing that as I mistook it for the same function as libpod>stage

libpod/events.go Outdated
Comment on lines 82 to 83
intExitCode := int(exitCode)
e.ContainerExitCode = &intExitCode
Copy link
Member

Choose a reason for hiding this comment

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

because we now have a proper nil as default you should only set the if there was no error returned by the call.

@cgiradkar cgiradkar force-pushed the 19124_remove_event_fix branch from 70e7804 to 83d27dd Compare November 8, 2023 16:48
cgiradkar
cgiradkar commented Nov 8, 2023
View reviewed changes
libpod/service.go Outdated
@@ -126,7 +126,7 @@ func (c *Container) canStopServiceContainer() (*serviceContainerReport, error) {
if err != nil {
return nil, err
}
if exitCode != 0 {
if exitCode != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

would the check suffice @Luap99

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@cgiradkar cgiradkar force-pushed the 19124_remove_event_fix branch from 83d27dd to 4beb20b Compare November 8, 2023 17:30
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 83d27dd206d4d374d1ab90d2aafa958f66bf1700. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 83d27dd206d4d374d1ab90d2aafa958f66bf1700. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 4beb20b0ca8b4f36acc0f7c3d1f483b316a805b8. @martinpitt, @jelly, @mvollmer please check.

@rhatdan
Copy link
Member

rhatdan commented Nov 8, 2023

@cgiradkar why is this a draft state?

@cgiradkar
Copy link
Contributor Author

cgiradkar commented Nov 9, 2023 •
edited
Loading

@cgiradkar why is this a draft state?

Some extra code refactoring was done which was not part of original plan, hence, till @Luap99 and I decide on a final strat, will keep in draft PR mode.

@cgiradkar cgiradkar force-pushed the 19124_remove_event_fix branch from 4beb20b to ededad0 Compare November 9, 2023 14:09
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit ededad013f333ad19300bbfa528cef362d34b35c. @martinpitt, @jelly, @mvollmer please check.

@cgiradkar cgiradkar force-pushed the 19124_remove_event_fix branch from ededad0 to 2999dfb Compare November 16, 2023 12:06
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 2999dfb86bebade0d3228af5c4ebcd6137f0095f. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

This still breaks a lot of tests, even most of them. As this is still draft, can you please let me know when it's ready from your side, so that we can start going through the regressions? Thanks!

@cgiradkar
Copy link
Contributor Author

This still breaks a lot of tests, even most of them. As this is still draft, can you please let me know when it's ready from your side, so that we can start going through the regressions? Thanks!

sure @martinpitt

@cgiradkar cgiradkar force-pushed the 19124_remove_event_fix branch 4 times, most recently from 5157d21 to ced4a4b Compare November 17, 2023 14:45
@cgiradkar cgiradkar force-pushed the 19124_remove_event_fix branch 6 times, most recently from e816526 to e9d56b6 Compare November 23, 2023 16:15
@cgiradkar cgiradkar marked this pull request as ready for review November 24, 2023 02:34
@openshift-ci 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 Nov 24, 2023
Luap99
Luap99 reviewed Nov 27, 2023
View reviewed changes
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.

Can you please squash both commits into one here. Code itself LGTM.

@cgiradkar cgiradkar force-pushed the 19124_remove_event_fix branch 2 times, most recently from 32801a9 to 4186e17 Compare November 28, 2023 12:46
@cgiradkar
Set correct exitcode in remove events and change ContainerExitCode fr…
572f38c 
…om int to int ptr

Added additional check for event type to be remove and set the correct exitcode.
While it was getting difficult to maintain the omitempty notation for Event->ContainerExitCode, changing the type from int to int ptr gives us the ability to check for ContainerExitCode to be not nil and continue operations from there.

closes containers#19124

Signed-off-by: Chetan Giradkar <[email protected]>
@cgiradkar cgiradkar force-pushed the 19124_remove_event_fix branch from 4186e17 to 572f38c Compare November 28, 2023 13:31
@cgiradkar
Copy link
Contributor Author

Can you please squash both commits into one here. Code itself LGTM.

Done

Luap99
Luap99 approved these changes Nov 28, 2023
View reviewed changes
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, @containers/podman-maintainers PTAL

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgiradkar, Luap99

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 Nov 28, 2023
@mheon
Copy link
Member

mheon commented Nov 28, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 83c08a2 into containers:main Nov 28, 2023
93 checks passed
@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 Feb 27, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 approved these changes

Assignees

@mheon mheon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny 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. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

container-remove event does not set exit code
5 participants
@cgiradkar @rhatdan @martinpitt @mheon @Luap99