Compat API image remove events now have 'delete' status #15487
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon 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 |
openshift-ci
bot
added
the
approved
|
@edsantiago Mind checking my APIv2 test changes? I only vaguely know what I'm doing there. |
test/apiv2/10-images.at
Outdated
| # compat API delete image produces a delete event | ||
| #create, list and remove dangling image | ||
| podman image build -t test:test -<<EOF | ||
| from alpine |
| t DELETE libpod/images/test:test 200 | ||
| t GET "libpod/events?stream=false&since=$START" 200 \ | ||
| 'select(.status | contains("remove")).Action=remove' | ||
| t GET "events?stream=false&since=$START" 200 \ |
There was a problem hiding this comment.
LGTM, and fails when run against main.
test/apiv2/10-images.at
Outdated
| @@ -239,4 +239,18 @@ fi | |||
|
|
|||
| cleanBuildTest | |||
|
|
|||
| # compat API delete image produces a delete event | |||
There was a problem hiding this comment.
Suggestion: make the difference clearer, e.g., ...delete event; libpod API calls it 'remove'
|
I don't understand any of the test failures. The one in your new test: could that be a race? Is the event written to journal before the endpoint terminates, or could there be a delay such that The |
|
I'm going to throw in a sleep to see if it could be a race. |
mheon
force-pushed
the
image_delete_event
branch
from
3fb1280
to
c4bb002
Compare
test/apiv2/10-images.at
Outdated
| @@ -59,7 +59,7 @@ t GET "images/testimage:20210610/json" 200 \ | |||
|
|
|||
| # Make sure that new images are pulled | |||
| old_iid=$(podman image inspect --format "{{.ID}}" docker.io/library/alpine:latest) | |||
| podman rmi -f docker.io/library/alpine:latest | |||
| 11;rgb:0000/0000/0000podman rmi -f docker.io/library/alpine:latest | |||
There was a problem hiding this comment.
copy-paste error, methinks. I also methinks that this will make tests barf before you get to the actual real test, so, you might want to repush.
| @@ -89,6 +89,10 @@ func GetEvents(w http.ResponseWriter, r *http.Request) { | |||
| } | |||
|
|
|||
| e := entities.ConvertToEntitiesEvent(*evt) | |||
| if !utils.IsLibpodRequest(r) && e.Type == "image" && e.Status == "remove" { | |||
There was a problem hiding this comment.
y'know, as long as we're re-pushing, why not add a comment here for future maintainers, explaining exactly what the problem is?
mheon
force-pushed
the
image_delete_event
branch
from
c4bb002
to
ba4042a
Compare
|
Passed this time. Now what? |
|
I see two potential options. This could be bad behavior out of the delete endpoint for images (returning before work has fully completed), or this is a race in journald - we can't read the event's journal message until a short time after it's been written (possibly because the event needs time to be written?). I'll investigate the first option, but I have a feeling it's the second. If it is, safest option might be some form of retry loop - say 5 attempts, half second between each, to give the journal time to settle? |
|
Retry loop sounds attractive, except for the how-to-implement-it part. It sounds really, really hard. |
|
Could just leave the 1 second sleep in there and revisit if it flakes? |
|
Ostrich solution WFM. |
Change only the compat API, so we don't force a breaking change on Libpod API users. Partial fix for containers#15485 Signed-off-by: Matthew Heon <matthew.heon@pm.me>
mheon
force-pushed
the
image_delete_event
branch
from
ba4042a
to
c7fda06
Compare
|
Updated comment to note the offending sleep for future fixes (if necessary). @containers/podman-maintainers PTAL, this should go green |
github-actions
bot
added
the
locked - please file new issue/PR
Change only the compat API, so we don't force a breaking change on Libpod API users.
Partial fix for #15485