OSAC-1593: add console proxy e2e tests#71
Conversation
|
@sk-ilya: This pull request references OSAC-420 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sk-ilya 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 |
WalkthroughAdds ChangesSerial Console Session Test Suite
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant GRPCClient
participant ConsoleAPI as Console API (gRPC)
participant WSProxy as WebSocket Proxy
rect rgba(70, 130, 180, 0.5)
note over Test,WSProxy: Create console session and connect
Test->>GRPCClient: create_console_session(resource_type, resource_id, console_type)
GRPCClient->>ConsoleAPI: ConsoleSessions/Create
ConsoleAPI-->>GRPCClient: ticket, expiresAt
GRPCClient-->>Test: session dict
Test->>WSProxy: _ws_connect(url, ticket)
WSProxy-->>Test: WebSocket (binary subprotocol, initial output)
end
rect rgba(200, 80, 80, 0.5)
note over Test,WSProxy: Ticket enforcement and rejection
Test->>WSProxy: _ws_try_connect(url, same_ticket) x5
WSProxy-->>Test: true (1st), false (2nd–5th)
Test->>WSProxy: _ws_try_connect(url, expired_ticket)
WSProxy-->>Test: false (WebSocketException)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyproject.toml`:
- Line 9: Update the websocket-client dependency specification in pyproject.toml
from the floating version constraint (>=1.6) to an exact version pin (==1.9.0)
to match the version already pinned in the uv.lock file and align with
supply-chain best practices for explicit version pinning.
In `@tests/vmaas/test_console.py`:
- Around line 41-46: The test fixture setup code contains calls to
wait_for_provision and wait_for_running (lines 44-45) that can raise exceptions,
and if they do, the cleanup code at lines 50-56 is never executed, causing VM
resource leaks. Wrap the entire setup sequence from line 41 through line 48 in a
try-finally block, keeping the setup code in the try block and moving the
cleanup code to the finally block to guarantee cleanup runs regardless of
whether the setup succeeds or fails.
- Around line 264-271: The issue is that the min(60.0, ...) cap on the
wait_seconds calculation in the _ws_try_connect test can cause the test to
attempt connection before the ticket actually expires. When seconds_until_expiry
is greater than approximately 45 seconds, the 60-second cap prevents waiting the
full (expiry_time + 15_second_buffer), causing the connection attempt at line
271 to occur while the ticket is still valid rather than after it has expired.
Remove the min(60.0, ...) cap to ensure wait_seconds always equals
seconds_until_expiry + 15.0, guaranteeing the ticket has expired before the
assertion in line 271 is evaluated.
- Around line 75-83: The _ws_try_connect function contains a ws.recv() call that
waits for the first WebSocket frame, which can timeout on legitimate but silent
connections and produce false negatives. Since a successful return from
_ws_connect() already confirms the WebSocket handshake has succeeded, remove the
ws.recv() line entirely and keep only the ws.close() call before returning True.
This eliminates unnecessary waiting on frame reception and prevents false
negatives for valid ticket enforcement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: dce0293f-d520-4dba-a97c-4d00b2cb52ed
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomltests/core/grpc_client.pytests/vmaas/test_console.py
|
/retest-required |
Assisted-by: Claude <noreply@anthropic.com>
sk-ilya
changed the title
|
@sk-ilya: This pull request references OSAC-1593 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
1 similar comment
|
/retest-required |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/vmaas/test_console.py`:
- Around line 96-114: The proc.communicate(timeout=30) call can raise
subprocess.TimeoutExpired, which leaves the grpcurl process running and causes
process leaks. Wrap the proc.communicate(timeout=30) call in a try-except block
that catches subprocess.TimeoutExpired, and when caught, terminate the process
by calling proc.kill() or proc.terminate() before handling the exception
appropriately to ensure child processes are cleaned up and don't accumulate as
orphans.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b635959a-5c2d-48bf-90d5-2b9b09a23e02
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomltests/core/grpc_client.pytests/vmaas/test_console.py
| proc = subprocess.Popen( | ||
| ["grpcurl", "-insecure", "-H", f"Authorization: Bearer {ticket}", "-d", "@", address, CONSOLE_GRPC_SERVICE], | ||
| stdin=subprocess.PIPE, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| ) | ||
| time.sleep(wait) | ||
|
|
||
| if input_data is not None: | ||
| encoded: str = base64.b64encode(input_data).decode() | ||
| msg = json.dumps({"input": {"data": encoded}}) + "\n" | ||
| proc.stdin.write(msg.encode()) # type: ignore[union-attr] | ||
| proc.stdin.flush() # type: ignore[union-attr] | ||
| time.sleep(2) | ||
|
|
||
| proc.stdin.close() # type: ignore[union-attr] | ||
| proc.stdin = None # prevent communicate() from flushing closed stdin | ||
| stdout, stderr = proc.communicate(timeout=30) | ||
| return stdout.decode(), stderr.decode(), proc.returncode |
There was a problem hiding this comment.
Terminate grpcurl on communicate() timeout to prevent process leaks.
Line 113 can raise subprocess.TimeoutExpired; currently that leaves the child process running. In repeated CI runs, this can accumulate orphaned grpcurl processes and destabilize the suite.
Suggested fix
def _grpc_stream(
address: str, ticket: str, *, wait: float = 5, input_data: bytes | None = None
) -> tuple[str, str, int]:
@@
- proc.stdin.close() # type: ignore[union-attr]
- proc.stdin = None # prevent communicate() from flushing closed stdin
- stdout, stderr = proc.communicate(timeout=30)
- return stdout.decode(), stderr.decode(), proc.returncode
+ proc.stdin.close() # type: ignore[union-attr]
+ proc.stdin = None # prevent communicate() from flushing closed stdin
+ try:
+ stdout, stderr = proc.communicate(timeout=30)
+ except subprocess.TimeoutExpired:
+ proc.kill()
+ stdout, stderr = proc.communicate()
+ raise
+ return stdout.decode(), stderr.decode(), proc.returncode🧰 Tools
🪛 ast-grep (0.43.0)
[info] 105-105: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"input": {"data": encoded}})
Note: Security best practice.
(use-jsonify)
🪛 Ruff (0.15.17)
[error] 96-96: subprocess call: check for execution of untrusted input
(S603)
[error] 97-97: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/vmaas/test_console.py` around lines 96 - 114, The
proc.communicate(timeout=30) call can raise subprocess.TimeoutExpired, which
leaves the grpcurl process running and causes process leaks. Wrap the
proc.communicate(timeout=30) call in a try-except block that catches
subprocess.TimeoutExpired, and when caught, terminate the process by calling
proc.kill() or proc.terminate() before handling the exception appropriately to
ensure child processes are cleaned up and don't accumulate as orphans.
Add pytest-xdist dependency and configure addopts to run test files in parallel. Each file gets its own worker (--dist loadfile), preserving module/class fixture scoping while reducing wall clock from ~63 min to ~15 min. Assisted-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyproject.toml`:
- Line 19: The pytest addopts configuration in pyproject.toml hardcodes the
worker count to 4 with `-n 4`, which assumes all test environments have at least
4 CPU cores and reduces portability. Replace `-n 4` with `-n auto` in the
addopts line to dynamically detect the CPU count at runtime, making the
configuration more adaptable across different environments with varying core
counts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: df74a789-49d2-4a47-ab10-1354a1ab0a11
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
pyproject.toml
| [tool.pytest.ini_options] | ||
| testpaths = ["tests"] | ||
| pythonpath = ["."] | ||
| addopts = "-n 4 --dist loadfile" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider -n auto for better portability across environments.
Hardcoding -n 4 assumes all test environments have at least 4 cores. The -n auto option dynamically detects CPU count and is more portable. If the fixed count is intentional for your specific CI environment, consider documenting the rationale.
⚡ Suggested change for dynamic worker count
-addopts = "-n 4 --dist loadfile"
+addopts = "-n auto --dist loadfile"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addopts = "-n 4 --dist loadfile" | |
| addopts = "-n auto --dist loadfile" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` at line 19, The pytest addopts configuration in
pyproject.toml hardcodes the worker count to 4 with `-n 4`, which assumes all
test environments have at least 4 CPU cores and reduces portability. Replace `-n
4` with `-n auto` in the addopts line to dynamically detect the CPU count at
runtime, making the configuration more adaptable across different environments
with varying core counts.
|
@sk-ilya: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
2 participants
Add 9 tests covering both WebSocket and gRPC stream transports for the serial console feature. A module-scoped fixture provisions a single VM shared across all tests.
Tests cover: bidirectional data flow, 90-second idle survival, single-use ticket enforcement (JTI), expired ticket rejection, concurrent session rejection, non-existent VM, and invalid ticket handling.
Summary by CodeRabbit
Tests
Chores / Configuration
websocket-clientandpytest-xdistto runtime/test dependencies.