diff --git a/workspaces/controller/internal/controller/workspace_controller.go b/workspaces/controller/internal/controller/workspace_controller.go index c985b0e9..fa64ebaf 100644 --- a/workspaces/controller/internal/controller/workspace_controller.go +++ b/workspaces/controller/internal/controller/workspace_controller.go @@ -19,12 +19,11 @@ package controller import ( "context" "fmt" - "reflect" "strings" - "k8s.io/apimachinery/pkg/util/intstr" - "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/util/intstr" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -367,7 +366,7 @@ func (r *WorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if err != nil { return ctrl.Result{}, err } - if !reflect.DeepEqual(workspace.Status, workspaceStatus) { + if !equality.Semantic.DeepEqual(workspace.Status, workspaceStatus) { workspace.Status = workspaceStatus if err := r.Status().Update(ctx, workspace); err != nil { if apierrors.IsConflict(err) { diff --git a/workspaces/controller/internal/helper/helper.go b/workspaces/controller/internal/helper/helper.go index d4d6bdef..fac3d179 100644 --- a/workspaces/controller/internal/helper/helper.go +++ b/workspaces/controller/internal/helper/helper.go @@ -17,13 +17,9 @@ limitations under the License. package helper import ( - "reflect" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" - - kubefloworgv1beta1 "github.com/kubeflow/notebooks/workspaces/controller/api/v1beta1" + "k8s.io/apimachinery/pkg/api/equality" ) // CopyStatefulSetFields updates a target StatefulSet with the fields from a desired StatefulSet, returning true if an update is required. @@ -57,7 +53,7 @@ func CopyStatefulSetFields(desired *appsv1.StatefulSet, target *appsv1.StatefulS // TODO: confirm if StatefulSets support updates to the selector // if not, we might need to recreate the StatefulSet // - if !reflect.DeepEqual(target.Spec.Selector, desired.Spec.Selector) { + if !equality.Semantic.DeepEqual(target.Spec.Selector, desired.Spec.Selector) { target.Spec.Selector = desired.Spec.Selector requireUpdate = true } @@ -67,7 +63,7 @@ func CopyStatefulSetFields(desired *appsv1.StatefulSet, target *appsv1.StatefulS // TODO: confirm if there is a problem with doing the update at the `spec.template` level // or if only `spec.template.spec` should be updated // - if !reflect.DeepEqual(target.Spec.Template, desired.Spec.Template) { + if !equality.Semantic.DeepEqual(target.Spec.Template, desired.Spec.Template) { target.Spec.Template = desired.Spec.Template requireUpdate = true } @@ -98,13 +94,13 @@ func CopyServiceFields(desired *corev1.Service, target *corev1.Service) bool { // NOTE: we don't copy the entire `spec` because we can't overwrite the `spec.clusterIp` and similar fields // copy `spec.ports` - if !reflect.DeepEqual(target.Spec.Ports, desired.Spec.Ports) { + if !equality.Semantic.DeepEqual(target.Spec.Ports, desired.Spec.Ports) { target.Spec.Ports = desired.Spec.Ports requireUpdate = true } // copy `spec.selector` - if !reflect.DeepEqual(target.Spec.Selector, desired.Spec.Selector) { + if !equality.Semantic.DeepEqual(target.Spec.Selector, desired.Spec.Selector) { target.Spec.Selector = desired.Spec.Selector requireUpdate = true } @@ -117,68 +113,3 @@ func CopyServiceFields(desired *corev1.Service, target *corev1.Service) bool { return requireUpdate } - -// NormalizePodConfigSpec normalizes a PodConfigSpec so that it can be compared with reflect.DeepEqual -func NormalizePodConfigSpec(spec kubefloworgv1beta1.PodConfigSpec) error { - - // normalize Affinity - if spec.Affinity != nil { - - // set Affinity to nil if it is empty - if reflect.DeepEqual(spec.Affinity, corev1.Affinity{}) { - spec.Affinity = nil - } - } - - // normalize NodeSelector - if spec.NodeSelector != nil { - - // set NodeSelector to nil if it is empty - if len(spec.NodeSelector) == 0 { - spec.NodeSelector = nil - } - } - - // normalize Tolerations - if spec.Tolerations != nil { - - // set Tolerations to nil if it is empty - if len(spec.Tolerations) == 0 { - spec.Tolerations = nil - } - } - - // normalize Resources - if spec.Resources != nil { - - // if Resources.Requests is empty, set it to nil - if len(spec.Resources.Requests) == 0 { - spec.Resources.Requests = nil - } else { - // otherwise, normalize the values in Resources.Requests - for key, value := range spec.Resources.Requests { - q, err := resource.ParseQuantity(value.String()) - if err != nil { - return err - } - spec.Resources.Requests[key] = q - } - } - - // if Resources.Limits is empty, set it to nil - if len(spec.Resources.Limits) == 0 { - spec.Resources.Limits = nil - } else { - // otherwise, normalize the values in Resources.Limits - for key, value := range spec.Resources.Limits { - q, err := resource.ParseQuantity(value.String()) - if err != nil { - return err - } - spec.Resources.Limits[key] = q - } - } - } - - return nil -} diff --git a/workspaces/controller/internal/webhook/workspacekind_webhook.go b/workspaces/controller/internal/webhook/workspacekind_webhook.go index 4c591731..1b751bd2 100644 --- a/workspaces/controller/internal/webhook/workspacekind_webhook.go +++ b/workspaces/controller/internal/webhook/workspacekind_webhook.go @@ -20,9 +20,9 @@ import ( "context" "errors" "fmt" - "reflect" "sync" + "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" @@ -141,7 +141,7 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new getImageConfigUsageCount, getPodConfigUsageCount := v.getLazyOptionUsageCountFuncs(ctx, oldWorkspaceKind) // validate the extra environment variables - if !reflect.DeepEqual(newWorkspaceKind.Spec.PodTemplate.ExtraEnv, oldWorkspaceKind.Spec.PodTemplate.ExtraEnv) { + if !equality.Semantic.DeepEqual(newWorkspaceKind.Spec.PodTemplate.ExtraEnv, oldWorkspaceKind.Spec.PodTemplate.ExtraEnv) { allErrs = append(allErrs, validateExtraEnv(newWorkspaceKind)...) } @@ -173,12 +173,12 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new } else { // if we haven't already decided to validate the imageConfig redirects, // check if the redirect has changed - if !shouldValidateImageConfigRedirects && !reflect.DeepEqual(oldImageConfigIdMap[imageConfigValue.Id].Redirect, imageConfigValue.Redirect) { + if !shouldValidateImageConfigRedirects && !equality.Semantic.DeepEqual(oldImageConfigIdMap[imageConfigValue.Id].Redirect, imageConfigValue.Redirect) { shouldValidateImageConfigRedirects = true } // check if the spec has changed - if !reflect.DeepEqual(oldImageConfigIdMap[imageConfigValue.Id].Spec, imageConfigValue.Spec) { + if !equality.Semantic.DeepEqual(oldImageConfigIdMap[imageConfigValue.Id].Spec, imageConfigValue.Spec) { // we need to validate this imageConfig value since it has changed toValidateImageConfigIds[imageConfigValue.Id] = true @@ -244,33 +244,15 @@ func (v *WorkspaceKindValidator) ValidateUpdate(ctx context.Context, oldObj, new } else { // if we haven't already decided to validate the podConfig redirects, // check if the redirect has changed - if !shouldValidatePodConfigRedirects && !reflect.DeepEqual(oldPodConfigIdMap[podConfigValue.Id].Redirect, podConfigValue.Redirect) { + if !shouldValidatePodConfigRedirects && !equality.Semantic.DeepEqual(oldPodConfigIdMap[podConfigValue.Id].Redirect, podConfigValue.Redirect) { shouldValidatePodConfigRedirects = true } - // we must normalize the podConfig specs so that we can compare them newPodConfigSpec := podConfigValue.Spec - err := helper.NormalizePodConfigSpec(newPodConfigSpec) - if err != nil { - podConfigValueSpecPath := field.NewPath("spec", "podTemplate", "options", "podConfig", "values").Key(podConfigValue.Id).Child("spec") - allErrs = append(allErrs, field.InternalError(podConfigValueSpecPath, fmt.Errorf("failed to normalize podConfig spec: %w", err))) - - // if the spec could not be normalized, we cannot validate the WorkspaceKind further - return nil, apierrors.NewInvalid( - schema.GroupKind{Group: kubefloworgv1beta1.GroupVersion.Group, Kind: "WorkspaceKind"}, - newWorkspaceKind.Name, - allErrs, - ) - } oldPodConfigSpec := oldPodConfigIdMap[podConfigValue.Id].Spec - err = helper.NormalizePodConfigSpec(oldPodConfigSpec) - if err != nil { - // this should never happen, as it would indicate that the old podConfig spec is invalid - return nil, apierrors.NewInternalError(fmt.Errorf("old podConfig spec of %q could not be normalized: %w", podConfigValue.Id, err)) - } // check if the spec has changed - if !reflect.DeepEqual(oldPodConfigSpec, newPodConfigSpec) { + if !equality.Semantic.DeepEqual(oldPodConfigSpec, newPodConfigSpec) { // check how many workspaces are using this podConfig value usageCount, err := getPodConfigUsageCount(podConfigValue.Id) if err != nil {