Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve(ws): replace reflect.DeepEqual with equality.Semantic.DeepEqual #152

Open
wants to merge 1 commit into
base: notebooks-v2
Choose a base branch
from
Open

improve(ws): replace reflect.DeepEqual with equality.Semantic.DeepEqual #152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 3 additions & 4 deletions workspaces/controller/internal/controller/workspace_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down
79 changes: 5 additions & 74 deletions workspaces/controller/internal/helper/helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
30 changes: 6 additions & 24 deletions workspaces/controller/internal/webhook/workspacekind_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)...)
}

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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 {
Expand Down
Loading