diff --git a/README.md b/README.md index 7901e7f..f609f98 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ Designed to run against a Kubernetes (or OpenShift) cluster — the server manag - **Per-session agent auth** — MCP server → sandbox `/exec` uses an HMAC-derived bearer token (defense in depth beyond NetworkPolicy) - **Customizable CLIs** — available tools are whatever is in the sandbox agent image (the default image includes `oc`/`kubectl`) - **Stateless, multi-replica ready** — any server replica can handle any request; Kubernetes is the source of truth -- **Optional warm pool** — pre-warmed pods cut cold-start latency when enabled +- **Claim then create** — MCP claims an instance-labeled unassigned pod if one exists, otherwise creates on demand ## Usage @@ -51,7 +51,7 @@ Typical client practice (agent harness / orchestrator — not the LLM): 2. Send that same ID on every follow-up `bash` call to reuse the persistent shell and `/workspace` 3. Call `DELETE /sessions/{id}` when finished (HTTP transport only) — removes the sandbox pod, auth secret, and cache entry -If the client never deletes the session, the sandbox is garbage-collected after `--idle-timeout` (default 30m). +If the client never deletes the session, the sandbox pod remains until something else deletes it (operator idle GC, or a manual `DELETE`). ## Configuration @@ -62,12 +62,21 @@ If the client never deletes the session, the sandbox is garbage-collected after | `--transport` | `stdio` | `stdio` or `http` (`http` requires `--stateless`) | | `--address` | `localhost:8080` | Listen address (HTTP; must be loopback) | | `--stateless` | `false` | Required for HTTP / multi-replica | -| `--namespace` | `tarsy` | Namespace for sandbox pods | +| `--namespace` | _(required)_ | Namespace for sandbox pods | +| `--instance-name` | _(required)_ | Instance id on sandbox labels (`cli-mcp.redhat.com/instance`) | | `--sandbox-image` | _(required)_ | Container image for sandbox pods | | `--hmac-key-file` | _(required)_ | Path to shared HMAC secret | -| `--kubeconfig` | _(in-cluster)_ | Kubeconfig for managing sandbox pods | -| `--idle-timeout` | `30m` | Delete idle sandbox pods after this duration | -| `--warm-pool-size` | `0` | Pre-warmed pods (`0` = create on demand) | +| `--kubeconfig-secret` | _(required)_ | Investigation kubeconfig Secret mounted into sandbox pods | +| `--sandbox-service-account` | _(required)_ | ServiceAccount name for sandbox pods | +| `--kubeconfig` | _(in-cluster)_ | Kubeconfig for the MCP process's Kubernetes client (not the investigation Secret) | +| `--sandbox-cpu-request` | `100m` | Sandbox CPU request | +| `--sandbox-cpu-limit` | `500m` | Sandbox CPU limit | +| `--sandbox-memory-request` | `128Mi` | Sandbox memory request | +| `--sandbox-memory-limit` | `512Mi` | Sandbox memory limit | +| `--sandbox-image-pull-policy` | _(empty)_ | Sandbox `imagePullPolicy` (`Always`, `Never`, `IfNotPresent`) | +| `--sandbox-env` | _(empty)_ | JSON `[]corev1.EnvVar` overlay for sandbox pods | +| `--idle-timeout` | `30m` | Parsed for CLI compatibility; MCP does not idle-GC | +| `--warm-pool-size` | `0` | Parsed for CLI compatibility; MCP does not replenish a pool | ### Sandbox image (available CLIs) @@ -89,7 +98,7 @@ No MCP server code changes are required. Tell the LLM what is available via your flowchart TB Client[MCP Client] Server["cli-mcp-server
stateless · N replicas"] - Sandbox["Sandbox pods
assigned sessions · optional warm pool"] + Sandbox["Sandbox pods
assigned sessions · claim or create"] Target["Target infrastructure
e.g. Kubernetes API"] Client -->|"bash + X-Session-ID"| Server @@ -103,14 +112,14 @@ Bash commands run in the sandbox pods. What they can reach (for example a Kubern The MCP server holds no durable session state. Pod identity is stored in Kubernetes labels; an in-memory cache speeds up routing. Any replica can serve any request, so you can scale the Deployment horizontally behind a load balancer with no sticky sessions. -Optional `--warm-pool-size` keeps ready pods on hand so new sessions skip cold start (image pull + container boot). +MCP always claims an instance-labeled unassigned pod if one exists, otherwise creates on demand. Pool replenishment and idle GC are owned by the operator, not this process. ### Sandboxing and security Each session gets its own pod. That pod is the security boundary: - **Isolation** — non-root (runAsNonRoot), no privilege escalation, all capabilities dropped, resource limits -- **Credentials** — read-only kubeconfig mounted from a dedicated investigation ServiceAccount (typically view/read-only RBAC) +- **Credentials** — investigation kubeconfig Secret mounted into the sandbox; dedicated sandbox SA with `automountServiceAccountToken: false` - **Network** — NetworkPolicy can restrict ingress to the MCP server and egress to intended APIs - **Agent auth** — per-session HMAC bearer token; unauthenticated `/exec` calls are rejected - **Ephemeral workspace** — `/workspace` is an `emptyDir`; destroyed with the pod @@ -139,7 +148,7 @@ make build-prod # Production build (static, CGO disabled) cli-mcp-server/ ├── cmd/server/ # MCP server entry point ├── cmd/agent/ # Sandbox agent entry point -├── pkg/session/ # Pod lifecycle, warm pool, cache +├── pkg/session/ # Pod lifecycle, claim, cache ├── pkg/sandbox/ # Bash session + agent HTTP handlers ├── pkg/tools/ # MCP tool handlers ├── pkg/server/ # MCP server + HTTP mux diff --git a/cmd/server/flags.go b/cmd/server/flags.go new file mode 100644 index 0000000..0deac93 --- /dev/null +++ b/cmd/server/flags.go @@ -0,0 +1,101 @@ +package main + +import ( + "encoding/json" + "fmt" + "strings" + + "github.com/codeready-toolchain/cli-mcp-operator/pkg/session" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +func buildSandboxConfig(cfg runConfig, hmacKey string) (session.SandboxConfig, error) { + if cfg.sandboxImage == "" { + return session.SandboxConfig{}, fmt.Errorf("--sandbox-image is required") + } + if cfg.namespace == "" { + return session.SandboxConfig{}, fmt.Errorf("--namespace is required") + } + if cfg.instanceName == "" { + return session.SandboxConfig{}, fmt.Errorf("--instance-name is required") + } + if cfg.kubeconfigSecret == "" { + return session.SandboxConfig{}, fmt.Errorf("--kubeconfig-secret is required") + } + if cfg.sandboxServiceAccount == "" { + return session.SandboxConfig{}, fmt.Errorf("--sandbox-service-account is required") + } + + defaults := session.DefaultConfig() + cpuRequest, err := parseQuantityFlag("--sandbox-cpu-request", cfg.cpuRequest, defaults.CPURequest) + if err != nil { + return session.SandboxConfig{}, err + } + cpuLimit, err := parseQuantityFlag("--sandbox-cpu-limit", cfg.cpuLimit, defaults.CPULimit) + if err != nil { + return session.SandboxConfig{}, err + } + memoryRequest, err := parseQuantityFlag("--sandbox-memory-request", cfg.memoryRequest, defaults.MemoryRequest) + if err != nil { + return session.SandboxConfig{}, err + } + memoryLimit, err := parseQuantityFlag("--sandbox-memory-limit", cfg.memoryLimit, defaults.MemoryLimit) + if err != nil { + return session.SandboxConfig{}, err + } + pullPolicy, err := parseImagePullPolicy(cfg.imagePullPolicy) + if err != nil { + return session.SandboxConfig{}, err + } + env, err := parseSandboxEnv(cfg.sandboxEnv) + if err != nil { + return session.SandboxConfig{}, err + } + + return session.SandboxConfig{ + Image: cfg.sandboxImage, + HMACKey: hmacKey, + Namespace: cfg.namespace, + InstanceName: cfg.instanceName, + ServiceAccountName: cfg.sandboxServiceAccount, + KubeconfigSecret: cfg.kubeconfigSecret, + CPURequest: cpuRequest, + CPULimit: cpuLimit, + MemoryRequest: memoryRequest, + MemoryLimit: memoryLimit, + ImagePullPolicy: pullPolicy, + Env: env, + AgentPort: defaults.AgentPort, + }, nil +} + +func parseQuantityFlag(flag, value, fallback string) (string, error) { + if value == "" { + value = fallback + } + if _, err := resource.ParseQuantity(value); err != nil { + return "", fmt.Errorf("%s: invalid quantity %q: %w", flag, value, err) + } + return value, nil +} + +func parseImagePullPolicy(s string) (corev1.PullPolicy, error) { + switch corev1.PullPolicy(s) { + case "", corev1.PullAlways, corev1.PullNever, corev1.PullIfNotPresent: + return corev1.PullPolicy(s), nil + default: + return "", fmt.Errorf("--sandbox-image-pull-policy must be Always, Never, or IfNotPresent") + } +} + +func parseSandboxEnv(raw string) ([]corev1.EnvVar, error) { + if strings.TrimSpace(raw) == "" { + return nil, nil + } + var env []corev1.EnvVar + if err := json.Unmarshal([]byte(raw), &env); err != nil { + return nil, fmt.Errorf("--sandbox-env must be JSON []corev1.EnvVar: %w", err) + } + return env, nil +} diff --git a/cmd/server/flags_test.go b/cmd/server/flags_test.go new file mode 100644 index 0000000..470c6fb --- /dev/null +++ b/cmd/server/flags_test.go @@ -0,0 +1,148 @@ +package main + +import ( + "testing" + + "github.com/codeready-toolchain/cli-mcp-operator/pkg/session" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" +) + +func validRunConfig() runConfig { + return runConfig{ //nolint:gosec // G101: K8s Secret resource name, not a credential + sandboxImage: "quay.io/example/sandbox:test", + namespace: "cli-mcp", + instanceName: "oc", + kubeconfigSecret: "cli-mcp-oc-kubeconfig", + sandboxServiceAccount: "cli-mcp-oc-sandbox", + } +} + +func TestBuildSandboxConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + mutate func(*runConfig) + wantErr string + check func(*testing.T, session.SandboxConfig) + }{ + { + name: "requires instance-name", + mutate: func(c *runConfig) { + c.instanceName = "" + }, + wantErr: "--instance-name is required", + }, + { + name: "requires kubeconfig-secret", + mutate: func(c *runConfig) { + c.kubeconfigSecret = "" + }, + wantErr: "--kubeconfig-secret is required", + }, + { + name: "requires sandbox-service-account", + mutate: func(c *runConfig) { + c.sandboxServiceAccount = "" + }, + wantErr: "--sandbox-service-account is required", + }, + { + name: "requires namespace", + mutate: func(c *runConfig) { + c.namespace = "" + }, + wantErr: "--namespace is required", + }, + { + name: "requires sandbox-image", + mutate: func(c *runConfig) { + c.sandboxImage = "" + }, + wantErr: "--sandbox-image is required", + }, + { + name: "uses DefaultConfig resources when quantity flags are empty", + check: func(t *testing.T, cfg session.SandboxConfig) { + t.Helper() + defaults := session.DefaultConfig() + assert.Equal(t, defaults.CPURequest, cfg.CPURequest) + assert.Equal(t, defaults.CPULimit, cfg.CPULimit) + assert.Equal(t, defaults.MemoryRequest, cfg.MemoryRequest) + assert.Equal(t, defaults.MemoryLimit, cfg.MemoryLimit) + assert.Empty(t, cfg.ImagePullPolicy) + assert.Empty(t, cfg.Env) + }, + }, + { + name: "parses overlay flags", + mutate: func(c *runConfig) { + c.cpuRequest = "200m" + c.cpuLimit = "1" + c.memoryRequest = "256Mi" + c.memoryLimit = "1Gi" + c.imagePullPolicy = "IfNotPresent" + c.sandboxEnv = `[{"name":"AWS_REGION","value":"us-east-1"}]` + }, + check: func(t *testing.T, cfg session.SandboxConfig) { + t.Helper() + assert.Equal(t, "200m", cfg.CPURequest) + assert.Equal(t, "1", cfg.CPULimit) + assert.Equal(t, "256Mi", cfg.MemoryRequest) + assert.Equal(t, "1Gi", cfg.MemoryLimit) + assert.Equal(t, corev1.PullIfNotPresent, cfg.ImagePullPolicy) + require.Len(t, cfg.Env, 1) + assert.Equal(t, "AWS_REGION", cfg.Env[0].Name) + assert.Equal(t, "us-east-1", cfg.Env[0].Value) + }, + }, + { + name: "rejects invalid pull policy", + mutate: func(c *runConfig) { + c.imagePullPolicy = "Sometimes" + }, + wantErr: "--sandbox-image-pull-policy", + }, + { + name: "rejects invalid quantity", + mutate: func(c *runConfig) { + c.cpuRequest = "not-a-quantity" + }, + wantErr: "--sandbox-cpu-request", + }, + { + name: "rejects invalid sandbox-env JSON", + mutate: func(c *runConfig) { + c.sandboxEnv = "{not json" + }, + wantErr: "--sandbox-env", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + cfg := validRunConfig() + if tt.mutate != nil { + tt.mutate(&cfg) + } + + got, err := buildSandboxConfig(cfg, "hmac") + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, "oc", got.InstanceName) + assert.Equal(t, "cli-mcp", got.Namespace) + assert.Equal(t, "cli-mcp-oc-sandbox", got.ServiceAccountName) + assert.Equal(t, "cli-mcp-oc-kubeconfig", got.KubeconfigSecret) + if tt.check != nil { + tt.check(t, got) + } + }) + } +} diff --git a/cmd/server/health_test.go b/cmd/server/health_test.go new file mode 100644 index 0000000..21d1ea3 --- /dev/null +++ b/cmd/server/health_test.go @@ -0,0 +1,23 @@ +package main + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" +) + +func TestK8sHealthCheckerListsPodsInNamespace(t *testing.T) { + t.Parallel() + + client := fake.NewSimpleClientset() + client.PrependReactor("get", "namespaces", func(_ k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, fmt.Errorf("cluster-scoped namespace get must not be used") + }) + checker := &k8sHealthChecker{clientset: client, namespace: "cli-mcp"} + + require.NoError(t, checker.CheckHealth(t.Context())) +} diff --git a/cmd/server/main.go b/cmd/server/main.go index 12057fb..fe0675c 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -28,15 +28,24 @@ func main() { fmt.Fprintf(os.Stderr, "cli-mcp-server %s (built %s)\n", version.Commit, version.BuildTime) var ( - address string - transport string - stateless bool - namespace string - sandboxImage string - kubeconfig string - hmacKeyFile string - idleTimeout time.Duration - warmPoolSize int + address string + transport string + stateless bool + namespace string + instanceName string + sandboxImage string + kubeconfig string + kubeconfigSecret string + sandboxServiceAccount string + hmacKeyFile string + cpuRequest string + cpuLimit string + memoryRequest string + memoryLimit string + imagePullPolicy string + sandboxEnv string + idleTimeout time.Duration + warmPoolSize int ) rootCmd := &cobra.Command{ @@ -44,15 +53,24 @@ func main() { Short: "Sandboxed exec environment MCP server for LLM investigation", RunE: func(_ *cobra.Command, _ []string) error { return runServer(runConfig{ - address: address, - transport: transport, - stateless: stateless, - namespace: namespace, - sandboxImage: sandboxImage, - kubeconfig: kubeconfig, - hmacKeyFile: hmacKeyFile, - idleTimeout: idleTimeout, - warmPoolSize: warmPoolSize, + address: address, + transport: transport, + stateless: stateless, + namespace: namespace, + instanceName: instanceName, + sandboxImage: sandboxImage, + kubeconfig: kubeconfig, + kubeconfigSecret: kubeconfigSecret, + sandboxServiceAccount: sandboxServiceAccount, + hmacKeyFile: hmacKeyFile, + cpuRequest: cpuRequest, + cpuLimit: cpuLimit, + memoryRequest: memoryRequest, + memoryLimit: memoryLimit, + imagePullPolicy: imagePullPolicy, + sandboxEnv: sandboxEnv, + idleTimeout: idleTimeout, + warmPoolSize: warmPoolSize, }) }, } @@ -60,12 +78,21 @@ func main() { rootCmd.Flags().StringVarP(&address, "address", "a", "localhost:8080", "Server address (host:port)") rootCmd.Flags().StringVarP(&transport, "transport", "t", "stdio", "Transport (stdio, http)") rootCmd.Flags().BoolVar(&stateless, "stateless", false, "Enable stateless mode (required for HTTP)") - rootCmd.Flags().StringVar(&namespace, "namespace", "tarsy", "Namespace for sandbox pods") + rootCmd.Flags().StringVar(&namespace, "namespace", "", "Namespace for sandbox pods (required)") + rootCmd.Flags().StringVar(&instanceName, "instance-name", "", "Instance id used on sandbox labels (required)") rootCmd.Flags().StringVar(&sandboxImage, "sandbox-image", "", "Container image for sandbox pods (required)") - rootCmd.Flags().StringVar(&kubeconfig, "kubeconfig", "", "Path to kubeconfig for sandbox pods") + rootCmd.Flags().StringVar(&kubeconfig, "kubeconfig", "", "Path to kubeconfig for the MCP process's Kubernetes client (empty = in-cluster); not the investigation Secret") + rootCmd.Flags().StringVar(&kubeconfigSecret, "kubeconfig-secret", "", "Name of the investigation kubeconfig Secret mounted into sandbox pods (required)") + rootCmd.Flags().StringVar(&sandboxServiceAccount, "sandbox-service-account", "", "ServiceAccount name for sandbox pods (required)") rootCmd.Flags().StringVar(&hmacKeyFile, "hmac-key-file", "", "Path to HMAC shared secret file (required)") - rootCmd.Flags().DurationVar(&idleTimeout, "idle-timeout", 30*time.Minute, "Idle timeout for sandbox pods") - rootCmd.Flags().IntVar(&warmPoolSize, "warm-pool-size", 0, "Pre-warmed sandbox pods (0 = disabled)") + rootCmd.Flags().StringVar(&cpuRequest, "sandbox-cpu-request", "", "Sandbox CPU request (empty = 100m)") + rootCmd.Flags().StringVar(&cpuLimit, "sandbox-cpu-limit", "", "Sandbox CPU limit (empty = 500m)") + rootCmd.Flags().StringVar(&memoryRequest, "sandbox-memory-request", "", "Sandbox memory request (empty = 128Mi)") + rootCmd.Flags().StringVar(&memoryLimit, "sandbox-memory-limit", "", "Sandbox memory limit (empty = 512Mi)") + rootCmd.Flags().StringVar(&imagePullPolicy, "sandbox-image-pull-policy", "", "Sandbox imagePullPolicy (Always, Never, IfNotPresent)") + rootCmd.Flags().StringVar(&sandboxEnv, "sandbox-env", "", "JSON []corev1.EnvVar overlay for sandbox pods") + rootCmd.Flags().DurationVar(&idleTimeout, "idle-timeout", 30*time.Minute, "Accepted for CLI compatibility; MCP does not idle-GC (operator owns that)") + rootCmd.Flags().IntVar(&warmPoolSize, "warm-pool-size", 0, "Accepted for CLI compatibility; MCP does not replenish a pool (operator owns that)") if err := rootCmd.Execute(); err != nil { os.Exit(1) @@ -73,24 +100,30 @@ func main() { } type runConfig struct { - address string - transport string - stateless bool - namespace string - sandboxImage string - kubeconfig string - hmacKeyFile string - idleTimeout time.Duration - warmPoolSize int + address string + transport string + stateless bool + namespace string + instanceName string + sandboxImage string + kubeconfig string + kubeconfigSecret string + sandboxServiceAccount string + hmacKeyFile string + cpuRequest string + cpuLimit string + memoryRequest string + memoryLimit string + imagePullPolicy string + sandboxEnv string + idleTimeout time.Duration + warmPoolSize int } func runServer(cfg runConfig) error { if err := server.ValidateTransportFlags(cfg.transport, cfg.stateless, cfg.address); err != nil { return err } - if cfg.sandboxImage == "" { - return fmt.Errorf("--sandbox-image is required") - } if cfg.idleTimeout <= 0 { return fmt.Errorf("--idle-timeout must be greater than zero") } @@ -101,6 +134,10 @@ func runServer(cfg runConfig) error { if err != nil { return err } + sandboxCfg, err := buildSandboxConfig(cfg, hmacKey) + if err != nil { + return err + } logger := slog.New(slog.NewJSONHandler(os.Stderr, &slog.HandlerOptions{ Level: slog.LevelInfo, @@ -112,13 +149,6 @@ func runServer(cfg runConfig) error { return fmt.Errorf("failed to create kubernetes client: %w", err) } - sandboxCfg := session.DefaultConfig() - sandboxCfg.Image = cfg.sandboxImage - sandboxCfg.HMACKey = hmacKey - sandboxCfg.Namespace = cfg.namespace - sandboxCfg.IdleTimeout = cfg.idleTimeout - sandboxCfg.WarmPoolSize = cfg.warmPoolSize - mgr, err := session.NewSessionManager(clientset, sandboxCfg, logger) if err != nil { return fmt.Errorf("failed to create session manager: %w", err) @@ -131,11 +161,6 @@ func runServer(cfg runConfig) error { ctx, cancel := signal.NotifyContext(context.Background(), syscall.SIGTERM, syscall.SIGINT) defer cancel() - startCleanupLoop(ctx, mgr, logger) - if cfg.warmPoolSize > 0 { - mgr.StartPool(ctx) - } - switch cfg.transport { case "http": return serveHTTP(ctx, cancel, cfg.address, cfg.stateless, mcpServer, mgr, clientset, cfg.namespace, logger) @@ -190,26 +215,6 @@ func serveStdio(ctx context.Context, mcpServer *mcp.Server, logger *slog.Logger) return nil } -func startCleanupLoop(ctx context.Context, mgr *session.SessionManager, logger *slog.Logger) { - go func() { - ticker := time.NewTicker(5 * time.Minute) - defer ticker.Stop() - for { - select { - case <-ctx.Done(): - return - case <-ticker.C: - cleaned, err := mgr.CleanupStale(ctx) - if err != nil { - logger.Error("stale cleanup failed", "error", err) - } else if cleaned > 0 { - logger.Info("cleaned stale sessions", "count", cleaned) - } - } - } - }() -} - func buildClientset(kubeconfigPath string) (kubernetes.Interface, error) { var config *rest.Config var err error @@ -230,6 +235,6 @@ type k8sHealthChecker struct { } func (c *k8sHealthChecker) CheckHealth(ctx context.Context) error { - _, err := c.clientset.CoreV1().Namespaces().Get(ctx, c.namespace, metav1.GetOptions{}) + _, err := c.clientset.CoreV1().Pods(c.namespace).List(ctx, metav1.ListOptions{Limit: 1}) return err } diff --git a/docs/proposals/cli-mcp-operator-design.md b/docs/proposals/cli-mcp-operator-design.md index b33bb4b..4b3d1e2 100644 --- a/docs/proposals/cli-mcp-operator-design.md +++ b/docs/proposals/cli-mcp-operator-design.md @@ -494,7 +494,8 @@ Depends on Phase 4 (instance exists; builder and idle GC already shipped). This - Operator **Watches** instance sandbox Pods (claim does not call `TriggerReplenish`; same predicated watch as Phase 4 — assignment enqueues, last-activity does not). Keep unassigned count == `spec.sandbox.warmPoolSize`; surplus deleted immediately (re-get; skip if `session-id` appeared); recreate on spec/image/env/resources hash (no 2× age-drain). Assigned pods are left on overlay change. Enqueue Ready/Failed/backoff here so pool Ready can see them. - Ready: first Ready / pool-size increase waits for full pool; claim does not flap (Q15). Idle GC already in Phase 4; do not treat assigned session count as Ready. - Operator must not SSA/delete a pod that just gained `session-id` except idle GC / finalizer. -- **Test tips:** envtest for the two-writer contract (pool size, surplus trim, stale-list vs claim, Ready must not flap on claim). Extend Kind e2e for warm pool if that is the cheap place. +- **MCP claim-fail rediscover:** after `ClaimPod` fails, `GetOrCreatePod` currently creates on demand without listing again. Once a warm pool exists, a losing replica can create `cli-mcp-sandbox-` while a sibling already claimed a UUID-named pool pod for the same session (`AlreadyExists` does not catch that). Rediscover after failed claim before create. As-built/Phase 3 left this; it only bites when unassigned pods exist. +- **Test tips:** envtest for the two-writer contract (pool size, surplus trim, stale-list vs claim, Ready must not flap on claim). Extend Kind e2e for warm pool if that is the cheap place. Include the claim-fail rediscover (sibling already assigned a pool pod; losing replica must not create a second named pod). - **Done when:** the operator maintains `warmPoolSize` and Ready follows Q15 (including no flap on claim). - **Coverage check:** [Testing](#testing-all-code-phases) against this PR’s diff. - **Out of this PR:** proxy children, extra sandbox volume knobs / `imagePullSecrets`. diff --git a/pkg/session/config.go b/pkg/session/config.go index b4a1b8c..141b344 100644 --- a/pkg/session/config.go +++ b/pkg/session/config.go @@ -1,38 +1,68 @@ package session -import "time" +import corev1 "k8s.io/api/core/v1" -// SandboxConfig holds all tunables for sandbox pod lifecycle management. +const ( + LabelSessionID = "cli-mcp.redhat.com/session-id" + LabelComponent = "cli-mcp.redhat.com/component" + LabelInstance = "cli-mcp.redhat.com/instance" + + ComponentSandbox = "sandbox" + + AnnotationCreatedAt = "cli-mcp.redhat.com/created-at" + AnnotationLastActivity = "cli-mcp.redhat.com/last-activity" +) + +// reservedSandboxEnv names are owned by the sandbox builder. Overlay env +// entries with these names are ignored. +var reservedSandboxEnv = map[string]struct{}{ + "KUBECONFIG": {}, + "HOME": {}, + "SANDBOX_AUTH_TOKEN": {}, +} + +// SandboxConfig holds CRD-agnostic tunables for sandbox pod lifecycle. +// InstanceName, Namespace, ServiceAccountName, and KubeconfigSecret have +// no production defaults — callers (flags, tests, operator) must set them. type SandboxConfig struct { Image string CPURequest string CPULimit string MemoryRequest string MemoryLimit string - IdleTimeout time.Duration HMACKey string - WarmPoolSize int - ReconcileInterval time.Duration Namespace string + InstanceName string ServiceAccountName string KubeconfigSecret string AgentPort int + ImagePullPolicy corev1.PullPolicy + Env []corev1.EnvVar } -// DefaultConfig returns a SandboxConfig with production-ready defaults. -// Callers must set Image and HMACKey before use; these have no safe defaults. +// DefaultConfig returns resource and agent-port defaults. Identity fields +// (namespace, instance, SA, kubeconfig secret) are left empty. func DefaultConfig() SandboxConfig { - return SandboxConfig{ // #nosec G101 -- K8s Secret resource name, not a credential - CPURequest: "100m", - CPULimit: "500m", - MemoryRequest: "128Mi", - MemoryLimit: "512Mi", - IdleTimeout: 30 * time.Minute, - WarmPoolSize: 0, - ReconcileInterval: 30 * time.Second, - Namespace: "tarsy", - ServiceAccountName: "cli-mcp-investigation-sa", - KubeconfigSecret: "cli-mcp-investigation-kubeconfig", - AgentPort: 8090, + return SandboxConfig{ + CPURequest: "100m", + CPULimit: "500m", + MemoryRequest: "128Mi", + MemoryLimit: "512Mi", + AgentPort: 8090, } } + +// SandboxSelector matches sandbox pods for one instance (component + instance). +func SandboxSelector(instance string) string { + return LabelInstance + "=" + instance + "," + LabelComponent + "=" + ComponentSandbox +} + +// AssignedSelector matches assigned sandbox pods for one instance and session. +func AssignedSelector(instance, sessionID string) string { + return SandboxSelector(instance) + "," + LabelSessionID + "=" + sessionID +} + +// UnassignedSelector matches sandbox pods for one instance that have no session-id. +func UnassignedSelector(instance string) string { + return SandboxSelector(instance) + ",!" + LabelSessionID +} diff --git a/pkg/session/manager.go b/pkg/session/manager.go index 6b42466..b7a315e 100644 --- a/pkg/session/manager.go +++ b/pkg/session/manager.go @@ -1,6 +1,7 @@ package session import ( + "cmp" "context" "crypto/hmac" "crypto/sha256" @@ -16,20 +17,12 @@ import ( "github.com/codeready-toolchain/cli-mcp-operator/pkg/agent" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes" ) const ( - labelSessionID = "tarsy.redhat.com/session-id" - labelComponent = "tarsy.redhat.com/component" - componentValue = "cli-mcp-sandbox" - - annotationCreatedAt = "tarsy.redhat.com/created-at" - annotationLastActivity = "tarsy.redhat.com/last-activity" - podNamePrefix = "cli-mcp-sandbox-" //nolint:gosec // G101: K8s resource names, not credentials secretNamePrefix = "cli-mcp-sandbox-auth-" @@ -53,8 +46,9 @@ type SessionManager struct { logger *slog.Logger } -// NewSessionManager creates a SessionManager with a default PodCache and agent client. -// It returns an error if required config fields (HMACKey, Image) are missing. +// NewSessionManager creates a SessionManager with a default PodCache, agent +// client, and claim helper. Claim is always available; the MCP process does +// not replenish a warm pool or idle-GC assigned pods. func NewSessionManager(clientset kubernetes.Interface, config SandboxConfig, logger *slog.Logger) (*SessionManager, error) { if config.HMACKey == "" { return nil, fmt.Errorf("SandboxConfig.HMACKey must not be empty") @@ -62,13 +56,27 @@ func NewSessionManager(clientset kubernetes.Interface, config SandboxConfig, log if config.Image == "" { return nil, fmt.Errorf("SandboxConfig.Image must not be empty") } + if config.InstanceName == "" { + return nil, fmt.Errorf("SandboxConfig.InstanceName must not be empty") + } + if config.Namespace == "" { + return nil, fmt.Errorf("SandboxConfig.Namespace must not be empty") + } + if config.ServiceAccountName == "" { + return nil, fmt.Errorf("SandboxConfig.ServiceAccountName must not be empty") + } + if config.KubeconfigSecret == "" { + return nil, fmt.Errorf("SandboxConfig.KubeconfigSecret must not be empty") + } if logger == nil { logger = slog.Default() } + config.AgentPort = cmp.Or(config.AgentPort, DefaultConfig().AgentPort) mgr := &SessionManager{ clientset: clientset, config: config, cache: NewPodCache(defaultCacheTTL), + pool: NewWarmPool(clientset, config, logger), // Timeout 0: command duration is bounded by the ExecRequest timeout, not the HTTP client. agentClient: agent.NewAgentClient( agent.WithPort(config.AgentPort), @@ -76,21 +84,10 @@ func NewSessionManager(clientset kubernetes.Interface, config SandboxConfig, log ), logger: logger, } - if config.WarmPoolSize > 0 { - mgr.pool = NewWarmPool(clientset, config, logger) - } return mgr, nil } -// StartPool starts the warm pool reconciler if the pool is enabled. -// It should be called once after NewSessionManager during server startup. -func (m *SessionManager) StartPool(ctx context.Context) { - if m.pool != nil { - m.pool.StartReconciler(ctx) - } -} - -// Pool returns the warm pool (nil when disabled). Exposed for testing. +// Pool returns the claim helper. Exposed for testing. func (m *SessionManager) Pool() *WarmPool { return m.pool } @@ -118,7 +115,7 @@ func ValidateSessionID(sessionID string) error { } // GetOrCreatePod resolves or creates a sandbox pod for the session. -// Lookup order: cache → label-based K8s API discovery → idempotent create. +// Lookup order: cache → label-based K8s API discovery → claim unassigned → on-demand create. func (m *SessionManager) GetOrCreatePod(ctx context.Context, sessionID string) (podIP string, err error) { if validationErr := ValidateSessionID(sessionID); validationErr != nil { return "", validationErr @@ -137,28 +134,30 @@ func (m *SessionManager) GetOrCreatePod(ctx context.Context, sessionID string) ( return ip, nil } - if m.pool != nil { - claimedIP, claimedPodName, claimErr := m.pool.ClaimPod(ctx, sessionID) - if claimErr == nil { - // Claim only guarantees IP + /assign; wait for PodReady before caching. - readyIP, readyErr := m.waitForReady(ctx, claimedPodName) - if readyErr != nil { - // On caller abort, leave the pod for sibling replicas still in - // discover/waitForReady. On ready-timeout failure the agent already - // has the token, so delete — it cannot return to the warm pool. - if shouldCleanupAfterWaitFailure(readyErr) { - m.bestEffortCleanupFailedPod(claimedPodName, sessionID) - } - return "", fmt.Errorf("warm pool pod not ready after claim: %w", readyErr) - } - if readyIP == "" { - readyIP = claimedIP + claimedIP, claimedPodName, claimErr := m.pool.ClaimPod(ctx, sessionID) + if claimErr == nil { + // Claim only guarantees IP + /assign; wait for PodReady before caching. + readyIP, readyErr := m.waitForReady(ctx, claimedPodName) + if readyErr != nil { + // On caller abort, leave the pod for sibling replicas still in + // discover/waitForReady. On ready-timeout failure the agent already + // has the token, so delete — it cannot return to the unassigned set. + if shouldCleanupAfterWaitFailure(readyErr) { + m.bestEffortCleanupFailedPod(claimedPodName, sessionID) } - m.cache.Set(sessionID, readyIP, claimedPodName) - return readyIP, nil + return "", fmt.Errorf("claimed pod not ready: %w", readyErr) } - m.logger.Info("warm pool claim failed, falling back to on-demand creation", "session", sessionID, "error", claimErr) + if readyIP == "" { + readyIP = claimedIP + } + m.cache.Set(sessionID, readyIP, claimedPodName) + return readyIP, nil } + m.logger.Debug("no unassigned pod claimed, creating on demand", "session", sessionID, "error", claimErr) + + // Phase 5 follow-up: after a failed claim, rediscover before create. A sibling + // may have already claimed a UUID-named pool pod for this session; on-demand + // create uses cli-mcp-sandbox- so AlreadyExists will not catch that. ip, podName, err = m.createSandboxPod(ctx, sessionID) if err != nil { @@ -182,9 +181,8 @@ func (m *SessionManager) GetOrCreatePod(ctx context.Context, sessionID string) ( // discoverPod lists pods by label selector and returns the oldest Ready pod's IP. // If no Ready pod exists but a non-terminal pod is found, it waits for readiness. func (m *SessionManager) discoverPod(ctx context.Context, sessionID string) (podIP, podName string, err error) { - selector := fmt.Sprintf("%s=%s,%s=%s", labelSessionID, sessionID, labelComponent, componentValue) pods, err := m.clientset.CoreV1().Pods(m.config.Namespace).List(ctx, metav1.ListOptions{ - LabelSelector: selector, + LabelSelector: AssignedSelector(m.config.InstanceName, sessionID), }) if err != nil { return "", "", fmt.Errorf("list pods: %w", err) @@ -237,108 +235,11 @@ func isPodReady(pod *corev1.Pod) bool { return false } -// buildBasePodSpec constructs the shared sandbox pod spec used by both -// on-demand creation and warm pool pre-creation. It includes the pod name, -// component label, created-at and last-activity annotations, security context, -// readiness probe, volumes, and base env vars. Callers add session-specific -// fields (session-id label, SANDBOX_AUTH_TOKEN env var) as needed. -func buildBasePodSpec(name string, config SandboxConfig) *corev1.Pod { - now := time.Now().UTC().Format(time.RFC3339) - runAsNonRoot := true - allowPrivEsc := false - - return &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: config.Namespace, - Labels: map[string]string{ - labelComponent: componentValue, - }, - Annotations: map[string]string{ - annotationCreatedAt: now, - annotationLastActivity: now, - }, - }, - Spec: corev1.PodSpec{ - ServiceAccountName: config.ServiceAccountName, - SecurityContext: &corev1.PodSecurityContext{ - RunAsNonRoot: &runAsNonRoot, - }, - Containers: []corev1.Container{ - { - Name: "sandbox", - Image: config.Image, - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse(config.CPURequest), - corev1.ResourceMemory: resource.MustParse(config.MemoryRequest), - }, - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse(config.CPULimit), - corev1.ResourceMemory: resource.MustParse(config.MemoryLimit), - }, - }, - // Exec probe hits /health on loopback so kubelet does not need NetworkPolicy - // ingress (HTTPGet from the node IP is blocked when only app=cli-mcp-server - // may reach :8090). /health reflects bash session liveness (IsAlive), not - // merely TCP accept. curl-minimal is part of the sandbox image contract. - ReadinessProbe: &corev1.Probe{ - ProbeHandler: corev1.ProbeHandler{ - Exec: &corev1.ExecAction{ - Command: []string{ - "curl", - "-fsS", - "--max-time", - "1", - fmt.Sprintf("http://127.0.0.1:%d/health", config.AgentPort), - }, - }, - }, - InitialDelaySeconds: 2, - TimeoutSeconds: 2, // > curl --max-time so shell/startup does not consume the whole budget - PeriodSeconds: 10, - }, - SecurityContext: &corev1.SecurityContext{ - AllowPrivilegeEscalation: &allowPrivEsc, - Capabilities: &corev1.Capabilities{ - Drop: []corev1.Capability{"ALL"}, - }, - }, - Env: []corev1.EnvVar{ - {Name: "KUBECONFIG", Value: "/config/kubeconfig"}, - {Name: "HOME", Value: "/workspace"}, - }, - VolumeMounts: []corev1.VolumeMount{ - {Name: "kubeconfig", MountPath: "/config", ReadOnly: true}, - {Name: "workspace", MountPath: "/workspace"}, - }, - }, - }, - Volumes: []corev1.Volume{ - { - Name: "kubeconfig", - VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{ - SecretName: config.KubeconfigSecret, - }, - }, - }, - { - Name: "workspace", - VolumeSource: corev1.VolumeSource{ - EmptyDir: &corev1.EmptyDirVolumeSource{}, - }, - }, - }, - }, - } -} - // buildPodSpec constructs a session-specific pod by applying session fields // (session-id label, SANDBOX_AUTH_TOKEN env) on top of the shared base pod spec. func (m *SessionManager) buildPodSpec(sessionID string) *corev1.Pod { - pod := buildBasePodSpec(podNamePrefix+sessionID, m.config) - pod.Labels[labelSessionID] = sessionID + pod := BuildBasePodSpec(podNamePrefix+sessionID, m.config) + pod.Labels[LabelSessionID] = sessionID pod.Spec.Containers[0].Env = append(pod.Spec.Containers[0].Env, corev1.EnvVar{ Name: "SANDBOX_AUTH_TOKEN", ValueFrom: &corev1.EnvVarSource{ @@ -355,14 +256,15 @@ func (m *SessionManager) buildPodSpec(sessionID string) *corev1.Pod { // buildAuthSecret constructs the per-session auth Secret containing the // HMAC-derived token. Shared by SessionManager and WarmPool. -func buildAuthSecret(namespace, sessionID, token string) *corev1.Secret { +func buildAuthSecret(namespace, instance, sessionID, token string) *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretNamePrefix + sessionID, Namespace: namespace, Labels: map[string]string{ - labelSessionID: sessionID, - labelComponent: componentValue, + LabelSessionID: sessionID, + LabelComponent: ComponentSandbox, + LabelInstance: instance, }, }, StringData: map[string]string{ @@ -420,7 +322,7 @@ func (m *SessionManager) waitForReady(ctx context.Context, podName string) (stri func (m *SessionManager) createSandboxPod(ctx context.Context, sessionID string) (podIP, podName string, err error) { token := computeToken(m.config.HMACKey, sessionID) - secret := buildAuthSecret(m.config.Namespace, sessionID, token) + secret := buildAuthSecret(m.config.Namespace, m.config.InstanceName, sessionID, token) _, err = m.clientset.CoreV1().Secrets(m.config.Namespace).Create(ctx, secret, metav1.CreateOptions{}) if err != nil && !k8serrors.IsAlreadyExists(err) { return "", "", fmt.Errorf("create auth secret: %w", err) @@ -440,7 +342,7 @@ func (m *SessionManager) createSandboxPod(ctx context.Context, sessionID string) if waitErr != nil { // Delete only when the pod failed our ready wait. If the request context // was canceled or timed out, another replica may still be waiting on the - // same pod via discoverPod — leave resources for idle GC instead. + // same pod via discoverPod — leave resources for a sibling waiter. if shouldCleanupAfterWaitFailure(waitErr) { m.bestEffortCleanupFailedPod(created.Name, sessionID) } @@ -519,7 +421,7 @@ func (m *SessionManager) updateLastActivity(ctx context.Context, podName string) defer cancel() now := time.Now().UTC().Format(time.RFC3339) - patch := fmt.Sprintf(`{"metadata":{"annotations":{%q:%q}}}`, annotationLastActivity, now) + patch := fmt.Sprintf(`{"metadata":{"annotations":{%q:%q}}}`, AnnotationLastActivity, now) _, err := m.clientset.CoreV1().Pods(m.config.Namespace).Patch( ctx, podName, types.MergePatchType, []byte(patch), metav1.PatchOptions{}, @@ -533,9 +435,8 @@ func (m *SessionManager) updateLastActivity(ctx context.Context, podName string) func (m *SessionManager) CleanupSession(ctx context.Context, sessionID string) error { m.cache.Delete(sessionID) - selector := fmt.Sprintf("%s=%s,%s=%s", labelSessionID, sessionID, labelComponent, componentValue) pods, err := m.clientset.CoreV1().Pods(m.config.Namespace).List(ctx, metav1.ListOptions{ - LabelSelector: selector, + LabelSelector: AssignedSelector(m.config.InstanceName, sessionID), }) if err != nil { return fmt.Errorf("list pods for cleanup: %w", err) @@ -555,50 +456,3 @@ func (m *SessionManager) CleanupSession(ctx context.Context, sessionID string) e return errors.Join(errs...) } - -// CleanupStale iterates all sandbox pods and cleans up sessions idle longer -// than IdleTimeout. This method is designed to be called externally on a ticker. -func (m *SessionManager) CleanupStale(ctx context.Context) (int, error) { - selector := fmt.Sprintf("%s=%s", labelComponent, componentValue) - pods, err := m.clientset.CoreV1().Pods(m.config.Namespace).List(ctx, metav1.ListOptions{ - LabelSelector: selector, - }) - if err != nil { - return 0, fmt.Errorf("list sandbox pods: %w", err) - } - - now := time.Now() - cleaned := 0 - - for i := range pods.Items { - p := &pods.Items[i] - sessionID := p.Labels[labelSessionID] - if sessionID == "" { - continue - } - - lastActive := p.Annotations[annotationLastActivity] - if lastActive == "" { - lastActive = p.Annotations[annotationCreatedAt] - } - if lastActive == "" { - continue - } - - t, parseErr := time.Parse(time.RFC3339, lastActive) - if parseErr != nil { - m.logger.Warn("unparseable timestamp annotation", "pod", p.Name, "value", lastActive) - continue - } - - if now.Sub(t) > m.config.IdleTimeout { - if cleanErr := m.CleanupSession(ctx, sessionID); cleanErr != nil { - m.logger.Warn("failed to cleanup stale session", "session", sessionID, "error", cleanErr) - continue - } - cleaned++ - } - } - - return cleaned, nil -} diff --git a/pkg/session/manager_test.go b/pkg/session/manager_test.go index 7671fca..87ce3b8 100644 --- a/pkg/session/manager_test.go +++ b/pkg/session/manager_test.go @@ -27,13 +27,22 @@ import ( k8stesting "k8s.io/client-go/testing" ) -const testNamespace = "tarsy" +const ( + testNamespace = "cli-mcp" + testInstance = "oc" + testSA = "cli-mcp-oc-sandbox" + //nolint:gosec // G101: K8s Secret resource name, not a credential + testKubeSecret = "cli-mcp-oc-kubeconfig" +) func newTestConfig() SandboxConfig { cfg := DefaultConfig() cfg.HMACKey = "test-hmac-key" cfg.Image = "quay.io/codeready-toolchain/cli-mcp-sandbox:v0.1.0-test" cfg.Namespace = testNamespace + cfg.InstanceName = testInstance + cfg.ServiceAccountName = testSA + cfg.KubeconfigSecret = testKubeSecret return cfg } @@ -62,12 +71,13 @@ func readyPod(sessionID, ip string, createdAt time.Time) corev1.Pod { Namespace: testNamespace, CreationTimestamp: metav1.NewTime(createdAt), Labels: map[string]string{ - labelSessionID: sessionID, - labelComponent: componentValue, + LabelSessionID: sessionID, + LabelComponent: ComponentSandbox, + LabelInstance: testInstance, }, Annotations: map[string]string{ - annotationCreatedAt: createdAt.Format(time.RFC3339), - annotationLastActivity: createdAt.Format(time.RFC3339), + AnnotationCreatedAt: createdAt.Format(time.RFC3339), + AnnotationLastActivity: createdAt.Format(time.RFC3339), }, }, Status: corev1.PodStatus{ @@ -185,6 +195,46 @@ func TestNewSessionManagerValidation(t *testing.T) { assert.Contains(t, err.Error(), "Image") }) + t.Run("rejects empty InstanceName", func(t *testing.T) { + cfg := newTestConfig() + cfg.InstanceName = "" + + _, err := NewSessionManager(client, cfg, slog.Default()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "InstanceName") + }) + + t.Run("rejects empty Namespace", func(t *testing.T) { + cfg := newTestConfig() + cfg.Namespace = "" + + _, err := NewSessionManager(client, cfg, slog.Default()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "Namespace") + }) + + t.Run("rejects empty ServiceAccountName", func(t *testing.T) { + cfg := newTestConfig() + cfg.ServiceAccountName = "" + + _, err := NewSessionManager(client, cfg, slog.Default()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "ServiceAccountName") + }) + + t.Run("rejects empty KubeconfigSecret", func(t *testing.T) { + cfg := newTestConfig() + cfg.KubeconfigSecret = "" + + _, err := NewSessionManager(client, cfg, slog.Default()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "KubeconfigSecret") + }) + t.Run("accepts valid config", func(t *testing.T) { mgr, err := NewSessionManager(client, newTestConfig(), slog.Default()) @@ -270,6 +320,28 @@ func TestGetOrCreatePod(t *testing.T) { assert.Equal(t, "10.0.0.10", ip) }) + t.Run("does not discover a pod from another instance", func(t *testing.T) { + other := readyPod("inv-other", "10.0.0.9", time.Now()) + other.Name = "cli-mcp-sandbox-aws-inv-other" + other.Labels[LabelInstance] = "aws" + mgr := newTestManager(t, other) + ctx := t.Context() + done := make(chan error, 1) + go func() { done <- markPodReady(mgr, "inv-other", "10.0.0.8") }() + + ip, err := mgr.GetOrCreatePod(ctx, "inv-other") + + require.NoError(t, <-done) + require.NoError(t, err) + assert.Equal(t, "10.0.0.8", ip, "should create a new pod for this instance, not reuse the other instance's pod") + pods, listErr := mgr.clientset.CoreV1().Pods(testNamespace).List(ctx, metav1.ListOptions{ + LabelSelector: AssignedSelector(testInstance, "inv-other"), + }) + require.NoError(t, listErr) + require.Len(t, pods.Items, 1) + assert.Equal(t, testInstance, pods.Items[0].Labels[LabelInstance]) + }) + t.Run("rejects invalid session ID", func(t *testing.T) { // given mgr := newTestManager(t) @@ -360,12 +432,13 @@ func TestBuildPodSpec(t *testing.T) { assert.Equal(t, testNamespace, pod.Namespace) // then — labels - assert.Equal(t, "inv-spec", pod.Labels[labelSessionID]) - assert.Equal(t, componentValue, pod.Labels[labelComponent]) + assert.Equal(t, "inv-spec", pod.Labels[LabelSessionID]) + assert.Equal(t, ComponentSandbox, pod.Labels[LabelComponent]) + assert.Equal(t, testInstance, pod.Labels[LabelInstance]) // then — annotations - assert.NotEmpty(t, pod.Annotations[annotationCreatedAt]) - assert.NotEmpty(t, pod.Annotations[annotationLastActivity]) + assert.NotEmpty(t, pod.Annotations[AnnotationCreatedAt]) + assert.NotEmpty(t, pod.Annotations[AnnotationLastActivity]) // then — pod security context (no hardcoded UID/GID; OpenShift assigns from namespace range) require.NotNil(t, pod.Spec.SecurityContext) @@ -406,6 +479,12 @@ func TestBuildPodSpec(t *testing.T) { assert.True(t, envNames["KUBECONFIG"]) assert.True(t, envNames["HOME"]) assert.True(t, envNames["SANDBOX_AUTH_TOKEN"]) + + require.NotNil(t, pod.Spec.AutomountServiceAccountToken) + assert.False(t, *pod.Spec.AutomountServiceAccountToken) + assert.Equal(t, testSA, pod.Spec.ServiceAccountName) + require.Len(t, pod.Spec.Volumes, 2) + assert.Equal(t, testKubeSecret, pod.Spec.Volumes[0].Secret.SecretName) } func TestBuildAuthSecret(t *testing.T) { @@ -414,13 +493,14 @@ func TestBuildAuthSecret(t *testing.T) { token := computeToken(mgr.config.HMACKey, "inv-sec") // when - secret := buildAuthSecret(testNamespace, "inv-sec", token) + secret := buildAuthSecret(testNamespace, testInstance, "inv-sec", token) // then assert.Equal(t, secretNamePrefix+"inv-sec", secret.Name) assert.Equal(t, testNamespace, secret.Namespace) - assert.Equal(t, "inv-sec", secret.Labels[labelSessionID]) - assert.Equal(t, componentValue, secret.Labels[labelComponent]) + assert.Equal(t, "inv-sec", secret.Labels[LabelSessionID]) + assert.Equal(t, ComponentSandbox, secret.Labels[LabelComponent]) + assert.Equal(t, testInstance, secret.Labels[LabelInstance]) assert.Equal(t, token, secret.StringData["token"]) } @@ -498,7 +578,7 @@ func TestCleanupSession(t *testing.T) { mgr.cache.Set("inv-clean", "10.0.0.5", podNamePrefix+"inv-clean") ctx := context.Background() - secret := buildAuthSecret(testNamespace, "inv-clean", "tok") + secret := buildAuthSecret(testNamespace, testInstance, "inv-clean", "tok") _, err := mgr.clientset.CoreV1().Secrets(testNamespace).Create(ctx, secret, metav1.CreateOptions{}) require.NoError(t, err) @@ -512,7 +592,7 @@ func TestCleanupSession(t *testing.T) { assert.False(t, ok, "cache entry should be cleared") pods, err := mgr.clientset.CoreV1().Pods(testNamespace).List(ctx, metav1.ListOptions{ - LabelSelector: fmt.Sprintf("%s=%s", labelSessionID, "inv-clean"), + LabelSelector: AssignedSelector(testInstance, "inv-clean"), }) require.NoError(t, err) assert.Empty(t, pods.Items, "pods should be deleted") @@ -521,54 +601,6 @@ func TestCleanupSession(t *testing.T) { assert.Error(t, err, "secret should be deleted") } -func TestCleanupStale(t *testing.T) { - t.Run("stale pod is cleaned up", func(t *testing.T) { - // given - staleTime := time.Now().Add(-2 * time.Hour) - stalePod := readyPod("stale-1", "10.0.0.1", staleTime) - mgr := newTestManager(t, stalePod) - - // when - cleaned, err := mgr.CleanupStale(context.Background()) - - // then - require.NoError(t, err) - assert.Equal(t, 1, cleaned) - }) - - t.Run("fresh pod is preserved", func(t *testing.T) { - // given - freshPod := readyPod("fresh-1", "10.0.0.2", time.Now()) - mgr := newTestManager(t, freshPod) - - // when - cleaned, err := mgr.CleanupStale(context.Background()) - - // then - require.NoError(t, err) - assert.Equal(t, 0, cleaned) - - pods, err := mgr.clientset.CoreV1().Pods(testNamespace).List(context.Background(), metav1.ListOptions{}) - require.NoError(t, err) - assert.Len(t, pods.Items, 1) - }) - - t.Run("falls back to created-at when last-activity is missing", func(t *testing.T) { - // given - staleTime := time.Now().Add(-2 * time.Hour) - pod := readyPod("no-activity", "10.0.0.3", staleTime) - delete(pod.Annotations, annotationLastActivity) - mgr := newTestManager(t, pod) - - // when - cleaned, err := mgr.CleanupStale(context.Background()) - - // then - require.NoError(t, err) - assert.Equal(t, 1, cleaned) - }) -} - func TestExecuteCommand(t *testing.T) { t.Run("proxies command and returns response", func(t *testing.T) { // given @@ -704,7 +736,7 @@ func TestExecuteCommand(t *testing.T) { if getErr != nil { return false } - return updated.Annotations[annotationLastActivity] != oldActivity + return updated.Annotations[AnnotationLastActivity] != oldActivity }, time.Second, 10*time.Millisecond, "last-activity should be patched using pre-Execute podName") }) } diff --git a/pkg/session/podspec.go b/pkg/session/podspec.go new file mode 100644 index 0000000..9a930ef --- /dev/null +++ b/pkg/session/podspec.go @@ -0,0 +1,126 @@ +package session + +import ( + "cmp" + "fmt" + "time" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// BuildBasePodSpec constructs the shared sandbox pod spec used by MCP on-demand +// create and (later) the operator warm pool. It sets instance+component labels, +// dedicated SA, automountServiceAccountToken false, kubeconfig mount, and +// today's non-root / drop-caps security context, then merges the class overlay +// (image, resources, env, imagePullPolicy). Callers add session-specific +// fields (session-id label, SANDBOX_AUTH_TOKEN env) on assigned pods only. +func BuildBasePodSpec(name string, config SandboxConfig) *corev1.Pod { + now := time.Now().UTC().Format(time.RFC3339) + runAsNonRoot := true + allowPrivEsc := false + automount := false + defaults := DefaultConfig() + agentPort := cmp.Or(config.AgentPort, defaults.AgentPort) + + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: config.Namespace, + Labels: map[string]string{ + LabelComponent: ComponentSandbox, + LabelInstance: config.InstanceName, + }, + Annotations: map[string]string{ + AnnotationCreatedAt: now, + AnnotationLastActivity: now, + }, + }, + Spec: corev1.PodSpec{ + ServiceAccountName: config.ServiceAccountName, + AutomountServiceAccountToken: &automount, + SecurityContext: &corev1.PodSecurityContext{ + RunAsNonRoot: &runAsNonRoot, + }, + Containers: []corev1.Container{ + { + Name: "sandbox", + Image: config.Image, + ImagePullPolicy: config.ImagePullPolicy, + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(cmp.Or(config.CPURequest, defaults.CPURequest)), + corev1.ResourceMemory: resource.MustParse(cmp.Or(config.MemoryRequest, defaults.MemoryRequest)), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse(cmp.Or(config.CPULimit, defaults.CPULimit)), + corev1.ResourceMemory: resource.MustParse(cmp.Or(config.MemoryLimit, defaults.MemoryLimit)), + }, + }, + // Exec probe hits /health on loopback so kubelet does not need NetworkPolicy + // ingress (HTTPGet from the node IP is blocked when only component=server + // may reach :8090). /health reflects bash session liveness (IsAlive), not + // merely TCP accept. curl-minimal is part of the sandbox image contract. + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + Exec: &corev1.ExecAction{ + Command: []string{ + "curl", + "-fsS", + "--max-time", + "1", + fmt.Sprintf("http://127.0.0.1:%d/health", agentPort), + }, + }, + }, + InitialDelaySeconds: 2, + TimeoutSeconds: 2, // > curl --max-time so shell/startup does not consume the whole budget + PeriodSeconds: 10, + }, + SecurityContext: &corev1.SecurityContext{ + AllowPrivilegeEscalation: &allowPrivEsc, + Capabilities: &corev1.Capabilities{ + Drop: []corev1.Capability{"ALL"}, + }, + }, + Env: overlaySandboxEnv(config.Env), + VolumeMounts: []corev1.VolumeMount{ + {Name: "kubeconfig", MountPath: "/config", ReadOnly: true}, + {Name: "workspace", MountPath: "/workspace"}, + }, + }, + }, + Volumes: []corev1.Volume{ + { + Name: "kubeconfig", + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: config.KubeconfigSecret, + }, + }, + }, + { + Name: "workspace", + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, + }, + }, + }, + }, + } +} + +func overlaySandboxEnv(userEnv []corev1.EnvVar) []corev1.EnvVar { + env := []corev1.EnvVar{ + {Name: "KUBECONFIG", Value: "/config/kubeconfig"}, + {Name: "HOME", Value: "/workspace"}, + } + for _, e := range userEnv { + if _, reserved := reservedSandboxEnv[e.Name]; reserved { + continue + } + env = append(env, e) + } + return env +} diff --git a/pkg/session/podspec_test.go b/pkg/session/podspec_test.go new file mode 100644 index 0000000..30c3f91 --- /dev/null +++ b/pkg/session/podspec_test.go @@ -0,0 +1,108 @@ +package session + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" +) + +func TestBuildBasePodSpecOverlay(t *testing.T) { + t.Run("merges class env and skips reserved names", func(t *testing.T) { + cfg := newTestConfig() + cfg.Env = []corev1.EnvVar{ + {Name: "AWS_REGION", Value: "us-east-1"}, + {Name: "KUBECONFIG", Value: "/evil"}, + {Name: "HOME", Value: "/evil"}, + {Name: "SANDBOX_AUTH_TOKEN", Value: "stolen"}, + { + Name: "AWS_SHARED_CREDENTIALS_FILE", + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "aws-cli-creds"}, + Key: "path", + }, + }, + }, + } + cfg.ImagePullPolicy = corev1.PullIfNotPresent + cfg.CPURequest = "200m" + cfg.CPULimit = "1" + cfg.MemoryRequest = "256Mi" + cfg.MemoryLimit = "1Gi" + + pod := BuildBasePodSpec("cli-mcp-sandbox-warm", cfg) + container := pod.Spec.Containers[0] + + assert.Equal(t, ComponentSandbox, pod.Labels[LabelComponent]) + assert.Equal(t, testInstance, pod.Labels[LabelInstance]) + _, hasSession := pod.Labels[LabelSessionID] + assert.False(t, hasSession) + require.NotNil(t, pod.Spec.AutomountServiceAccountToken) + assert.False(t, *pod.Spec.AutomountServiceAccountToken) + assert.Equal(t, corev1.PullIfNotPresent, container.ImagePullPolicy) + assert.Equal(t, "200m", container.Resources.Requests.Cpu().String()) + assert.Equal(t, "1", container.Resources.Limits.Cpu().String()) + assert.Equal(t, "256Mi", container.Resources.Requests.Memory().String()) + assert.Equal(t, "1Gi", container.Resources.Limits.Memory().String()) + + got := envByName(container.Env) + assert.Equal(t, "/config/kubeconfig", got["KUBECONFIG"].Value) + assert.Equal(t, "/workspace", got["HOME"].Value) + assert.Equal(t, "us-east-1", got["AWS_REGION"].Value) + require.NotNil(t, got["AWS_SHARED_CREDENTIALS_FILE"].ValueFrom) + assert.Equal(t, "aws-cli-creds", got["AWS_SHARED_CREDENTIALS_FILE"].ValueFrom.SecretKeyRef.Name) + _, hasToken := got["SANDBOX_AUTH_TOKEN"] + assert.False(t, hasToken) + }) + + t.Run("uses DefaultConfig resources when overlay quantities are empty", func(t *testing.T) { + cfg := newTestConfig() + cfg.CPURequest = "" + cfg.CPULimit = "" + cfg.MemoryRequest = "" + cfg.MemoryLimit = "" + + pod := BuildBasePodSpec("cli-mcp-sandbox-defaults", cfg) + container := pod.Spec.Containers[0] + defaults := DefaultConfig() + assert.Equal(t, defaults.CPURequest, container.Resources.Requests.Cpu().String()) + assert.Equal(t, defaults.CPULimit, container.Resources.Limits.Cpu().String()) + assert.Equal(t, defaults.MemoryRequest, container.Resources.Requests.Memory().String()) + assert.Equal(t, defaults.MemoryLimit, container.Resources.Limits.Memory().String()) + }) +} + +func TestSelectorsIncludeInstance(t *testing.T) { + assert.Equal(t, + "cli-mcp.redhat.com/instance=oc,cli-mcp.redhat.com/component=sandbox", + SandboxSelector("oc"), + ) + assert.Equal(t, + "cli-mcp.redhat.com/instance=oc,cli-mcp.redhat.com/component=sandbox,cli-mcp.redhat.com/session-id=sess-1", + AssignedSelector("oc", "sess-1"), + ) + assert.Equal(t, + "cli-mcp.redhat.com/instance=oc,cli-mcp.redhat.com/component=sandbox,!cli-mcp.redhat.com/session-id", + UnassignedSelector("oc"), + ) +} + +func TestDefaultConfigHasNoIdentityDefaults(t *testing.T) { + cfg := DefaultConfig() + assert.Empty(t, cfg.Namespace) + assert.Empty(t, cfg.InstanceName) + assert.Empty(t, cfg.ServiceAccountName) + assert.Empty(t, cfg.KubeconfigSecret) + assert.Equal(t, "100m", cfg.CPURequest) + assert.Equal(t, 8090, cfg.AgentPort) +} + +func envByName(env []corev1.EnvVar) map[string]corev1.EnvVar { + out := make(map[string]corev1.EnvVar, len(env)) + for _, e := range env { + out[e.Name] = e + } + return out +} diff --git a/pkg/session/pool.go b/pkg/session/pool.go index a84273c..7fee024 100644 --- a/pkg/session/pool.go +++ b/pkg/session/pool.go @@ -6,11 +6,9 @@ import ( "log/slog" "net/http" "sort" - "strings" "time" "github.com/codeready-toolchain/cli-mcp-operator/pkg/agent" - "github.com/google/uuid" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -18,23 +16,19 @@ import ( "k8s.io/client-go/kubernetes" ) -const ( - defaultReconcileInterval = 30 * time.Second - assignTimeout = 10 * time.Second -) +const assignTimeout = 10 * time.Second -// WarmPool manages a pre-created set of unassigned sandbox pods that can be -// claimed instantly by new sessions, eliminating cold-start latency. +// WarmPool claims unassigned sandbox pods for new sessions. It does not +// replenish the pool; that is the operator's job (Phase 5). type WarmPool struct { clientset kubernetes.Interface config SandboxConfig agentClient *agent.AgentClient logger *slog.Logger - replenishCh chan struct{} } -// NewWarmPool creates a WarmPool. The pool does not start reconciliation -// until StartReconciler is called. +// NewWarmPool creates a claim helper. MCP always constructs one so claim +// runs even when no unassigned pods exist (then GetOrCreatePod creates). func NewWarmPool(clientset kubernetes.Interface, config SandboxConfig, logger *slog.Logger) *WarmPool { if logger == nil { logger = slog.Default() @@ -46,8 +40,7 @@ func NewWarmPool(clientset kubernetes.Interface, config SandboxConfig, logger *s agent.WithPort(config.AgentPort), agent.WithTimeout(assignTimeout), ), - logger: logger, - replenishCh: make(chan struct{}, 1), + logger: logger, } } @@ -60,18 +53,9 @@ func (p *WarmPool) SetHTTPClient(c *http.Client) { ) } -// unassignedSelector returns a label selector matching sandbox pods without a session-id. -func unassignedSelector() string { - return fmt.Sprintf("%s=%s,!%s", labelComponent, componentValue, labelSessionID) -} - -// listUnassignedPods returns all unassigned sandbox pods (including terminal) -// sorted by creation time (oldest first). Callers must filter terminal pods -// as appropriate — ReconcilePool needs to see them for stale cleanup, while -// ClaimPod should skip them. func (p *WarmPool) listUnassignedPods(ctx context.Context) ([]corev1.Pod, error) { podList, err := p.clientset.CoreV1().Pods(p.config.Namespace).List(ctx, metav1.ListOptions{ - LabelSelector: unassignedSelector(), + LabelSelector: UnassignedSelector(p.config.InstanceName), }) if err != nil { return nil, fmt.Errorf("list unassigned pods: %w", err) @@ -89,76 +73,7 @@ func isTerminalPod(pod *corev1.Pod) bool { return pod.Status.Phase == corev1.PodFailed || pod.Status.Phase == corev1.PodSucceeded } -// buildWarmPodSpec constructs a Pod manifest for a warm pool pod. It uses the -// shared base pod spec with a UUID-suffixed name since warm pods are -// interchangeable. No session-id label or SANDBOX_AUTH_TOKEN env var. -func (p *WarmPool) buildWarmPodSpec() *corev1.Pod { - suffix := strings.ReplaceAll(uuid.New().String(), "-", "")[:8] - return buildBasePodSpec(podNamePrefix+suffix, p.config) -} - -// ReconcilePool ensures the number of unassigned warm pods matches WarmPoolSize. -// It first deletes stale unassigned pods (age > 2× IdleTimeout), then creates -// new pods to fill the deficit. -func (p *WarmPool) ReconcilePool(ctx context.Context) { - pods, err := p.listUnassignedPods(ctx) - if err != nil { - p.logger.Error("reconcile: failed to list unassigned pods", "error", err) - return - } - - staleThreshold := 2 * p.config.IdleTimeout - now := time.Now() - var live int - - for i := range pods { - createdAt := pods[i].Annotations[annotationCreatedAt] - if createdAt == "" { - if !isTerminalPod(&pods[i]) { - live++ - } - continue - } - t, parseErr := time.Parse(time.RFC3339, createdAt) - if parseErr != nil { - p.logger.Warn("reconcile: unparseable created-at annotation", "pod", pods[i].Name, "value", createdAt) - if !isTerminalPod(&pods[i]) { - live++ - } - continue - } - if now.Sub(t) > staleThreshold { - p.logger.Info("reconcile: deleting stale unassigned pod", "pod", pods[i].Name, "age", now.Sub(t)) - if delErr := p.clientset.CoreV1().Pods(p.config.Namespace).Delete(ctx, pods[i].Name, metav1.DeleteOptions{}); delErr != nil { - if k8serrors.IsNotFound(delErr) { - continue - } - p.logger.Warn("reconcile: failed to delete stale pod; skipping refill", "pod", pods[i].Name, "error", delErr) - return - } - continue - } - if !isTerminalPod(&pods[i]) { - live++ - } - } - - deficit := p.config.WarmPoolSize - live - if deficit <= 0 { - return - } - - p.logger.Info("reconcile: creating warm pods", "current", live, "target", p.config.WarmPoolSize, "creating", deficit) - for range deficit { - pod := p.buildWarmPodSpec() - _, createErr := p.clientset.CoreV1().Pods(p.config.Namespace).Create(ctx, pod, metav1.CreateOptions{}) - if createErr != nil { - p.logger.Error("reconcile: failed to create warm pod", "error", createErr) - } - } -} - -// ClaimPod atomically claims an unassigned warm pod for the given session. +// ClaimPod atomically claims an unassigned sandbox pod for the given session. // It patches the oldest unassigned pod with the session-id label using // resourceVersion for optimistic locking, creates the auth Secret, and // delivers the token via POST /assign to the agent. @@ -171,7 +86,7 @@ func (p *WarmPool) ClaimPod(ctx context.Context, sessionID string) (podIP, podNa return "", "", fmt.Errorf("claim: %w", err) } if len(pods) == 0 { - return "", "", fmt.Errorf("warm pool exhausted: no unassigned pods available") + return "", "", fmt.Errorf("no unassigned pods available") } for i := range pods { @@ -181,7 +96,6 @@ func (p *WarmPool) ClaimPod(ctx context.Context, sessionID string) (podIP, podNa } ip, name, claimErr := p.tryClaimPod(ctx, pod, sessionID) if claimErr == nil { - p.TriggerReplenish() return ip, name, nil } if k8serrors.IsConflict(claimErr) { @@ -191,14 +105,14 @@ func (p *WarmPool) ClaimPod(ctx context.Context, sessionID string) (podIP, podNa p.logger.Warn("claim: failed to claim pod", "pod", pod.Name, "error", claimErr) } - return "", "", fmt.Errorf("warm pool exhausted: all claim attempts failed") + return "", "", fmt.Errorf("all unassigned claim attempts failed") } // tryClaimPod attempts to claim a single pod. On failure after label patch, it rolls back. func (p *WarmPool) tryClaimPod(ctx context.Context, pod *corev1.Pod, sessionID string) (podIP, podName string, err error) { patchData := fmt.Sprintf( `{"metadata":{"labels":{%q:%q},"resourceVersion":%q}}`, - labelSessionID, sessionID, pod.ResourceVersion, + LabelSessionID, sessionID, pod.ResourceVersion, ) patched, patchErr := p.clientset.CoreV1().Pods(p.config.Namespace).Patch( ctx, pod.Name, types.MergePatchType, []byte(patchData), metav1.PatchOptions{}, @@ -208,7 +122,7 @@ func (p *WarmPool) tryClaimPod(ctx context.Context, pod *corev1.Pod, sessionID s } token := computeToken(p.config.HMACKey, sessionID) - secret := buildAuthSecret(p.config.Namespace, sessionID, token) + secret := buildAuthSecret(p.config.Namespace, p.config.InstanceName, sessionID, token) _, secretErr := p.clientset.CoreV1().Secrets(p.config.Namespace).Create(ctx, secret, metav1.CreateOptions{}) if secretErr != nil { p.rollbackLabel(ctx, pod.Name) @@ -236,7 +150,6 @@ func (p *WarmPool) tryClaimPod(ctx context.Context, pod *corev1.Pod, sessionID s return ip, patched.Name, nil } -// assignToken sends POST /assign to the agent running on the pod to deliver the HMAC token. func (p *WarmPool) assignToken(ctx context.Context, pod *corev1.Pod, token string) error { ip := pod.Status.PodIP if ip == "" { @@ -249,8 +162,6 @@ func (p *WarmPool) assignToken(ctx context.Context, pod *corev1.Pod, token strin return nil } -// waitForIP polls for the pod to have an IP assigned (for pods that are ready -// but the IP wasn't yet reflected when we read the pod). func (p *WarmPool) waitForIP(ctx context.Context, podName string) (string, error) { deadline := time.After(readyTimeout) ticker := time.NewTicker(readyPollPeriod) @@ -291,9 +202,8 @@ func (p *WarmPool) waitForIP(ctx context.Context, podName string) (string, error } } -// rollbackLabel removes the session-id label from a pod (best-effort). func (p *WarmPool) rollbackLabel(ctx context.Context, podName string) { - patch := fmt.Sprintf(`{"metadata":{"labels":{%q:null}}}`, labelSessionID) + patch := fmt.Sprintf(`{"metadata":{"labels":{%q:null}}}`, LabelSessionID) _, err := p.clientset.CoreV1().Pods(p.config.Namespace).Patch( ctx, podName, types.MergePatchType, []byte(patch), metav1.PatchOptions{}, ) @@ -301,41 +211,3 @@ func (p *WarmPool) rollbackLabel(ctx context.Context, podName string) { p.logger.Warn("rollback: failed to remove session-id label", "pod", podName, "error", err) } } - -// TriggerReplenish sends a non-blocking signal to the reconciler to run -// immediately. Multiple rapid calls coalesce into a single reconciliation. -func (p *WarmPool) TriggerReplenish() { - select { - case p.replenishCh <- struct{}{}: - default: - } -} - -// StartReconciler runs a background goroutine that calls ReconcilePool on a -// periodic ticker and whenever TriggerReplenish signals. It exits when ctx -// is cancelled. -func (p *WarmPool) StartReconciler(ctx context.Context) { - interval := p.config.ReconcileInterval - if interval <= 0 { - interval = defaultReconcileInterval - } - - ticker := time.NewTicker(interval) - - go func() { - defer ticker.Stop() - - p.ReconcilePool(ctx) - - for { - select { - case <-ctx.Done(): - return - case <-ticker.C: - p.ReconcilePool(ctx) - case <-p.replenishCh: - p.ReconcilePool(ctx) - } - } - }() -} diff --git a/pkg/session/pool_test.go b/pkg/session/pool_test.go index ac72c8e..b6ce4e8 100644 --- a/pkg/session/pool_test.go +++ b/pkg/session/pool_test.go @@ -20,14 +20,12 @@ import ( func newTestPool(t *testing.T, objects ...corev1.Pod) *WarmPool { t.Helper() client := fake.NewSimpleClientset() - ctx := context.Background() + ctx := t.Context() for i := range objects { _, err := client.CoreV1().Pods(testNamespace).Create(ctx, &objects[i], metav1.CreateOptions{}) require.NoError(t, err) } - cfg := newTestConfig() - cfg.WarmPoolSize = 3 - return NewWarmPool(client, cfg, slog.Default()) + return NewWarmPool(client, newTestConfig(), slog.Default()) } func unassignedPod(name, ip string, createdAt time.Time) corev1.Pod { @@ -37,10 +35,11 @@ func unassignedPod(name, ip string, createdAt time.Time) corev1.Pod { Namespace: testNamespace, CreationTimestamp: metav1.NewTime(createdAt), Labels: map[string]string{ - labelComponent: componentValue, + LabelComponent: ComponentSandbox, + LabelInstance: testInstance, }, Annotations: map[string]string{ - annotationCreatedAt: createdAt.Format(time.RFC3339), + AnnotationCreatedAt: createdAt.Format(time.RFC3339), }, }, Status: corev1.PodStatus{ @@ -53,172 +52,8 @@ func unassignedPod(name, ip string, createdAt time.Time) corev1.Pod { } } -func TestBuildWarmPodSpec(t *testing.T) { - // given - pool := newTestPool(t) - - // when - pod := pool.buildWarmPodSpec() - - // then — uses a generated unique name with the standard prefix - assert.Greater(t, len(pod.Name), len(podNamePrefix), "name should have a random suffix") - assert.Contains(t, pod.Name, podNamePrefix) - assert.Empty(t, pod.GenerateName, "should use Name, not GenerateName") - assert.Equal(t, testNamespace, pod.Namespace) - - // then — has component label but no session-id label - assert.Equal(t, componentValue, pod.Labels[labelComponent]) - _, hasSessionID := pod.Labels[labelSessionID] - assert.False(t, hasSessionID, "warm pod must not have session-id label") - - // then — has both created-at and last-activity annotations - assert.NotEmpty(t, pod.Annotations[annotationCreatedAt]) - assert.NotEmpty(t, pod.Annotations[annotationLastActivity]) - - // then — pod security context (no hardcoded UID/GID; OpenShift assigns from namespace range) - require.NotNil(t, pod.Spec.SecurityContext) - assert.True(t, *pod.Spec.SecurityContext.RunAsNonRoot) - assert.Nil(t, pod.Spec.SecurityContext.RunAsUser) - assert.Nil(t, pod.Spec.SecurityContext.RunAsGroup) - - // then — container security - require.Len(t, pod.Spec.Containers, 1) - container := pod.Spec.Containers[0] - assert.False(t, *container.SecurityContext.AllowPrivilegeEscalation) - assert.Contains(t, container.SecurityContext.Capabilities.Drop, corev1.Capability("ALL")) - - // then — readiness probe (exec curl /health on loopback; works with server-only NetworkPolicy) - require.NotNil(t, container.ReadinessProbe) - require.NotNil(t, container.ReadinessProbe.Exec) - require.Equal(t, []string{ - "curl", - "-fsS", - "--max-time", - "1", - "http://127.0.0.1:8090/health", - }, container.ReadinessProbe.Exec.Command) - assert.Nil(t, container.ReadinessProbe.HTTPGet) - assert.Equal(t, int32(2), container.ReadinessProbe.TimeoutSeconds) - - // then — volumes and mounts match session manager - assert.Len(t, pod.Spec.Volumes, 2) - assert.Len(t, container.VolumeMounts, 2) - - // then — no SANDBOX_AUTH_TOKEN env var - envNames := make(map[string]bool) - for _, e := range container.Env { - envNames[e.Name] = true - } - assert.True(t, envNames["KUBECONFIG"]) - assert.True(t, envNames["HOME"]) - assert.False(t, envNames["SANDBOX_AUTH_TOKEN"], "warm pod must not have SANDBOX_AUTH_TOKEN") -} - -func TestReconcilePool(t *testing.T) { - t.Run("creates pods to reach target size", func(t *testing.T) { - // given - pool := newTestPool(t) - ctx := context.Background() - - // when - pool.ReconcilePool(ctx) - - // then - pods, err := pool.clientset.CoreV1().Pods(testNamespace).List(ctx, metav1.ListOptions{ - LabelSelector: unassignedSelector(), - }) - require.NoError(t, err) - assert.Len(t, pods.Items, 3) - - for _, p := range pods.Items { - assert.Equal(t, componentValue, p.Labels[labelComponent]) - _, hasSessionID := p.Labels[labelSessionID] - assert.False(t, hasSessionID) - } - }) - - t.Run("no-op when pool is full", func(t *testing.T) { - // given - existing := []corev1.Pod{ - unassignedPod("warm-1", "10.0.0.1", time.Now()), - unassignedPod("warm-2", "10.0.0.2", time.Now()), - unassignedPod("warm-3", "10.0.0.3", time.Now()), - } - pool := newTestPool(t, existing...) - ctx := context.Background() - - // when - pool.ReconcilePool(ctx) - - // then — no additional pods created - pods, err := pool.clientset.CoreV1().Pods(testNamespace).List(ctx, metav1.ListOptions{ - LabelSelector: unassignedSelector(), - }) - require.NoError(t, err) - assert.Len(t, pods.Items, 3) - }) - - t.Run("cleans up stale unassigned pods", func(t *testing.T) { - // given — one stale pod (created 2× IdleTimeout ago) and one fresh - cfg := newTestConfig() - staleTime := time.Now().Add(-(2*cfg.IdleTimeout + time.Minute)) - freshTime := time.Now() - - stalePod := unassignedPod("warm-stale", "10.0.0.1", staleTime) - freshPod := unassignedPod("warm-fresh", "10.0.0.2", freshTime) - - pool := newTestPool(t, stalePod, freshPod) - ctx := context.Background() - - // when - pool.ReconcilePool(ctx) - - // then — stale pod deleted; fresh preserved + new pods created to fill deficit - pods, err := pool.clientset.CoreV1().Pods(testNamespace).List(ctx, metav1.ListOptions{ - LabelSelector: unassignedSelector(), - }) - require.NoError(t, err) - - var names []string - for _, p := range pods.Items { - names = append(names, p.Name) - } - assert.NotContains(t, names, "warm-stale", "stale pod should be deleted") - assert.Contains(t, names, "warm-fresh", "fresh pod should be preserved") - // fresh pod + 2 new pods = 3 total - assert.Len(t, pods.Items, 3) - }) - - t.Run("skips terminal pods when counting", func(t *testing.T) { - // given — a Succeeded (terminal) unassigned pod should not count toward pool size - terminalPod := unassignedPod("warm-done", "10.0.0.1", time.Now()) - terminalPod.Status.Phase = corev1.PodSucceeded - - pool := newTestPool(t, terminalPod) - ctx := context.Background() - - // when - pool.ReconcilePool(ctx) - - // then — should create 3 pods (terminal pod doesn't count) - pods, err := pool.clientset.CoreV1().Pods(testNamespace).List(ctx, metav1.ListOptions{ - LabelSelector: unassignedSelector(), - }) - require.NoError(t, err) - - nonTerminal := 0 - for _, p := range pods.Items { - if p.Status.Phase != corev1.PodFailed && p.Status.Phase != corev1.PodSucceeded { - nonTerminal++ - } - } - assert.Equal(t, 3, nonTerminal) - }) -} - func TestClaimPod(t *testing.T) { t.Run("successful claim assigns pod and creates secret", func(t *testing.T) { - // given ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, "/assign", r.URL.Path) assert.Equal(t, http.MethodPost, r.Method) @@ -231,30 +66,25 @@ func TestClaimPod(t *testing.T) { pool.config.AgentPort = agentPortFromURL(t, ts.URL) pool.SetHTTPClient(ts.Client()) - ctx := context.Background() - - // when + ctx := t.Context() ip, name, err := pool.ClaimPod(ctx, "session-abc") - // then require.NoError(t, err) assert.Equal(t, "127.0.0.1", ip) assert.Equal(t, "warm-claim-1", name) - // then — pod now has session-id label claimed, err := pool.clientset.CoreV1().Pods(testNamespace).Get(ctx, "warm-claim-1", metav1.GetOptions{}) require.NoError(t, err) - assert.Equal(t, "session-abc", claimed.Labels[labelSessionID]) + assert.Equal(t, "session-abc", claimed.Labels[LabelSessionID]) - // then — auth secret created secret, err := pool.clientset.CoreV1().Secrets(testNamespace).Get(ctx, secretNamePrefix+"session-abc", metav1.GetOptions{}) require.NoError(t, err) - assert.Equal(t, "session-abc", secret.Labels[labelSessionID]) + assert.Equal(t, "session-abc", secret.Labels[LabelSessionID]) + assert.Equal(t, testInstance, secret.Labels[LabelInstance]) assert.NotEmpty(t, secret.StringData["token"]) }) t.Run("claims oldest pod first", func(t *testing.T) { - // given ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) @@ -266,16 +96,24 @@ func TestClaimPod(t *testing.T) { pool.config.AgentPort = agentPortFromURL(t, ts.URL) pool.SetHTTPClient(ts.Client()) - // when - _, name, err := pool.ClaimPod(context.Background(), "session-order") + _, name, err := pool.ClaimPod(t.Context(), "session-order") - // then — should claim the older pod require.NoError(t, err) assert.Equal(t, "warm-old", name) }) + t.Run("does not claim a pod from another instance", func(t *testing.T) { + other := unassignedPod("warm-aws", "127.0.0.1", time.Now().Add(-10*time.Minute)) + other.Labels[LabelInstance] = "aws" + pool := newTestPool(t, other) + + _, _, err := pool.ClaimPod(t.Context(), "session-cross") + + require.Error(t, err) + assert.Contains(t, err.Error(), "no unassigned pods available") + }) + t.Run("waits for empty PodIP then claims", func(t *testing.T) { - // given — Ready pod whose IP is not yet reflected on the patched object ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) @@ -285,7 +123,7 @@ func TestClaimPod(t *testing.T) { pool := newTestPool(t, pod) pool.config.AgentPort = agentPortFromURL(t, ts.URL) pool.SetHTTPClient(ts.Client()) - ctx := context.Background() + ctx := t.Context() done := make(chan error, 1) go func() { @@ -296,7 +134,7 @@ func TestClaimPod(t *testing.T) { time.Sleep(20 * time.Millisecond) continue } - if _, claimed := p.Labels[labelSessionID]; !claimed { + if _, claimed := p.Labels[LabelSessionID]; !claimed { time.Sleep(20 * time.Millisecond) continue } @@ -308,33 +146,27 @@ func TestClaimPod(t *testing.T) { done <- fmt.Errorf("pod was not claimed before deadline") }() - // when ip, name, err := pool.ClaimPod(ctx, "session-wait-ip") - // then require.NoError(t, <-done) require.NoError(t, err) assert.Equal(t, "127.0.0.1", ip) assert.Equal(t, "warm-no-ip", name) }) - t.Run("returns error when pool exhausted", func(t *testing.T) { - // given — empty pool + t.Run("returns error when no unassigned pods exist", func(t *testing.T) { pool := newTestPool(t) - // when - _, _, err := pool.ClaimPod(context.Background(), "session-empty") + _, _, err := pool.ClaimPod(t.Context(), "session-empty") - // then require.Error(t, err) - assert.Contains(t, err.Error(), "warm pool exhausted") + assert.Contains(t, err.Error(), "no unassigned pods available") }) t.Run("rolls back label on auth secret already exists", func(t *testing.T) { - // given — pre-create a conflicting secret so the pool's Create returns AlreadyExists pod := unassignedPod("warm-rollback-1", "127.0.0.1", time.Now()) pool := newTestPool(t, pod) - ctx := context.Background() + ctx := t.Context() conflictingSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -346,21 +178,18 @@ func TestClaimPod(t *testing.T) { _, err := pool.clientset.CoreV1().Secrets(testNamespace).Create(ctx, conflictingSecret, metav1.CreateOptions{}) require.NoError(t, err) - // when — claim should fail because pre-existing secret is not trusted _, _, err = pool.ClaimPod(ctx, "session-conflict") - // then — should fail and roll back the label require.Error(t, err) - assert.Contains(t, err.Error(), "warm pool exhausted") + assert.Contains(t, err.Error(), "all unassigned claim attempts failed") rolledBack, getErr := pool.clientset.CoreV1().Pods(testNamespace).Get(ctx, "warm-rollback-1", metav1.GetOptions{}) require.NoError(t, getErr) - _, hasSession := rolledBack.Labels[labelSessionID] + _, hasSession := rolledBack.Labels[LabelSessionID] assert.False(t, hasSession, "session-id label should be removed on rollback") }) t.Run("rolls back on assign failure", func(t *testing.T) { - // given — agent returns 500 ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusInternalServerError) })) @@ -370,28 +199,23 @@ func TestClaimPod(t *testing.T) { pool := newTestPool(t, pod) pool.config.AgentPort = agentPortFromURL(t, ts.URL) pool.SetHTTPClient(ts.Client()) - ctx := context.Background() + ctx := t.Context() - // when _, _, err := pool.ClaimPod(ctx, "session-assign-fail") - // then require.Error(t, err) - assert.Contains(t, err.Error(), "warm pool exhausted") + assert.Contains(t, err.Error(), "all unassigned claim attempts failed") - // then — label should be rolled back (no session-id) rolledBack, getErr := pool.clientset.CoreV1().Pods(testNamespace).Get(ctx, "warm-assign-fail", metav1.GetOptions{}) require.NoError(t, getErr) - _, hasSession := rolledBack.Labels[labelSessionID] + _, hasSession := rolledBack.Labels[LabelSessionID] assert.False(t, hasSession, "session-id label should be removed on rollback") - // then — secret should be deleted _, secretErr := pool.clientset.CoreV1().Secrets(testNamespace).Get(ctx, secretNamePrefix+"session-assign-fail", metav1.GetOptions{}) assert.Error(t, secretErr, "secret should be deleted on rollback") }) t.Run("sends correct token in assign request", func(t *testing.T) { - // given cfg := newTestConfig() expectedToken := computeToken(cfg.HMACKey, "session-token-check") @@ -411,219 +235,60 @@ func TestClaimPod(t *testing.T) { pool.config.AgentPort = agentPortFromURL(t, ts.URL) pool.SetHTTPClient(ts.Client()) - // when - _, _, err := pool.ClaimPod(context.Background(), "session-token-check") + _, _, err := pool.ClaimPod(t.Context(), "session-token-check") - // then require.NoError(t, err) assert.Equal(t, expectedToken, receivedToken) }) - - t.Run("triggers replenishment after claim", func(t *testing.T) { - // given - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - })) - t.Cleanup(ts.Close) - - pod := unassignedPod("warm-replenish", "127.0.0.1", time.Now()) - pool := newTestPool(t, pod) - pool.config.AgentPort = agentPortFromURL(t, ts.URL) - pool.SetHTTPClient(ts.Client()) - - // when - _, _, err := pool.ClaimPod(context.Background(), "session-replenish") - require.NoError(t, err) - - // then — replenish channel should have a signal - select { - case <-pool.replenishCh: - // expected - default: - t.Fatal("expected replenish signal after claim") - } - }) } -func TestStartReconciler(t *testing.T) { - t.Run("falls back to 30s default on zero ReconcileInterval", func(t *testing.T) { - // given - pool := newTestPool(t) - pool.config.ReconcileInterval = 0 - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - // when — should not panic - pool.StartReconciler(ctx) - - // then — wait briefly and verify initial reconciliation ran - time.Sleep(200 * time.Millisecond) - pods, err := pool.clientset.CoreV1().Pods(testNamespace).List(ctx, metav1.ListOptions{ - LabelSelector: unassignedSelector(), - }) - require.NoError(t, err) - assert.Len(t, pods.Items, 3, "initial reconciliation should create 3 pods") - }) - - t.Run("falls back to 30s default on negative ReconcileInterval", func(t *testing.T) { - // given - pool := newTestPool(t) - pool.config.ReconcileInterval = -5 * time.Second - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - // when — should not panic - pool.StartReconciler(ctx) - - // then — wait briefly and verify initial reconciliation ran - time.Sleep(200 * time.Millisecond) - pods, err := pool.clientset.CoreV1().Pods(testNamespace).List(ctx, metav1.ListOptions{ - LabelSelector: unassignedSelector(), - }) - require.NoError(t, err) - assert.Len(t, pods.Items, 3) - }) - - t.Run("responds to replenish signal", func(t *testing.T) { - // given — pool with 1 pod (target is 3) - pod := unassignedPod("warm-existing", "10.0.0.1", time.Now()) - pool := newTestPool(t, pod) - pool.config.ReconcileInterval = 10 * time.Minute // long interval so ticker doesn't fire - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - pool.StartReconciler(ctx) - // wait for initial reconciliation to fill pool to 3 - time.Sleep(200 * time.Millisecond) - - // given — delete one pod to create a deficit - pods, err := pool.clientset.CoreV1().Pods(testNamespace).List(ctx, metav1.ListOptions{ - LabelSelector: unassignedSelector(), - }) - require.NoError(t, err) - require.NotEmpty(t, pods.Items) - err = pool.clientset.CoreV1().Pods(testNamespace).Delete(ctx, pods.Items[0].Name, metav1.DeleteOptions{}) - require.NoError(t, err) - - // when — trigger replenish to fill the deficit - pool.TriggerReplenish() - time.Sleep(200 * time.Millisecond) - - // then — should be back to 3 pods - pods, err = pool.clientset.CoreV1().Pods(testNamespace).List(ctx, metav1.ListOptions{ - LabelSelector: unassignedSelector(), - }) - require.NoError(t, err) - assert.Len(t, pods.Items, 3) - }) - - t.Run("exits on context cancellation", func(t *testing.T) { - // given - pool := newTestPool(t) - pool.config.ReconcileInterval = 50 * time.Millisecond - - ctx, cancel := context.WithCancel(context.Background()) - pool.StartReconciler(ctx) - time.Sleep(100 * time.Millisecond) - - // when - cancel() - time.Sleep(100 * time.Millisecond) - - // then — reconciler goroutine has exited (no way to assert directly, - // but no panic and clean shutdown is the test) - }) -} - -func TestTriggerReplenish(t *testing.T) { - t.Run("non-blocking when channel is full", func(t *testing.T) { - // given - pool := newTestPool(t) - - // when — fill the channel - pool.TriggerReplenish() - // second call should not block - pool.TriggerReplenish() - - // then — channel has exactly 1 signal - select { - case <-pool.replenishCh: - // expected - default: - t.Fatal("expected at least one signal in replenish channel") - } - - select { - case <-pool.replenishCh: - t.Fatal("expected only one signal (coalesced)") - default: - // expected - } - }) -} - -func TestGetOrCreatePodWithPool(t *testing.T) { - t.Run("claims from pool when available", func(t *testing.T) { - // given +func TestGetOrCreatePodAlwaysClaims(t *testing.T) { + t.Run("claims an unassigned pod even without a configured pool size", func(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) t.Cleanup(ts.Close) warmPod := unassignedPod("warm-for-session", "127.0.0.1", time.Now()) - client := fake.NewSimpleClientset() - ctx := context.Background() + ctx := t.Context() _, err := client.CoreV1().Pods(testNamespace).Create(ctx, &warmPod, metav1.CreateOptions{}) require.NoError(t, err) cfg := newTestConfig() - cfg.WarmPoolSize = 3 cfg.AgentPort = agentPortFromURL(t, ts.URL) mgr, err := NewSessionManager(client, cfg, slog.Default()) require.NoError(t, err) - require.NotNil(t, mgr.Pool(), "pool should be enabled when WarmPoolSize > 0") + require.NotNil(t, mgr.Pool()) mgr.Pool().SetHTTPClient(ts.Client()) - // when ip, getErr := mgr.GetOrCreatePod(ctx, "session-from-pool") - // then — should have claimed the warm pod require.NoError(t, getErr) assert.Equal(t, "127.0.0.1", ip) claimed, _ := client.CoreV1().Pods(testNamespace).Get(ctx, "warm-for-session", metav1.GetOptions{}) - assert.Equal(t, "session-from-pool", claimed.Labels[labelSessionID]) + assert.Equal(t, "session-from-pool", claimed.Labels[LabelSessionID]) }) - t.Run("falls back to on-demand when pool exhausted", func(t *testing.T) { - // given — pool enabled but empty + t.Run("falls back to on-demand when no unassigned pod exists", func(t *testing.T) { client := fake.NewSimpleClientset() - cfg := newTestConfig() - cfg.WarmPoolSize = 3 - - mgr, err := NewSessionManager(client, cfg, slog.Default()) + mgr, err := NewSessionManager(client, newTestConfig(), slog.Default()) require.NoError(t, err) - // when — pool claim fails; on-demand create then wait fails via request deadline - ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) + ctx, cancel := context.WithTimeout(t.Context(), 500*time.Millisecond) defer cancel() _, getErr := mgr.GetOrCreatePod(ctx, "session-fallback") - // then — on-demand path was taken (create + wait), then caller deadline fired require.Error(t, getErr) assert.Contains(t, getErr.Error(), "create sandbox pod") assert.Contains(t, getErr.Error(), "wait for pod ready") require.ErrorIs(t, getErr, context.DeadlineExceeded) - // then — caller abort does not delete; sibling replicas may still be waiting pods, listErr := client.CoreV1().Pods(testNamespace).List(context.Background(), metav1.ListOptions{ - LabelSelector: fmt.Sprintf("%s=%s", labelSessionID, "session-fallback"), + LabelSelector: AssignedSelector(testInstance, "session-fallback"), }) require.NoError(t, listErr) assert.NotEmpty(t, pods.Items, "pod should remain after request deadline for sibling waiters") @@ -631,8 +296,7 @@ func TestGetOrCreatePodWithPool(t *testing.T) { require.NoError(t, secretErr, "auth secret should remain after request deadline") }) - t.Run("leaves claimed warm pod when waitForReady hits request deadline", func(t *testing.T) { - // given — claimable pod (has IP for /assign) but not Ready + t.Run("leaves claimed pod when waitForReady hits request deadline", func(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) @@ -644,12 +308,11 @@ func TestGetOrCreatePodWithPool(t *testing.T) { } client := fake.NewSimpleClientset() - ctx := context.Background() + ctx := t.Context() _, err := client.CoreV1().Pods(testNamespace).Create(ctx, &warmPod, metav1.CreateOptions{}) require.NoError(t, err) cfg := newTestConfig() - cfg.WarmPoolSize = 3 cfg.AgentPort = agentPortFromURL(t, ts.URL) mgr, err := NewSessionManager(client, cfg, slog.Default()) @@ -659,39 +322,16 @@ func TestGetOrCreatePodWithPool(t *testing.T) { waitCtx, cancel := context.WithTimeout(ctx, 500*time.Millisecond) defer cancel() - // when _, getErr := mgr.GetOrCreatePod(waitCtx, "session-claim-not-ready") - // then — claim succeeded but request deadline aborted wait; leave pod+secret require.Error(t, getErr) - assert.Contains(t, getErr.Error(), "warm pool pod not ready after claim") + assert.Contains(t, getErr.Error(), "claimed pod not ready") require.ErrorIs(t, getErr, context.DeadlineExceeded) claimed, podErr := client.CoreV1().Pods(testNamespace).Get(context.Background(), "warm-not-ready", metav1.GetOptions{}) require.NoError(t, podErr, "claimed pod should remain after request deadline") - assert.Equal(t, "session-claim-not-ready", claimed.Labels[labelSessionID]) + assert.Equal(t, "session-claim-not-ready", claimed.Labels[LabelSessionID]) _, secretErr := client.CoreV1().Secrets(testNamespace).Get(context.Background(), secretNamePrefix+"session-claim-not-ready", metav1.GetOptions{}) require.NoError(t, secretErr, "auth secret should remain after request deadline") }) - - t.Run("pool disabled when WarmPoolSize is 0", func(t *testing.T) { - // given - client := fake.NewSimpleClientset() - cfg := newTestConfig() - cfg.WarmPoolSize = 0 - - mgr, err := NewSessionManager(client, cfg, slog.Default()) - require.NoError(t, err) - - // then - assert.Nil(t, mgr.Pool(), "pool should be nil when WarmPoolSize is 0") - }) -} - -func TestDefaultConfigReconcileInterval(t *testing.T) { - // given / when - cfg := DefaultConfig() - - // then - assert.Equal(t, 30*time.Second, cfg.ReconcileInterval) }