diff --git a/README.md b/README.md index 5a7f0bf..4c2cd7c 100644 --- a/README.md +++ b/README.md @@ -203,6 +203,7 @@ Supported fields include: - `imagePullSecrets` - `dnsConfig` - `securityContext` +- `command`, `args` - `workingDir` All fields are optional. @@ -239,6 +240,43 @@ browsers: Each browser version supports the same override fields as the template. +#### `command` and `args` + +`command` overrides the container's `ENTRYPOINT` and `args` overrides the container's `CMD`. These fields are available on three levels: + +- **Template** — sets defaults for the main browser container across all browsers/versions +- **Browser version** — overrides the template for a specific browser version +- **Sidecar / init container** — sets entrypoint and arguments per sidecar + +If `command` or `args` is `nil` at the browser version level, the value is inherited from the template. Sidecars inherit from the template sidecar with the same name. + +Example — starting a Playwright server with a custom entrypoint: + +```yaml +browsers: + playwright-chromium: + "1.59.1": + image: mcr.microsoft.com/playwright:v1.59.1 + command: + - "sh" + - "-c" + - "cd /opt/pw && exec ./node_modules/.bin/playwright-core run-server --port 4444 --host 0.0.0.0" +``` + +Example — passing CLI arguments to an MCP server image that has a default entrypoint: + +```yaml +browsers: + playwright-mcp: + "0.0.75": + image: mcr.microsoft.com/playwright/mcp:v0.0.75 + args: + - "--port" + - "8808" + - "--host" + - "0.0.0.0" +``` + --- ### Merge Semantics @@ -252,11 +290,17 @@ Configuration is merged in the following order (later overrides earlier): Rules: - `nil` fields inherit values from the template -- Maps and lists are **merged**, not replaced +- Maps and lists are **merged with deduplication**, not replaced — override wins on key conflict - Sidecars and init containers are merged by **name** - Environment variables are merged by **variable name** +- Volumes are merged by **name** +- Tolerations are merged by **key** +- Host aliases are merged by **IP** +- Image pull secrets are merged by **name** +- Container ports are merged by **container port number** +- Volume mounts are merged by **mount path** -This ensures predictable and reusable configuration without duplication. +This ensures predictable and reusable configuration without duplication or rejected Pods due to duplicate entries. --- @@ -338,7 +382,11 @@ Most failure paths follow a **two-step process** across two reconcile cycles: | Scenario | Pod | Browser CR | |---|---|---| | No matching `BrowserConfig` | never created | set to `Failed` → next reconcile: **deleted** | -| `podCreationTimeout` exceeded (pod stuck `Pending` > 5 min) | force-deleted (grace=0) | set to `Failed` → next reconcile: **deleted** | +| Pod creation blocked by `ResourceQuota` (403 Forbidden) | never created | set to `Failed / QuotaExceeded` immediately → next reconcile: **deleted** | +| `browserPendingTimeout` exceeded (Browser stayed `Pending` without a Pod longer than the timeout) | never created | set to `Failed / PendingTimeoutExceeded` → next reconcile: **deleted** | +| `podCreationTimeout` exceeded (pod stuck `Pending` > 5 min) — applies to both init containers and regular containers | force-deleted (grace=0) | set to `Failed` → next reconcile: **deleted** | +| `PodPending` + init container `Terminated` with non-zero exit code | force-deleted (grace=0) | set to `Failed` → next reconcile: **deleted** | +| `PodPending` + init container `Waiting` with non-transient reason (`ErrImagePull`, `ImagePullBackOff`, etc.) | force-deleted (grace=0) | set to `Failed` → next reconcile: **deleted** | | `PodPending` + container `Terminated` | force-deleted (grace=0) | set to `Failed` → next reconcile: **deleted** | | `PodPending` + container `Waiting` with non-transient reason (`CrashLoopBackOff`, `ErrImagePull`, `ImagePullBackOff`, etc.) | force-deleted (grace=0) | set to `Failed` → next reconcile: **deleted** | | Pod phase `Failed` | force-deleted (grace=0) | set to `Failed` → next reconcile: **deleted** | @@ -355,15 +403,49 @@ Every failure path writes a human-readable message to `Browser.status.message` b | Cause | Example message | |---|---| | No `BrowserConfig` | `Browser configuration not found` | +| Quota exceeded | `pods "b1" is forbidden: exceeded quota: ...` | +| Pending timeout exceeded | `Browser did not start within 5m0s` | | Creation timeout | `pod creation timeout exceeded after 5m0s, container browser: ContainerCreating` | +| Init container creation timeout | `pod creation timeout exceeded after 5m0s, init container init-setup: PodInitializing` | +| Init container terminated | `pod init container init-setup terminated: Error (exit code 1)` | +| Init container not ready | `Browser pod init container init-setup failed: ImagePullBackOff - back-off pulling image` | | Container terminated | `pod container browser terminated: OOMKilled (exit code 137)` | -| Container not ready | `pod container browser failed: CrashLoopBackOff - back-off restarting failed container` | -| Pod failed | `pod has failed with reason: OOMKilled - container exceeded memory limit` | +| Container not ready | `Browser pod container browser failed: CrashLoopBackOff - back-off restarting failed container` | +| Pod failed | `Browser pod has failed with reason: OOMKilled - container exceeded memory limit` | This message is available in `Browser.status` until the CR is removed and is propagated as an SSE event by `browser-service`, making it observable to clients before the CR disappears. --- +## Configuration Flags + +The controller binary accepts the following flags: + +| Flag | Default | Description | +|---|---|---| +| `--metrics-addr` | `:8080` | Address the metrics endpoint binds to | +| `--health-probe-bind-address` | `:8081` | Address the health/readiness probe binds to | +| `--enable-leader-election` | `false` | Enable leader election for HA deployments | +| `--browser-pod-creation-timeout` | `5m` | How long the controller waits for a new Pod to leave `Pending` before force-deleting it and marking the `Browser` as `Failed` | +| `--browser-pod-deletion-timeout` | `5m` | How long the controller waits for a Pod to finish terminating before issuing a force-delete | +| `--browser-pending-timeout` | `0` (disabled) | How long a `Browser` may stay in `Pending` without an associated Pod before being marked `Failed / PendingTimeoutExceeded`. Useful when Pod creation is blocked by `ResourceQuota` and the backlog needs a deterministic cap. Set to `0` to disable. | +| `--max-retries` | `3` | Max retries for conflict resolution on Browser patch and status updates | +| `--max-workers` | `4` | Max concurrent reconcile workers for the Browser controller | +| `--rate-limiter-base-delay` | `100ms` | Base delay for the exponential failure rate limiter | +| `--rate-limiter-max-delay` | `30s` | Max delay for the exponential failure rate limiter | + +--- + +### `--browser-pending-timeout` in detail + +When the cluster runs out of Pod quota (or another admission plugin blocks Pod creation), the controller keeps retrying and the `Browser` CR stays in `Pending` indefinitely. `--browser-pending-timeout` gives the controller a deterministic deadline: if a `Browser` CR has lived without a Pod for longer than the configured duration, the next reconcile marks it `Failed` with reason `PendingTimeoutExceeded` and message `Browser did not start within `. On the following reconcile the standard failure path removes the CR. + +The timeout is measured against `Browser.metadata.creationTimestamp`, not the time the controller first saw the CR, so a restart of the controller does not reset the window. + +The default is `0`, which disables the timeout entirely and preserves the previous behavior of retrying forever. + +--- + ## Build & Generate This project uses `make` to generate code, manifests, and build the controller image. @@ -400,6 +482,22 @@ Or combined: make deploy ``` +### Build variables + +The build process is controlled via the following Makefile variables: + +| Variable | Description | +|------------------|--------------------------------------------------------------| +| `BINARY_NAME` | Name of the produced binary (fixed: `browser-controller`) | +| `REGISTRY` | Docker registry prefix (default: `localhost:5000`) | +| `IMAGE_NAME` | Full image name, derived as `$(REGISTRY)/$(BINARY_NAME)` | +| `VERSION` | Image version/tag (default: `develop`) | +| `EXTRA_TAGS` | Additional `-t` tags passed to `docker-push` (default: none) | +| `PLATFORM` | Target platform (default: `linux/amd64`) | +| `CONTAINER_TOOL` | Container build tool (default: `docker`) | + +`REGISTRY` and `VERSION` are expected to be provided externally, which allows the same Makefile to be used locally and in CI. + ## Deployment Helm chart [selenosis-deploy](https://github.com/alcounit/selenosis-deploy) diff --git a/apis/browser/v1/selenosis_keys.go b/apis/browser/v1/selenosis_keys.go index 958de59..c940957 100644 --- a/apis/browser/v1/selenosis_keys.go +++ b/apis/browser/v1/selenosis_keys.go @@ -3,4 +3,7 @@ package v1 var ( SelenosisOptionsAnnotationKey = "selenosis.io/options" SelenosisOwnerLabelKey = "selenosis.io/owner" + BrowserLabelKey = "selenosis.io/browser" + BrowserNameLabelKey = "selenosis.io/browser.name" + BrowserVersionLabelKey = "selenosis.io/browser.version" ) diff --git a/apis/browserconfig/v1/browser_config.go b/apis/browserconfig/v1/browser_config.go index fe0dd05..3d3e692 100644 --- a/apis/browserconfig/v1/browser_config.go +++ b/apis/browserconfig/v1/browser_config.go @@ -48,7 +48,7 @@ type Template struct { // Resources defines CPU/memory requests and limits for the main container. Resources *corev1.ResourceRequirements `json:"resources,omitempty"` - //ImagePullPolicy defines container image pull policy + // ImagePullPolicy defines container image pull policy. ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"` // Volumes defines pod volumes. @@ -69,7 +69,7 @@ type Template struct { // HostAliases defines custom /etc/hosts entries. HostAliases *[]corev1.HostAlias `json:"hostAliases,omitempty"` - // List of initialization containers belonging to the pod. + // InitContainers defines initialization containers for the pod. InitContainers *[]Sidecar `json:"initContainers,omitempty"` // Sidecars defines additional containers in the pod (minimum 1). @@ -89,8 +89,13 @@ type Template struct { // SecurityContext defines security context for the pod. SecurityContext *corev1.PodSecurityContext `json:"securityContext,omitempty"` - // Container's working directory. - // +optional + // Command overrides the entrypoint of the main browser container. + Command *[]string `json:"command,omitempty"` + + // Args overrides the arguments passed to the main browser container entrypoint. + Args *[]string `json:"args,omitempty"` + + // WorkingDir sets the working directory for the main browser container. WorkingDir *string `json:"workingDir,omitempty"` } @@ -102,10 +107,13 @@ type Sidecar struct { // Image is the container image. Image string `json:"image"` - // Command overrides the container entrypoint.Template. + // Command overrides the container entrypoint. Command *[]string `json:"command,omitempty"` - // Container's working directory. + // Args overrides the arguments passed to the container entrypoint. + Args *[]string `json:"args,omitempty"` + + // WorkingDir sets the working directory for the container. WorkingDir *string `json:"workingDir,omitempty"` // Ports defines container ports. @@ -117,7 +125,7 @@ type Sidecar struct { // VolumeMounts defines mounts for pod volumes. VolumeMounts *[]corev1.VolumeMount `json:"volumeMounts,omitempty"` - //ImagePullPolicy defines container image pull policy + // ImagePullPolicy defines container image pull policy. ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"` // Resources defines CPU/memory requests and limits for the sidecar container. @@ -130,24 +138,65 @@ type BrowserVersionConfigSpec struct { // Image is the browser container image. Image string `json:"image"` - Labels *map[string]string `json:"labels,omitempty"` - Annotations *map[string]string `json:"annotations,omitempty"` - Env *[]corev1.EnvVar `json:"env,omitempty"` - Resources *corev1.ResourceRequirements `json:"resources,omitempty"` - ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"` - Volumes *[]corev1.Volume `json:"volumes,omitempty"` - VolumeMounts *[]corev1.VolumeMount `json:"volumeMounts,omitempty"` - NodeSelector *map[string]string `json:"nodeSelector,omitempty"` - Affinity *corev1.Affinity `json:"affinity,omitempty"` - Tolerations *[]corev1.Toleration `json:"tolerations,omitempty"` - HostAliases *[]corev1.HostAlias `json:"hostAliases,omitempty"` - InitContainers *[]Sidecar `json:"initContainers,omitempty"` - Sidecars *[]Sidecar `json:"sidecars,omitempty"` - Privileged *bool `json:"privileged,omitempty"` + // Labels are additional pod labels. + Labels *map[string]string `json:"labels,omitempty"` + + // Annotations are additional pod annotations. + Annotations *map[string]string `json:"annotations,omitempty"` + + // Env defines environment variables for the main container. + Env *[]corev1.EnvVar `json:"env,omitempty"` + + // Resources defines CPU/memory requests and limits for the main container. + Resources *corev1.ResourceRequirements `json:"resources,omitempty"` + + // ImagePullPolicy defines container image pull policy. + ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"` + + // Volumes defines pod volumes. + Volumes *[]corev1.Volume `json:"volumes,omitempty"` + + // VolumeMounts defines mounts for pod volumes. + VolumeMounts *[]corev1.VolumeMount `json:"volumeMounts,omitempty"` + + // NodeSelector defines node selection constraints. + NodeSelector *map[string]string `json:"nodeSelector,omitempty"` + + // Affinity defines pod affinity/anti-affinity rules. + Affinity *corev1.Affinity `json:"affinity,omitempty"` + + // Tolerations defines tolerations for node taints. + Tolerations *[]corev1.Toleration `json:"tolerations,omitempty"` + + // HostAliases defines custom /etc/hosts entries. + HostAliases *[]corev1.HostAlias `json:"hostAliases,omitempty"` + + // InitContainers defines initialization containers for the pod. + InitContainers *[]Sidecar `json:"initContainers,omitempty"` + + // Sidecars defines additional containers in the pod. + Sidecars *[]Sidecar `json:"sidecars,omitempty"` + + // Privileged indicates if the main container should run in privileged mode. + Privileged *bool `json:"privileged,omitempty"` + + // ImagePullSecrets specifies secrets for pulling private images. ImagePullSecrets *[]corev1.LocalObjectReference `json:"imagePullSecrets,omitempty"` - DNSConfig *corev1.PodDNSConfig `json:"dnsConfig,omitempty"` - SecurityContext *corev1.PodSecurityContext `json:"securityContext,omitempty"` - WorkingDir *string `json:"workingDir,omitempty"` + + // DNSConfig defines pod-level DNS settings. + DNSConfig *corev1.PodDNSConfig `json:"dnsConfig,omitempty"` + + // SecurityContext defines security context for the pod. + SecurityContext *corev1.PodSecurityContext `json:"securityContext,omitempty"` + + // Command overrides the entrypoint of the main browser container. + Command *[]string `json:"command,omitempty"` + + // Args overrides the arguments passed to the main browser container entrypoint. + Args *[]string `json:"args,omitempty"` + + // WorkingDir sets the working directory for the main browser container. + WorkingDir *string `json:"workingDir,omitempty"` } // ConfigStatus defines the observed state of BrowserConfig. @@ -257,6 +306,14 @@ func (b *BrowserVersionConfigSpec) mergeWithSpec(t *BrowserConfigSpec) { b.SecurityContext = t.Template.SecurityContext } + if b.Command == nil { + b.Command = t.Template.Command + } + + if b.Args == nil { + b.Args = t.Template.Args + } + if b.WorkingDir == nil { b.WorkingDir = t.Template.WorkingDir } @@ -322,54 +379,102 @@ func firstNonNilResource(template, override *corev1.ResourceRequirements) *corev } func mergeVolumePtr(template, override *[]corev1.Volume) *[]corev1.Volume { - result := []corev1.Volume{} + if template == nil && override == nil { + return nil + } + + index := make(map[string]int) + merged := make([]corev1.Volume, 0) + if template != nil { - result = append(result, *template...) + for _, v := range *template { + index[v.Name] = len(merged) + merged = append(merged, v) + } } if override != nil { - result = append(result, *override...) + for _, v := range *override { + if i, exists := index[v.Name]; exists { + merged[i] = v + } else { + index[v.Name] = len(merged) + merged = append(merged, v) + } + } } - if len(result) == 0 { + if len(merged) == 0 { return nil } - return &result + return &merged } func mergeTolerationPtr(template, override *[]corev1.Toleration) *[]corev1.Toleration { - result := []corev1.Toleration{} + if template == nil && override == nil { + return nil + } + + index := make(map[string]int) + merged := make([]corev1.Toleration, 0) + if template != nil { - result = append(result, *template...) + for _, t := range *template { + index[t.Key] = len(merged) + merged = append(merged, t) + } } if override != nil { - result = append(result, *override...) + for _, t := range *override { + if i, exists := index[t.Key]; exists { + merged[i] = t + } else { + index[t.Key] = len(merged) + merged = append(merged, t) + } + } } - if len(result) == 0 { + if len(merged) == 0 { return nil } - return &result + return &merged } func mergeHostAliasPtr(template, override *[]corev1.HostAlias) *[]corev1.HostAlias { - result := []corev1.HostAlias{} + if template == nil && override == nil { + return nil + } + + index := make(map[string]int) + merged := make([]corev1.HostAlias, 0) + if template != nil { - result = append(result, *template...) + for _, h := range *template { + index[h.IP] = len(merged) + merged = append(merged, h) + } } if override != nil { - result = append(result, *override...) + for _, h := range *override { + if i, exists := index[h.IP]; exists { + merged[i] = h + } else { + index[h.IP] = len(merged) + merged = append(merged, h) + } + } } - if len(result) == 0 { + if len(merged) == 0 { return nil } - return &result + return &merged } func mergeSidecarPtr(template, override *[]Sidecar) *[]Sidecar { @@ -399,44 +504,71 @@ func mergeSidecarPtr(template, override *[]Sidecar) *[]Sidecar { } func mergeVolumeMountsPtr(template, override *[]corev1.VolumeMount) *[]corev1.VolumeMount { - result := []corev1.VolumeMount{} + if template == nil && override == nil { + return nil + } + + index := make(map[string]int) + merged := make([]corev1.VolumeMount, 0) if template != nil { for _, t := range *template { - copy := t.DeepCopy() - result = append(result, *copy) + cp := t.DeepCopy() + index[cp.MountPath] = len(merged) + merged = append(merged, *cp) } } if override != nil { for _, o := range *override { - copy := o.DeepCopy() - result = append(result, *copy) + cp := o.DeepCopy() + if i, exists := index[cp.MountPath]; exists { + merged[i] = *cp + } else { + index[cp.MountPath] = len(merged) + merged = append(merged, *cp) + } } } - if len(result) == 0 { + if len(merged) == 0 { return nil } - return &result + return &merged } func mergeLocalObjectRefPtr(template, override *[]corev1.LocalObjectReference) *[]corev1.LocalObjectReference { - result := []corev1.LocalObjectReference{} + if template == nil && override == nil { + return nil + } + + seen := make(map[string]int) + merged := make([]corev1.LocalObjectReference, 0) + if template != nil { - result = append(result, *template...) + for _, r := range *template { + seen[r.Name] = len(merged) + merged = append(merged, r) + } } if override != nil { - result = append(result, *override...) + for _, r := range *override { + if i, exists := seen[r.Name]; exists { + merged[i] = r + } else { + seen[r.Name] = len(merged) + merged = append(merged, r) + } + } } - if len(result) == 0 { + if len(merged) == 0 { return nil } - return &result + return &merged } func (s *Sidecar) mergeWithTemplate(t *Sidecar) { @@ -444,6 +576,10 @@ func (s *Sidecar) mergeWithTemplate(t *Sidecar) { s.Command = t.Command } + if s.Args == nil { + s.Args = t.Args + } + if s.WorkingDir == nil { s.WorkingDir = t.WorkingDir } @@ -458,37 +594,69 @@ func (s *Sidecar) mergeWithTemplate(t *Sidecar) { } func mergeContainerPortPtr(template, override *[]corev1.ContainerPort) *[]corev1.ContainerPort { - result := []corev1.ContainerPort{} + if template == nil && override == nil { + return nil + } + + index := make(map[int32]int) + merged := make([]corev1.ContainerPort, 0) + if template != nil { - result = append(result, *template...) + for _, p := range *template { + index[p.ContainerPort] = len(merged) + merged = append(merged, p) + } } if override != nil { - result = append(result, *override...) + for _, p := range *override { + if i, exists := index[p.ContainerPort]; exists { + merged[i] = p + } else { + index[p.ContainerPort] = len(merged) + merged = append(merged, p) + } + } } - if len(result) == 0 { + if len(merged) == 0 { return nil } - return &result + return &merged } func mergeVolumeMountPtr(template, override *[]corev1.VolumeMount) *[]corev1.VolumeMount { - result := []corev1.VolumeMount{} + if template == nil && override == nil { + return nil + } + + index := make(map[string]int) + merged := make([]corev1.VolumeMount, 0) + if template != nil { - result = append(result, *template...) + for _, m := range *template { + index[m.MountPath] = len(merged) + merged = append(merged, m) + } } if override != nil { - result = append(result, *override...) + for _, m := range *override { + if i, exists := index[m.MountPath]; exists { + merged[i] = m + } else { + index[m.MountPath] = len(merged) + merged = append(merged, m) + } + } } - if len(result) == 0 { + if len(merged) == 0 { return nil } - return &result + return &merged } func findTemplateSidecar(template *[]Sidecar, name string) *Sidecar { diff --git a/apis/browserconfig/v1/browser_config_test.go b/apis/browserconfig/v1/browser_config_test.go index cbd7f5e..6794462 100644 --- a/apis/browserconfig/v1/browser_config_test.go +++ b/apis/browserconfig/v1/browser_config_test.go @@ -195,6 +195,7 @@ func TestBrowserVersionConfigSpecMergeWithSpecMergesAllPaths(t *testing.T) { templateMounts := []corev1.VolumeMount{{Name: "template-vol", MountPath: "/template"}} overrideMounts := []corev1.VolumeMount{{Name: "override-vol", MountPath: "/override"}} templateCommand := []string{"tmpl-cmd"} + templateSidecarArgs := []string{"tmpl-sc-arg"} templateSidecarEnv := []corev1.EnvVar{{Name: "TMPL_SC_ENV", Value: "1"}} overrideSidecarEnv := []corev1.EnvVar{{Name: "OVR_SC_ENV", Value: "1"}} templatePorts := []corev1.ContainerPort{{ContainerPort: 8080}} @@ -204,6 +205,7 @@ func TestBrowserVersionConfigSpecMergeWithSpecMergesAllPaths(t *testing.T) { Name: "shared-sidecar", Image: "template-sc", Command: &templateCommand, + Args: &templateSidecarArgs, WorkingDir: strPtr("/template-sidecar-workdir"), Env: &templateSidecarEnv, Ports: &templatePorts, @@ -238,6 +240,7 @@ func TestBrowserVersionConfigSpecMergeWithSpecMergesAllPaths(t *testing.T) { templateDNSConfig := &corev1.PodDNSConfig{} templateSecurityContext := &corev1.PodSecurityContext{} templateWorkingDir := "/template-workdir" + templateArgs := []string{"tmpl-arg"} spec := BrowserConfigSpec{ Template: &Template{ @@ -258,6 +261,8 @@ func TestBrowserVersionConfigSpecMergeWithSpecMergesAllPaths(t *testing.T) { ImagePullSecrets: &templatePullSecrets, DNSConfig: templateDNSConfig, SecurityContext: templateSecurityContext, + Command: &templateCommand, + Args: &templateArgs, WorkingDir: &templateWorkingDir, }, } @@ -292,6 +297,12 @@ func TestBrowserVersionConfigSpecMergeWithSpecMergesAllPaths(t *testing.T) { if b.DNSConfig != templateDNSConfig || b.SecurityContext != templateSecurityContext || b.WorkingDir == nil || *b.WorkingDir != templateWorkingDir { t.Fatalf("expected dns/securityContext/workingDir to inherit from template") } + if b.Command == nil || len(*b.Command) != 1 || (*b.Command)[0] != "tmpl-cmd" { + t.Fatalf("expected command to inherit from template, got %+v", b.Command) + } + if b.Args == nil || len(*b.Args) != 1 || (*b.Args)[0] != "tmpl-arg" { + t.Fatalf("expected args to inherit from template, got %+v", b.Args) + } if b.Labels == nil || (*b.Labels)["from-template"] != "1" || (*b.Labels)["from-override"] != "1" { t.Fatalf("expected labels to merge, got %+v", b.Labels) @@ -327,6 +338,9 @@ func TestBrowserVersionConfigSpecMergeWithSpecMergesAllPaths(t *testing.T) { if shared.Command == nil || len(*shared.Command) != 1 || (*shared.Command)[0] != "tmpl-cmd" { t.Fatalf("expected shared sidecar command from template, got %+v", shared.Command) } + if shared.Args == nil || len(*shared.Args) != 1 || (*shared.Args)[0] != "tmpl-sc-arg" { + t.Fatalf("expected shared sidecar args from template, got %+v", shared.Args) + } if shared.Resources != &templateResources { t.Fatalf("expected shared sidecar resources from template") } @@ -414,6 +428,7 @@ func TestFindTemplateSidecar(t *testing.T) { func TestSidecarMergeWithTemplate(t *testing.T) { templateCommand := []string{"run"} + templateArgs := []string{"--flag"} templateEnv := []corev1.EnvVar{{Name: "TMPL", Value: "1"}} overrideEnv := []corev1.EnvVar{{Name: "OVR", Value: "1"}} templatePorts := []corev1.ContainerPort{{ContainerPort: 8080}} @@ -427,6 +442,7 @@ func TestSidecarMergeWithTemplate(t *testing.T) { tmpl := Sidecar{ Name: "s", Command: &templateCommand, + Args: &templateArgs, WorkingDir: strPtr("/work"), Env: &templateEnv, Ports: &templatePorts, @@ -439,6 +455,9 @@ func TestSidecarMergeWithTemplate(t *testing.T) { if s.Command == nil || len(*s.Command) != 1 || (*s.Command)[0] != "run" { t.Fatalf("expected command from template, got %+v", s.Command) } + if s.Args == nil || len(*s.Args) != 1 || (*s.Args)[0] != "--flag" { + t.Fatalf("expected args from template, got %+v", s.Args) + } if s.WorkingDir == nil || *s.WorkingDir != "/work" { t.Fatalf("expected workingDir from template, got %+v", s.WorkingDir) } @@ -472,6 +491,200 @@ func TestMergeContainerPortPtrAndMergeVolumeMountPtr(t *testing.T) { } } +func TestMergeVolumePtrDedupsByName(t *testing.T) { + template := []corev1.Volume{ + {Name: "shared", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}, + {Name: "template-only"}, + } + override := []corev1.Volume{ + {Name: "shared", VolumeSource: corev1.VolumeSource{HostPath: &corev1.HostPathVolumeSource{Path: "/data"}}}, + {Name: "override-only"}, + } + + merged := mergeVolumePtr(&template, &override) + if merged == nil || len(*merged) != 3 { + t.Fatalf("expected 3 volumes after dedup, got %d", len(*merged)) + } + for _, v := range *merged { + if v.Name == "shared" && v.HostPath == nil { + t.Fatalf("expected override to win for shared volume") + } + } +} + +func TestMergeTolerationPtrDedupsByKey(t *testing.T) { + template := []corev1.Toleration{ + {Key: "shared", Value: "template", Effect: corev1.TaintEffectNoSchedule}, + {Key: "template-only"}, + } + override := []corev1.Toleration{ + {Key: "shared", Value: "override", Effect: corev1.TaintEffectNoExecute}, + {Key: "override-only"}, + } + + merged := mergeTolerationPtr(&template, &override) + if merged == nil || len(*merged) != 3 { + t.Fatalf("expected 3 tolerations after dedup, got %d", len(*merged)) + } + for _, tol := range *merged { + if tol.Key == "shared" && tol.Value != "override" { + t.Fatalf("expected override to win for shared toleration key") + } + } +} + +func TestMergeHostAliasPtrDedupsByIP(t *testing.T) { + template := []corev1.HostAlias{ + {IP: "10.0.0.1", Hostnames: []string{"template-host"}}, + {IP: "10.0.0.2", Hostnames: []string{"template-only"}}, + } + override := []corev1.HostAlias{ + {IP: "10.0.0.1", Hostnames: []string{"override-host"}}, + {IP: "10.0.0.3", Hostnames: []string{"override-only"}}, + } + + merged := mergeHostAliasPtr(&template, &override) + if merged == nil || len(*merged) != 3 { + t.Fatalf("expected 3 hostAliases after dedup, got %d", len(*merged)) + } + for _, h := range *merged { + if h.IP == "10.0.0.1" && h.Hostnames[0] != "override-host" { + t.Fatalf("expected override to win for shared IP") + } + } +} + +func TestMergeLocalObjectRefPtrDedupsByName(t *testing.T) { + template := []corev1.LocalObjectReference{{Name: "shared"}, {Name: "template-only"}} + override := []corev1.LocalObjectReference{{Name: "shared"}, {Name: "override-only"}} + + merged := mergeLocalObjectRefPtr(&template, &override) + if merged == nil || len(*merged) != 3 { + t.Fatalf("expected 3 refs after dedup, got %d", len(*merged)) + } +} + +func TestMergeContainerPortPtrDedupsByPort(t *testing.T) { + template := []corev1.ContainerPort{{Name: "http", ContainerPort: 80}, {ContainerPort: 9090}} + override := []corev1.ContainerPort{{Name: "http-override", ContainerPort: 80}, {ContainerPort: 443}} + + merged := mergeContainerPortPtr(&template, &override) + if merged == nil || len(*merged) != 3 { + t.Fatalf("expected 3 ports after dedup, got %d", len(*merged)) + } + for _, p := range *merged { + if p.ContainerPort == 80 && p.Name != "http-override" { + t.Fatalf("expected override to win for port 80") + } + } +} + +func TestMergeVolumeMountsPtrDedupsByMountPath(t *testing.T) { + template := []corev1.VolumeMount{{Name: "vol-a", MountPath: "/shared"}, {Name: "vol-b", MountPath: "/template"}} + override := []corev1.VolumeMount{{Name: "vol-c", MountPath: "/shared"}, {Name: "vol-d", MountPath: "/override"}} + + merged := mergeVolumeMountsPtr(&template, &override) + if merged == nil || len(*merged) != 3 { + t.Fatalf("expected 3 mounts after dedup, got %d", len(*merged)) + } + for _, m := range *merged { + if m.MountPath == "/shared" && m.Name != "vol-c" { + t.Fatalf("expected override to win for mount path /shared") + } + } +} + +func TestMergeVolumeMountPtrDedupsByMountPath(t *testing.T) { + template := []corev1.VolumeMount{{Name: "vol-a", MountPath: "/shared"}} + override := []corev1.VolumeMount{{Name: "vol-b", MountPath: "/shared"}} + + merged := mergeVolumeMountPtr(&template, &override) + if merged == nil || len(*merged) != 1 { + t.Fatalf("expected 1 mount after dedup, got %d", len(*merged)) + } + if (*merged)[0].Name != "vol-b" { + t.Fatalf("expected override to win, got %q", (*merged)[0].Name) + } +} + +func TestMergeWithSpecCommandArgsInheritFromTemplate(t *testing.T) { + templateCmd := []string{"entrypoint"} + templateArgs := []string{"--verbose"} + spec := BrowserConfigSpec{ + Template: &Template{ + Command: &templateCmd, + Args: &templateArgs, + }, + Browsers: map[string]map[string]*BrowserVersionConfigSpec{ + "chrome": { + "130.0": {Image: "chrome:130"}, + }, + }, + } + + spec.MergeWithTemplate() + + cfg := spec.Browsers["chrome"]["130.0"] + if cfg.Command == nil || (*cfg.Command)[0] != "entrypoint" { + t.Fatalf("expected command to inherit from template, got %+v", cfg.Command) + } + if cfg.Args == nil || (*cfg.Args)[0] != "--verbose" { + t.Fatalf("expected args to inherit from template, got %+v", cfg.Args) + } +} + +func TestMergeWithSpecCommandArgsOverridePreserved(t *testing.T) { + templateCmd := []string{"entrypoint"} + templateArgs := []string{"--verbose"} + overrideCmd := []string{"custom-cmd"} + overrideArgs := []string{"--custom"} + spec := BrowserConfigSpec{ + Template: &Template{ + Command: &templateCmd, + Args: &templateArgs, + }, + Browsers: map[string]map[string]*BrowserVersionConfigSpec{ + "chrome": { + "131.0": { + Image: "chrome:131", + Command: &overrideCmd, + Args: &overrideArgs, + }, + }, + }, + } + + spec.MergeWithTemplate() + + cfg := spec.Browsers["chrome"]["131.0"] + if cfg.Command == nil || (*cfg.Command)[0] != "custom-cmd" { + t.Fatalf("expected override command to be preserved, got %+v", cfg.Command) + } + if cfg.Args == nil || (*cfg.Args)[0] != "--custom" { + t.Fatalf("expected override args to be preserved, got %+v", cfg.Args) + } +} + +func TestSidecarMergeWithTemplateArgsOverridePreserved(t *testing.T) { + templateArgs := []string{"--template"} + overrideArgs := []string{"--override"} + + s := Sidecar{ + Name: "s", + Args: &overrideArgs, + } + tmpl := Sidecar{ + Name: "s", + Args: &templateArgs, + } + + s.mergeWithTemplate(&tmpl) + + if s.Args == nil || (*s.Args)[0] != "--override" { + t.Fatalf("expected override args to be preserved, got %+v", s.Args) + } +} + func strPtr(v string) *string { return &v } diff --git a/apis/browserconfig/v1/zz_generated.deepcopy.go b/apis/browserconfig/v1/zz_generated.deepcopy.go index dec5890..391f80e 100644 --- a/apis/browserconfig/v1/zz_generated.deepcopy.go +++ b/apis/browserconfig/v1/zz_generated.deepcopy.go @@ -278,6 +278,24 @@ func (in *BrowserVersionConfigSpec) DeepCopyInto(out *BrowserVersionConfigSpec) *out = new(corev1.PodSecurityContext) (*in).DeepCopyInto(*out) } + if in.Command != nil { + in, out := &in.Command, &out.Command + *out = new([]string) + if **in != nil { + in, out := *in, *out + *out = make([]string, len(*in)) + copy(*out, *in) + } + } + if in.Args != nil { + in, out := &in.Args, &out.Args + *out = new([]string) + if **in != nil { + in, out := *in, *out + *out = make([]string, len(*in)) + copy(*out, *in) + } + } if in.WorkingDir != nil { in, out := &in.WorkingDir, &out.WorkingDir *out = new(string) @@ -323,6 +341,15 @@ func (in *Sidecar) DeepCopyInto(out *Sidecar) { copy(*out, *in) } } + if in.Args != nil { + in, out := &in.Args, &out.Args + *out = new([]string) + if **in != nil { + in, out := *in, *out + *out = make([]string, len(*in)) + copy(*out, *in) + } + } if in.WorkingDir != nil { in, out := &in.WorkingDir, &out.WorkingDir *out = new(string) @@ -523,6 +550,24 @@ func (in *Template) DeepCopyInto(out *Template) { *out = new(corev1.PodSecurityContext) (*in).DeepCopyInto(*out) } + if in.Command != nil { + in, out := &in.Command, &out.Command + *out = new([]string) + if **in != nil { + in, out := *in, *out + *out = make([]string, len(*in)) + copy(*out, *in) + } + } + if in.Args != nil { + in, out := &in.Args, &out.Args + *out = new([]string) + if **in != nil { + in, out := *in, *out + *out = make([]string, len(*in)) + copy(*out, *in) + } + } if in.WorkingDir != nil { in, out := &in.WorkingDir, &out.WorkingDir *out = new(string) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 3434212..d29c9d8 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -33,12 +33,22 @@ func init() { } func main() { - var metricsAddr string - var enableLeaderElection bool - var probeAddr string + var ( + metricsAddr string + enableLeaderElection bool + probeAddr string + reconcilerCfg browser.ReconcilerConfig + ) flag.StringVar(&metricsAddr, "metrics-addr", ":8080", "The address the metric endpoint binds to.") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") + flag.DurationVar(&reconcilerCfg.PodCreationTimeout, "browser-pod-creation-timeout", time.Minute*5, "The timeout for browser pod creation.") + flag.DurationVar(&reconcilerCfg.PodDeletionTimeout, "browser-pod-deletion-timeout", time.Minute*5, "The timeout for browser pod deletion.") + flag.DurationVar(&reconcilerCfg.PendingTimeout, "browser-pending-timeout", 0, "Max time a Browser can stay Pending without a Pod (0 = disabled).") + flag.IntVar(&reconcilerCfg.MaxRetries, "max-retries", 3, "Max retries for conflict resolution on Browser status updates.") + flag.IntVar(&reconcilerCfg.MaxWorkers, "max-workers", 4, "Max concurrent reconcile workers for the Browser controller.") + flag.DurationVar(&reconcilerCfg.RateLimiterBaseDelay, "rate-limiter-base-delay", 100*time.Millisecond, "Base delay for the exponential failure rate limiter.") + flag.DurationVar(&reconcilerCfg.RateLimiterMaxDelay, "rate-limiter-max-delay", 30*time.Second, "Max delay for the exponential failure rate limiter.") flag.BoolVar(&enableLeaderElection, "enable-leader-election", false, "Enable leader election for controller manager.") flag.Parse() @@ -88,7 +98,7 @@ func main() { } // Add Browser controller - browserCtrl := browser.NewBrowserReconciler(mgr.GetClient(), browserCfgStore, mgr.GetScheme()) + browserCtrl := browser.NewBrowserReconciler(mgr.GetClient(), browserCfgStore, mgr.GetScheme(), reconcilerCfg) if err = browserCtrl.SetupWithManager(mgr); err != nil { log.Error(err, "unable to create browser controller") os.Exit(1) diff --git a/config/crd/browserconfig.selenosis.io_browserconfigs.yaml b/config/crd/browserconfig.selenosis.io_browserconfigs.yaml index 8de59c7..bd92404 100644 --- a/config/crd/browserconfig.selenosis.io_browserconfigs.yaml +++ b/config/crd/browserconfig.selenosis.io_browserconfigs.yaml @@ -473,6 +473,14 @@ spec: additionalProperties: type: string type: object + args: + items: + type: string + type: array + command: + items: + type: string + type: array dnsConfig: properties: nameservers: @@ -609,6 +617,10 @@ spec: initContainers: items: properties: + args: + items: + type: string + type: array command: items: type: string @@ -909,6 +921,10 @@ spec: sidecars: items: properties: + args: + items: + type: string + type: array command: items: type: string @@ -2379,6 +2395,14 @@ spec: additionalProperties: type: string type: object + args: + items: + type: string + type: array + command: + items: + type: string + type: array dnsConfig: properties: nameservers: @@ -2513,6 +2537,10 @@ spec: initContainers: items: properties: + args: + items: + type: string + type: array command: items: type: string @@ -2814,6 +2842,10 @@ spec: sidecars: items: properties: + args: + items: + type: string + type: array command: items: type: string diff --git a/controllers/browser/browser_reconciler.go b/controllers/browser/browser_reconciler.go index fb4d2d0..bc4eaed 100644 --- a/controllers/browser/browser_reconciler.go +++ b/controllers/browser/browser_reconciler.go @@ -4,7 +4,9 @@ import ( "context" "encoding/json" "fmt" + "math/rand/v2" "strconv" + "strings" "time" browserv1 "github.com/alcounit/browser-controller/apis/browser/v1" @@ -15,26 +17,42 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" logger "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" ) const ( browserPodFinalizer = "browserpod.selenosis.io/finalizer" - maxRetries = 5 - mediumRetry = time.Second * 10 - periodicReconcile = time.Second * 30 - quickCheck = time.Second * 3 - podDeletionTimeout = time.Minute * 5 - podCreationTimeout = time.Minute * 5 + mediumRetry = time.Second * 10 + quickCheck = time.Second * 15 browserContainerName = "browser" sidecarContainerName = "seleniferous" ) +func jitter(base time.Duration) time.Duration { + half := base / 2 + return half + time.Duration(rand.Int64N(int64(half))) +} + +type ReconcilerConfig struct { + PodCreationTimeout time.Duration + PodDeletionTimeout time.Duration + PendingTimeout time.Duration + MaxRetries int + MaxWorkers int + RateLimiterBaseDelay time.Duration + RateLimiterMaxDelay time.Duration +} + type SelenosisOptions struct { Labels map[string]string `json:"labels,omitempty"` Containers map[string]ContainerOption `json:"containers,omitempty"` @@ -51,18 +69,19 @@ type ContainerOption struct { // +kubebuilder:rbac:groups=browser.selenosis.io,resources=browsers/status,verbs=get;update;patch // +kubebuilder:rbac:groups=browser.selenosis.io,resources=browsers/finalizers,verbs=update -// BrowserReconciler reconciles Browser resources type BrowserReconciler struct { client client.Client config *store.BrowserConfigStore scheme *runtime.Scheme + cfg ReconcilerConfig } -func NewBrowserReconciler(client client.Client, config *store.BrowserConfigStore, scheme *runtime.Scheme) *BrowserReconciler { +func NewBrowserReconciler(client client.Client, config *store.BrowserConfigStore, scheme *runtime.Scheme, cfg ReconcilerConfig) *BrowserReconciler { return &BrowserReconciler{ client: client, config: config, scheme: scheme, + cfg: cfg, } } @@ -70,7 +89,14 @@ func NewBrowserReconciler(client client.Client, config *store.BrowserConfigStore func (r *BrowserReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&browserv1.Browser{}). - Owns(&corev1.Pod{}). + Owns(&corev1.Pod{}, builder.WithPredicates(podChangedPredicate{})). + WithOptions(controller.Options{ + MaxConcurrentReconciles: r.cfg.MaxWorkers, + RateLimiter: workqueue.NewTypedItemExponentialFailureRateLimiter[ctrl.Request]( + r.cfg.RateLimiterBaseDelay, + r.cfg.RateLimiterMaxDelay, + ), + }). Complete(r) } @@ -80,7 +106,7 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct log.Info("start reconcile Browser") - // get the Browser resource + // Get the Browser resource browser := &browserv1.Browser{} if err := r.client.Get(ctx, types.NamespacedName{Name: req.Name, Namespace: req.Namespace}, browser); err != nil { if errors.IsNotFound(err) { @@ -91,21 +117,25 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return ctrl.Result{}, err } - // check if Browser deletion timestamp is set, if set handle deletion + // Check if Browser deletion timestamp is set, if set handle deletion if !browser.DeletionTimestamp.IsZero() { + if !controllerutil.ContainsFinalizer(browser, browserPodFinalizer) { + return ctrl.Result{}, nil + } log.Info("deleting Browser") return r.handleDeletion(ctx, browser) } + // Delete pod and delete Browser if Browser in corev1.PodFailed state if browser.Status.Phase == corev1.PodFailed { pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: browser.Name, Namespace: browser.Namespace}} if err := r.deletePod(ctx, pod); err != nil { - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } return r.deleteBrowser(ctx, browser) } - // ensure finalizer is set + // Ensure finalizer is set if !controllerutil.ContainsFinalizer(browser, browserPodFinalizer) { if err := r.retryUpdate(ctx, browser, func(b *browserv1.Browser) { controllerutil.AddFinalizer(b, browserPodFinalizer) @@ -116,18 +146,18 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct log.Info("adding finalizer to Browser") } - // ensure label selenosis.io/browser.name exists - if browser.Labels["selenosis.io/browser"] != browser.Name { + // Ensure label selenosis.io/browser.name exists + if browser.Labels[browserv1.BrowserLabelKey] != browser.Name { if err := r.retryUpdate(ctx, browser, func(b *browserv1.Browser) { if b.Labels == nil { b.Labels = map[string]string{} } - b.Labels["selenosis.io/browser"] = b.Name - b.Labels["selenosis.io/browser.name"] = b.Spec.BrowserName - b.Labels["selenosis.io/browser.version"] = b.Spec.BrowserVersion + b.Labels[browserv1.BrowserLabelKey] = b.Name + b.Labels[browserv1.BrowserNameLabelKey] = b.Spec.BrowserName + b.Labels[browserv1.BrowserVersionLabelKey] = b.Spec.BrowserVersion }); err != nil { log.Error(err, "failed to update Browser with name label") - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } log.Info("labels assigned to Browser") } @@ -141,12 +171,13 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct log.Error(err, "failed to set initial Browser status") return ctrl.Result{}, err } + log.Info("Browser status set to Pending") } log = log.WithValues("browserName", browser.Spec.BrowserName, "browserVersion", browser.Spec.BrowserVersion) - //get the associated Pod + //Get the associated Pod pod := &corev1.Pod{} if err := r.client.Get(ctx, types.NamespacedName{Name: browser.GetName(), Namespace: browser.GetNamespace()}, pod); err != nil { if errors.IsNotFound(err) { @@ -164,12 +195,12 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return r.deleteBrowser(ctx, browser) } - // Handle failed pod + // Delete pod and Browser if pod in corev1.PodFailed state if pod.Status.Phase == corev1.PodFailed { if err := r.deletePod(ctx, pod); err != nil { log.Info("deleting Browser pod after pod failure") - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { @@ -178,12 +209,76 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct log.Info("Browser pod has failed", "reason", pod.Status.Reason, "message", pod.Status.Message) }); err != nil { log.Error(err, "failed to update Browser status to Failed") - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } + + return r.deleteBrowser(ctx, browser) } + // Check pod InitContainerStatuses, delete pod and Browser if containers failed or timeouts exceeded if pod.Status.Phase == corev1.PodPending { + podAge := time.Since(pod.CreationTimestamp.Time) + for _, cs := range pod.Status.InitContainerStatuses { + if cs.State.Terminated != nil && cs.State.Terminated.ExitCode != 0 { + log.Info("Browser pod init container terminated", + "container", cs.Name, + "reason", cs.State.Terminated.Reason, + "message", cs.State.Terminated.Message, + "exitCode", cs.State.Terminated.ExitCode) + if err := r.deletePod(ctx, pod); err != nil { + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err + } + if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { + b.Status.Phase = corev1.PodFailed + b.Status.Message = fmt.Sprintf( + "pod init container %s terminated: %s (exit code %d)", + cs.Name, cs.State.Terminated.Reason, cs.State.Terminated.ExitCode) + }); err != nil { + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err + } + return r.deleteBrowser(ctx, browser) + } + + if cs.State.Waiting != nil { + if !pod.CreationTimestamp.IsZero() { + if podAge > r.cfg.PodCreationTimeout { + log.Info("Browser pod creation timeout exceeded on init container", "age", podAge.String(), "podStatus", pod.Status.Phase, "container", cs.Name) + if err := r.deletePod(ctx, pod); err != nil { + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err + } + if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { + b.Status.Phase = corev1.PodFailed + b.Status.Message = fmt.Sprintf( + "pod creation timeout exceeded after %s, init container %s: %s", + r.cfg.PodCreationTimeout.String(), cs.Name, cs.State.Waiting.Reason) + }); err != nil { + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err + } + return r.deleteBrowser(ctx, browser) + } + } + + reason := cs.State.Waiting.Reason + if reason != "PodInitializing" && reason != "ContainerCreating" { + log.Info("Browser pod init container not ready", "container", cs.Name, "reason", reason, "message", cs.State.Waiting.Message, "podStatus", pod.Status.Phase) + if err := r.deletePod(ctx, pod); err != nil { + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err + } + if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { + b.Status.Phase = corev1.PodFailed + b.Status.Message = fmt.Sprintf( + "Browser pod init container %s failed: %s - %s", + cs.Name, reason, cs.State.Waiting.Message) + }); err != nil { + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err + } + return r.deleteBrowser(ctx, browser) + } + } + } + + // Check pod ContainerStatuses, delete pod and Browser if containers failed or timeouts exceeded for _, cs := range pod.Status.ContainerStatuses { if cs.State.Terminated != nil { log.Info("Browser pod container terminated", @@ -192,7 +287,7 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct "message", pod.Status.Message, "exitCode", cs.State.Terminated.ExitCode) if err := r.deletePod(ctx, pod); err != nil { - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { b.Status.Phase = corev1.PodFailed @@ -200,28 +295,27 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct "pod container %s terminated: %s (exit code %d)", cs.Name, cs.State.Terminated.Reason, cs.State.Terminated.ExitCode) }); err != nil { - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } - return ctrl.Result{}, nil + return r.deleteBrowser(ctx, browser) } if cs.State.Waiting != nil { if !pod.CreationTimestamp.IsZero() { - podAge := time.Since(pod.CreationTimestamp.Time) - if podAge > podCreationTimeout { + if podAge > r.cfg.PodCreationTimeout { log.Info("Browser pod creation timeout exceeded", "age", podAge.String(), "podStatus", pod.Status.Phase, "container", cs.Name) if err := r.deletePod(ctx, pod); err != nil { - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { b.Status.Phase = corev1.PodFailed b.Status.Message = fmt.Sprintf( "pod creation timeout exceeded after %s, container %s: %s", - podCreationTimeout.String(), cs.Name, cs.State.Waiting.Reason) + r.cfg.PodCreationTimeout.String(), cs.Name, cs.State.Waiting.Reason) }); err != nil { - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } - return ctrl.Result{}, nil + return r.deleteBrowser(ctx, browser) } } @@ -230,7 +324,7 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct log.Info("Browser pod container not ready", "container", cs.Name, "reason", reason, "message", cs.State.Waiting.Message, "podStatus", pod.Status.Phase) if err := r.deletePod(ctx, pod); err != nil { - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { @@ -239,9 +333,9 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct "Browser pod container %s failed: %s - %s", cs.Name, reason, cs.State.Waiting.Message) }); err != nil { - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } - return ctrl.Result{}, nil + return r.deleteBrowser(ctx, browser) } } } @@ -254,11 +348,6 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct func (r *BrowserReconciler) handleDeletion(ctx context.Context, browser *browserv1.Browser) (ctrl.Result, error) { log := logger.FromContext(ctx) - if !controllerutil.ContainsFinalizer(browser, browserPodFinalizer) { - log.Info("Browser finalizer is not set, resource will be deleted during next reconcile") - return ctrl.Result{}, nil - } - // Get the pod pod := &corev1.Pod{} err := r.client.Get(ctx, types.NamespacedName{Name: browser.GetName(), Namespace: browser.GetNamespace()}, pod) @@ -276,14 +365,14 @@ func (r *BrowserReconciler) handleDeletion(ctx context.Context, browser *browser if err := r.client.Delete(ctx, pod, deleteOptions...); err != nil && !errors.IsNotFound(err) { log.Error(err, "failed to delete Browser pod") - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } } // Check if pod deletion is taking too long if pod.DeletionTimestamp != nil { deletionTime := pod.DeletionTimestamp.Time - if time.Since(deletionTime) > podDeletionTimeout { + if time.Since(deletionTime) > r.cfg.PodDeletionTimeout { log.Info("Browser pod deletion is taking too long, attempting force delete") if err := r.deletePod(ctx, pod); err != nil { log.Error(err, "Failed to force delete Browser pod after timeout") @@ -292,17 +381,16 @@ func (r *BrowserReconciler) handleDeletion(ctx context.Context, browser *browser } else { // Wait for pod to be deleted log.Info("waiting for Browser pod to be deleted") - return ctrl.Result{RequeueAfter: quickCheck}, nil + return ctrl.Result{RequeueAfter: jitter(quickCheck)}, nil } } else { // Wait for pod to be deleted log.Info("waiting for Browser pod to be deleted") - return ctrl.Result{RequeueAfter: quickCheck}, nil + return ctrl.Result{RequeueAfter: jitter(quickCheck)}, nil } } else if !errors.IsNotFound(err) { - log.Error(err, "error checking Browser pod for deletion") - // Don't block Browser deletion if we can't get the Pod - log.Info("proceeding with finalizer removal despite Browser pod check error") + log.Error(err, "error checking Browser pod for deletion, requeueing") + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } // Remove finalizer @@ -311,7 +399,7 @@ func (r *BrowserReconciler) handleDeletion(ctx context.Context, browser *browser controllerutil.RemoveFinalizer(b, browserPodFinalizer) }); err != nil { log.Error(err, "error removing Browser pod finalizer") - return ctrl.Result{RequeueAfter: mediumRetry}, err + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err } } @@ -323,10 +411,11 @@ func (r *BrowserReconciler) deletePod(ctx context.Context, pod *corev1.Pod) erro log := logger.FromContext(ctx) if err := r.client.Delete(ctx, pod, client.GracePeriodSeconds(0)); err != nil { - if !errors.IsNotFound(err) { - log.Error(err, "failed to force delete Browser pod") - return err + if errors.IsNotFound(err) { + return nil } + log.Error(err, "failed to force delete Browser pod") + return err } log.Info("Browser pod forcibly deleted") @@ -337,6 +426,22 @@ func (r *BrowserReconciler) deletePod(ctx context.Context, pod *corev1.Pod) erro func (r *BrowserReconciler) handleMissingPod(ctx context.Context, browser *browserv1.Browser) (ctrl.Result, error) { log := logger.FromContext(ctx) + if r.cfg.PendingTimeout > 0 { + age := time.Since(browser.CreationTimestamp.Time) + if age > r.cfg.PendingTimeout { + log.Info("Browser pending timeout exceeded", "age", age, "timeout", r.cfg.PendingTimeout) + if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { + b.Status.Phase = corev1.PodFailed + b.Status.Reason = "PendingTimeoutExceeded" + b.Status.Message = fmt.Sprintf("Browser did not start within %s", r.cfg.PendingTimeout) + }); err != nil { + log.Error(err, "failed to update Browser status") + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, err + } + return r.deleteBrowser(ctx, browser) + } + } + key := fmt.Sprintf("%s/%s:%s", browser.Namespace, browser.Spec.BrowserName, @@ -360,7 +465,7 @@ func (r *BrowserReconciler) handleMissingPod(ctx context.Context, browser *brows } log.Info("Browser config not found", "key", key, "browserName", browser.Spec.BrowserName, "BrowserVersion", browser.Spec.BrowserVersion) - return ctrl.Result{}, nil + return r.deleteBrowser(ctx, browser) } opts, err := parseSelenosisOptions(browser.Annotations) @@ -376,7 +481,7 @@ func (r *BrowserReconciler) handleMissingPod(ctx context.Context, browser *brows } log.Info("Invalid selenosis options") - return ctrl.Result{}, nil + return r.deleteBrowser(ctx, browser) } log.Info("parsed selenosis options", "hasOptions", opts != nil) @@ -385,14 +490,31 @@ func (r *BrowserReconciler) handleMissingPod(ctx context.Context, browser *brows if err := r.createPod(ctx, browser, browserSpec, opts); err != nil { if errors.IsAlreadyExists(err) { log.Info("Browser pod already exists, will reconcile on next iteration") - return ctrl.Result{RequeueAfter: quickCheck}, nil + return ctrl.Result{RequeueAfter: jitter(quickCheck)}, nil + } + if errors.IsForbidden(err) { + reason := "Forbidden" + if strings.Contains(err.Error(), "exceeded quota") { + reason = "QuotaExceeded" + } + + log.Error(err, "Browser pod creation blocked", "reason", reason) + if statusErr := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { + b.Status.Phase = corev1.PodFailed + b.Status.Reason = reason + b.Status.Message = err.Error() + }); statusErr != nil { + log.Error(statusErr, "failed to update Browser status") + return ctrl.Result{RequeueAfter: jitter(mediumRetry)}, statusErr + } + return r.deleteBrowser(ctx, browser) } log.Error(err, "failed to create Browser pod") return ctrl.Result{}, err } log.Info("Browser pod created") - return ctrl.Result{RequeueAfter: quickCheck}, nil + return ctrl.Result{RequeueAfter: jitter(quickCheck)}, nil } // createPod creates a Pod for Browser with optimized memory usage @@ -445,28 +567,25 @@ func (r *BrowserReconciler) updateBrowserStatus(ctx context.Context, browser *br browserStatusChanged := browser.Status.Phase != pod.Status.Phase || browser.Status.PodIP != pod.Status.PodIP || (pod.Status.StartTime != nil && (browser.Status.StartTime == nil || !browser.Status.StartTime.Equal(pod.Status.StartTime))) - containersStatusChanged := false - - newContainerStatuses := make([]browserv1.ContainerStatus, 0, len(pod.Status.ContainerStatuses)) + containersStatusChanged := !podStatusMatchesBrowser( + pod.Status.ContainerStatuses, browser.Status.ContainerStatuses) - // Efficiently collect container statuses - if len(pod.Status.ContainerStatuses) > 0 { - for _, containerStatus := range pod.Status.ContainerStatuses { - status := browserv1.ContainerStatus{ - Name: containerStatus.Name, - State: containerStatus.State, - Image: containerStatus.Image, - RestartCount: containerStatus.RestartCount, - Ports: getContainerPorts(containerStatus.Name, pod), + // Update status if changed + if browserStatusChanged || containersStatusChanged { + var newContainerStatuses []browserv1.ContainerStatus + if containersStatusChanged && len(pod.Status.ContainerStatuses) > 0 { + newContainerStatuses = make([]browserv1.ContainerStatus, 0, len(pod.Status.ContainerStatuses)) + for _, containerStatus := range pod.Status.ContainerStatuses { + newContainerStatuses = append(newContainerStatuses, browserv1.ContainerStatus{ + Name: containerStatus.Name, + State: containerStatus.State, + Image: containerStatus.Image, + RestartCount: containerStatus.RestartCount, + Ports: getContainerPorts(containerStatus.Name, pod), + }) } - newContainerStatuses = append(newContainerStatuses, status) } - containersStatusChanged = !containerStatusesEqual(newContainerStatuses, browser.Status.ContainerStatuses) - } - - // Update status if changed - if browserStatusChanged || containersStatusChanged { if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { if browserStatusChanged { b.Status.PodIP = pod.Status.PodIP @@ -485,57 +604,42 @@ func (r *BrowserReconciler) updateBrowserStatus(ctx context.Context, browser *br } log.Info("reconcilation completed") - return ctrl.Result{RequeueAfter: periodicReconcile}, nil + return ctrl.Result{}, nil } -func (r *BrowserReconciler) retryUpdate(ctx context.Context, browser *browserv1.Browser, updateFunc func(*browserv1.Browser)) error { - namespacedName := types.NamespacedName{ - Name: browser.Name, - Namespace: browser.Namespace, - } - - for i := 0; i < maxRetries; i++ { - current := &browserv1.Browser{} - if err := r.client.Get(ctx, namespacedName, current); err != nil { - return err - } - - before := current.DeepCopy() - updateFunc(current) - - patch := client.MergeFrom(before) - err := r.client.Patch(ctx, current, patch) - if err == nil { - return nil - } - - if !errors.IsConflict(err) { - return err - } - - time.Sleep(time.Millisecond * time.Duration(100*(1<= base { + t.Fatalf("expected RequeueAfter in [%v, %v), got %v", half, base, got) + } +} + func envValue(env []corev1.EnvVar, key string) (string, bool) { for _, item := range env { if item.Name == key { @@ -290,7 +308,7 @@ func TestHandleMissingPodConfigNotFound(t *testing.T) { scheme := newBrowserScheme(t) cfgStore := store.NewBrowserConfigStore() cl := newBrowserClient(scheme) - r := NewBrowserReconciler(cl, cfgStore, scheme) + r := NewBrowserReconciler(cl, cfgStore, scheme, defaultCfg) brw := &browserv1.Browser{ ObjectMeta: metav1.ObjectMeta{ @@ -311,12 +329,8 @@ func TestHandleMissingPodConfigNotFound(t *testing.T) { t.Fatalf("expected no error, got %v", err) } - got := &browserv1.Browser{} - if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, got); err != nil { - t.Fatalf("get browser: %v", err) - } - if got.Status.Phase != corev1.PodFailed { - t.Fatalf("expected failed status, got %s", got.Status.Phase) + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected browser to be deleted after config not found, got err=%v", err) } } @@ -329,16 +343,16 @@ func TestHandleMissingPodStatusUpdateError(t *testing.T) { Namespace: "ns", Finalizers: []string{browserPodFinalizer}, Labels: map[string]string{ - "selenosis.io/browser": "b1", - "selenosis.io/browser.name": "chrome", - "selenosis.io/browser.version": "120", + browserv1.BrowserLabelKey: "b1", + browserv1.BrowserNameLabelKey: "chrome", + browserv1.BrowserVersionLabelKey: "120", }, }, Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, Status: browserv1.BrowserStatus{Phase: corev1.PodPending}, } base := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, cfgStore, scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, cfgStore, scheme, defaultCfg) _, err := r.handleMissingPod(context.Background(), brw) if err == nil { @@ -353,7 +367,7 @@ func TestHandleMissingPodCreatesPod(t *testing.T) { setStoreConfig(t, cfgStore, "ns/chrome:120", spec) cl := newBrowserClient(scheme) - r := NewBrowserReconciler(cl, cfgStore, scheme) + r := NewBrowserReconciler(cl, cfgStore, scheme, defaultCfg) brw := &browserv1.Browser{ ObjectMeta: metav1.ObjectMeta{ @@ -373,9 +387,100 @@ func TestHandleMissingPodCreatesPod(t *testing.T) { if err != nil { t.Fatalf("expected no error, got %v", err) } - if res.RequeueAfter != quickCheck { - t.Fatalf("expected quick requeue, got %v", res.RequeueAfter) + assertJitteredRequeue(t, res.RequeueAfter, quickCheck) + + pod := &corev1.Pod{} + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, pod); err != nil { + t.Fatalf("expected pod to be created: %v", err) + } +} + +func TestHandleMissingPodPendingTimeoutExceededFailsBrowser(t *testing.T) { + scheme := newBrowserScheme(t) + cfgStore := store.NewBrowserConfigStore() + spec := &configv1.BrowserVersionConfigSpec{Image: "img"} + setStoreConfig(t, cfgStore, "ns/chrome:120", spec) + + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + CreationTimestamp: metav1.NewTime(time.Now().Add(-10 * time.Minute)), + }, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + } + cl := newBrowserClient(scheme, brw) + cfg := defaultCfg + cfg.PendingTimeout = time.Minute + r := NewBrowserReconciler(cl, cfgStore, scheme, cfg) + + _, err := r.handleMissingPod(context.Background(), brw) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected browser to be deleted after pending timeout, got err=%v", err) + } + + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected no pod to be created, got err=%v", err) + } +} + +func TestHandleMissingPodPendingTimeoutNotYetExceeded(t *testing.T) { + scheme := newBrowserScheme(t) + cfgStore := store.NewBrowserConfigStore() + spec := &configv1.BrowserVersionConfigSpec{Image: "img"} + setStoreConfig(t, cfgStore, "ns/chrome:120", spec) + + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + CreationTimestamp: metav1.NewTime(time.Now()), + }, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + } + cl := newBrowserClient(scheme, brw) + cfg := defaultCfg + cfg.PendingTimeout = time.Hour + r := NewBrowserReconciler(cl, cfgStore, scheme, cfg) + + res, err := r.handleMissingPod(context.Background(), brw) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + assertJitteredRequeue(t, res.RequeueAfter, quickCheck) + + pod := &corev1.Pod{} + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, pod); err != nil { + t.Fatalf("expected pod to be created: %v", err) + } +} + +func TestHandleMissingPodPendingTimeoutDisabled(t *testing.T) { + scheme := newBrowserScheme(t) + cfgStore := store.NewBrowserConfigStore() + spec := &configv1.BrowserVersionConfigSpec{Image: "img"} + setStoreConfig(t, cfgStore, "ns/chrome:120", spec) + + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + CreationTimestamp: metav1.NewTime(time.Now().Add(-24 * time.Hour)), + }, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + } + cl := newBrowserClient(scheme, brw) + r := NewBrowserReconciler(cl, cfgStore, scheme, defaultCfg) + + res, err := r.handleMissingPod(context.Background(), brw) + if err != nil { + t.Fatalf("expected no error, got %v", err) } + assertJitteredRequeue(t, res.RequeueAfter, quickCheck) pod := &corev1.Pod{} if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, pod); err != nil { @@ -390,7 +495,7 @@ func TestHandleMissingPodInvalidSelenosisOptions(t *testing.T) { setStoreConfig(t, cfgStore, "ns/chrome:120", spec) cl := newBrowserClient(scheme) - r := NewBrowserReconciler(cl, cfgStore, scheme) + r := NewBrowserReconciler(cl, cfgStore, scheme, defaultCfg) brw := &browserv1.Browser{ ObjectMeta: metav1.ObjectMeta{ @@ -409,39 +514,24 @@ func TestHandleMissingPodInvalidSelenosisOptions(t *testing.T) { t.Fatalf("create browser: %v", err) } - res, err := r.handleMissingPod(context.Background(), brw) + _, err := r.handleMissingPod(context.Background(), brw) if err != nil { t.Fatalf("expected no error, got %v", err) } - if res.RequeueAfter != 0 { - t.Fatalf("expected no requeue, got %v", res.RequeueAfter) - } - updated := &browserv1.Browser{} - if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, updated); err != nil { - t.Fatalf("get browser: %v", err) - } - if updated.Status.Phase != corev1.PodFailed { - t.Fatalf("expected failed status, got %s", updated.Status.Phase) - } - if updated.Status.Reason != "InvalidSelenosisOptions" { - t.Fatalf("expected reason InvalidSelenosisOptions, got %s", updated.Status.Reason) + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected browser to be deleted after invalid options, got err=%v", err) } - pod := &corev1.Pod{} - err = cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, pod) - if err == nil { - t.Fatalf("expected no pod to be created") - } - if !apierrors.IsNotFound(err) { - t.Fatalf("expected not found error, got %v", err) + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected no pod to be created, got err=%v", err) } } func TestUpdateBrowserStatusCriticalContainer(t *testing.T) { scheme := newBrowserScheme(t) cl := newBrowserClient(scheme) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) now := metav1.NewTime(time.Now().UTC()) brw := &browserv1.Browser{ @@ -492,7 +582,7 @@ func TestUpdateBrowserStatusCriticalContainer(t *testing.T) { func TestUpdateBrowserStatusUpdatesFields(t *testing.T) { scheme := newBrowserScheme(t) cl := newBrowserClient(scheme) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) now := metav1.NewTime(time.Now().UTC()) brw := &browserv1.Browser{ @@ -529,8 +619,8 @@ func TestUpdateBrowserStatusUpdatesFields(t *testing.T) { if err != nil { t.Fatalf("expected no error, got %v", err) } - if res.RequeueAfter != periodicReconcile { - t.Fatalf("expected periodic requeue, got %v", res.RequeueAfter) + if res.RequeueAfter != 0 { + t.Fatalf("expected no requeue, got %v", res.RequeueAfter) } got := &browserv1.Browser{} @@ -548,7 +638,7 @@ func TestUpdateBrowserStatusUpdatesFields(t *testing.T) { func TestReconcileNotFound(t *testing.T) { scheme := newBrowserScheme(t) cl := newBrowserClient(scheme) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "missing"}, @@ -561,7 +651,7 @@ func TestReconcileNotFound(t *testing.T) { func TestReconcileAddsFinalizerAndLabels(t *testing.T) { scheme := newBrowserScheme(t) cl := newBrowserClient(scheme) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) brw := &browserv1.Browser{ ObjectMeta: metav1.ObjectMeta{ @@ -591,7 +681,7 @@ func TestReconcileAddsFinalizerAndLabels(t *testing.T) { if !controllerutil.ContainsFinalizer(got, browserPodFinalizer) { t.Fatalf("expected finalizer to be set") } - if got.Labels["selenosis.io/browser"] != "b1" { + if got.Labels[browserv1.BrowserLabelKey] != "b1" { t.Fatalf("expected browser label to be set") } } @@ -609,7 +699,7 @@ func TestReconcileFailedBrowserDeletesBrowser(t *testing.T) { }, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -638,7 +728,7 @@ func TestReconcileFailedBrowserWithPodDeletesPodAndBrowser(t *testing.T) { Status: corev1.PodStatus{Phase: corev1.PodPending}, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -670,7 +760,7 @@ func TestReconcileFailedBrowserPodDeleteError(t *testing.T) { } base := newBrowserClient(scheme, brw, pod) cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -678,9 +768,7 @@ func TestReconcileFailedBrowserPodDeleteError(t *testing.T) { if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestReconcileFailedBrowserFinalizerRemoveError(t *testing.T) { @@ -696,7 +784,7 @@ func TestReconcileFailedBrowserFinalizerRemoveError(t *testing.T) { }, } base := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -704,9 +792,7 @@ func TestReconcileFailedBrowserFinalizerRemoveError(t *testing.T) { if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestHandleDeletionNoFinalizer(t *testing.T) { @@ -718,7 +804,7 @@ func TestHandleDeletionNoFinalizer(t *testing.T) { }, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) now := metav1.NewTime(time.Now().UTC()) brw.DeletionTimestamp = &now @@ -744,7 +830,7 @@ func TestHandleDeletionPodNotFound(t *testing.T) { }, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.handleDeletion(context.Background(), brw) if err != nil { @@ -772,20 +858,18 @@ func TestHandleDeletionPodDeletionInProgress(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.handleDeletion(context.Background(), brw) if err != nil { t.Fatalf("expected no error, got %v", err) } - if res.RequeueAfter != quickCheck { - t.Fatalf("expected quick requeue, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, quickCheck) } func TestHandleDeletionPodTimeout(t *testing.T) { scheme := newBrowserScheme(t) - old := metav1.NewTime(time.Now().Add(-podDeletionTimeout - time.Second).UTC()) + old := metav1.NewTime(time.Now().Add(-defaultCfg.PodDeletionTimeout - time.Second).UTC()) now := metav1.NewTime(time.Now().UTC()) pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -804,7 +888,7 @@ func TestHandleDeletionPodTimeout(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.handleDeletion(context.Background(), brw) if err != nil { @@ -835,15 +919,13 @@ func TestHandleMissingPodAlreadyExists(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, cfgStore, scheme) + r := NewBrowserReconciler(cl, cfgStore, scheme, defaultCfg) res, err := r.handleMissingPod(context.Background(), brw) if err != nil { t.Fatalf("expected no error, got %v", err) } - if res.RequeueAfter != quickCheck { - t.Fatalf("expected quick requeue, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, quickCheck) } func TestReconcilePodFailedUpdatesStatus(t *testing.T) { @@ -870,7 +952,7 @@ func TestReconcilePodFailedUpdatesStatus(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -921,7 +1003,7 @@ func TestReconcilePodPendingContainerTerminated(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -971,7 +1053,7 @@ func TestReconcilePodPendingContainerTerminatedPodDeleteError(t *testing.T) { } base := newBrowserClient(scheme, brw, pod) cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -979,9 +1061,7 @@ func TestReconcilePodPendingContainerTerminatedPodDeleteError(t *testing.T) { if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestReconcilePodPendingWaitingBadReason(t *testing.T) { @@ -1018,7 +1098,7 @@ func TestReconcilePodPendingWaitingBadReason(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) req := ctrl.Request{NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}} @@ -1068,7 +1148,7 @@ func TestReconcilePodPendingCreationTimeout(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "b1", Namespace: "ns", - CreationTimestamp: metav1.NewTime(time.Now().Add(-podCreationTimeout - time.Second).UTC()), + CreationTimestamp: metav1.NewTime(time.Now().Add(-defaultCfg.PodCreationTimeout - time.Second).UTC()), }, Status: corev1.PodStatus{ Phase: corev1.PodPending, @@ -1085,7 +1165,7 @@ func TestReconcilePodPendingCreationTimeout(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1123,7 +1203,7 @@ func TestReconcilePodPendingCreationTimeoutPodDeleteError(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "b1", Namespace: "ns", - CreationTimestamp: metav1.NewTime(time.Now().Add(-podCreationTimeout - time.Second).UTC()), + CreationTimestamp: metav1.NewTime(time.Now().Add(-defaultCfg.PodCreationTimeout - time.Second).UTC()), }, Status: corev1.PodStatus{ Phase: corev1.PodPending, @@ -1139,7 +1219,7 @@ func TestReconcilePodPendingCreationTimeoutPodDeleteError(t *testing.T) { } base := newBrowserClient(scheme, brw, pod) cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1147,9 +1227,7 @@ func TestReconcilePodPendingCreationTimeoutPodDeleteError(t *testing.T) { if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestUpdateBrowserStatusNoChanges(t *testing.T) { @@ -1170,7 +1248,7 @@ func TestUpdateBrowserStatusNoChanges(t *testing.T) { }, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ @@ -1232,6 +1310,7 @@ func TestBuildBrowserPodWithInitContainersAndVolumes(t *testing.T) { func TestBuildBrowserPodInitContainerFields(t *testing.T) { workDir := "/work" cmd := []string{"sh"} + args := []string{"-c"} ports := []corev1.ContainerPort{{ContainerPort: 8080}} env := []corev1.EnvVar{{Name: "A", Value: "B"}} mounts := []corev1.VolumeMount{{Name: "v", MountPath: "/m"}} @@ -1240,6 +1319,7 @@ func TestBuildBrowserPodInitContainerFields(t *testing.T) { Name: "init", Image: "img", Command: &cmd, + Args: &args, WorkingDir: &workDir, Ports: &ports, Env: &env, @@ -1265,6 +1345,94 @@ func TestBuildBrowserPodInitContainerFields(t *testing.T) { if len(pod.Spec.InitContainers[0].Env) != 1 || len(pod.Spec.InitContainers[0].Ports) != 1 { t.Fatalf("expected init container fields to be set") } + ic := pod.Spec.InitContainers[0] + if len(ic.Command) != 1 || ic.Command[0] != "sh" { + t.Fatalf("expected init container command, got %+v", ic.Command) + } + if len(ic.Args) != 1 || ic.Args[0] != "-c" { + t.Fatalf("expected init container args, got %+v", ic.Args) + } +} + +func TestBuildBrowserPodBrowserCommandAndArgs(t *testing.T) { + cmd := []string{"custom-entrypoint"} + args := []string{"--headless", "--port=9222"} + + cfg := &configv1.BrowserVersionConfigSpec{ + Image: "browser", + Command: &cmd, + Args: &args, + } + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + }, + } + + pod := buildBrowserPod(brw, cfg, nil) + bc := pod.Spec.Containers[0] + if len(bc.Command) != 1 || bc.Command[0] != "custom-entrypoint" { + t.Fatalf("expected browser container command, got %+v", bc.Command) + } + if len(bc.Args) != 2 || bc.Args[0] != "--headless" || bc.Args[1] != "--port=9222" { + t.Fatalf("expected browser container args, got %+v", bc.Args) + } +} + +func TestBuildBrowserPodSidecarArgs(t *testing.T) { + sidecarCmd := []string{"run"} + sidecarArgs := []string{"--flag", "--verbose"} + sidecars := []configv1.Sidecar{{ + Name: "proxy", + Image: "proxy-img", + Command: &sidecarCmd, + Args: &sidecarArgs, + }} + + cfg := &configv1.BrowserVersionConfigSpec{ + Image: "browser", + Sidecars: &sidecars, + } + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + }, + } + + pod := buildBrowserPod(brw, cfg, nil) + if len(pod.Spec.Containers) < 2 { + t.Fatalf("expected at least 2 containers") + } + sc := pod.Spec.Containers[1] + if len(sc.Command) != 1 || sc.Command[0] != "run" { + t.Fatalf("expected sidecar command, got %+v", sc.Command) + } + if len(sc.Args) != 2 || sc.Args[0] != "--flag" || sc.Args[1] != "--verbose" { + t.Fatalf("expected sidecar args, got %+v", sc.Args) + } +} + +func TestBuildBrowserPodNilCommandAndArgs(t *testing.T) { + cfg := &configv1.BrowserVersionConfigSpec{ + Image: "browser", + } + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + }, + } + + pod := buildBrowserPod(brw, cfg, nil) + bc := pod.Spec.Containers[0] + if bc.Command != nil { + t.Fatalf("expected nil command on browser container, got %+v", bc.Command) + } + if bc.Args != nil { + t.Fatalf("expected nil args on browser container, got %+v", bc.Args) + } } func TestBuildBrowserPodAllFields(t *testing.T) { @@ -1389,7 +1557,7 @@ func (e errorClient) Delete(ctx context.Context, obj client.Object, opts ...clie func TestDeletePodNotFound(t *testing.T) { scheme := newBrowserScheme(t) cl := newBrowserClient(scheme) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) err := r.deletePod(context.Background(), &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "missing", Namespace: "ns"}, @@ -1404,7 +1572,7 @@ func TestDeletePodError(t *testing.T) { pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}} base := newBrowserClient(scheme, pod) cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) err := r.deletePod(context.Background(), pod) if err == nil { @@ -1424,9 +1592,9 @@ func TestHandleMissingPodCreateError(t *testing.T) { Namespace: "ns", Finalizers: []string{browserPodFinalizer}, Labels: map[string]string{ - "selenosis.io/browser": "b1", - "selenosis.io/browser.name": "chrome", - "selenosis.io/browser.version": "120", + browserv1.BrowserLabelKey: "b1", + browserv1.BrowserNameLabelKey: "chrome", + browserv1.BrowserVersionLabelKey: "120", }, }, Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, @@ -1434,7 +1602,7 @@ func TestHandleMissingPodCreateError(t *testing.T) { } base := newBrowserClient(scheme, brw) cl := errorClient{Client: base, createErr: apierrors.NewInternalError(errors.New("boom"))} - r := NewBrowserReconciler(cl, cfgStore, scheme) + r := NewBrowserReconciler(cl, cfgStore, scheme, defaultCfg) _, err := r.handleMissingPod(context.Background(), brw) if err == nil { @@ -1465,7 +1633,7 @@ func TestReconcilePodDeletedDeletesBrowser(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1486,7 +1654,7 @@ func TestReconcilePodNotFoundBrowserFailedDeletesBrowser(t *testing.T) { }, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1508,9 +1676,9 @@ func TestReconcilePodPendingContainerCreatingNoTimeout(t *testing.T) { Namespace: "ns", Finalizers: []string{browserPodFinalizer}, Labels: map[string]string{ - "selenosis.io/browser": "b1", - "selenosis.io/browser.name": "chrome", - "selenosis.io/browser.version": "120", + browserv1.BrowserLabelKey: "b1", + browserv1.BrowserNameLabelKey: "chrome", + browserv1.BrowserVersionLabelKey: "120", }, }, Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, @@ -1535,7 +1703,7 @@ func TestReconcilePodPendingContainerCreatingNoTimeout(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1591,7 +1759,7 @@ func (s *statusPatchErrorWriter) Patch(ctx context.Context, obj client.Object, p func TestRetryUpdateGetError(t *testing.T) { scheme := newBrowserScheme(t) base := newBrowserClient(scheme) - r := NewBrowserReconciler(patchErrorClient{Client: base, getErr: apierrors.NewBadRequest("bad")}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, getErr: apierrors.NewBadRequest("bad")}, store.NewBrowserConfigStore(), scheme, defaultCfg) err := r.retryUpdate(context.Background(), &browserv1.Browser{ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}}, func(*browserv1.Browser) {}) if err == nil { @@ -1603,7 +1771,7 @@ func TestRetryUpdatePatchError(t *testing.T) { scheme := newBrowserScheme(t) brw := &browserv1.Browser{ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}} base := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) err := r.retryUpdate(context.Background(), brw, func(b *browserv1.Browser) { b.Labels = map[string]string{"k": "v"} }) if err == nil { @@ -1615,7 +1783,7 @@ func TestRetryStatusUpdatePatchError(t *testing.T) { scheme := newBrowserScheme(t) brw := &browserv1.Browser{ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}} base := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) err := r.retryStatusUpdate(context.Background(), brw, func(b *browserv1.Browser) { b.Status.Phase = corev1.PodRunning }) if err == nil { @@ -1627,7 +1795,7 @@ func TestDeleteBrowserNoFinalizer(t *testing.T) { scheme := newBrowserScheme(t) brw := &browserv1.Browser{ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}} cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.deleteBrowser(context.Background(), brw) if err != nil { @@ -1649,7 +1817,7 @@ func TestDeleteBrowserDeleteError(t *testing.T) { } base := newBrowserClient(scheme, brw) cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.deleteBrowser(context.Background(), brw) if err == nil { @@ -1673,15 +1841,13 @@ func TestHandleDeletionPodDeleteError(t *testing.T) { } base := newBrowserClient(scheme, brw, pod) cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.handleDeletion(context.Background(), brw) if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestHandleDeletionDeleteSuccess(t *testing.T) { @@ -1699,15 +1865,13 @@ func TestHandleDeletionDeleteSuccess(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.handleDeletion(context.Background(), brw) if err != nil { t.Fatalf("expected no error, got %v", err) } - if res.RequeueAfter != quickCheck { - t.Fatalf("expected quick requeue, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, quickCheck) } func TestHandleDeletionFailedPodGraceDelete(t *testing.T) { @@ -1726,15 +1890,13 @@ func TestHandleDeletionFailedPodGraceDelete(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.handleDeletion(context.Background(), brw) if err != nil { t.Fatalf("expected no error, got %v", err) } - if res.RequeueAfter != quickCheck { - t.Fatalf("expected quick requeue, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, quickCheck) } func TestHandleDeletionPodGetError(t *testing.T) { @@ -1749,12 +1911,13 @@ func TestHandleDeletionPodGetError(t *testing.T) { }, } base := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(patchErrorClient{Client: base, getPodErr: apierrors.NewInternalError(errors.New("pod"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, getPodErr: apierrors.NewInternalError(errors.New("pod"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) - _, err := r.handleDeletion(context.Background(), brw) - if err != nil { - t.Fatalf("expected no error, got %v", err) + res, err := r.handleDeletion(context.Background(), brw) + if err == nil { + t.Fatalf("expected error for transient pod get failure") } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestHandleDeletionFinalizerRemoveError(t *testing.T) { @@ -1769,15 +1932,13 @@ func TestHandleDeletionFinalizerRemoveError(t *testing.T) { }, } base := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.handleDeletion(context.Background(), brw) if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestReconcileDeletionTimestamp(t *testing.T) { @@ -1792,7 +1953,7 @@ func TestReconcileDeletionTimestamp(t *testing.T) { }, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1802,6 +1963,144 @@ func TestReconcileDeletionTimestamp(t *testing.T) { } } +type conflictPatchClient struct { + client.Client + patchCalls int + failCount int +} + +func (c *conflictPatchClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + c.patchCalls++ + if c.patchCalls <= c.failCount { + return apierrors.NewConflict(schema.GroupResource{Resource: "browsers"}, obj.GetName(), errors.New("conflict")) + } + return c.Client.Patch(ctx, obj, patch, opts...) +} + +type conflictStatusPatchClient struct { + client.Client + statusPatchCalls int + statusFailCount int +} + +func (c *conflictStatusPatchClient) Status() client.StatusWriter { + return &conflictStatusPatchWriter{ + StatusWriter: c.Client.Status(), + calls: &c.statusPatchCalls, + failCount: c.statusFailCount, + } +} + +type conflictStatusPatchWriter struct { + client.StatusWriter + calls *int + failCount int +} + +func (w *conflictStatusPatchWriter) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + *w.calls++ + if *w.calls <= w.failCount { + return apierrors.NewConflict(schema.GroupResource{Resource: "browsers"}, obj.GetName(), errors.New("conflict")) + } + return w.StatusWriter.Patch(ctx, obj, patch, opts...) +} + +// ───────── retryBackoff ───────── + +func TestRetryBackoffContextCancelled(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + start := time.Now() + retryBackoff(ctx, 0) + if elapsed := time.Since(start); elapsed > 50*time.Millisecond { + t.Fatalf("retryBackoff took %v on cancelled context, expected < 50ms", elapsed) + } +} + +func TestRetryBackoffLargeAttemptNoPanic(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + retryBackoff(ctx, 100) // overflowed int without min(attempt, 10) guard +} + +// ───────── retryUpdate / retryPatch conflict ───────── + +func TestRetryUpdateConflictRetry(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}} + base := newBrowserClient(scheme, brw) + cc := &conflictPatchClient{Client: base, failCount: 1} + r := NewBrowserReconciler(cc, store.NewBrowserConfigStore(), scheme, defaultCfg) + + err := r.retryUpdate(context.Background(), brw, func(b *browserv1.Browser) { + if b.Labels == nil { + b.Labels = map[string]string{} + } + b.Labels["k"] = "v" + }) + if err != nil { + t.Fatalf("expected success after conflict retry, got %v", err) + } + if cc.patchCalls < 2 { + t.Fatalf("expected at least 2 patch calls, got %d", cc.patchCalls) + } +} + +func TestRetryUpdateConflictExhausted(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}} + base := newBrowserClient(scheme, brw) + cc := &conflictPatchClient{Client: base, failCount: defaultCfg.MaxRetries + 1} + r := NewBrowserReconciler(cc, store.NewBrowserConfigStore(), scheme, defaultCfg) + + err := r.retryUpdate(context.Background(), brw, func(*browserv1.Browser) {}) + if err == nil { + t.Fatalf("expected error after exhausting retries") + } + if cc.patchCalls != defaultCfg.MaxRetries { + t.Fatalf("expected %d patch calls, got %d", defaultCfg.MaxRetries, cc.patchCalls) + } +} + +func TestRetryStatusUpdateConflictRetry(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}} + base := newBrowserClient(scheme, brw) + cc := &conflictStatusPatchClient{Client: base, statusFailCount: 1} + r := NewBrowserReconciler(cc, store.NewBrowserConfigStore(), scheme, defaultCfg) + + err := r.retryStatusUpdate(context.Background(), brw, func(b *browserv1.Browser) { + b.Status.Phase = corev1.PodRunning + }) + if err != nil { + t.Fatalf("expected success after conflict retry, got %v", err) + } + if cc.statusPatchCalls < 2 { + t.Fatalf("expected at least 2 status patch calls, got %d", cc.statusPatchCalls) + } +} + +func TestRetryPatchContextCancelledDuringBackoff(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}} + base := newBrowserClient(scheme, brw) + cc := &conflictPatchClient{Client: base, failCount: defaultCfg.MaxRetries + 1} + r := NewBrowserReconciler(cc, store.NewBrowserConfigStore(), scheme, defaultCfg) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + start := time.Now() + err := r.retryUpdate(ctx, brw, func(*browserv1.Browser) {}) + if err == nil { + t.Fatalf("expected error from cancelled context") + } + if elapsed := time.Since(start); elapsed > 200*time.Millisecond { + t.Fatalf("retryUpdate took %v on cancelled context, expected < 200ms", elapsed) + } +} + func TestReconcileFinalizerAddError(t *testing.T) { scheme := newBrowserScheme(t) brw := &browserv1.Browser{ @@ -1809,7 +2108,7 @@ func TestReconcileFinalizerAddError(t *testing.T) { Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, } base := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1826,12 +2125,12 @@ func TestReconcileLabelUpdateError(t *testing.T) { Name: "b1", Namespace: "ns", Finalizers: []string{browserPodFinalizer}, - Labels: map[string]string{"selenosis.io/browser": "wrong"}, + Labels: map[string]string{browserv1.BrowserLabelKey: "wrong"}, }, Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, } base := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1839,9 +2138,7 @@ func TestReconcileLabelUpdateError(t *testing.T) { if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestReconcilePendingStatusUpdateError(t *testing.T) { @@ -1851,7 +2148,7 @@ func TestReconcilePendingStatusUpdateError(t *testing.T) { Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, } base := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1869,9 +2166,9 @@ func TestReconcilePendingTerminatedStatusUpdateError(t *testing.T) { Namespace: "ns", Finalizers: []string{browserPodFinalizer}, Labels: map[string]string{ - "selenosis.io/browser": "b1", - "selenosis.io/browser.name": "chrome", - "selenosis.io/browser.version": "120", + browserv1.BrowserLabelKey: "b1", + browserv1.BrowserNameLabelKey: "chrome", + browserv1.BrowserVersionLabelKey: "120", }, }, Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, @@ -1892,7 +2189,7 @@ func TestReconcilePendingTerminatedStatusUpdateError(t *testing.T) { }, } base := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1900,9 +2197,7 @@ func TestReconcilePendingTerminatedStatusUpdateError(t *testing.T) { if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestReconcilePendingCreationTimeoutStatusUpdateError(t *testing.T) { @@ -1913,9 +2208,9 @@ func TestReconcilePendingCreationTimeoutStatusUpdateError(t *testing.T) { Namespace: "ns", Finalizers: []string{browserPodFinalizer}, Labels: map[string]string{ - "selenosis.io/browser": "b1", - "selenosis.io/browser.name": "chrome", - "selenosis.io/browser.version": "120", + browserv1.BrowserLabelKey: "b1", + browserv1.BrowserNameLabelKey: "chrome", + browserv1.BrowserVersionLabelKey: "120", }, }, Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, @@ -1925,7 +2220,7 @@ func TestReconcilePendingCreationTimeoutStatusUpdateError(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "b1", Namespace: "ns", - CreationTimestamp: metav1.NewTime(time.Now().Add(-podCreationTimeout - time.Second).UTC()), + CreationTimestamp: metav1.NewTime(time.Now().Add(-defaultCfg.PodCreationTimeout - time.Second).UTC()), }, Status: corev1.PodStatus{ Phase: corev1.PodPending, @@ -1940,7 +2235,7 @@ func TestReconcilePendingCreationTimeoutStatusUpdateError(t *testing.T) { }, } base := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1948,9 +2243,7 @@ func TestReconcilePendingCreationTimeoutStatusUpdateError(t *testing.T) { if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestReconcilePendingWaitingStatusUpdateError(t *testing.T) { @@ -1961,9 +2254,9 @@ func TestReconcilePendingWaitingStatusUpdateError(t *testing.T) { Namespace: "ns", Finalizers: []string{browserPodFinalizer}, Labels: map[string]string{ - "selenosis.io/browser": "b1", - "selenosis.io/browser.name": "chrome", - "selenosis.io/browser.version": "120", + browserv1.BrowserLabelKey: "b1", + browserv1.BrowserNameLabelKey: "chrome", + browserv1.BrowserVersionLabelKey: "120", }, }, Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, @@ -1984,7 +2277,7 @@ func TestReconcilePendingWaitingStatusUpdateError(t *testing.T) { }, } base := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, statusPatchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -1992,9 +2285,7 @@ func TestReconcilePendingWaitingStatusUpdateError(t *testing.T) { if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestReconcileHandleMissingPodCreateError(t *testing.T) { @@ -2009,7 +2300,7 @@ func TestReconcileHandleMissingPodCreateError(t *testing.T) { } base := newBrowserClient(scheme, brw) cl := errorClient{Client: base, createErr: apierrors.NewInternalError(errors.New("create"))} - r := NewBrowserReconciler(cl, cfgStore, scheme) + r := NewBrowserReconciler(cl, cfgStore, scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -2040,7 +2331,7 @@ func TestReconcilePodDeletingDeleteBrowserError(t *testing.T) { } base := newBrowserClient(scheme, brw, pod) cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -2072,7 +2363,7 @@ func TestReconcilePodPendingWaitingDeleteError(t *testing.T) { } base := newBrowserClient(scheme, brw, pod) cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -2080,9 +2371,7 @@ func TestReconcilePodPendingWaitingDeleteError(t *testing.T) { if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestUpdateBrowserStatusCriticalSidecar(t *testing.T) { @@ -2095,7 +2384,7 @@ func TestUpdateBrowserStatusCriticalSidecar(t *testing.T) { }, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, @@ -2131,7 +2420,7 @@ func TestDeleteBrowserFinalizerSuccess(t *testing.T) { }, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.deleteBrowser(context.Background(), brw) if err != nil { @@ -2149,7 +2438,7 @@ func TestDeleteBrowserRetryUpdateError(t *testing.T) { }, } base := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, patchErr: apierrors.NewInternalError(errors.New("patch"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.deleteBrowser(context.Background(), brw) if err == nil { @@ -2168,7 +2457,7 @@ func TestUpdateBrowserStatusCriticalAlreadyFailed(t *testing.T) { Status: browserv1.BrowserStatus{Phase: corev1.PodFailed}, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, @@ -2197,7 +2486,7 @@ func TestUpdateBrowserStatusNoContainerStatuses(t *testing.T) { Status: browserv1.BrowserStatus{Phase: corev1.PodPending}, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, @@ -2222,7 +2511,7 @@ func TestReconcilePodFailedDeleteError(t *testing.T) { } base := newBrowserClient(scheme, brw, pod) cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) res, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -2230,9 +2519,7 @@ func TestReconcilePodFailedDeleteError(t *testing.T) { if err == nil { t.Fatalf("expected error") } - if res.RequeueAfter != mediumRetry { - t.Fatalf("expected medium retry, got %v", res.RequeueAfter) - } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) } func TestReconcilePodPendingPodInitializing(t *testing.T) { @@ -2256,7 +2543,7 @@ func TestReconcilePodPendingPodInitializing(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -2269,7 +2556,7 @@ func TestReconcilePodPendingPodInitializing(t *testing.T) { func TestRetryStatusUpdateGetError(t *testing.T) { scheme := newBrowserScheme(t) base := newBrowserClient(scheme) - r := NewBrowserReconciler(patchErrorClient{Client: base, getErr: apierrors.NewBadRequest("bad")}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, getErr: apierrors.NewBadRequest("bad")}, store.NewBrowserConfigStore(), scheme, defaultCfg) err := r.retryStatusUpdate(context.Background(), &browserv1.Browser{ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}}, func(*browserv1.Browser) {}) if err == nil { @@ -2292,7 +2579,7 @@ func TestUpdateBrowserStatusBrowserStatusChangedOnly(t *testing.T) { }, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, @@ -2327,7 +2614,7 @@ func TestUpdateBrowserStatusContainerStateChange(t *testing.T) { }, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, @@ -2355,7 +2642,7 @@ func TestUpdateBrowserStatusContainerStateChange(t *testing.T) { func TestReconcileBrowserGetError(t *testing.T) { scheme := newBrowserScheme(t) base := newBrowserClient(scheme) - r := NewBrowserReconciler(patchErrorClient{Client: base, getErr: apierrors.NewBadRequest("bad")}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, getErr: apierrors.NewBadRequest("bad")}, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -2372,7 +2659,7 @@ func TestReconcilePodGetError(t *testing.T) { Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, } base := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(patchErrorClient{Client: base, getPodErr: apierrors.NewInternalError(errors.New("pod"))}, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(patchErrorClient{Client: base, getPodErr: apierrors.NewInternalError(errors.New("pod"))}, store.NewBrowserConfigStore(), scheme, defaultCfg) _, err := r.Reconcile(context.Background(), ctrl.Request{ NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, @@ -2390,7 +2677,7 @@ func TestUpdateBrowserStatusContainerStatusLengthChange(t *testing.T) { Status: browserv1.BrowserStatus{Phase: corev1.PodPending}, } cl := newBrowserClient(scheme, brw) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, @@ -2454,7 +2741,7 @@ func TestRetryUpdateConflictThenSuccess(t *testing.T) { } base := newBrowserClient(scheme, brw) c := &conflictClient{Client: base} - r := NewBrowserReconciler(c, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(c, store.NewBrowserConfigStore(), scheme, defaultCfg) err := r.retryUpdate(context.Background(), brw, func(b *browserv1.Browser) { if b.Labels == nil { @@ -2480,7 +2767,7 @@ func TestRetryStatusUpdateConflictThenSuccess(t *testing.T) { } base := newBrowserClient(scheme, brw) c := &conflictClient{Client: base} - r := NewBrowserReconciler(c, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(c, store.NewBrowserConfigStore(), scheme, defaultCfg) err := r.retryStatusUpdate(context.Background(), brw, func(b *browserv1.Browser) { b.Status.Phase = corev1.PodRunning @@ -2520,7 +2807,7 @@ func TestRetryUpdateMaxConflict(t *testing.T) { } base := newBrowserClient(scheme, brw) c := &alwaysConflictClient{Client: base} - r := NewBrowserReconciler(c, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(c, store.NewBrowserConfigStore(), scheme, defaultCfg) err := r.retryUpdate(context.Background(), brw, func(b *browserv1.Browser) { if b.Labels == nil { @@ -2533,19 +2820,153 @@ func TestRetryUpdateMaxConflict(t *testing.T) { } } -func TestReconcilePodPendingCreationTimeoutFullCycle(t *testing.T) { +func TestHandleMissingPodQuotaExceededFailsBrowser(t *testing.T) { scheme := newBrowserScheme(t) + cfgStore := store.NewBrowserConfigStore() + spec := &configv1.BrowserVersionConfigSpec{Image: "img"} + setStoreConfig(t, cfgStore, "ns/chrome:120", spec) + brw := &browserv1.Browser{ ObjectMeta: metav1.ObjectMeta{ - Name: "b1", - Namespace: "ns", - Finalizers: []string{browserPodFinalizer}, - Labels: map[string]string{ - "selenosis.io/browser": "b1", - "selenosis.io/browser.name": "chrome", - "selenosis.io/browser.version": "120", - }, - }, + Name: "b1", + Namespace: "ns", + }, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + } + base := newBrowserClient(scheme, brw) + quotaErr := apierrors.NewForbidden(schema.GroupResource{Resource: "pods"}, "b1", errors.New("exceeded quota")) + cl := errorClient{Client: base, createErr: quotaErr} + r := NewBrowserReconciler(cl, cfgStore, scheme, defaultCfg) + + res, err := r.handleMissingPod(context.Background(), brw) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if res.RequeueAfter != 0 { + t.Fatalf("expected no requeue, got %v", res.RequeueAfter) + } + + if err := cl.Client.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected browser to be deleted after quota exceeded, got err=%v", err) + } + + if err := cl.Client.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected no pod to be created, got err=%v", err) + } +} + +func TestHandleMissingPodQuotaExceededStatusUpdateError(t *testing.T) { + scheme := newBrowserScheme(t) + cfgStore := store.NewBrowserConfigStore() + spec := &configv1.BrowserVersionConfigSpec{Image: "img"} + setStoreConfig(t, cfgStore, "ns/chrome:120", spec) + + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + Finalizers: []string{browserPodFinalizer}, + Labels: map[string]string{ + browserv1.BrowserLabelKey: "b1", + browserv1.BrowserNameLabelKey: "chrome", + browserv1.BrowserVersionLabelKey: "120", + }, + }, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + Status: browserv1.BrowserStatus{Phase: corev1.PodPending}, + } + quotaErr := apierrors.NewForbidden(schema.GroupResource{Resource: "pods"}, "b1", errors.New("exceeded quota")) + base := newBrowserClient(scheme, brw) + cl := quotaCreatePatchErrorClient{ + Client: base, + quotaCreateErr: quotaErr, + statusPatchErr: apierrors.NewInternalError(errors.New("patch")), + } + r := NewBrowserReconciler(cl, cfgStore, scheme, defaultCfg) + + res, err := r.handleMissingPod(context.Background(), brw) + if err == nil { + t.Fatalf("expected error") + } + assertJitteredRequeue(t, res.RequeueAfter, mediumRetry) +} + +func TestReconcileQuotaExceededDeletesBrowserOnNextReconcile(t *testing.T) { + scheme := newBrowserScheme(t) + cfgStore := store.NewBrowserConfigStore() + spec := &configv1.BrowserVersionConfigSpec{Image: "img"} + setStoreConfig(t, cfgStore, "ns/chrome:120", spec) + + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + }, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + } + base := newBrowserClient(scheme, brw) + quotaErr := apierrors.NewForbidden(schema.GroupResource{Resource: "pods"}, "b1", errors.New("exceeded quota")) + cl := errorClient{Client: base, createErr: quotaErr} + r := NewBrowserReconciler(cl, cfgStore, scheme, defaultCfg) + + req := ctrl.Request{NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}} + + _, err := r.Reconcile(context.Background(), req) + if err != nil { + t.Fatalf("first reconcile: %v", err) + } + + got := &browserv1.Browser{} + if err := cl.Client.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, got); err != nil { + t.Fatalf("get browser after first reconcile: %v", err) + } + if got.Status.Phase != corev1.PodFailed { + t.Fatalf("expected Failed after quota error, got %s", got.Status.Phase) + } + + r2 := NewBrowserReconciler(cl.Client, cfgStore, scheme, defaultCfg) + _, err = r2.Reconcile(context.Background(), req) + if err != nil { + t.Fatalf("second reconcile: %v", err) + } + + if err := cl.Client.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); err == nil { + t.Fatalf("expected browser to be deleted after second reconcile") + } +} + +type quotaCreatePatchErrorClient struct { + client.Client + quotaCreateErr error + statusPatchErr error +} + +func (q quotaCreatePatchErrorClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if q.quotaCreateErr != nil { + if _, ok := obj.(*corev1.Pod); ok { + return q.quotaCreateErr + } + } + return q.Client.Create(ctx, obj, opts...) +} + +func (q quotaCreatePatchErrorClient) Status() client.StatusWriter { + return &statusPatchErrorWriter{StatusWriter: q.Client.Status(), err: q.statusPatchErr} +} + +func TestReconcilePodPendingCreationTimeoutFullCycle(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + Finalizers: []string{browserPodFinalizer}, + Labels: map[string]string{ + browserv1.BrowserLabelKey: "b1", + browserv1.BrowserNameLabelKey: "chrome", + browserv1.BrowserVersionLabelKey: "120", + }, + }, Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, Status: browserv1.BrowserStatus{Phase: corev1.PodPending}, } @@ -2553,7 +2974,7 @@ func TestReconcilePodPendingCreationTimeoutFullCycle(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "b1", Namespace: "ns", - CreationTimestamp: metav1.NewTime(time.Now().Add(-podCreationTimeout - time.Second).UTC()), + CreationTimestamp: metav1.NewTime(time.Now().Add(-defaultCfg.PodCreationTimeout - time.Second).UTC()), }, Status: corev1.PodStatus{ Phase: corev1.PodPending, @@ -2568,28 +2989,17 @@ func TestReconcilePodPendingCreationTimeoutFullCycle(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) req := ctrl.Request{NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}} if _, err := r.Reconcile(context.Background(), req); err != nil { - t.Fatalf("first reconcile: %v", err) - } - if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); err == nil { - t.Fatalf("expected pod to be deleted after first reconcile") + t.Fatalf("reconcile: %v", err) } - got := &browserv1.Browser{} - if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, got); err != nil { - t.Fatalf("get browser after first reconcile: %v", err) + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected pod to be deleted after reconcile, got err=%v", err) } - if got.Status.Phase != corev1.PodFailed { - t.Fatalf("expected Status.Phase=Failed after first reconcile, got %s", got.Status.Phase) - } - - if _, err := r.Reconcile(context.Background(), req); err != nil { - t.Fatalf("second reconcile: %v", err) - } - if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); err == nil { - t.Fatalf("expected browser to be deleted after second reconcile") + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected browser to be deleted after reconcile, got err=%v", err) } } @@ -2601,9 +3011,9 @@ func TestReconcilePodPendingContainerTerminatedFullCycle(t *testing.T) { Namespace: "ns", Finalizers: []string{browserPodFinalizer}, Labels: map[string]string{ - "selenosis.io/browser": "b1", - "selenosis.io/browser.name": "chrome", - "selenosis.io/browser.version": "120", + browserv1.BrowserLabelKey: "b1", + browserv1.BrowserNameLabelKey: "chrome", + browserv1.BrowserVersionLabelKey: "120", }, }, Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, @@ -2627,28 +3037,17 @@ func TestReconcilePodPendingContainerTerminatedFullCycle(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) req := ctrl.Request{NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}} if _, err := r.Reconcile(context.Background(), req); err != nil { - t.Fatalf("first reconcile: %v", err) - } - if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); err == nil { - t.Fatalf("expected pod to be deleted after first reconcile") + t.Fatalf("reconcile: %v", err) } - got := &browserv1.Browser{} - if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, got); err != nil { - t.Fatalf("get browser after first reconcile: %v", err) + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected pod to be deleted after reconcile, got err=%v", err) } - if got.Status.Phase != corev1.PodFailed { - t.Fatalf("expected Status.Phase=Failed after first reconcile, got %s", got.Status.Phase) - } - - if _, err := r.Reconcile(context.Background(), req); err != nil { - t.Fatalf("second reconcile: %v", err) - } - if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); err == nil { - t.Fatalf("expected browser to be deleted after second reconcile") + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected browser to be deleted after reconcile, got err=%v", err) } } @@ -2660,9 +3059,9 @@ func TestReconcilePodFailedFullCycle(t *testing.T) { Namespace: "ns", Finalizers: []string{browserPodFinalizer}, Labels: map[string]string{ - "selenosis.io/browser": "b1", - "selenosis.io/browser.name": "chrome", - "selenosis.io/browser.version": "120", + browserv1.BrowserLabelKey: "b1", + browserv1.BrowserNameLabelKey: "chrome", + browserv1.BrowserVersionLabelKey: "120", }, }, Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, @@ -2677,31 +3076,298 @@ func TestReconcilePodFailedFullCycle(t *testing.T) { }, } cl := newBrowserClient(scheme, brw, pod) - r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) req := ctrl.Request{NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}} if _, err := r.Reconcile(context.Background(), req); err != nil { - t.Fatalf("first reconcile: %v", err) + t.Fatalf("reconcile: %v", err) + } + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected pod to be deleted after reconcile, got err=%v", err) + } + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); !apierrors.IsNotFound(err) { + t.Fatalf("expected browser to be deleted after reconcile, got err=%v", err) + } +} + +func TestReconcilePodPendingInitContainerTerminated(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + InitContainerStatuses: []corev1.ContainerStatus{ + { + Name: "init-setup", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + ExitCode: 1, + Reason: "Error", + }, + }, + }, + }, + }, + } + cl := newBrowserClient(scheme, brw, pod) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) + + _, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, + }) + if err != nil { + t.Fatalf("expected no error, got %v", err) } + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); err == nil { - t.Fatalf("expected pod to be deleted after first reconcile") + t.Fatalf("expected pod to be deleted") } + got := &browserv1.Browser{} if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, got); err != nil { - t.Fatalf("get browser after first reconcile: %v", err) + t.Fatalf("get browser: %v", err) } if got.Status.Phase != corev1.PodFailed { - t.Fatalf("expected Status.Phase=Failed after first reconcile, got %s", got.Status.Phase) + t.Fatalf("expected failed status, got %s", got.Status.Phase) } - if !strings.Contains(got.Status.Message, "OOMKilled") { + if !strings.Contains(got.Status.Message, "init container") { + t.Fatalf("expected message to mention init container, got %q", got.Status.Message) + } +} + +func TestReconcilePodPendingInitContainerTerminatedExitZero(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + InitContainerStatuses: []corev1.ContainerStatus{ + { + Name: "init-setup", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + ExitCode: 0, + Reason: "Completed", + }, + }, + }, + }, + }, + } + cl := newBrowserClient(scheme, brw, pod) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) + + _, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, + }) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); err != nil { + t.Fatalf("expected pod to still exist, got %v", err) + } +} + +func TestReconcilePodPendingInitContainerCreationTimeout(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + CreationTimestamp: metav1.NewTime(time.Now().Add(-defaultCfg.PodCreationTimeout - time.Second).UTC()), + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + InitContainerStatuses: []corev1.ContainerStatus{ + { + Name: "init-setup", + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Reason: "PodInitializing", + }, + }, + }, + }, + }, + } + cl := newBrowserClient(scheme, brw, pod) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) + + _, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, + }) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); err == nil { + t.Fatalf("expected pod to be deleted") + } + + got := &browserv1.Browser{} + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, got); err != nil { + t.Fatalf("get browser: %v", err) + } + if got.Status.Phase != corev1.PodFailed { + t.Fatalf("expected failed status, got %s", got.Status.Phase) + } + if !strings.Contains(got.Status.Message, "init container") { + t.Fatalf("expected message to mention init container, got %q", got.Status.Message) + } +} + +func TestReconcilePodPendingInitContainerImagePullBackOff(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + InitContainerStatuses: []corev1.ContainerStatus{ + { + Name: "init-setup", + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Reason: "ImagePullBackOff", + Message: "back-off pulling image", + }, + }, + }, + }, + }, + } + cl := newBrowserClient(scheme, brw, pod) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) + + _, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, + }) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); err == nil { + t.Fatalf("expected pod to be deleted") + } + + got := &browserv1.Browser{} + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, got); err != nil { + t.Fatalf("get browser: %v", err) + } + if got.Status.Phase != corev1.PodFailed { + t.Fatalf("expected failed status, got %s", got.Status.Phase) + } + if !strings.Contains(got.Status.Message, "init container") { + t.Fatalf("expected message to mention init container, got %q", got.Status.Message) + } + if !strings.Contains(got.Status.Message, "ImagePullBackOff") { t.Fatalf("expected message to contain reason, got %q", got.Status.Message) } +} - if _, err := r.Reconcile(context.Background(), req); err != nil { - t.Fatalf("second reconcile: %v", err) +func TestReconcilePodPendingInitContainerNoTimeoutYet(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, } - if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); err == nil { - t.Fatalf("expected browser to be deleted after second reconcile") + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + CreationTimestamp: metav1.NewTime(time.Now().UTC()), + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + InitContainerStatuses: []corev1.ContainerStatus{ + { + Name: "init-setup", + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Reason: "PodInitializing", + }, + }, + }, + }, + }, + } + cl := newBrowserClient(scheme, brw, pod) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) + + _, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, + }) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); err != nil { + t.Fatalf("expected pod to still exist, got %v", err) + } +} + +func TestReconcilePodPendingEmptyStatusesWithInitRunning(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + CreationTimestamp: metav1.NewTime(time.Now().Add(-defaultCfg.PodCreationTimeout - time.Second).UTC()), + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + InitContainerStatuses: []corev1.ContainerStatus{ + { + Name: "init-setup", + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Reason: "ContainerCreating", + }, + }, + }, + }, + }, + } + cl := newBrowserClient(scheme, brw, pod) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme, defaultCfg) + + _, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, + }) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &corev1.Pod{}); err == nil { + t.Fatalf("expected pod to be deleted after init container timeout") + } + + got := &browserv1.Browser{} + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, got); err != nil { + t.Fatalf("get browser: %v", err) + } + if got.Status.Phase != corev1.PodFailed { + t.Fatalf("expected failed status, got %s", got.Status.Phase) } } @@ -2712,7 +3378,7 @@ func TestRetryStatusUpdateMaxConflict(t *testing.T) { } base := newBrowserClient(scheme, brw) c := &alwaysConflictClient{Client: base} - r := NewBrowserReconciler(c, store.NewBrowserConfigStore(), scheme) + r := NewBrowserReconciler(c, store.NewBrowserConfigStore(), scheme, defaultCfg) err := r.retryStatusUpdate(context.Background(), brw, func(b *browserv1.Browser) { b.Status.Phase = corev1.PodRunning @@ -2721,3 +3387,344 @@ func TestRetryStatusUpdateMaxConflict(t *testing.T) { t.Fatalf("expected error") } } + +func TestContainerStatusesEqualPorts(t *testing.T) { + withPorts := func(ports []browserv1.ContainerPort) []browserv1.ContainerStatus { + return []browserv1.ContainerStatus{{Name: "c", Ports: ports}} + } + + p1 := []browserv1.ContainerPort{{ContainerPort: 4444}} + p2 := []browserv1.ContainerPort{{ContainerPort: 4445}} + p3 := []browserv1.ContainerPort{{ContainerPort: 4444}, {ContainerPort: 5555}} + + if !containerStatusesEqual(withPorts(p1), withPorts(p1)) { + t.Fatal("expected equal ports to be equal") + } + if containerStatusesEqual(withPorts(p1), withPorts(p3)) { + t.Fatal("expected different port count to be unequal") + } + if containerStatusesEqual(withPorts(p1), withPorts(p2)) { + t.Fatal("expected different port values to be unequal") + } +} + +func TestContainerStateEqualWaiting(t *testing.T) { + a := corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: "Init", Message: "msg"}} + b := corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: "Init", Message: "msg"}} + c := corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: "Other"}} + + if !containerStateEqual(a, b) { + t.Fatal("expected identical waiting states to be equal") + } + if containerStateEqual(a, c) { + t.Fatal("expected different waiting reasons to be unequal") + } +} + +func TestApplySelenosisOptionsNilLabels(t *testing.T) { + pod := &corev1.Pod{} + opts := &SelenosisOptions{ + Labels: map[string]string{"env": "test"}, + } + applySelenosisOptions(pod, opts) + if pod.Labels["env"] != "test" { + t.Fatalf("expected label env=test, got %v", pod.Labels) + } +} + +func TestMergeEnvVarsEmptyOverride(t *testing.T) { + base := []corev1.EnvVar{{Name: "A", Value: "1"}} + result := mergeEnvVars(base, nil) + if len(result) != 1 || result[0].Name != "A" { + t.Fatalf("expected base unchanged, got %v", result) + } +} + +func TestPodChangedPredicate(t *testing.T) { + now := metav1.Now() + later := metav1.NewTime(now.Add(time.Second)) + + basePod := func() *corev1.Pod { + return &corev1.Pod{ + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + PodIP: "10.0.0.1", + StartTime: &now, + ContainerStatuses: []corev1.ContainerStatus{ + {Name: "browser", Ready: true}, + }, + InitContainerStatuses: []corev1.ContainerStatus{ + {Name: "init", Ready: true}, + }, + }, + } + } + + p := podChangedPredicate{} + + cases := []struct { + name string + old client.Object + new client.Object + want bool + }{ + { + name: "no changes", + old: basePod(), + new: basePod(), + want: false, + }, + { + name: "phase changed", + old: basePod(), + new: func() *corev1.Pod { + pod := basePod() + pod.Status.Phase = corev1.PodFailed + return pod + }(), + want: true, + }, + { + name: "pod ip changed", + old: basePod(), + new: func() *corev1.Pod { + pod := basePod() + pod.Status.PodIP = "10.0.0.2" + return pod + }(), + want: true, + }, + { + name: "start time changed", + old: basePod(), + new: func() *corev1.Pod { + pod := basePod() + pod.Status.StartTime = &later + return pod + }(), + want: true, + }, + { + name: "start time nil to non-nil", + old: func() *corev1.Pod { + pod := basePod() + pod.Status.StartTime = nil + return pod + }(), + new: basePod(), + want: true, + }, + { + name: "container statuses changed", + old: basePod(), + new: func() *corev1.Pod { + pod := basePod() + pod.Status.ContainerStatuses[0].RestartCount = 1 + return pod + }(), + want: true, + }, + { + name: "init container statuses changed", + old: basePod(), + new: func() *corev1.Pod { + pod := basePod() + pod.Status.InitContainerStatuses[0].RestartCount = 1 + return pod + }(), + want: true, + }, + { + name: "only Ready changed (not tracked)", + old: basePod(), + new: func() *corev1.Pod { + pod := basePod() + pod.Status.ContainerStatuses[0].Ready = false + return pod + }(), + want: false, + }, + { + name: "deletion timestamp set", + old: basePod(), + new: func() *corev1.Pod { + pod := basePod() + pod.DeletionTimestamp = &now + return pod + }(), + want: true, + }, + { + name: "non-pod old object", + old: &corev1.ConfigMap{}, + new: basePod(), + want: true, + }, + { + name: "non-pod new object", + old: basePod(), + new: &corev1.ConfigMap{}, + want: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := p.Update(event.UpdateEvent{ + ObjectOld: tc.old, + ObjectNew: tc.new, + }) + if got != tc.want { + t.Fatalf("Update() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestPodContainerStatusesChanged(t *testing.T) { + base := []corev1.ContainerStatus{ + {Name: "browser", RestartCount: 0, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}}, + } + + cases := []struct { + name string + old []corev1.ContainerStatus + new []corev1.ContainerStatus + want bool + }{ + { + name: "no changes", + old: base, + new: []corev1.ContainerStatus{{Name: "browser", RestartCount: 0, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}}}, + want: false, + }, + { + name: "both empty", + old: nil, + new: nil, + want: false, + }, + { + name: "length differs", + old: base, + new: nil, + want: true, + }, + { + name: "restart count changed", + old: base, + new: []corev1.ContainerStatus{{Name: "browser", RestartCount: 1, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}}}, + want: true, + }, + { + name: "state changed running to waiting", + old: base, + new: []corev1.ContainerStatus{{Name: "browser", RestartCount: 0, State: corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: "CrashLoopBackOff"}}}}, + want: true, + }, + { + name: "name changed", + old: base, + new: []corev1.ContainerStatus{{Name: "sidecar", RestartCount: 0, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}}}, + want: true, + }, + { + name: "Ready changed only (not tracked)", + old: base, + new: []corev1.ContainerStatus{{Name: "browser", RestartCount: 0, Ready: true, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}}}, + want: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := podContainerStatusesChanged(tc.old, tc.new) + if got != tc.want { + t.Fatalf("podContainerStatusesChanged() = %v, want %v", got, tc.want) + } + }) + } +} + +func TestPodStatusMatchesBrowser(t *testing.T) { + cases := []struct { + name string + pod []corev1.ContainerStatus + browser []browserv1.ContainerStatus + want bool + }{ + { + name: "both empty", + pod: nil, + browser: nil, + want: true, + }, + { + name: "equal", + pod: []corev1.ContainerStatus{ + {Name: "browser", Image: "chrome:123", RestartCount: 0, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}}, + }, + browser: []browserv1.ContainerStatus{ + {Name: "browser", Image: "chrome:123", RestartCount: 0, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}}, + }, + want: true, + }, + { + name: "length differs", + pod: []corev1.ContainerStatus{ + {Name: "browser"}, + }, + browser: nil, + want: false, + }, + { + name: "name differs", + pod: []corev1.ContainerStatus{ + {Name: "browser", Image: "chrome:123"}, + }, + browser: []browserv1.ContainerStatus{ + {Name: "sidecar", Image: "chrome:123"}, + }, + want: false, + }, + { + name: "image differs", + pod: []corev1.ContainerStatus{ + {Name: "browser", Image: "chrome:124"}, + }, + browser: []browserv1.ContainerStatus{ + {Name: "browser", Image: "chrome:123"}, + }, + want: false, + }, + { + name: "restart count differs", + pod: []corev1.ContainerStatus{ + {Name: "browser", Image: "chrome:123", RestartCount: 1, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}}, + }, + browser: []browserv1.ContainerStatus{ + {Name: "browser", Image: "chrome:123", RestartCount: 0, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}}, + }, + want: false, + }, + { + name: "state differs", + pod: []corev1.ContainerStatus{ + {Name: "browser", Image: "chrome:123", State: corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: "err"}}}, + }, + browser: []browserv1.ContainerStatus{ + {Name: "browser", Image: "chrome:123", State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{}}}, + }, + want: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := podStatusMatchesBrowser(tc.pod, tc.browser) + if got != tc.want { + t.Fatalf("podStatusMatchesBrowser() = %v, want %v", got, tc.want) + } + }) + } +} diff --git a/controllers/browserconfig/browserconfig_reconciler.go b/controllers/browserconfig/browserconfig_reconciler.go index bb2d6ad..987c7ca 100644 --- a/controllers/browserconfig/browserconfig_reconciler.go +++ b/controllers/browserconfig/browserconfig_reconciler.go @@ -2,6 +2,8 @@ package browserconfig import ( "context" + "fmt" + "math/rand/v2" "time" configv1 "github.com/alcounit/browser-controller/apis/browserconfig/v1" @@ -15,16 +17,16 @@ import ( ) const ( - browserConfigFinalizer string = "browserconfig.selenosis.io/finalizer" - shortRetry = time.Second * 5 - mediumRetry = time.Second * 10 + browserConfigFinalizer = "browserconfig.selenosis.io/finalizer" + maxRetries = 3 + shortRetry = time.Second * 5 + mediumRetry = time.Second * 10 ) // +kubebuilder:rbac:groups=browserconfig.selenosis.io,resources=browserconfigs,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=browserconfig.selenosis.io,resources=browserconfigs/status,verbs=get;update;patch // +kubebuilder:rbac:groups=browserconfig.selenosis.io,resources=browserconfigs/finalizers,verbs=update -// BrowserConfigReconciler reconciles BrowserConfig resources type BrowserConfigReconciler struct { client client.Client scheme *runtime.Scheme @@ -37,14 +39,12 @@ func NewBrowserConfigReconciler(client client.Client, scheme *runtime.Scheme) *B } } -// SetupWithManager sets up the controller with the Manager func (r *BrowserConfigReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&configv1.BrowserConfig{}). Complete(r) } -// Reconcile synchronizes the state of BrowserConfig func (r *BrowserConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := logger.FromContext(ctx) @@ -59,8 +59,9 @@ func (r *BrowserConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques if !browserConfig.DeletionTimestamp.IsZero() { if controllerutil.ContainsFinalizer(browserConfig, browserConfigFinalizer) { - controllerutil.RemoveFinalizer(browserConfig, browserConfigFinalizer) - if err := r.client.Update(ctx, browserConfig); err != nil { + if err := r.retryPatch(ctx, browserConfig, func(bc *configv1.BrowserConfig) { + controllerutil.RemoveFinalizer(bc, browserConfigFinalizer) + }); err != nil { log.Error(err, "failed to remove finalizer") return ctrl.Result{RequeueAfter: shortRetry}, err } @@ -69,8 +70,9 @@ func (r *BrowserConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques } if !controllerutil.ContainsFinalizer(browserConfig, browserConfigFinalizer) { - controllerutil.AddFinalizer(browserConfig, browserConfigFinalizer) - if err := r.client.Update(ctx, browserConfig); err != nil { + if err := r.retryPatch(ctx, browserConfig, func(bc *configv1.BrowserConfig) { + controllerutil.AddFinalizer(bc, browserConfigFinalizer) + }); err != nil { log.Error(err, "failed to add finalizer") return ctrl.Result{RequeueAfter: shortRetry}, err } @@ -78,3 +80,38 @@ func (r *BrowserConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, nil } + +func (r *BrowserConfigReconciler) retryPatch(ctx context.Context, bc *configv1.BrowserConfig, mutate func(*configv1.BrowserConfig)) error { + nn := types.NamespacedName{Name: bc.Name, Namespace: bc.Namespace} + + for i := range maxRetries { + current := &configv1.BrowserConfig{} + if err := r.client.Get(ctx, nn, current); err != nil { + return err + } + + before := current.DeepCopy() + mutate(current) + + err := r.client.Patch(ctx, current, client.MergeFrom(before)) + if err == nil { + return nil + } + + if !errors.IsConflict(err) { + return err + } + + base := time.Millisecond * time.Duration(100*(1< 200*time.Millisecond { + t.Fatalf("retryPatch took %v on cancelled context, expected < 200ms", elapsed) + } +} diff --git a/store/browserconfig_store.go b/store/browserconfig_store.go index fffb49d..2afa44c 100644 --- a/store/browserconfig_store.go +++ b/store/browserconfig_store.go @@ -102,15 +102,24 @@ func (s *BrowserConfigStore) onAddOrUpdate(oldObj, newObj any, log logr.Logger) s.mu.Lock() defer s.mu.Unlock() - copy := new.DeepCopy() - copy.Spec.MergeWithTemplate() + if old != nil { + for browserName, versions := range old.Spec.Browsers { + for version := range versions { + key := keyFor(old.Namespace, browserName, version) + delete(s.config, key) + } + } + } - for browserName, versions := range copy.Spec.Browsers { + cp := new.DeepCopy() + cp.Spec.MergeWithTemplate() + + for browserName, versions := range cp.Spec.Browsers { for version, cfg := range versions { if cfg == nil { continue } - key := keyFor(copy.Namespace, browserName, version) + key := keyFor(cp.Namespace, browserName, version) s.config[key] = cfg log.Info("BrowserConfig added/updated", "key", key) } @@ -146,6 +155,17 @@ func (s *BrowserConfigStore) onDelete(obj any, log logr.Logger) { } } +func (s *BrowserConfigStore) DeleteConfig(namespace string, browsers map[string]map[string]*configv1.BrowserVersionConfigSpec) { + s.mu.Lock() + defer s.mu.Unlock() + + for browserName, versions := range browsers { + for version := range versions { + delete(s.config, keyFor(namespace, browserName, version)) + } + } +} + // Get retrieves BrowserVersionConfig from the in-memory store. func (s *BrowserConfigStore) Get(namespace, browserName, version string) (*configv1.BrowserVersionConfigSpec, bool) { s.mu.RLock() @@ -155,5 +175,5 @@ func (s *BrowserConfigStore) Get(namespace, browserName, version string) (*confi return nil, false } - return cfg.DeepCopy(), exists + return cfg, true } diff --git a/store/browserconfig_store_test.go b/store/browserconfig_store_test.go index 2caf2a7..4c6ad1c 100644 --- a/store/browserconfig_store_test.go +++ b/store/browserconfig_store_test.go @@ -172,6 +172,104 @@ func TestBrowserConfigStoreOnAddOrUpdateSkipsSameResourceVersion(t *testing.T) { } } +func TestBrowserConfigStoreOnUpdateRemovesStaleKeys(t *testing.T) { + oldObj := &configv1.BrowserConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cfg", + Namespace: "ns", + ResourceVersion: "1", + }, + Spec: configv1.BrowserConfigSpec{ + Browsers: map[string]map[string]*configv1.BrowserVersionConfigSpec{ + "Chrome": { + "99.0": {Image: "chrome:99"}, + "100.0": {Image: "chrome:100"}, + }, + }, + }, + } + + store := NewBrowserConfigStore() + store.onAddOrUpdate(nil, oldObj, logr.Discard()) + + if _, ok := store.Get("ns", "chrome", "99.0"); !ok { + t.Fatalf("expected chrome:99 to exist after initial add") + } + if _, ok := store.Get("ns", "chrome", "100.0"); !ok { + t.Fatalf("expected chrome:100 to exist after initial add") + } + + newObj := &configv1.BrowserConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cfg", + Namespace: "ns", + ResourceVersion: "2", + }, + Spec: configv1.BrowserConfigSpec{ + Browsers: map[string]map[string]*configv1.BrowserVersionConfigSpec{ + "Chrome": { + "100.0": {Image: "chrome:100-updated"}, + }, + }, + }, + } + + store.onAddOrUpdate(oldObj, newObj, logr.Discard()) + + if _, ok := store.Get("ns", "chrome", "99.0"); ok { + t.Fatalf("expected chrome:99 to be removed after update") + } + + cfg, ok := store.Get("ns", "chrome", "100.0") + if !ok || cfg == nil { + t.Fatalf("expected chrome:100 to still exist after update") + } + if cfg.Image != "chrome:100-updated" { + t.Fatalf("expected chrome:100 to be updated, got %q", cfg.Image) + } +} + +func TestBrowserConfigStoreOnUpdateRemovesStaleBrowser(t *testing.T) { + oldObj := &configv1.BrowserConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cfg", + Namespace: "ns", + ResourceVersion: "1", + }, + Spec: configv1.BrowserConfigSpec{ + Browsers: map[string]map[string]*configv1.BrowserVersionConfigSpec{ + "Chrome": {"100.0": {Image: "chrome:100"}}, + "Firefox": {"120.0": {Image: "firefox:120"}}, + }, + }, + } + + store := NewBrowserConfigStore() + store.onAddOrUpdate(nil, oldObj, logr.Discard()) + + newObj := &configv1.BrowserConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cfg", + Namespace: "ns", + ResourceVersion: "2", + }, + Spec: configv1.BrowserConfigSpec{ + Browsers: map[string]map[string]*configv1.BrowserVersionConfigSpec{ + "Chrome": {"100.0": {Image: "chrome:100"}}, + }, + }, + } + + store.onAddOrUpdate(oldObj, newObj, logr.Discard()) + + if _, ok := store.Get("ns", "firefox", "120.0"); ok { + t.Fatalf("expected firefox:120 to be removed after browser was dropped from config") + } + if _, ok := store.Get("ns", "chrome", "100.0"); !ok { + t.Fatalf("expected chrome:100 to still exist") + } +} + func TestBrowserConfigStoreOnDeleteRemovesKeys(t *testing.T) { bc := &configv1.BrowserConfig{ ObjectMeta: metav1.ObjectMeta{