Fail earlier when no containers exist in stats #22707
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
bot
added
the
approved
|
@edsantiago PTAL |
|
LGTM! Thank you! |
|
Fixes: #22612 |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Cockpit tests failed for commit 5cf82c6. @martinpitt, @jelly, @mvollmer please check. |
test/e2e/stats_test.go
Outdated
| @@ -25,10 +25,6 @@ var _ = Describe("Podman stats", func() { | |||
| session := podmanTest.Podman([]string{"stats", "--no-stream", "123"}) | |||
| session.WaitWithDefaultTimeout() | |||
| expect := `unable to get list of containers: unable to look up container 123: no container with name or ID "123" found: no such container` | |||
There was a problem hiding this comment.
As long as you have to repush, though, might as well move the string below into ExitWithError(), and get rid of the temporary variable...
There was a problem hiding this comment.
This is changing the behavior, if you get the list of containers before the loop it will not update on each inteerval which means newly created contianer will not be added later which is wrong.
The proper way to fix is doing something like this for stats as well. #22691
rhatdan
force-pushed
the
stats
branch
2 times, most recently
from
026052f
to
0851695
Compare
github-actions
bot
added
the
kind/api-change
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Cockpit tests failed for commit 026052f. @martinpitt, @jelly, @mvollmer please check. |
|
Cockpit tests failed for commit 0851695. @martinpitt, @jelly, @mvollmer please check. |
rhatdan
force-pushed
the
stats
branch
2 times, most recently
from
23d9c2b
to
8e7cc2e
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Cockpit tests failed for commit 23d9c2b. @martinpitt, @jelly, @mvollmer please check. |
|
Cockpit tests failed for commit 8e7cc2e. @martinpitt, @jelly, @mvollmer please check. |
|
The ELN copr environment fixes should land today/tomorrow so ignore failures for now. |
|
@martinpitt the cockpit tests on rawhide have been failing consistently recently. PTAL. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
Hey, sorry we had some issues with the new systemd release landing in rawhide. The tests should run now. |
| if !wroteContent { | ||
| // Write header and content type. | ||
| w.WriteHeader(http.StatusOK) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| if flusher, ok := w.(http.Flusher); ok { | ||
| flusher.Flush() | ||
| } | ||
| wroteContent = true | ||
| } |
There was a problem hiding this comment.
this would need happen before the coder.encode call as headers/status code must be written before content.
and you can remove the flush here as it flushes below anyway.
rhatdan
force-pushed
the
stats
branch
3 times, most recently
from
6965b8c
to
1e1df0f
Compare
|
@Luap99 PTAL |
test/apiv2/20-containers.at
Outdated
| t GET libpod/containers/$cid/stats?stream=false 200 | ||
| else | ||
| if have_cgroupsv2; then | ||
| t GET libpod/containers/stats?containers='[$cid]' 200 | ||
| t GET libpod/containers/$cid/stats?stream=false 200 | ||
| else | ||
| t GET libpod/containers/stats?containers='[$cid]' 409 | ||
| t GET libpod/containers/$cid/stats?stream=false 409 |
There was a problem hiding this comment.
These are two different endpoints, I would suggest you keep testing the endpoint you actually fixed and juts fix the tests instead.
The correct URl param would be ?containers=$cid
And then add some basic output checks as well.
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Does this PR introduce a user-facing change?