Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/v1alpha1/marimonotebook_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ type MarimoNotebookSpec struct {

// Storage configures persistent storage for notebooks
// +optional
// +kubebuilder:validation:XValidation:rule="oldSelf == null || self == oldSelf",message="storage is immutable once set"
Storage *StorageSpec `json:"storage,omitempty"`

// Resources for the marimo container
Expand Down
3 changes: 3 additions & 0 deletions config/crd/bases/marimo.io_marimos.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,9 @@ spec:
if empty)
type: string
type: object
x-kubernetes-validations:
- message: storage is immutable once set
rule: oldSelf == null || self == oldSelf
type: object
x-kubernetes-validations:
- message: storage is required when sidecars are specified
Expand Down
3 changes: 3 additions & 0 deletions deploy/install.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,9 @@ spec:
if empty)
type: string
type: object
x-kubernetes-validations:
- message: storage is immutable once set
rule: oldSelf == null || self == oldSelf
type: object
x-kubernetes-validations:
- message: storage is required when sidecars are specified
Expand Down
65 changes: 42 additions & 23 deletions internal/controller/marimonotebook_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,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"
Expand Down Expand Up @@ -192,9 +193,19 @@ func (r *MarimoNotebookReconciler) reconcilePod(ctx context.Context, notebook *m
return nil, err
}

// Compute hash of desired pod spec for change detection
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)
}
desired.Annotations[resources.PodSpecHashAnnotation] = specHash

// 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)
Expand All @@ -213,41 +224,49 @@ 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
}

func (r *MarimoNotebookReconciler) reconcileService(ctx context.Context, notebook *marimov1alpha1.MarimoNotebook) (*corev1.Service, error) {
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
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Service reconciliation updates labels, ports, and selector but does not apply other desired ServiceSpec fields (e.g., Type). Even if Type currently defaults to ClusterIP, explicitly setting svc.Spec.Type = desired.Spec.Type (and any other fields BuildService considers authoritative) will keep the actual Service aligned with the desired state and avoid surprising drift.

Suggested change
svc.Spec.Selector = desired.Spec.Selector
svc.Spec.Selector = desired.Spec.Selector
svc.Spec.Type = desired.Spec.Type

Copilot uses AI. Check for mistakes.
svc.Spec.Type = desired.Spec.Type
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) {
Expand Down
114 changes: 114 additions & 0 deletions internal/controller/marimonotebook_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,120 @@ var _ = Describe("MarimoNotebook Controller", func() {
})
})

Context("When updating a MarimoNotebook", func() {
var notebook *marimov1alpha1.MarimoNotebook
var namespacedName types.NamespacedName

BeforeEach(func() {
notebook = &marimov1alpha1.MarimoNotebook{
ObjectMeta: metav1.ObjectMeta{
Name: "test-update-" + randString(),
Namespace: "default",
},
Spec: marimov1alpha1.MarimoNotebookSpec{
Source: "https://github.com/marimo-team/marimo.git",
Storage: &marimov1alpha1.StorageSpec{Size: "1Gi"},
},
}
namespacedName = types.NamespacedName{
Name: notebook.Name,
Namespace: notebook.Namespace,
}
})

AfterEach(func() {
_ = k8sClient.Delete(ctx, notebook)
})

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")
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.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))
})

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")
})

Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new pod-recreate-on-change behavior is intended to fix auth reconciliation (issue #6), but there is no controller test covering an update that adds/removes spec.auth.password.secretKeyRef and verifies the Pod is recreated with the expected secret volume mount. Adding a test for this specific regression would help prevent future breakage.

Suggested change
It("should recreate Pod when auth password secretKeyRef is added and removed", func() {
By("creating the MarimoNotebook without auth")
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("creating a Secret to be used for auth password")
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "auth-secret-" + randString(),
Namespace: notebook.Namespace,
},
StringData: map[string]string{
"password": "super-secret-password",
},
}
Expect(k8sClient.Create(ctx, secret)).To(Succeed())
By("updating the notebook to reference the auth password Secret")
nb := &marimov1alpha1.MarimoNotebook{}
Expect(k8sClient.Get(ctx, namespacedName, nb)).To(Succeed())
if nb.Spec.Auth == nil {
nb.Spec.Auth = &marimov1alpha1.AuthSpec{}
}
if nb.Spec.Auth.Password == nil {
nb.Spec.Auth.Password = &marimov1alpha1.PasswordAuthSpec{}
}
nb.Spec.Auth.Password.SecretKeyRef = &corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{
Name: secret.Name,
},
Key: "password",
}
Expect(k8sClient.Update(ctx, nb)).To(Succeed())
By("verifying Pod is recreated and mounts the auth Secret")
var secretVolumeName string
Eventually(func() bool {
if err := k8sClient.Get(ctx, namespacedName, pod); err != nil {
return false
}
if pod.UID == originalUID {
return false // still the old pod
}
secretVolumeName = ""
for _, v := range pod.Spec.Volumes {
if v.Secret != nil && v.Secret.SecretName == secret.Name {
secretVolumeName = v.Name
break
}
}
if secretVolumeName == "" {
return false
}
for _, vm := range pod.Spec.Containers[0].VolumeMounts {
if vm.Name == secretVolumeName {
return true
}
}
return false
}, timeout, interval).Should(BeTrue(), "Pod should be recreated and mount the auth Secret")
authUID := pod.UID
By("updating the notebook to remove the auth password SecretKeyRef")
nb2 := &marimov1alpha1.MarimoNotebook{}
Expect(k8sClient.Get(ctx, namespacedName, nb2)).To(Succeed())
if nb2.Spec.Auth != nil && nb2.Spec.Auth.Password != nil {
nb2.Spec.Auth.Password.SecretKeyRef = nil
}
Expect(k8sClient.Update(ctx, nb2)).To(Succeed())
By("verifying Pod is recreated and no longer mounts the auth Secret")
Eventually(func() bool {
if err := k8sClient.Get(ctx, namespacedName, pod); err != nil {
return false
}
if pod.UID == authUID {
return false // still the pod with auth secret
}
for _, v := range pod.Spec.Volumes {
if v.Secret != nil && v.Secret.SecretName == secret.Name {
// still has the secret volume
return false
}
}
for _, vm := range pod.Spec.Containers[0].VolumeMounts {
if vm.Name == secretVolumeName {
// still mounts the secret volume
return false
}
}
return true
}, timeout, interval).Should(BeTrue(), "Pod should be recreated and no longer mount the auth Secret")
})

Copilot uses AI. Check for mistakes.
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 once set")))
})
})

Context("When deleting a MarimoNotebook", func() {
It("should clean up owned resources via garbage collection", func() {
notebook := &marimov1alpha1.MarimoNotebook{
Expand Down
14 changes: 14 additions & 0 deletions pkg/resources/pod.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package resources

import (
"crypto/sha256"
"encoding/hex"
"encoding/json"
"fmt"
"strings"
Expand All @@ -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, error) {
data, err := json.Marshal(pod.Spec)
if err != nil {
return "", err
}
sum := sha256.Sum256(data)
return hex.EncodeToString(sum[:]), nil
}
Comment on lines +27 to +35
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PodSpecHash silently returns an empty string if JSON marshaling fails, and reconcilePod treats that as a valid hash. If this ever happens, spec changes may not be detected and pods may not be recreated. Consider returning (string, error) and failing/requeueing on error (or at least logging and forcing a recreate).

Copilot uses AI. Check for mistakes.

// BuildPod creates a Pod spec for a MarimoNotebook.
// Supports two content modes:
// - source: clone from git URL
Expand Down
Loading