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

Closes #15617: emit container labels for container exited and exec died events #15633

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

thediveo
Copy link
Contributor

@thediveo thediveo commented Sep 5, 2022

Does this PR introduce a user-facing change?

The container-related events (with ``Type`=`container`) for `Status` of `died` and `exec_died`
now carry the container's labels in the `Attributes` event field, as for instance the `start` container event
already does.

@thediveo
Copy link
Contributor Author

thediveo commented Sep 5, 2022

  • I don't see any event_test.go, so creating a new one is currently out of reach for me for a first podman PR.
  • in the same way test/system/090-events.bats is out of reach for me, more so as this is a fully unknown testing tool to me.

@edsantiago
Copy link
Collaborator

Here ya go

# Prior to #15633, container labels would not appear in 'die' log events
@test "events - labels included in container die" {
    local cname=c$(random_string 15)
    local lname=l$(random_string 10)
    local lvalue="v$(random_string 10) $(random_string 5)"

    run_podman 17 --events-backend=file run --rm \
               --name=$cname \
               --label=$lname="$lvalue" \
               $IMAGE sh -c 'exit 17'
    run_podman --events-backend=file events \
               --filter=container=$cname \
               --filter=status=died \
               --stream=false \
               --format=json
    local label_found=$(jq -r ".Attributes.$lname" <<<"$output")
    assert "$label_found" = "$lvalue" "podman-events output includes container label"
}

@vrothberg
Copy link
Member

Thanks, @edsantiago !

@rhatdan
Copy link
Member

rhatdan commented Sep 6, 2022

LGTM other then handling tests.

@edsantiago
Copy link
Collaborator

UPDATE: now that #15644 has merged, please rebase and change the last few lines of my suggestion above to:

    ...
               --stream=false \
               --format="{{.Attributes.$lname}}"
    assert "$output" = "$lvalue" "podman-events output includes container label"

@edsantiago
Copy link
Collaborator

LGTM. Thanks again.

@thediveo
Copy link
Contributor Author

thediveo commented Sep 6, 2022

@edsantiago @vrothberg many thanks for helping me over the bats-based unit test hurdles; things start to finally dawn for me now.

And now for something not completely different, the obligatory xkcd https://xkcd.com/1296/ ;)

@edsantiago
Copy link
Collaborator

Oops! My bad, I didn't test with podman-remote, which apparently rejects --events-backend. You will need to add skip_if_remote as the first line of the new test. Sorry about that.

- adds unit test for container labels on container die event
- implements #15617

Signed-off-by: Harald Albrecht <harald.albrecht@gmx.net>
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thediveo, vrothberg

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 Sep 7, 2022
@openshift-merge-robot openshift-merge-robot merged commit b231e73 into containers:main Sep 7, 2022
@thediveo thediveo deleted the events branch September 7, 2022 08:59
@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 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 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. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants