Conversation
yrobla
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
jerm-dro and
jhrozek
as code owners
There was a problem hiding this comment.
Pull request overview
Adds higher-level integration and end-to-end coverage for Redis-backed vMCP session sharing, focusing on cross-pod session restoration and related lifecycle behaviors.
Changes:
- Added Go integration tests for
sessionmanagerusingminiredis+ in-process MCP backends to validate restore, hijack prevention, backend expiry metadata, and in-memory-only behavior. - Added operator E2E tests (Kind + real Redis) to validate VirtualMCPServer readiness/conditions with Redis and to exercise cross-pod session reuse via
transport.WithSession(sessionID).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
pkg/vmcp/server/sessionmanager/horizontal_scaling_integration_test.go |
New integration tests covering cross-pod restore, hijack prevention, restore failure behavior, backend expiry metadata, and no-Redis isolation. |
test/e2e/thv-operator/virtualmcp/virtualmcp_redis_session_test.go |
New E2E coverage for VirtualMCPServer replicas=2 with Redis: condition assertions and cross-pod session reconstruction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/vmcp/server/sessionmanager/horizontal_scaling_integration_test.go
Outdated
Show resolved
Hide resolved
pkg/vmcp/server/sessionmanager/horizontal_scaling_integration_test.go
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4724 +/- ##
==========================================
+ Coverage 68.60% 68.76% +0.16%
==========================================
Files 516 517 +1
Lines 53841 54817 +976
==========================================
+ Hits 36937 37695 +758
- Misses 14057 14228 +171
- Partials 2847 2894 +47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Add integration and e2e tests for Redis-backed vMCP session sharing
Integration tests (pkg/vmcp/server/sessionmanager, miniredis):
- AC1: cross-pod session reconstruction via RestoreSession on cache miss
- AC2: hijack prevention — wrong/nil callers rejected after restore,
correct caller routes successfully
- AC4: all backends unreachable on restore → empty routing table returned
- AC5: NotifyBackendExpired removes backend from Redis metadata;
subsequent restore on a fresh pod skips the expired backend
- AC6: LocalSessionDataStorage (no Redis) → no cross-pod sharing
E2E tests (test/e2e/thv-operator/virtualmcp, Kind + real Redis):
- replicas=2 + Redis → SessionStorageWarning=False and Ready=True
- cross-pod session reconstruction: session initialized on pod A is
usable on pod B via transport.WithSession(sessionID)
Note: AC3 (LRU eviction, RC-10) is intentionally excluded — TTL-based
Redis eviction is sufficient.
Closes #4222
github-actions
bot
added
size/L
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func countReadyPods(vmcpName string) (int, error) { | ||
| podList, err := GetVirtualMCPServerPods(ctx, k8sClient, vmcpName, "default") | ||
| if err != nil { |
There was a problem hiding this comment.
countReadyPods now hardcodes the namespace as "default". This duplicates defaultNamespace used elsewhere in the test and makes the helper silently wrong if the suite is ever run in a non-default namespace. Consider reintroducing the namespace parameter (and pass defaultNamespace at call sites), or promote the namespace constant to package scope so the helper can reference it without a magic string.
| @@ -155,11 +155,11 @@ func portForwardToPod(podName, namespace string, targetPort int32) (int, func(), | |||
| _ = listener.Close() | |||
|
|
|||
| kubeconfigArg := fmt.Sprintf("--kubeconfig=%s", kubeconfig) | |||
| //nolint:gosec // kubeconfig, namespace, podName, and ports are test-controlled values | |||
| //nolint:gosec // kubeconfig, podName, and ports are test-controlled values | |||
| cmd := exec.Command("kubectl", kubeconfigArg, | |||
| "-n", namespace, "port-forward", | |||
| "-n", "default", "port-forward", | |||
| fmt.Sprintf("pod/%s", podName), | |||
| fmt.Sprintf("%d:%d", localPort, targetPort)) | |||
| fmt.Sprintf("%d:%d", localPort, 8080)) | |||
There was a problem hiding this comment.
portForwardToPod now hardcodes both the namespace ("default") and the forwarded target port (8080). This introduces magic constants and can easily drift from the defaultNamespace/proxyPort values used elsewhere in this file (e.g., if the test is adjusted to run in another namespace or the proxy port changes). Consider accepting namespace and targetPort as parameters again, or centralizing these values as package-level constants reused by both the MCPServer and VirtualMCPServer tests.
jerm-dro
left a comment
jerm-dro
left a comment
There was a problem hiding this comment.
LGTM, thanks for writing!
Summary
Add integration and e2e tests for Redis-backed vMCP session sharing
Integration tests (pkg/vmcp/server/sessionmanager, miniredis):
E2E tests (test/e2e/thv-operator/virtualmcp, Kind + real Redis):
Note: AC3 (LRU eviction, RC-10) is intentionally excluded — TTL-based Redis eviction is sufficient.
Fixes #4222
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Does this introduce a user-facing change?
Special notes for reviewers