diff --git a/api/v1alpha1/marimonotebook_types.go b/api/v1alpha1/marimonotebook_types.go index 3774f02..1b6fbf1 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="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 b2627e6..1ab8ba0 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 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 799168c..f1cf1df 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 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 3008b87..fc712de 100644 --- a/internal/controller/marimonotebook_controller.go +++ b/internal/controller/marimonotebook_controller.go @@ -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" @@ -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) @@ -213,7 +224,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 } @@ -221,33 +241,32 @@ 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 + 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) { diff --git a/internal/controller/marimonotebook_controller_test.go b/internal/controller/marimonotebook_controller_test.go index 5e6a3d2..17412a3 100644 --- a/internal/controller/marimonotebook_controller_test.go +++ b/internal/controller/marimonotebook_controller_test.go @@ -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") + }) + + 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{ diff --git a/pkg/resources/pod.go b/pkg/resources/pod.go index 24e9f41..e6395c4 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, error) { + data, err := json.Marshal(pod.Spec) + if err != nil { + return "", err + } + sum := sha256.Sum256(data) + return hex.EncodeToString(sum[:]), nil +} + // BuildPod creates a Pod spec for a MarimoNotebook. // Supports two content modes: // - source: clone from git URL