diff --git a/README.md b/README.md index 55ec9ba..5a7f0bf 100644 --- a/README.md +++ b/README.md @@ -317,6 +317,53 @@ kubectl get browserconfig default-browser-config -o yaml --- +## Resource Cleanup + +The controller is responsible for ensuring that both the `Browser` CR and its associated Pod are always cleaned up, regardless of the failure mode. Cleanup is enforced through a **finalizer** (`browserpod.selenosis.io/finalizer`) placed on every `Browser` CR. + +### Mechanism + +Every `Browser` CR receives the finalizer on creation. The controller uses two internal primitives: + +- **`deletePod`** — force-deletes the Pod with `gracePeriodSeconds=0`. Ignores `NotFound` (safe to call even if Pod is already gone). +- **`deleteBrowser`** — removes the finalizer from the `Browser` CR, then calls `Delete` on the CR. After the finalizer is removed Kubernetes completes the deletion immediately. + +Most failure paths follow a **two-step process** across two reconcile cycles: + +1. **First reconcile** — detects the failure, force-deletes the Pod, sets `Browser.status.phase=Failed` with a descriptive message. +2. **Second reconcile** — sees `status.phase=Failed`, calls `deletePod` (no-op if already gone) and `deleteBrowser`, which removes the CR. + +### Cleanup Scenarios + +| 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** | +| `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** | +| `Browser.status.phase=Failed` (failed early exit — any of the above on the next reconcile) | force-deleted (grace=0) | finalizer removed → `Delete` → **deleted** | +| Critical container (`browser` or `seleniferous`) `Terminated` while pod is `Running` | **deleted** via OwnerReference GC after CR deletion | `deleteBrowser` → finalizer removed → **deleted** | +| `Browser` CR `DeletionTimestamp` set (external `kubectl delete`) | explicit `Delete` in `handleDeletion`, waits for pod termination | finalizer removed after pod is gone → **deleted** | +| Pod `DeletionTimestamp` set while CR is alive | already terminating | `deleteBrowser` triggered → **deleted** | +| Pod stuck `Terminating` beyond `podDeletionTimeout` (5 min) | force-deleted (grace=0, best-effort) | finalizer removed regardless → **deleted** | + +### `Browser.status.message` on Failure + +Every failure path writes a human-readable message to `Browser.status.message` before deletion: + +| Cause | Example message | +|---|---| +| No `BrowserConfig` | `Browser configuration not found` | +| Creation timeout | `pod creation timeout exceeded after 5m0s, container browser: ContainerCreating` | +| 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` | + +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. + +--- + ## Build & Generate This project uses `make` to generate code, manifests, and build the controller image. diff --git a/apis/browserconfig/v1/browser_config.go b/apis/browserconfig/v1/browser_config.go index 063ea6f..fe0dd05 100644 --- a/apis/browserconfig/v1/browser_config.go +++ b/apis/browserconfig/v1/browser_config.go @@ -205,7 +205,7 @@ func (b *BrowserVersionConfigSpec) mergeWithSpec(t *BrowserConfigSpec) { b.Tolerations = mergeTolerationPtr(t.Template.Tolerations, b.Tolerations) b.HostAliases = mergeHostAliasPtr(t.Template.HostAliases, b.HostAliases) - b.VolumeMounts = mergeVolumeMountsPtr(b.VolumeMounts, t.Template.VolumeMounts) + b.VolumeMounts = mergeVolumeMountsPtr(t.Template.VolumeMounts, b.VolumeMounts) originalSidecars := b.Sidecars @@ -227,7 +227,7 @@ func (b *BrowserVersionConfigSpec) mergeWithSpec(t *BrowserConfigSpec) { originalInitContainers := b.InitContainers - b.InitContainers = mergeSidecarPtr(b.InitContainers, t.Template.InitContainers) + b.InitContainers = mergeSidecarPtr(t.Template.InitContainers, b.InitContainers) if originalInitContainers != nil && t.Template.InitContainers != nil { for i := range *b.InitContainers { @@ -288,24 +288,28 @@ func mergeEnvPtr(template, override *[]corev1.EnvVar) *[]corev1.EnvVar { return nil } - result := map[string]corev1.EnvVar{} + // Build merged slice preserving template order; override vars replace template vars in-place, + // new override-only vars are appended at the end. + index := make(map[string]int) // name -> position in merged + merged := make([]corev1.EnvVar, 0) + if template != nil { for _, env := range *template { - result[env.Name] = env + index[env.Name] = len(merged) + merged = append(merged, env) } } if override != nil { for _, env := range *override { - result[env.Name] = env + if i, exists := index[env.Name]; exists { + merged[i] = env + } else { + merged = append(merged, env) + } } } - merged := make([]corev1.EnvVar, 0, len(result)) - for _, env := range result { - merged = append(merged, env) - } - return &merged } @@ -440,7 +444,10 @@ func (s *Sidecar) mergeWithTemplate(t *Sidecar) { s.Command = t.Command } - s.WorkingDir = t.WorkingDir + if s.WorkingDir == nil { + s.WorkingDir = t.WorkingDir + } + s.Env = mergeEnvPtr(t.Env, s.Env) s.Ports = mergeContainerPortPtr(t.Ports, s.Ports) s.VolumeMounts = mergeVolumeMountPtr(t.VolumeMounts, s.VolumeMounts) diff --git a/controllers/browser/browser_reconciler.go b/controllers/browser/browser_reconciler.go index e4ce445..fb4d2d0 100644 --- a/controllers/browser/browser_reconciler.go +++ b/controllers/browser/browser_reconciler.go @@ -97,19 +97,12 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return r.handleDeletion(ctx, browser) } - // check if Browser is in Failed state, remove finalizer gc will take care Browser if browser.Status.Phase == corev1.PodFailed { - // Remove finalizer - if controllerutil.ContainsFinalizer(browser, browserPodFinalizer) { - if err := r.retryUpdate(ctx, browser, func(b *browserv1.Browser) { - controllerutil.RemoveFinalizer(b, browserPodFinalizer) - }); err != nil { - log.Error(err, "error removing Browser pod finalizer") - return ctrl.Result{RequeueAfter: mediumRetry}, err - } + 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 } - log.Info("Browser is in Failed state, nothing to do") - return ctrl.Result{}, nil + return r.deleteBrowser(ctx, browser) } // ensure finalizer is set @@ -136,7 +129,7 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct log.Error(err, "failed to update Browser with name label") return ctrl.Result{RequeueAfter: mediumRetry}, err } - log.Info("label selenosis.io/browser.name assigned to Browser") + log.Info("labels assigned to Browser") } // Set Pending status if not set @@ -157,11 +150,6 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct pod := &corev1.Pod{} if err := r.client.Get(ctx, types.NamespacedName{Name: browser.GetName(), Namespace: browser.GetNamespace()}, pod); err != nil { if errors.IsNotFound(err) { - if browser.Status.Phase == corev1.PodFailed { - log.Info("Browser is Failed state, browser pod not found. Ignoring since must be deleted") - return ctrl.Result{}, nil - } - log.Info("Browser pod not found, creating new Browser pod") return r.handleMissingPod(ctx, browser) } @@ -172,7 +160,7 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct // Handle pod being deleted if !pod.DeletionTimestamp.IsZero() && browser.DeletionTimestamp.IsZero() { - log.Info("Browser Pod is being deleted, deleting Browser resource") + log.Info("Browser pod is being deleted, deleting Browser resource") return r.deleteBrowser(ctx, browser) } @@ -180,14 +168,14 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct if pod.Status.Phase == corev1.PodFailed { if err := r.deletePod(ctx, pod); err != nil { - log.Info("deleting Browser Pod") + log.Info("deleting Browser pod after pod failure") return ctrl.Result{RequeueAfter: mediumRetry}, err } if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { b.Status.Phase = corev1.PodFailed - b.Status.Message = fmt.Sprintf("pod has failed with reason: %s - %s", pod.Status.Reason, pod.Status.Message) - log.Info("Browser Pod has failed", "reason", pod.Status.Reason, "message", pod.Status.Message) + b.Status.Message = fmt.Sprintf("Browser pod has failed with reason: %s - %s", pod.Status.Reason, pod.Status.Message) + 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 @@ -198,19 +186,19 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct for _, cs := range pod.Status.ContainerStatuses { if cs.State.Terminated != nil { + log.Info("Browser pod container terminated", + "container", cs.Name, + "reason", cs.State.Terminated.Reason, + "message", pod.Status.Message, + "exitCode", cs.State.Terminated.ExitCode) + if err := r.deletePod(ctx, pod); err != nil { + return ctrl.Result{RequeueAfter: mediumRetry}, err + } if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { b.Status.Phase = corev1.PodFailed - b.Status.Message = fmt.Sprintf("pod container %s terminated", cs.Name) - - log.Info("Browser Pod container terminated", - "container", - cs.Name, - "reason", - cs.State.Terminated.Reason, - "message", - pod.Status.Message, - "exitCode", - cs.State.Terminated.ExitCode) + b.Status.Message = fmt.Sprintf( + "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 } @@ -221,13 +209,15 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct if !pod.CreationTimestamp.IsZero() { podAge := time.Since(pod.CreationTimestamp.Time) if podAge > podCreationTimeout { - log.Info("Browser Pod creation timeout exceeded", "age", podAge.String(), "podStatus", pod.Status.Phase, "container", cs.Name) - + 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 + } 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", - podCreationTimeout.String()) + "pod creation timeout exceeded after %s, container %s: %s", + podCreationTimeout.String(), cs.Name, cs.State.Waiting.Reason) }); err != nil { return ctrl.Result{RequeueAfter: mediumRetry}, err } @@ -237,7 +227,7 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct reason := cs.State.Waiting.Reason if reason != "ContainerCreating" && reason != "PodInitializing" { - log.Info("Browser Pod container not ready", "container", cs.Name, "reason", reason, "message", cs.State.Waiting.Message, "podStatus", pod.Status.Phase) + 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 @@ -246,7 +236,7 @@ func (r *BrowserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct if err := r.retryStatusUpdate(ctx, browser, func(b *browserv1.Browser) { b.Status.Phase = corev1.PodFailed b.Status.Message = fmt.Sprintf( - "pod container %s failed: %s - %s", + "Browser pod container %s failed: %s - %s", cs.Name, reason, cs.State.Waiting.Message) }); err != nil { return ctrl.Result{RequeueAfter: mediumRetry}, err @@ -276,12 +266,12 @@ func (r *BrowserReconciler) handleDeletion(ctx context.Context, browser *browser // Delete pod if exists if err == nil { if pod.DeletionTimestamp.IsZero() { - log.Info("deleting associated pod") + log.Info("deleting associated Browser pod") var deleteOptions []client.DeleteOption if pod.Status.Phase == corev1.PodFailed { deleteOptions = append(deleteOptions, client.GracePeriodSeconds(0)) - log.Info("using force delete for failed pod") + log.Info("using force delete for failed Browser pod") } if err := r.client.Delete(ctx, pod, deleteOptions...); err != nil && !errors.IsNotFound(err) { @@ -294,25 +284,25 @@ func (r *BrowserReconciler) handleDeletion(ctx context.Context, browser *browser if pod.DeletionTimestamp != nil { deletionTime := pod.DeletionTimestamp.Time if time.Since(deletionTime) > podDeletionTimeout { - log.Info("Pod deletion is taking too long, attempting force delete") + 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 pod after timeout") + log.Error(err, "Failed to force delete Browser pod after timeout") // Continue anyway to remove finalizer } } else { // Wait for pod to be deleted - log.Info("waiting for pod to be deleted") + log.Info("waiting for Browser pod to be deleted") return ctrl.Result{RequeueAfter: quickCheck}, nil } } else { // Wait for pod to be deleted - log.Info("waiting for pod to be deleted") + log.Info("waiting for Browser pod to be deleted") return ctrl.Result{RequeueAfter: 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 pod check error") + log.Info("proceeding with finalizer removal despite Browser pod check error") } // Remove finalizer @@ -334,12 +324,12 @@ func (r *BrowserReconciler) deletePod(ctx context.Context, pod *corev1.Pod) erro 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") + log.Error(err, "failed to force delete Browser pod") return err } } - log.Info("Browser Pod forcibly deleted") + log.Info("Browser pod forcibly deleted") return nil } @@ -394,14 +384,14 @@ func (r *BrowserReconciler) handleMissingPod(ctx context.Context, browser *brows // Create pod from template 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") + log.Info("Browser pod already exists, will reconcile on next iteration") return ctrl.Result{RequeueAfter: quickCheck}, nil } - log.Error(err, "failed to create Browser Pod") + log.Error(err, "failed to create Browser pod") return ctrl.Result{}, err } - log.Info("Browser Pod created") + log.Info("Browser pod created") return ctrl.Result{RequeueAfter: quickCheck}, nil } @@ -437,7 +427,7 @@ func (r *BrowserReconciler) updateBrowserStatus(ctx context.Context, browser *br log.Info("Browser status set to Failed") } - log.Info("Browser Pod container statuses", + log.Info("Browser pod container statuses", "containerName", containerStatus.Name, "containerReady", @@ -472,20 +462,7 @@ func (r *BrowserReconciler) updateBrowserStatus(ctx context.Context, browser *br newContainerStatuses = append(newContainerStatuses, status) } - // Check if container statuses changed (simplified check, could be improved with DeepEqual) - if len(newContainerStatuses) != len(browser.Status.ContainerStatuses) { - containersStatusChanged = true - } else { - // Simple check for changes (could be improved with full comparison) - for i := range newContainerStatuses { - if i >= len(browser.Status.ContainerStatuses) || - newContainerStatuses[i].RestartCount != browser.Status.ContainerStatuses[i].RestartCount || - !containerStateEqual(newContainerStatuses[i].State, browser.Status.ContainerStatuses[i].State) { - containersStatusChanged = true - break - } - } - } + containersStatusChanged = !containerStatusesEqual(newContainerStatuses, browser.Status.ContainerStatuses) } // Update status if changed @@ -511,58 +488,6 @@ func (r *BrowserReconciler) updateBrowserStatus(ctx context.Context, browser *br return ctrl.Result{RequeueAfter: periodicReconcile}, nil } -func containerStateEqual(a, b corev1.ContainerState) bool { - if (a.Running != nil) != (b.Running != nil) { - return false - } - if (a.Terminated != nil) != (b.Terminated != nil) { - return false - } - if (a.Waiting != nil) != (b.Waiting != nil) { - return false - } - - if a.Running != nil && b.Running != nil { - return a.Running.StartedAt.Equal(&b.Running.StartedAt) - } - if a.Terminated != nil && b.Terminated != nil { - return a.Terminated.ExitCode == b.Terminated.ExitCode && - a.Terminated.Reason == b.Terminated.Reason && - a.Terminated.Message == b.Terminated.Message && - a.Terminated.StartedAt.Equal(&b.Terminated.StartedAt) && - a.Terminated.FinishedAt.Equal(&b.Terminated.FinishedAt) - } - if a.Waiting != nil && b.Waiting != nil { - return a.Waiting.Reason == b.Waiting.Reason && - a.Waiting.Message == b.Waiting.Message - } - - return true -} - -// getContainerPorts returns ports for a container with optimized memory usage -func getContainerPorts(containerName string, pod *corev1.Pod) []browserv1.ContainerPort { - if pod.Spec.Containers == nil { - return []browserv1.ContainerPort{} - } - - for _, container := range pod.Spec.Containers { - if container.Name == containerName && len(container.Ports) > 0 { - ports := make([]browserv1.ContainerPort, 0, len(container.Ports)) - for _, port := range container.Ports { - ports = append(ports, browserv1.ContainerPort{ - Name: port.Name, - ContainerPort: port.ContainerPort, - Protocol: port.Protocol, - HostPort: port.HostPort, - }) - } - return ports - } - } - return []browserv1.ContainerPort{} -} - func (r *BrowserReconciler) retryUpdate(ctx context.Context, browser *browserv1.Browser, updateFunc func(*browserv1.Browser)) error { namespacedName := types.NamespacedName{ Name: browser.Name, @@ -721,13 +646,8 @@ func buildBrowserPod(browser *browserv1.Browser, cfg *configv1.BrowserVersionCon } if cfg.Volumes != nil { - volumes := make([]corev1.Volume, 0, len(*cfg.Volumes)) - - for _, v := range *cfg.Volumes { - volume := v.DeepCopy() - volumes = append(volumes, *volume) - } - + volumes := make([]corev1.Volume, len(*cfg.Volumes)) + copy(volumes, *cfg.Volumes) pod.Spec.Volumes = volumes } @@ -789,43 +709,38 @@ func buildBrowserPod(browser *browserv1.Browser, cfg *configv1.BrowserVersionCon pod.Spec.Containers = sidecarContainers - if browser.Labels != nil { - if pod.Labels == nil { - pod.Labels = map[string]string{} - } + labelCount := len(browser.Labels) + if cfg.Labels != nil { + labelCount += len(*cfg.Labels) + } + if labelCount > 0 { + pod.Labels = make(map[string]string, labelCount) for k, v := range browser.Labels { pod.Labels[k] = v } - } - - // Pod-level fields - if cfg.Labels != nil { - if pod.Labels == nil { - pod.Labels = map[string]string{} - } - for k, v := range *cfg.Labels { - pod.Labels[k] = v + if cfg.Labels != nil { + for k, v := range *cfg.Labels { + pod.Labels[k] = v + } } } - if browser.Annotations != nil { - if pod.Annotations == nil { - pod.Annotations = map[string]string{} - } + annotationCount := len(browser.Annotations) + if cfg.Annotations != nil { + annotationCount += len(*cfg.Annotations) + } + if annotationCount > 0 { + pod.Annotations = make(map[string]string, annotationCount) for k, v := range browser.Annotations { if k == browserv1.SelenosisOptionsAnnotationKey { continue } pod.Annotations[k] = v } - } - - if cfg.Annotations != nil { - if pod.Annotations == nil { - pod.Annotations = map[string]string{} - } - for k, v := range *cfg.Annotations { - pod.Annotations[k] = v + if cfg.Annotations != nil { + for k, v := range *cfg.Annotations { + pod.Annotations[k] = v + } } } @@ -910,6 +825,76 @@ func applySelenosisOptions(pod *corev1.Pod, opts *SelenosisOptions) { } } +func getContainerPorts(containerName string, pod *corev1.Pod) []browserv1.ContainerPort { + if pod.Spec.Containers == nil { + return []browserv1.ContainerPort{} + } + + for _, container := range pod.Spec.Containers { + if container.Name == containerName && len(container.Ports) > 0 { + ports := make([]browserv1.ContainerPort, 0, len(container.Ports)) + for _, port := range container.Ports { + ports = append(ports, browserv1.ContainerPort{ + Name: port.Name, + ContainerPort: port.ContainerPort, + Protocol: port.Protocol, + HostPort: port.HostPort, + }) + } + return ports + } + } + return nil +} + +func containerStatusesEqual(a, b []browserv1.ContainerStatus) bool { + if len(a) != len(b) { + return false + } + for i := range a { + ai, bi := a[i], b[i] + if ai.Name != bi.Name || ai.Image != bi.Image || ai.RestartCount != bi.RestartCount { + return false + } + if !containerStateEqual(ai.State, bi.State) { + return false + } + if len(ai.Ports) != len(bi.Ports) { + return false + } + for j := range ai.Ports { + if ai.Ports[j] != bi.Ports[j] { + return false + } + } + } + return true +} + +func containerStateEqual(a, b corev1.ContainerState) bool { + if (a.Running != nil) != (b.Running != nil) || + (a.Terminated != nil) != (b.Terminated != nil) || + (a.Waiting != nil) != (b.Waiting != nil) { + return false + } + if a.Running != nil { + return a.Running.StartedAt.Equal(&b.Running.StartedAt) + } + if a.Terminated != nil { + return a.Terminated.ExitCode == b.Terminated.ExitCode && + a.Terminated.Signal == b.Terminated.Signal && + a.Terminated.Reason == b.Terminated.Reason && + a.Terminated.Message == b.Terminated.Message && + a.Terminated.StartedAt.Equal(&b.Terminated.StartedAt) && + a.Terminated.FinishedAt.Equal(&b.Terminated.FinishedAt) + } + if a.Waiting != nil { + return a.Waiting.Reason == b.Waiting.Reason && + a.Waiting.Message == b.Waiting.Message + } + return true +} + func mergeEnvVars(base []corev1.EnvVar, override map[string]string) []corev1.EnvVar { if len(override) == 0 { return base diff --git a/controllers/browser/browser_reconciler_test.go b/controllers/browser/browser_reconciler_test.go index 45e5142..139876b 100644 --- a/controllers/browser/browser_reconciler_test.go +++ b/controllers/browser/browser_reconciler_test.go @@ -65,41 +65,39 @@ func envValue(env []corev1.EnvVar, key string) (string, bool) { return "", false } -func TestContainerStateEqual(t *testing.T) { +func TestContainerStatusesEqual(t *testing.T) { now := metav1.NewTime(time.Now().UTC()) - a := corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: now}} - b := corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: now}} - if !containerStateEqual(a, b) { - t.Fatalf("expected running states to be equal") + later := metav1.NewTime(now.Add(1 * time.Second)) + + wrap := func(state corev1.ContainerState) []browserv1.ContainerStatus { + return []browserv1.ContainerStatus{{Name: "c", State: state, Image: "img", RestartCount: 0}} } - b = corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: "Init"}} - if containerStateEqual(a, b) { - t.Fatalf("expected different states to be unequal") + running := corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: now}} + if !containerStatusesEqual(wrap(running), wrap(running)) { + t.Fatal("expected identical running states to be equal") } - a = corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: "A", Message: "m"}} - b = corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: "A", Message: "m"}} - if !containerStateEqual(a, b) { - t.Fatalf("expected waiting states to be equal") + waiting := corev1.ContainerState{Waiting: &corev1.ContainerStateWaiting{Reason: "Init"}} + if containerStatusesEqual(wrap(running), wrap(waiting)) { + t.Fatal("expected running vs waiting to be unequal") } - a = corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 1, Reason: "r"}} - b = corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 1, Reason: "r"}} - if !containerStateEqual(a, b) { - t.Fatalf("expected terminated states to be equal") + runningLater := corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: later}} + if containerStatusesEqual(wrap(running), wrap(runningLater)) { + t.Fatal("expected running states with different timestamps to be unequal") } - a = corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: now}} - b = corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: metav1.NewTime(now.Add(1 * time.Second))}} - if containerStateEqual(a, b) { - t.Fatalf("expected running states to be different") + terminated := corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 1, Reason: "r"}} + if !containerStatusesEqual(wrap(terminated), wrap(terminated)) { + t.Fatal("expected identical terminated states to be equal") } - a = corev1.ContainerState{Terminated: &corev1.ContainerStateTerminated{ExitCode: 1}} - b = corev1.ContainerState{} - if containerStateEqual(a, b) { - t.Fatalf("expected terminated presence mismatch to be different") + // image change must be detected + a := []browserv1.ContainerStatus{{Name: "c", State: running, Image: "img:v1", RestartCount: 0}} + b := []browserv1.ContainerStatus{{Name: "c", State: running, Image: "img:v2", RestartCount: 0}} + if containerStatusesEqual(a, b) { + t.Fatal("expected image change to be detected") } } @@ -598,7 +596,7 @@ func TestReconcileAddsFinalizerAndLabels(t *testing.T) { } } -func TestReconcileFailedBrowserRemovesFinalizer(t *testing.T) { +func TestReconcileFailedBrowserDeletesBrowser(t *testing.T) { scheme := newBrowserScheme(t) brw := &browserv1.Browser{ ObjectMeta: metav1.ObjectMeta{ @@ -620,12 +618,68 @@ func TestReconcileFailedBrowserRemovesFinalizer(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 err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); err == nil { + t.Fatalf("expected browser to be deleted") + } +} + +func TestReconcileFailedBrowserWithPodDeletesPodAndBrowser(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + Finalizers: []string{browserPodFinalizer}, + }, + Status: browserv1.BrowserStatus{Phase: corev1.PodFailed}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Status: corev1.PodStatus{Phase: corev1.PodPending}, } - if controllerutil.ContainsFinalizer(got, browserPodFinalizer) { - t.Fatalf("expected finalizer to be removed") + cl := newBrowserClient(scheme, brw, pod) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + + _, 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") + } + if err := cl.Get(context.Background(), client.ObjectKey{Name: "b1", Namespace: "ns"}, &browserv1.Browser{}); err == nil { + t.Fatalf("expected browser to be deleted") + } +} + +func TestReconcileFailedBrowserPodDeleteError(t *testing.T) { + scheme := newBrowserScheme(t) + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + Finalizers: []string{browserPodFinalizer}, + }, + Status: browserv1.BrowserStatus{Phase: corev1.PodFailed}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + } + base := newBrowserClient(scheme, brw, pod) + cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + + res, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, + }) + if err == nil { + t.Fatalf("expected error") + } + if res.RequeueAfter != mediumRetry { + t.Fatalf("expected medium retry, got %v", res.RequeueAfter) } } @@ -858,7 +912,8 @@ func TestReconcilePodPendingContainerTerminated(t *testing.T) { Name: "browser", State: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ - Reason: "Error", + Reason: "OOMKilled", + ExitCode: 137, }, }, }, @@ -874,6 +929,59 @@ func TestReconcilePodPendingContainerTerminated(t *testing.T) { 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, "OOMKilled") { + t.Fatalf("expected message to contain reason, got %q", got.Status.Message) + } + if !strings.Contains(got.Status.Message, "137") { + t.Fatalf("expected message to contain exit code, got %q", got.Status.Message) + } +} + +func TestReconcilePodPendingContainerTerminatedPodDeleteError(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, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "browser", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{Reason: "Error", ExitCode: 1}, + }, + }, + }, + }, + } + base := newBrowserClient(scheme, brw, pod) + cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + + res, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, + }) + if err == nil { + t.Fatalf("expected error") + } + if res.RequeueAfter != mediumRetry { + t.Fatalf("expected medium retry, got %v", res.RequeueAfter) + } } func TestReconcilePodPendingWaitingBadReason(t *testing.T) { @@ -912,12 +1020,36 @@ func TestReconcilePodPendingWaitingBadReason(t *testing.T) { cl := newBrowserClient(scheme, brw, pod) r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) - _, err := r.Reconcile(context.Background(), ctrl.Request{ - NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, - }) + req := ctrl.Request{NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}} + + _, err := r.Reconcile(context.Background(), req) 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 CrashLoopBackOff") + } + + 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, "CrashLoopBackOff") { + t.Fatalf("expected message to contain reason, got %q", got.Status.Message) + } + + _, err = r.Reconcile(context.Background(), req) + if 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") + } } func TestReconcilePodPendingCreationTimeout(t *testing.T) { @@ -961,6 +1093,63 @@ func TestReconcilePodPendingCreationTimeout(t *testing.T) { 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, "browser") { + t.Fatalf("expected message to contain container name, got %q", got.Status.Message) + } + if !strings.Contains(got.Status.Message, "ContainerCreating") { + t.Fatalf("expected message to contain waiting reason, got %q", got.Status.Message) + } +} + +func TestReconcilePodPendingCreationTimeoutPodDeleteError(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(-podCreationTimeout - time.Second).UTC()), + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "browser", + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{Reason: "ContainerCreating"}, + }, + }, + }, + }, + } + base := newBrowserClient(scheme, brw, pod) + cl := errorClient{Client: base, deleteErr: apierrors.NewInternalError(errors.New("delete"))} + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + + res, err := r.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: client.ObjectKey{Namespace: "ns", Name: "b1"}, + }) + if err == nil { + t.Fatalf("expected error") + } + if res.RequeueAfter != mediumRetry { + t.Fatalf("expected medium retry, got %v", res.RequeueAfter) + } } func TestUpdateBrowserStatusNoChanges(t *testing.T) { @@ -1286,7 +1475,7 @@ func TestReconcilePodDeletedDeletesBrowser(t *testing.T) { } } -func TestReconcilePodNotFoundBrowserFailed(t *testing.T) { +func TestReconcilePodNotFoundBrowserFailedDeletesBrowser(t *testing.T) { scheme := newBrowserScheme(t) brw := &browserv1.Browser{ ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, @@ -1305,6 +1494,10 @@ func TestReconcilePodNotFoundBrowserFailed(t *testing.T) { 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{}); err == nil { + t.Fatalf("expected browser to be deleted") + } } func TestReconcilePodPendingContainerCreatingNoTimeout(t *testing.T) { @@ -1922,6 +2115,10 @@ func TestUpdateBrowserStatusCriticalSidecar(t *testing.T) { 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{}); err == nil { + t.Fatalf("expected browser to be deleted after sidecar terminated") + } } func TestDeleteBrowserFinalizerSuccess(t *testing.T) { @@ -2336,6 +2533,178 @@ func TestRetryUpdateMaxConflict(t *testing.T) { } } +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{ + "selenosis.io/browser": "b1", + "selenosis.io/browser.name": "chrome", + "selenosis.io/browser.version": "120", + }, + }, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + Status: browserv1.BrowserStatus{Phase: corev1.PodPending}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + CreationTimestamp: metav1.NewTime(time.Now().Add(-podCreationTimeout - time.Second).UTC()), + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "browser", + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{Reason: "ContainerCreating"}, + }, + }, + }, + }, + } + cl := newBrowserClient(scheme, brw, pod) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + 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") + } + 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 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") + } +} + +func TestReconcilePodPendingContainerTerminatedFullCycle(t *testing.T) { + scheme := newBrowserScheme(t) + 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", + }, + }, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + Status: browserv1.BrowserStatus{Phase: corev1.PodPending}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "browser", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Reason: "OOMKilled", + ExitCode: 137, + }, + }, + }, + }, + }, + } + cl := newBrowserClient(scheme, brw, pod) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + 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") + } + 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 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") + } +} + +func TestReconcilePodFailedFullCycle(t *testing.T) { + scheme := newBrowserScheme(t) + 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", + }, + }, + Spec: browserv1.BrowserSpec{BrowserName: "chrome", BrowserVersion: "120"}, + Status: browserv1.BrowserStatus{Phase: corev1.PodPending}, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "ns"}, + Status: corev1.PodStatus{ + Phase: corev1.PodFailed, + Reason: "OOMKilled", + Message: "container exceeded memory limit", + }, + } + cl := newBrowserClient(scheme, brw, pod) + r := NewBrowserReconciler(cl, store.NewBrowserConfigStore(), scheme) + 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") + } + 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 got.Status.Phase != corev1.PodFailed { + t.Fatalf("expected Status.Phase=Failed after first reconcile, got %s", got.Status.Phase) + } + if !strings.Contains(got.Status.Message, "OOMKilled") { + 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) + } + 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") + } +} + func TestRetryStatusUpdateMaxConflict(t *testing.T) { scheme := newBrowserScheme(t) brw := &browserv1.Browser{ diff --git a/controllers/browserconfig/browserconfig_reconciler.go b/controllers/browserconfig/browserconfig_reconciler.go index 04eff08..bb2d6ad 100644 --- a/controllers/browserconfig/browserconfig_reconciler.go +++ b/controllers/browserconfig/browserconfig_reconciler.go @@ -45,7 +45,7 @@ func (r *BrowserConfigReconciler) SetupWithManager(mgr ctrl.Manager) error { } // Reconcile synchronizes the state of BrowserConfig -func (r BrowserConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +func (r *BrowserConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := logger.FromContext(ctx) browserConfig := &configv1.BrowserConfig{} diff --git a/store/browserconfig_store.go b/store/browserconfig_store.go index 721d9ff..fffb49d 100644 --- a/store/browserconfig_store.go +++ b/store/browserconfig_store.go @@ -107,6 +107,9 @@ func (s *BrowserConfigStore) onAddOrUpdate(oldObj, newObj any, log logr.Logger) for browserName, versions := range copy.Spec.Browsers { for version, cfg := range versions { + if cfg == nil { + continue + } key := keyFor(copy.Namespace, browserName, version) s.config[key] = cfg log.Info("BrowserConfig added/updated", "key", key) @@ -148,5 +151,9 @@ func (s *BrowserConfigStore) Get(namespace, browserName, version string) (*confi s.mu.RLock() defer s.mu.RUnlock() cfg, exists := s.config[keyFor(namespace, browserName, version)] - return cfg, exists + if !exists { + return nil, false + } + + return cfg.DeepCopy(), exists }