[WIP] implement pod exec v5#2486
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aojea The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
kind/api-change
k8s-ci-robot
added
kind/deprecation
|
Welcome @aojea! |
k8s-ci-robot
added
the
size/L
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
k8s-ci-robot
removed
the
lifecycle/rotten
|
/assign |
|
/lifecycle frozen |
|
@seans3: The DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
seans3
left a comment
seans3
left a comment
There was a problem hiding this comment.
This looks good and substantially complete.
One high-level observation: I believe falling back to v4.channel.k8s.io is the right approach for backwards compatibility, but we should probably note that for clusters < v1.30, the client will silently revert to the broken behavior and hang on EOF.
I've left a few minor inline comments mostly about tests and constants.
| del self._channels[channel] | ||
| return ret | ||
|
|
||
| if channel in self._closed_channels: |
There was a problem hiding this comment.
Do we need this same short-circuiting code in peek_channel and read_channel ?
| """Close a channel (v5 protocol only).""" | ||
| if self.subprotocol != V5_CHANNEL_PROTOCOL: | ||
| return | ||
| data = bytes([255, channel]) |
There was a problem hiding this comment.
Should we define a constant (e.g. CLOSE_CHANNEL = 255 or V5_HALF_CLOSE = 255) for this magic number, and reference the constant?
|
|
||
| mock_ws.send.assert_not_called() | ||
|
|
||
| def test_update_receives_close_v5(self): |
There was a problem hiding this comment.
Should we add an additional unit test to verify how readline_channel handles a closed channel with leftover data?
While the current unit tests cover the parsing of the close signal itself, the new logic you added inside readline_channel—which flushes the remaining buffer even if it lacks a newline—is currently untested. Having a test that asserts readline_channel successfully flushes leftover data (e.g. "hello") and then returns an empty string on the subsequent call would ensure this specific edge-case logic is protected from future regressions.
| # Wait for process to exit | ||
| resp.update(timeout=5) | ||
| start = time.time() | ||
| while resp.is_open() and time.time() - start < 10: |
There was a problem hiding this comment.
Should we simplify this polling loop by using resp.run_forever(timeout=15) instead of manually checking resp.is_open()? This would align it closely with how the other exec tests in this file wait for streams to close.
|
|
||
| # Setup frame with close signal for channel 0 | ||
| frame = MagicMock() | ||
| frame.data = b'\xff\x00' |
There was a problem hiding this comment.
Would this be better as constants, such as frame.data = bytes([CLOSE_CHANNEL, STDIN_CHANNEL]) or whatever the 255 constant is defined as. I believe other places that we see b'\xff\x00' would be more readable with the constants.
|
/assign @yliaog |
5 participants
/kind feature
/kind api-change
/kind deprecation
cc: @yliaog @siyuanfoundation