Send HTTP Hijack headers after successful attach #7451
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-robot
added
the
approved
|
@baude @edsantiago Bear witness to my sorrow. I think this probably fixes the #7195 flake but I have not performed anything more than trivial tests yet. |
mheon
force-pushed
the
fix_7195
branch
2 times, most recently
from
d839aa9 to
b67ee9d
Compare
|
This appears to have made things worse. My interruption of their delicate dance has offended the attach functions and they will work no longer. I will attempt to appease them tomorrow. |
mheon
force-pushed
the
fix_7195
branch
2 times, most recently
from
d662739 to
199158d
Compare
|
I think this is going to go green now. |
|
Apparently I broke exec exit codes here - it looks like a race where it's querying for exec status before the exec session is marked as exited. |
|
Alright, I know what's going on (connection is being closed earlier than when it used to be closed, thus causing the client to query for exec session status a lot earlier than it used to, before we have time to save it to disk). Still need to figure out how to actually fix it, the code is not structured in a way that is conducive to closing the connection later. |
mheon
force-pushed
the
fix_7195
branch
2 times, most recently
from
6db6b01 to
7d65478
Compare
|
Second time I've seen this... |
|
Flakes in our flake fixes... |
|
At least that one usually passes on rerun |
|
Nevermind, exec tests look red still. Damn it. Will look more after lunch. |
|
Re-pushed again. I think this might be it. |
Our previous flow was to perform a hijack before passing a connection into Libpod, and then Libpod would attach to the container's attach socket and begin forwarding traffic. A problem emerges: we write the attach header as soon as the attach complete. As soon as we write the header, the client assumes that all is ready, and sends a Start request. This Start may be processed *before* we successfully finish attaching, causing us to lose output. The solution is to handle hijacking inside Libpod. Unfortunately, this requires a downright extensive refactor of the Attach and HTTP Exec StartAndAttach code. I think the result is an improvement in some places (a lot more errors will be handled with a proper HTTP error code, before the hijack occurs) but other parts, like the relocation of printing container logs, are just *bad*. Still, we need this fixed now to get CI back into good shape... Fixes containers#7195 Signed-off-by: Matthew Heon <[email protected]>
|
Network flake in the docs job |
|
@rhatdan @baude @giuseppe @TomSweeneyRedHat @QiWang19 PTAL and merge, please. Fixes the flake keeping CI red. |
|
LGTM |
1 similar comment
|
LGTM |
|
Looks green to me. I am supremely unqualified to review the code, but the results talk to me and I'm about to take all my PRs, rebase them, and see how they go. So... /lgtm Thank you @mheon. This was a nasty one. |
|
Oh mergebot... yoo-hoo... |
- pause test: enable when rootless + cgroups v2 (was previously disabled for all rootless) - run --pull: now works with podman-remote (in containers#7647, thank you @jwhonce) - various other run/volumes tests: try reenabling It looks like containers#7195 was fixed (by containers#7451? I'm not sure if I'm reading the conversation correctly). Anyway, remove all the skip()s on 7195. Only time will tell if it's really fixed) Also: - new test for podman image tree --whatrequires (because TIL). Doesn't work with podman-remote. Signed-off-by: Ed Santiago <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
Our previous flow was to perform a hijack before passing a connection into Libpod, and then Libpod would attach to the container's attach socket and begin forwarding traffic.
A problem emerges: we write the attach header as soon as the attach complete. As soon as we write the header, the client assumes that all is ready, and sends a Start request. This Start may be processed before we successfully finish attaching, causing us to lose output.
The solution is to handle hijacking inside Libpod. Unfortunately, this requires a downright extensive refactor of the Attach and HTTP Exec StartAndAttach code. I think the result is an improvement in some places (a lot more errors will be handled with a proper HTTP error code, before the hijack occurs) but other parts, like the relocation of printing container logs, are just bad. Still, we need this fixed now to get CI back into good shape...
Fixes #7195