Merge pull request #447 from jiridanek/jd_merge_branch

RHOAIENG-15741: Sync v1.9-branch branch with main branch

components/notebook-controller/Makefile

@@ -148,7 +148,7 @@ controller-gen: ## Download controller-gen locally if necessary.
 KUSTOMIZE = $(shell pwd)/bin/kustomize
 .PHONY: kustomize
 kustomize: ## Download kustomize locally if necessary.
-	$(call go-get-tool,$(KUSTOMIZE),sigs.k8s.io/kustomize/v3/cmd/[email protected])
+	$(call go-get-tool,$(KUSTOMIZE),sigs.k8s.io/kustomize/kustomize/v5@v5.0.2)
 
 ENVTEST = $(shell pwd)/bin/setup-envtest
 ENVTEST_VERSION?=v0.0.0-20240923090159-236e448db12c

components/notebook-controller/config/overlays/openshift/params.env

@@ -1 +1 @@
-odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:1.9-8369940
+odh-kf-notebook-controller-image=quay.io/opendatahub/kubeflow-notebook-controller:main-3f931d2

components/odh-notebook-controller/Makefile

@@ -4,12 +4,12 @@ IMG ?= quay.io/opendatahub/odh-notebook-controller
 TAG ?= $(shell git describe --tags --always)
 
 KF_IMG ?= quay.io/opendatahub/kubeflow-notebook-controller
-KF_TAG ?= 1.9-8369940
+KF_TAG ?= main-3f931d2
 
 CONTAINER_ENGINE ?= podman
 
 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
-ENVTEST_K8S_VERSION = 1.23
+ENVTEST_K8S_VERSION = 1.26
 
 # Kubernetes configuration
 K8S_NAMESPACE ?= odh-notebook-controller-system
@@ -241,7 +241,7 @@ controller-gen: ## Download controller-gen locally if necessary.
 KUSTOMIZE = $(LOCALBIN)/kustomize
 .PHONY: kustomize
 kustomize: ## Download kustomize locally if necessary.
-	GOBIN=$(LOCALBIN) go install sigs.k8s.io/kustomize/v3/cmd/[email protected]
+	GOBIN=$(LOCALBIN) go install sigs.k8s.io/kustomize/kustomize/v5@v5.0.2
 
 ENVTEST = $(LOCALBIN)/setup-envtest
 .PHONY: envtest

components/odh-notebook-controller/config/base/params.env

@@ -1 +1 @@
-odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:1.9-8369940
+odh-notebook-controller-image=quay.io/opendatahub/odh-notebook-controller:main-3f931d2

components/odh-notebook-controller/controllers/notebook_controller_test.go

@@ -683,23 +683,21 @@ var _ = Describe("The Openshift Notebook controller", func() {
 			}, duration, interval).Should(BeTrue())
 		})
 
-		// RHOAIENG-14552: We *do not* reconcile OAuth in the notebook when it's modified
-
-		//It("Should reconcile the Notebook when modified", func() {
-		//	By("By simulating a manual Notebook modification")
-		//	notebook.Spec.Template.Spec.ServiceAccountName = "foo"
-		//	notebook.Spec.Template.Spec.Containers[1].Image = "bar"
-		//	notebook.Spec.Template.Spec.Volumes[1].VolumeSource = corev1.VolumeSource{}
-		//	Expect(cli.Update(ctx, notebook)).Should(Succeed())
-		//	time.Sleep(interval)
-		//
-		//	By("By checking that the webhook has restored the Notebook spec")
-		//	Eventually(func() error {
-		//		key := types.NamespacedName{Name: Name, Namespace: Namespace}
-		//		return cli.Get(ctx, key, notebook)
-		//	}, duration, interval).Should(Succeed())
-		//	Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrue())
-		//})
+		It("Should reconcile the Notebook when modified", func() {
+			By("By simulating a manual Notebook modification")
+			notebook.Spec.Template.Spec.ServiceAccountName = "foo"
+			notebook.Spec.Template.Spec.Containers[1].Image = "bar"
+			notebook.Spec.Template.Spec.Volumes[1].VolumeSource = corev1.VolumeSource{}
+			Expect(cli.Update(ctx, notebook)).Should(Succeed())
+			time.Sleep(interval)
+
+			By("By checking that the webhook has restored the Notebook spec")
+			Eventually(func() error {
+				key := types.NamespacedName{Name: Name, Namespace: Namespace}
+				return cli.Get(ctx, key, notebook)
+			}, duration, interval).Should(Succeed())
+			Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrue())
+		})
 
 		serviceAccount := &corev1.ServiceAccount{}
 		expectedServiceAccount := createOAuthServiceAccount(Name, Namespace)

