From e7f7005dd86803a26431fd320f4ee41403200ec5 Mon Sep 17 00:00:00 2001 From: Bittrance Date: Fri, 27 Mar 2026 17:00:48 +0100 Subject: [PATCH 1/5] debug log desired pod spec --- internal/controller/marimonotebook_controller.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/controller/marimonotebook_controller.go b/internal/controller/marimonotebook_controller.go index 3008b87..b27b3b4 100644 --- a/internal/controller/marimonotebook_controller.go +++ b/internal/controller/marimonotebook_controller.go @@ -19,6 +19,7 @@ package controller import ( "context" "crypto/sha256" + "encoding/json" "fmt" corev1 "k8s.io/api/core/v1" @@ -187,6 +188,9 @@ func (r *MarimoNotebookReconciler) reconcilePod(ctx context.Context, notebook *m logger := logf.FromContext(ctx) desired := resources.BuildPod(notebook) + podJSON, _ := json.Marshal(desired) + logger.V(1).Info("Desired pod spec", "pod", string(podJSON)) + // Set owner reference for automatic garbage collection if err := controllerutil.SetControllerReference(notebook, desired, r.Scheme); err != nil { return nil, err From 4f2c43a51857a10367d130a263ca895cf7c4b0c6 Mon Sep 17 00:00:00 2001 From: Bittrance Date: Sat, 28 Mar 2026 10:08:02 +0100 Subject: [PATCH 2/5] reconcile service updates --- .../controller/marimonotebook_controller.go | 41 ++++++------- .../marimonotebook_controller_test.go | 61 +++++++++++++++++++ 2 files changed, 81 insertions(+), 21 deletions(-) diff --git a/internal/controller/marimonotebook_controller.go b/internal/controller/marimonotebook_controller.go index b27b3b4..9e2cdfe 100644 --- a/internal/controller/marimonotebook_controller.go +++ b/internal/controller/marimonotebook_controller.go @@ -24,6 +24,7 @@ import ( corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -225,33 +226,31 @@ func (r *MarimoNotebookReconciler) reconcileService(ctx context.Context, noteboo logger := logf.FromContext(ctx) desired := resources.BuildService(notebook) - // Set owner reference - if err := controllerutil.SetControllerReference(notebook, desired, r.Scheme); err != nil { - return nil, err + svc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: desired.Name, + Namespace: desired.Namespace, + }, } - // Check if Service exists - existing := &corev1.Service{} - err := r.Get(ctx, client.ObjectKeyFromObject(desired), existing) - if err != nil { - if k8serrors.IsNotFound(err) { - logger.Info("Creating Service", "name", desired.Name) - if err := r.Create(ctx, desired); err != nil { - if k8serrors.IsAlreadyExists(err) { - // Service was created between Get and Create, re-fetch - if err := r.Get(ctx, client.ObjectKeyFromObject(desired), existing); err != nil { - return nil, err - } - return existing, nil - } - return nil, err - } - return desired, nil + op, err := controllerutil.CreateOrUpdate(ctx, r.Client, svc, func() error { + if err := controllerutil.SetControllerReference(notebook, svc, r.Scheme); err != nil { + return err } + svc.Labels = desired.Labels + svc.Spec.Ports = desired.Spec.Ports + svc.Spec.Selector = desired.Spec.Selector + return nil + }) + if err != nil { return nil, err } - return existing, nil + if op != controllerutil.OperationResultNone { + logger.Info("Reconciled Service", "name", svc.Name, "operation", op) + } + + return svc, nil } func (r *MarimoNotebookReconciler) updateStatus(ctx context.Context, notebook *marimov1alpha1.MarimoNotebook, pod *corev1.Pod, svc *corev1.Service) (ctrl.Result, error) { diff --git a/internal/controller/marimonotebook_controller_test.go b/internal/controller/marimonotebook_controller_test.go index 5e6a3d2..e4a6633 100644 --- a/internal/controller/marimonotebook_controller_test.go +++ b/internal/controller/marimonotebook_controller_test.go @@ -370,6 +370,67 @@ var _ = Describe("MarimoNotebook Controller", func() { }) }) + Context("When updating a MarimoNotebook", func() { + It("should update Service ports when a sidecar with a port is added", func() { + pausePort := int32(9090) + notebook := &marimov1alpha1.MarimoNotebook{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-update-svc-" + randString(), + Namespace: "default", + }, + Spec: marimov1alpha1.MarimoNotebookSpec{ + Source: "https://github.com/marimo-team/marimo.git", + }, + } + namespacedName := types.NamespacedName{ + Name: notebook.Name, + Namespace: notebook.Namespace, + } + + defer func() { + _ = k8sClient.Delete(ctx, notebook) + }() + + By("creating the MarimoNotebook without sidecars") + Expect(k8sClient.Create(ctx, notebook)).To(Succeed()) + + By("waiting for Service to be created with only the main port") + svc := &corev1.Service{} + Eventually(func() error { + return k8sClient.Get(ctx, namespacedName, svc) + }, timeout, interval).Should(Succeed()) + Expect(svc.Spec.Ports).To(HaveLen(1)) + + By("updating the MarimoNotebook to add a pause sidecar with a container port") + nb := &marimov1alpha1.MarimoNotebook{} + Expect(k8sClient.Get(ctx, namespacedName, nb)).To(Succeed()) + nb.Spec.Storage = &marimov1alpha1.StorageSpec{Size: "1Gi"} + nb.Spec.Sidecars = []marimov1alpha1.SidecarSpec{ + { + Name: "pause", + Image: "registry.k8s.io/pause:3.9", + ExposePort: &pausePort, + }, + } + Expect(k8sClient.Update(ctx, nb)).To(Succeed()) + + By("checking Service is updated to include the sidecar port") + Eventually(func() bool { + if err := k8sClient.Get(ctx, namespacedName, svc); err != nil { + return false + } + for _, port := range svc.Spec.Ports { + if port.Name == "pause" && port.Port == pausePort { + return true + } + } + return false + }, timeout, interval).Should(BeTrue(), "Service should expose the pause sidecar port") + + Expect(svc.Spec.Ports).To(HaveLen(2)) + }) + }) + Context("When deleting a MarimoNotebook", func() { It("should clean up owned resources via garbage collection", func() { notebook := &marimov1alpha1.MarimoNotebook{ From 47e9c32af69b9ede952c6dbb82260b1128a6d0f9 Mon Sep 17 00:00:00 2001 From: Bittrance Date: Sat, 28 Mar 2026 11:25:56 +0100 Subject: [PATCH 3/5] notebook storage configuration is immutable --- api/v1alpha1/marimonotebook_types.go | 1 + config/crd/bases/marimo.io_marimos.yaml | 3 ++ deploy/install.yaml | 3 ++ .../marimonotebook_controller_test.go | 39 +++++++++++++------ 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/api/v1alpha1/marimonotebook_types.go b/api/v1alpha1/marimonotebook_types.go index 3774f02..f8bef73 100644 --- a/api/v1alpha1/marimonotebook_types.go +++ b/api/v1alpha1/marimonotebook_types.go @@ -106,6 +106,7 @@ type MarimoNotebookSpec struct { // Storage configures persistent storage for notebooks // +optional + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="storage is immutable" Storage *StorageSpec `json:"storage,omitempty"` // Resources for the marimo container diff --git a/config/crd/bases/marimo.io_marimos.yaml b/config/crd/bases/marimo.io_marimos.yaml index b2627e6..fa55fba 100644 --- a/config/crd/bases/marimo.io_marimos.yaml +++ b/config/crd/bases/marimo.io_marimos.yaml @@ -767,6 +767,9 @@ spec: if empty) type: string type: object + x-kubernetes-validations: + - message: storage is immutable + rule: self == oldSelf type: object x-kubernetes-validations: - message: storage is required when sidecars are specified diff --git a/deploy/install.yaml b/deploy/install.yaml index 799168c..0b76ac2 100644 --- a/deploy/install.yaml +++ b/deploy/install.yaml @@ -775,6 +775,9 @@ spec: if empty) type: string type: object + x-kubernetes-validations: + - message: storage is immutable + rule: self == oldSelf type: object x-kubernetes-validations: - message: storage is required when sidecars are specified diff --git a/internal/controller/marimonotebook_controller_test.go b/internal/controller/marimonotebook_controller_test.go index e4a6633..e38d720 100644 --- a/internal/controller/marimonotebook_controller_test.go +++ b/internal/controller/marimonotebook_controller_test.go @@ -371,27 +371,34 @@ var _ = Describe("MarimoNotebook Controller", func() { }) Context("When updating a MarimoNotebook", func() { - It("should update Service ports when a sidecar with a port is added", func() { - pausePort := int32(9090) - notebook := &marimov1alpha1.MarimoNotebook{ + var notebook *marimov1alpha1.MarimoNotebook + var namespacedName types.NamespacedName + + BeforeEach(func() { + notebook = &marimov1alpha1.MarimoNotebook{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-update-svc-" + randString(), + Name: "test-update-" + randString(), Namespace: "default", }, Spec: marimov1alpha1.MarimoNotebookSpec{ - Source: "https://github.com/marimo-team/marimo.git", + Source: "https://github.com/marimo-team/marimo.git", + Storage: &marimov1alpha1.StorageSpec{Size: "1Gi"}, }, } - namespacedName := types.NamespacedName{ + namespacedName = types.NamespacedName{ Name: notebook.Name, Namespace: notebook.Namespace, } + }) - defer func() { - _ = k8sClient.Delete(ctx, notebook) - }() + AfterEach(func() { + _ = k8sClient.Delete(ctx, notebook) + }) - By("creating the MarimoNotebook without sidecars") + It("should update Service ports when a sidecar with a port is added", func() { + pausePort := int32(9090) + + By("creating the MarimoNotebook with storage") Expect(k8sClient.Create(ctx, notebook)).To(Succeed()) By("waiting for Service to be created with only the main port") @@ -404,7 +411,6 @@ var _ = Describe("MarimoNotebook Controller", func() { By("updating the MarimoNotebook to add a pause sidecar with a container port") nb := &marimov1alpha1.MarimoNotebook{} Expect(k8sClient.Get(ctx, namespacedName, nb)).To(Succeed()) - nb.Spec.Storage = &marimov1alpha1.StorageSpec{Size: "1Gi"} nb.Spec.Sidecars = []marimov1alpha1.SidecarSpec{ { Name: "pause", @@ -429,6 +435,17 @@ var _ = Describe("MarimoNotebook Controller", func() { Expect(svc.Spec.Ports).To(HaveLen(2)) }) + + It("should reject changes to the storage attribute", func() { + By("creating the MarimoNotebook with storage") + Expect(k8sClient.Create(ctx, notebook)).To(Succeed()) + + By("attempting to update the storage attribute") + nb := &marimov1alpha1.MarimoNotebook{} + Expect(k8sClient.Get(ctx, namespacedName, nb)).To(Succeed()) + nb.Spec.Storage = &marimov1alpha1.StorageSpec{Size: "2Gi"} + Expect(k8sClient.Update(ctx, nb)).To(MatchError(ContainSubstring("storage is immutable"))) + }) }) Context("When deleting a MarimoNotebook", func() { From ba652e69a1d8a57f8ade03eaca9e8b9c7c0b0b9c Mon Sep 17 00:00:00 2001 From: Bittrance Date: Sat, 28 Mar 2026 11:54:37 +0100 Subject: [PATCH 4/5] recreate pod on notebook changes --- .../controller/marimonotebook_controller.go | 18 +++++++++- .../marimonotebook_controller_test.go | 36 +++++++++++++++++++ pkg/resources/pod.go | 14 ++++++++ 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/internal/controller/marimonotebook_controller.go b/internal/controller/marimonotebook_controller.go index 9e2cdfe..d3033b2 100644 --- a/internal/controller/marimonotebook_controller.go +++ b/internal/controller/marimonotebook_controller.go @@ -197,6 +197,13 @@ func (r *MarimoNotebookReconciler) reconcilePod(ctx context.Context, notebook *m return nil, err } + // Compute hash of desired pod spec for change detection + specHash := resources.PodSpecHash(desired) + if desired.Annotations == nil { + desired.Annotations = make(map[string]string) + } + desired.Annotations[resources.PodSpecHashAnnotation] = specHash + // Check if Pod exists existing := &corev1.Pod{} err := r.Get(ctx, client.ObjectKeyFromObject(desired), existing) @@ -218,7 +225,16 @@ func (r *MarimoNotebookReconciler) reconcilePod(ctx context.Context, notebook *m return nil, err } - // Pod exists - we don't update running pods (recreate strategy) + // Pod exists - check if spec has changed and recreate if so + if existingHash := existing.Annotations[resources.PodSpecHashAnnotation]; existingHash != specHash { + logger.Info("Pod spec changed, recreating", "name", existing.Name) + if err := r.Delete(ctx, existing); err != nil && !k8serrors.IsNotFound(err) { + return nil, err + } + // Pod deleted - next reconcile will create the updated pod + return nil, nil + } + return existing, nil } diff --git a/internal/controller/marimonotebook_controller_test.go b/internal/controller/marimonotebook_controller_test.go index e38d720..903642a 100644 --- a/internal/controller/marimonotebook_controller_test.go +++ b/internal/controller/marimonotebook_controller_test.go @@ -436,6 +436,42 @@ var _ = Describe("MarimoNotebook Controller", func() { Expect(svc.Spec.Ports).To(HaveLen(2)) }) + It("should recreate Pod when env vars are changed", func() { + By("creating the MarimoNotebook") + Expect(k8sClient.Create(ctx, notebook)).To(Succeed()) + + By("waiting for initial Pod to be created") + pod := &corev1.Pod{} + Eventually(func() error { + return k8sClient.Get(ctx, namespacedName, pod) + }, timeout, interval).Should(Succeed()) + originalUID := pod.UID + + By("updating the notebook to add an env var") + nb := &marimov1alpha1.MarimoNotebook{} + Expect(k8sClient.Get(ctx, namespacedName, nb)).To(Succeed()) + nb.Spec.Env = []corev1.EnvVar{ + {Name: "MY_VAR", Value: "hello"}, + } + Expect(k8sClient.Update(ctx, nb)).To(Succeed()) + + By("verifying Pod is recreated with the new env var") + Eventually(func() bool { + if err := k8sClient.Get(ctx, namespacedName, pod); err != nil { + return false + } + if pod.UID == originalUID { + return false // still the old pod + } + for _, env := range pod.Spec.Containers[0].Env { + if env.Name == "MY_VAR" && env.Value == "hello" { + return true + } + } + return false + }, timeout, interval).Should(BeTrue(), "Pod should be recreated with MY_VAR env var") + }) + It("should reject changes to the storage attribute", func() { By("creating the MarimoNotebook with storage") Expect(k8sClient.Create(ctx, notebook)).To(Succeed()) diff --git a/pkg/resources/pod.go b/pkg/resources/pod.go index 24e9f41..ec98723 100644 --- a/pkg/resources/pod.go +++ b/pkg/resources/pod.go @@ -1,6 +1,8 @@ package resources import ( + "crypto/sha256" + "encoding/hex" "encoding/json" "fmt" "strings" @@ -18,8 +20,20 @@ const ( NotebookDir = "/home/marimo/notebooks" // DefaultMode is the default mode for running marimo. DefaultMode = "edit" + // PodSpecHashAnnotation is the annotation key used to store the hash of the desired pod spec. + PodSpecHashAnnotation = "marimo.io/pod-spec-hash" ) +// PodSpecHash returns a SHA-256 hash of the pod's spec for change detection. +func PodSpecHash(pod *corev1.Pod) string { + data, err := json.Marshal(pod.Spec) + if err != nil { + return "" + } + sum := sha256.Sum256(data) + return hex.EncodeToString(sum[:]) +} + // BuildPod creates a Pod spec for a MarimoNotebook. // Supports two content modes: // - source: clone from git URL From e13f2389bd170dbe2e7533e3ecc73bb61eb010f4 Mon Sep 17 00:00:00 2001 From: dmadisetti Date: Mon, 30 Mar 2026 16:22:55 -0700 Subject: [PATCH 5/5] tidy: address comments and remove dangerous loggin --- api/v1alpha1/marimonotebook_types.go | 2 +- config/crd/bases/marimo.io_marimos.yaml | 4 ++-- deploy/install.yaml | 4 ++-- internal/controller/marimonotebook_controller.go | 12 ++++++------ .../controller/marimonotebook_controller_test.go | 2 +- pkg/resources/pod.go | 6 +++--- 6 files changed, 15 insertions(+), 15 deletions(-) diff --git a/api/v1alpha1/marimonotebook_types.go b/api/v1alpha1/marimonotebook_types.go index f8bef73..1b6fbf1 100644 --- a/api/v1alpha1/marimonotebook_types.go +++ b/api/v1alpha1/marimonotebook_types.go @@ -106,7 +106,7 @@ type MarimoNotebookSpec struct { // Storage configures persistent storage for notebooks // +optional - // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="storage is immutable" + // +kubebuilder:validation:XValidation:rule="oldSelf == null || self == oldSelf",message="storage is immutable once set" Storage *StorageSpec `json:"storage,omitempty"` // Resources for the marimo container diff --git a/config/crd/bases/marimo.io_marimos.yaml b/config/crd/bases/marimo.io_marimos.yaml index fa55fba..1ab8ba0 100644 --- a/config/crd/bases/marimo.io_marimos.yaml +++ b/config/crd/bases/marimo.io_marimos.yaml @@ -768,8 +768,8 @@ spec: type: string type: object x-kubernetes-validations: - - message: storage is immutable - rule: self == oldSelf + - message: storage is immutable once set + rule: oldSelf == null || self == oldSelf type: object x-kubernetes-validations: - message: storage is required when sidecars are specified diff --git a/deploy/install.yaml b/deploy/install.yaml index 0b76ac2..f1cf1df 100644 --- a/deploy/install.yaml +++ b/deploy/install.yaml @@ -776,8 +776,8 @@ spec: type: string type: object x-kubernetes-validations: - - message: storage is immutable - rule: self == oldSelf + - message: storage is immutable once set + rule: oldSelf == null || self == oldSelf type: object x-kubernetes-validations: - message: storage is required when sidecars are specified diff --git a/internal/controller/marimonotebook_controller.go b/internal/controller/marimonotebook_controller.go index d3033b2..fc712de 100644 --- a/internal/controller/marimonotebook_controller.go +++ b/internal/controller/marimonotebook_controller.go @@ -19,7 +19,6 @@ package controller import ( "context" "crypto/sha256" - "encoding/json" "fmt" corev1 "k8s.io/api/core/v1" @@ -189,16 +188,16 @@ func (r *MarimoNotebookReconciler) reconcilePod(ctx context.Context, notebook *m logger := logf.FromContext(ctx) desired := resources.BuildPod(notebook) - podJSON, _ := json.Marshal(desired) - logger.V(1).Info("Desired pod spec", "pod", string(podJSON)) - // Set owner reference for automatic garbage collection if err := controllerutil.SetControllerReference(notebook, desired, r.Scheme); err != nil { return nil, err } // Compute hash of desired pod spec for change detection - specHash := resources.PodSpecHash(desired) + specHash, err := resources.PodSpecHash(desired) + if err != nil { + return nil, fmt.Errorf("computing pod spec hash: %w", err) + } if desired.Annotations == nil { desired.Annotations = make(map[string]string) } @@ -206,7 +205,7 @@ func (r *MarimoNotebookReconciler) reconcilePod(ctx context.Context, notebook *m // Check if Pod exists existing := &corev1.Pod{} - err := r.Get(ctx, client.ObjectKeyFromObject(desired), existing) + err = r.Get(ctx, client.ObjectKeyFromObject(desired), existing) if err != nil { if k8serrors.IsNotFound(err) { logger.Info("Creating Pod", "name", desired.Name) @@ -256,6 +255,7 @@ func (r *MarimoNotebookReconciler) reconcileService(ctx context.Context, noteboo svc.Labels = desired.Labels svc.Spec.Ports = desired.Spec.Ports svc.Spec.Selector = desired.Spec.Selector + svc.Spec.Type = desired.Spec.Type return nil }) if err != nil { diff --git a/internal/controller/marimonotebook_controller_test.go b/internal/controller/marimonotebook_controller_test.go index 903642a..17412a3 100644 --- a/internal/controller/marimonotebook_controller_test.go +++ b/internal/controller/marimonotebook_controller_test.go @@ -480,7 +480,7 @@ var _ = Describe("MarimoNotebook Controller", func() { nb := &marimov1alpha1.MarimoNotebook{} Expect(k8sClient.Get(ctx, namespacedName, nb)).To(Succeed()) nb.Spec.Storage = &marimov1alpha1.StorageSpec{Size: "2Gi"} - Expect(k8sClient.Update(ctx, nb)).To(MatchError(ContainSubstring("storage is immutable"))) + Expect(k8sClient.Update(ctx, nb)).To(MatchError(ContainSubstring("storage is immutable once set"))) }) }) diff --git a/pkg/resources/pod.go b/pkg/resources/pod.go index ec98723..e6395c4 100644 --- a/pkg/resources/pod.go +++ b/pkg/resources/pod.go @@ -25,13 +25,13 @@ const ( ) // PodSpecHash returns a SHA-256 hash of the pod's spec for change detection. -func PodSpecHash(pod *corev1.Pod) string { +func PodSpecHash(pod *corev1.Pod) (string, error) { data, err := json.Marshal(pod.Spec) if err != nil { - return "" + return "", err } sum := sha256.Sum256(data) - return hex.EncodeToString(sum[:]) + return hex.EncodeToString(sum[:]), nil } // BuildPod creates a Pod spec for a MarimoNotebook.