components/odh-notebook-controller/controllers/notebook_webhook.go

@@ -28,6 +28,7 @@ import (
 	"github.com/kubeflow/kubeflow/components/notebook-controller/pkg/culler"
 	admissionv1 "k8s.io/api/admission/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -232,6 +233,7 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm
 
 	// Initialize logger format
 	log := w.Log.WithValues("notebook", req.Name, "namespace", req.Namespace)
+	ctx = logr.NewContext(ctx, log)
 
 	notebook := &nbv1.Notebook{}
 
@@ -265,22 +267,31 @@ func (w *NotebookWebhook) Handle(ctx context.Context, req admission.Request) adm
 	}
 
 	// Inject the OAuth proxy if the annotation is present but only if Service Mesh is disabled
-	if OAuthInjectionIsEnabled(notebook.ObjectMeta) && ServiceMeshIsEnabled(notebook.ObjectMeta) {
-		return admission.Denied(fmt.Sprintf("Cannot have both %s and %s set to true. Pick one.", AnnotationServiceMesh, AnnotationInjectOAuth))
-	}
-	// RHOAIENG-14552: Updating oauth-proxy in a running notebook may lead to notebook restart. Updating only
-	// on Create is safe as it cannot cause a restart in already running notebook when oauth-proxy image changes.
-	if req.Operation == admissionv1.Create {
-		if OAuthInjectionIsEnabled(notebook.ObjectMeta) {
-			err = InjectOAuthProxy(notebook, w.OAuthConfig)
-			if err != nil {
-				return admission.Errored(http.StatusInternalServerError, err)
-			}
+	if OAuthInjectionIsEnabled(notebook.ObjectMeta) {
+		if ServiceMeshIsEnabled(notebook.ObjectMeta) {
+			return admission.Denied(fmt.Sprintf("Cannot have both %s and %s set to true. Pick one.", AnnotationServiceMesh, AnnotationInjectOAuth))
+		}
+		err = InjectOAuthProxy(notebook, w.OAuthConfig)
+		if err != nil {
+			return admission.Errored(http.StatusInternalServerError, err)
 		}
 	}
 
+	// RHOAIENG-14552: Running notebook cannot be updated carelessly, or we may end up restarting the pod when
+	// the webhook runs after e.g. the oauth-proxy image has been updated
+	mutatedNotebook, needsRestart, err := w.maybeRestartRunningNotebook(ctx, req, notebook)
+	if err != nil {
+		return admission.Errored(http.StatusInternalServerError, err)
+	}
+	updatePendingAnnotation := "notebooks.opendatahub.io/update-pending"
+	if needsRestart != NoPendingUpdates {
+		mutatedNotebook.ObjectMeta.Annotations[updatePendingAnnotation] = needsRestart.Reason
+	} else {
+		delete(mutatedNotebook.ObjectMeta.Annotations, updatePendingAnnotation)
+	}
+
 	// Create the mutated notebook object
-	marshaledNotebook, err := json.Marshal(notebook)
+	marshaledNotebook, err := json.Marshal(mutatedNotebook)
 	if err != nil {
 		return admission.Errored(http.StatusInternalServerError, err)
 	}
@@ -294,6 +305,68 @@ func (w *NotebookWebhook) InjectDecoder(d *admission.Decoder) error {
 	return nil
 }
 
+// maybeRestartRunningNotebook evaluates whether the updates being made cause notebook pod to restart.
+// If the restart is caused by updates made by the mutating webhook itself to already existing notebook,
+// and the notebook is not stopped, then these updates will be blocked until the notebook is stopped.
+// Returns the mutated notebook, a flag whether there's a pending restart (to apply blocked updates), and an error value.
+func (w *NotebookWebhook) maybeRestartRunningNotebook(ctx context.Context, req admission.Request, mutatedNotebook *nbv1.Notebook) (*nbv1.Notebook, *UpdatesPending, error) {
+	var err error
+	log := logr.FromContextOrDiscard(ctx)
+
+	// Notebook that was just created can be updated
+	if req.Operation == admissionv1.Create {
+		log.Info("Not blocking update, notebook is being newly created")
+		return mutatedNotebook, NoPendingUpdates, nil
+	}
+
+	// Stopped notebooks are ok to update
+	if metav1.HasAnnotation(mutatedNotebook.ObjectMeta, "kubeflow-resource-stopped") {
+		log.Info("Not blocking update, notebook is (to be) stopped")
+		return mutatedNotebook, NoPendingUpdates, nil
+	}
+
+	// Restarting notebooks are also ok to update
+	if metav1.HasAnnotation(mutatedNotebook.ObjectMeta, "notebooks.opendatahub.io/notebook-restart") {
+		log.Info("Not blocking update, notebook is (to be) restarted")
+		return mutatedNotebook, NoPendingUpdates, nil
+	}
+
+	// fetch the updated Notebook CR that was sent to the Webhook
+	updatedNotebook := &nbv1.Notebook{}
+	err = w.Decoder.Decode(req, updatedNotebook)
+	if err != nil {
+		log.Error(err, "Failed to fetch the updated Notebook CR")
+		return nil, NoPendingUpdates, err
+	}
+
+	// fetch the original Notebook CR
+	oldNotebook := &nbv1.Notebook{}
+	err = w.Decoder.DecodeRaw(req.OldObject, oldNotebook)
+	if err != nil {
+		log.Error(err, "Failed to fetch the original Notebook CR")
+		return nil, NoPendingUpdates, err
+	}
+
+	// The externally issued update already causes a restart, so we will just let all changes proceed
+	if !equality.Semantic.DeepEqual(oldNotebook.Spec.Template.Spec, updatedNotebook.Spec.Template.Spec) {
+		log.Info("Not blocking update, the externally issued update already modifies pod template, causing a restart")
+		return mutatedNotebook, NoPendingUpdates, nil
+	}
+
+	// Nothing about the Pod definition is actually changing and we can proceed
+	if equality.Semantic.DeepEqual(oldNotebook.Spec.Template.Spec, mutatedNotebook.Spec.Template.Spec) {
+		log.Info("Not blocking update, the pod template is not being modified at all")
+		return mutatedNotebook, NoPendingUpdates, nil
+	}
+
+	// Now we know we have to block the update
+	// Keep the old values and mark the Notebook as UpdatesPending
+	diff := getStructDiff(ctx, mutatedNotebook.Spec.Template.Spec, updatedNotebook.Spec.Template.Spec)
+	log.Info("Update blocked, notebook pod template would be changed by the webhook", "diff", diff)
+	mutatedNotebook.Spec.Template.Spec = updatedNotebook.Spec.Template.Spec
+	return mutatedNotebook, &UpdatesPending{Reason: diff}, nil
+}
+
 // CheckAndMountCACertBundle checks if the odh-trusted-ca-bundle ConfigMap is present
 func CheckAndMountCACertBundle(ctx context.Context, cli client.Client, notebook *nbv1.Notebook, log logr.Logger) error {
 

components/odh-notebook-controller/controllers/notebook_webhook_utils.go

@@ -0,0 +1,80 @@
+/*
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controllers
+
+import (
+	"context"
+	"fmt"
+	"github.com/google/go-cmp/cmp"
+
+	"github.com/go-logr/logr"
+)
+
+// UpdatesPending is either NoPendingUpdates, or a new value providing a Reason for the update.
+type UpdatesPending struct {
+	Reason string
+}
+
+var (
+	NoPendingUpdates = &UpdatesPending{}
+)
+
+// FirstDifferenceReporter is a custom go-cmp reporter that only records the first difference.
+type FirstDifferenceReporter struct {
+	path cmp.Path
+	diff string
+}
+
+func (r *FirstDifferenceReporter) PushStep(ps cmp.PathStep) {
+	r.path = append(r.path, ps)
+}
+
+func (r *FirstDifferenceReporter) Report(rs cmp.Result) {
+	if r.diff == "" && !rs.Equal() {
+		vx, vy := r.path.Last().Values()
+		r.diff = fmt.Sprintf("%#v: %+v != %+v", r.path, vx, vy)
+	}
+}
+
+func (r *FirstDifferenceReporter) PopStep() {
+	r.path = r.path[:len(r.path)-1]
+}
+
+func (r *FirstDifferenceReporter) String() string {
+	return r.diff
+}
+
+// getStructDiff compares a and b, reporting the first difference it found in a human-readable single-line string.
+func getStructDiff(ctx context.Context, a any, b any) (result string) {
+	log := logr.FromContextOrDiscard(ctx)
+
+	// calling cmp.Equal may panic, get ready for it
+	result = "failed to compute the reason for why there is a pending restart"
+	defer func() {
+		if r := recover(); r != nil {
+			log.Error(fmt.Errorf("failed to compute struct difference: %+v", r), "Cannot determine reason for restart")
+		}
+	}()
+
+	var comparator FirstDifferenceReporter
+	eq := cmp.Equal(a, b, cmp.Reporter(&comparator))
+	if eq {
+		log.Error(nil, "Unexpectedly attempted to diff structs that are actually equal")
+	}
+	result = comparator.String()
+
+	return
+}

components/odh-notebook-controller/controllers/notebook_webhook_utils_test.go

@@ -0,0 +1,64 @@
+/*
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controllers
+
+import (
+	"context"
+	"testing"
+
+	v1 "k8s.io/api/core/v1"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestFirstDifferenceReporter(t *testing.T) {
+	for _, tt := range []struct {
+		name string
+		a    any
+		b    any
+		diff string
+	}{
+		{"", 42, 42, ""},
+		{"", v1.Pod{Spec: v1.PodSpec{NodeName: "node1"}}, v1.Pod{Spec: v1.PodSpec{NodeName: "node2"}}, "{v1.Pod}.Spec.NodeName: node1 != node2"},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			var reporter FirstDifferenceReporter
+			eq := cmp.Equal(tt.a, tt.b, cmp.Reporter(&reporter))
+			assert.Equal(t, tt.diff == "", eq)
+			assert.Equal(t, tt.diff, reporter.String())
+		})
+	}
+}
+
+func TestGetStructDiff(t *testing.T) {
+	var tests = []struct {
+		name     string
+		a        any
+		b        any
+		expected string
+	}{
+		{"simple numbers", 42, 42, ""},
+		{"differing pods", v1.Pod{Spec: v1.PodSpec{NodeName: "node1"}}, v1.Pod{Spec: v1.PodSpec{NodeName: "node2"}}, "{v1.Pod}.Spec.NodeName: node1 != node2"},
+	}
+
+	for _, v := range tests {
+		t.Run(v.name, func(t *testing.T) {
+			diff := getStructDiff(context.Background(), v.a, v.b)
+			assert.Equal(t, diff, v.expected)
+		})
+	}
+}

components/odh-notebook-controller/go.mod

@@ -6,6 +6,7 @@ toolchain go1.21.9
 
 require (
 	github.com/go-logr/logr v1.4.1
+	github.com/google/go-cmp v0.6.0
 	github.com/kubeflow/kubeflow/components/notebook-controller v0.0.0-20220728153354-fc09bd1eefb8
 	github.com/onsi/ginkgo v1.16.5
 	github.com/onsi/gomega v1.30.0
@@ -36,7 +37,6 @@ require (
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
 	github.com/golang/protobuf v1.5.3 // indirect
 	github.com/google/gnostic-models v0.6.8 // indirect
-	github.com/google/go-cmp v0.6.0 // indirect
 	github.com/google/gofuzz v1.2.0 // indirect
 	github.com/google/uuid v1.3.0 // indirect
 	github.com/imdario/mergo v0.3.12 // indirect

components/profile-controller/Makefile

@@ -4,7 +4,7 @@ TAG ?= $(shell git describe --tags --always --dirty)
 ARCH ?= linux/amd64
 
 # ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
-ENVTEST_K8S_VERSION = 1.23
+ENVTEST_K8S_VERSION = 1.26
 
 # Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
 ifeq (,$(shell go env GOBIN))