Skip to content

OCPBUGS-36246: Improve resource merge diffs #1409

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

Open
2uasimojo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPBUGS-36246: Improve resource merge diffs #1409

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
4 changes: 1 addition & 3 deletions lib/resourceapply/admissionregistration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package resourceapply
import (
"context"

"github.com/google/go-cmp/cmp"

admissionregv1 "k8s.io/api/admissionregistration/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -38,7 +36,7 @@ func ApplyValidatingWebhookConfigurationv1(ctx context.Context, client admission
return existing, false, nil
}
if reconciling {
if diff := cmp.Diff(&original, existing); diff != "" {
if diff := ManifestDiff(&original, existing); diff != "" {
klog.V(2).Infof("Updating ValidatingWebhookConfiguration %s/%s due to diff: %v", required.Namespace, required.Name, diff)
} else {
klog.V(2).Infof("Updating ValidatingWebhookConfiguration %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name)
Expand Down
4 changes: 1 addition & 3 deletions lib/resourceapply/apiext.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package resourceapply
import (
"context"

"github.com/google/go-cmp/cmp"

apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
apiextclientv1 "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -39,7 +37,7 @@ func ApplyCustomResourceDefinitionv1(ctx context.Context, client apiextclientv1.
return existing, false, nil
}
if reconciling {
if diff := cmp.Diff(&original, existing); diff != "" {
if diff := ManifestDiff(&original, existing); diff != "" {
klog.V(2).Infof("Updating CRD %s due to diff: %v", required.Name, diff)
} else {
klog.V(2).Infof("Updating CRD %s with empty diff: possible hotloop after wrong comparison", required.Name)
Expand Down
8 changes: 4 additions & 4 deletions lib/resourceapply/apps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package resourceapply
import (
"context"

"github.com/google/go-cmp/cmp"

appsv1 "k8s.io/api/apps/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -39,7 +37,7 @@ func ApplyDeploymentv1(ctx context.Context, client appsclientv1.DeploymentsGette
return existing, false, nil
}
if reconciling {
if diff := cmp.Diff(&original, existing); diff != "" {
if diff := ManifestDiff(&original, existing); diff != "" {
klog.V(2).Infof("Updating Deployment %s/%s due to diff: %v", required.Namespace, required.Name, diff)
} else {
klog.V(2).Infof("Updating Deployment %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name)
Expand All @@ -66,14 +64,16 @@ func ApplyDaemonSetv1(ctx context.Context, client appsclientv1.DaemonSetsGetter,
return nil, false, nil
}

var original appsv1.DaemonSet
existing.DeepCopyInto(&original)
modified := ptr.To(false)
resourcemerge.EnsureDaemonSet(modified, existing, *required)
if !*modified {
return existing, false, nil
}

if reconciling {
klog.V(2).Infof("Updating DaemonSet %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required))
klog.V(2).Infof("Updating DaemonSet %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing))
}

actual, err := client.DaemonSets(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{})
Expand Down
6 changes: 2 additions & 4 deletions lib/resourceapply/batch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package resourceapply
import (
"context"

"github.com/google/go-cmp/cmp"

batchv1 "k8s.io/api/batch/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -39,7 +37,7 @@ func ApplyJobv1(ctx context.Context, client batchclientv1.JobsGetter, required *
return existing, false, nil
}
if reconciling {
if diff := cmp.Diff(&original, existing); diff != "" {
if diff := ManifestDiff(&original, existing); diff != "" {
klog.V(2).Infof("Updating Job %s/%s due to diff: %v", required.Namespace, required.Name, diff)
} else {
klog.V(2).Infof("Updating Job %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name)
Expand Down Expand Up @@ -74,7 +72,7 @@ func ApplyCronJobv1(ctx context.Context, client batchclientv1.CronJobsGetter, re
return existing, false, nil
}
if reconciling {
if diff := cmp.Diff(&original, existing); diff != "" {
if diff := ManifestDiff(&original, existing); diff != "" {
klog.V(2).Infof("Updating CronJob %s/%s due to diff: %v", required.Namespace, required.Name, diff)
} else {
klog.V(2).Infof("Updating CronJob %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name)
Expand Down
18 changes: 12 additions & 6 deletions lib/resourceapply/core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package resourceapply
import (
"context"

"github.com/google/go-cmp/cmp"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -32,13 +30,15 @@ func ApplyNamespacev1(ctx context.Context, client coreclientv1.NamespacesGetter,
return nil, false, nil
}

var original corev1.Namespace
existing.DeepCopyInto(&original)
modified := ptr.To(false)
resourcemerge.EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta)
if !*modified {
return existing, false, nil
}
if reconciling {
klog.V(2).Infof("Updating Namespace %s due to diff: %v", required.Name, cmp.Diff(existing, required))
klog.V(2).Infof("Updating Namespace %s due to diff: %v", required.Name, ManifestDiff(&original, existing))
}

actual, err := client.Namespaces().Update(ctx, existing, metav1.UpdateOptions{})
Expand All @@ -63,6 +63,8 @@ func ApplyServicev1(ctx context.Context, client coreclientv1.ServicesGetter, req
return nil, false, nil
}

var original corev1.Service
existing.DeepCopyInto(&original)
modified := ptr.To(false)
resourcemerge.EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta)
resourcemerge.EnsureServicePorts(modified, &existing.Spec.Ports, required.Spec.Ports)
Expand All @@ -75,7 +77,7 @@ func ApplyServicev1(ctx context.Context, client coreclientv1.ServicesGetter, req
existing.Spec.Selector = required.Spec.Selector

if reconciling {
klog.V(2).Infof("Updating Service %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required))
klog.V(2).Infof("Updating Service %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing))
}

actual, err := client.Services(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{})
Expand All @@ -98,14 +100,16 @@ func ApplyServiceAccountv1(ctx context.Context, client coreclientv1.ServiceAccou
return nil, false, nil
}

var original corev1.ServiceAccount
existing.DeepCopyInto(&original)
modified := ptr.To(false)
resourcemerge.EnsureServiceAccount(modified, existing, *required)
if !*modified {
return existing, false, nil
}

if reconciling {
klog.V(2).Infof("Updating ServiceAccount %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required))
klog.V(2).Infof("Updating ServiceAccount %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing))
}

actual, err := client.ServiceAccounts(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{})
Expand All @@ -128,14 +132,16 @@ func ApplyConfigMapv1(ctx context.Context, client coreclientv1.ConfigMapsGetter,
return nil, false, nil
}

var original corev1.ConfigMap
existing.DeepCopyInto(&original)
modified := ptr.To(false)
resourcemerge.EnsureConfigMap(modified, existing, *required)
if !*modified {
return existing, false, nil
}

if reconciling {
klog.V(2).Infof("Updating ConfigMap %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required))
klog.V(2).Infof("Updating ConfigMap %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing))
}

actual, err := client.ConfigMaps(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{})
Expand Down
4 changes: 1 addition & 3 deletions lib/resourceapply/cv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package resourceapply
import (
"context"

"github.com/google/go-cmp/cmp"

"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/klog/v2"
Expand Down Expand Up @@ -39,7 +37,7 @@ func ApplyClusterVersionFromCache(ctx context.Context, lister configlistersv1.Cl
return existing, false, nil
}

klog.V(2).Infof("Updating ClusterVersion %s due to diff: %v", required.Name, cmp.Diff(existing, required))
klog.V(2).Infof("Updating ClusterVersion %s due to diff: %v", required.Name, ManifestDiff(obj, existing))

actual, err := client.ClusterVersions().Update(ctx, existing, metav1.UpdateOptions{})
return actual, true, err
Expand Down
4 changes: 1 addition & 3 deletions lib/resourceapply/imagestream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package resourceapply
import (
"context"

"github.com/google/go-cmp/cmp"

"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/klog/v2"
Expand Down Expand Up @@ -38,7 +36,7 @@ func ApplyImageStreamv1(ctx context.Context, client imageclientv1.ImageStreamsGe
return existing, false, nil
}
if reconciling {
if diff := cmp.Diff(&original, existing); diff != "" {
if diff := ManifestDiff(&original, existing); diff != "" {
klog.V(2).Infof("Updating ImageStream %s/%s due to diff: %v", required.Namespace, required.Name, diff)
} else {
klog.V(2).Infof("Updating ImageStream %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name)
Expand Down
29 changes: 29 additions & 0 deletions lib/resourceapply/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package resourceapply
import (
"strings"

"github.com/google/go-cmp/cmp"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand All @@ -16,3 +18,30 @@ const CreateOnlyAnnotation = "release.openshift.io/create-only"
func IsCreateOnly(metadata metav1.Object) bool {
return strings.EqualFold(metadata.GetAnnotations()[CreateOnlyAnnotation], "true")
}

// ManifestDiff is a specialized case of cmp.Diff that ignores fields unlikely to match when one side is read
// from the APIServer (`fromKAS`) and the other from a manifest file (`fromManifest`). It is specifically
// aligned with EnsureObjectMeta when it comes to ObjectMeta.
func ManifestDiff(fromKAS, fromManifest any) string {
return cmp.Diff(fromKAS, fromManifest, cmp.FilterPath(
func(p cmp.Path) bool {
path := p.String()
if path == "TypeMeta" || path == "Status" {
// Ignore
return true
}
if strings.HasPrefix(path, "ObjectMeta.") {
if parts := strings.SplitN(path, ".", 3); len(parts) >= 2 {
switch parts[1] {
case "GenerateName", "SelfLink", "UID", "ResourceVersion", "Generation", "CreationTimestamp", "DeletionTimestamp", "DeletionGracePeriodSeconds", "Finalizers", "ManagedFields":
// Ignore
return true
}
}
// The remaining ObjectMeta fields are those curated by EnsureObjectMeta
}
return false
},
cmp.Ignore(),
))
}
3 changes: 1 addition & 2 deletions lib/resourceapply/operators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package resourceapply
import (
"context"

"github.com/google/go-cmp/cmp"
operatorsv1 "github.com/operator-framework/api/pkg/operators/v1"
operatorsclientv1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/typed/operators/v1"

Expand Down Expand Up @@ -39,7 +38,7 @@ func ApplyOperatorGroupv1(ctx context.Context, client operatorsclientv1.Operator
return existing, false, nil
}
if reconciling {
if diff := cmp.Diff(&original, existing); diff != "" {
if diff := ManifestDiff(&original, existing); diff != "" {
klog.V(2).Infof("Updating OperatorGroup %s/%s due to diff: %v", required.Namespace, required.Name, diff)
} else {
klog.V(2).Infof("Updating OperatorGroup %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name)
Expand Down
18 changes: 12 additions & 6 deletions lib/resourceapply/rbac.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package resourceapply
import (
"context"

"github.com/google/go-cmp/cmp"

rbacv1 "k8s.io/api/rbac/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -31,13 +29,15 @@ func ApplyClusterRoleBindingv1(ctx context.Context, client rbacclientv1.ClusterR
return nil, false, nil
}

var original rbacv1.ClusterRoleBinding
existing.DeepCopyInto(&original)
modified := ptr.To(false)
resourcemerge.EnsureClusterRoleBinding(modified, existing, *required)
if !*modified {
return existing, false, nil
}
if reconciling {
klog.V(2).Infof("Updating ClusterRoleBinding %s due to diff: %v", required.Name, cmp.Diff(existing, required))
klog.V(2).Infof("Updating ClusterRoleBinding %s due to diff: %v", required.Name, ManifestDiff(&original, existing))
}

actual, err := client.ClusterRoleBindings().Update(ctx, existing, metav1.UpdateOptions{})
Expand All @@ -60,13 +60,15 @@ func ApplyClusterRolev1(ctx context.Context, client rbacclientv1.ClusterRolesGet
return nil, false, nil
}

var original rbacv1.ClusterRole
existing.DeepCopyInto(&original)
modified := ptr.To(false)
resourcemerge.EnsureClusterRole(modified, existing, *required)
if !*modified {
return existing, false, nil
}
if reconciling {
klog.V(2).Infof("Updating ClusterRole %s due to diff: %v", required.Name, cmp.Diff(existing, required))
klog.V(2).Infof("Updating ClusterRole %s due to diff: %v", required.Name, ManifestDiff(&original, existing))
}

actual, err := client.ClusterRoles().Update(ctx, existing, metav1.UpdateOptions{})
Expand All @@ -89,13 +91,15 @@ func ApplyRoleBindingv1(ctx context.Context, client rbacclientv1.RoleBindingsGet
return nil, false, nil
}

var original rbacv1.RoleBinding
existing.DeepCopyInto(&original)
modified := ptr.To(false)
resourcemerge.EnsureRoleBinding(modified, existing, *required)
if !*modified {
return existing, false, nil
}
if reconciling {
klog.V(2).Infof("Updating RoleBinding %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required))
klog.V(2).Infof("Updating RoleBinding %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing))
}

actual, err := client.RoleBindings(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{})
Expand All @@ -118,13 +122,15 @@ func ApplyRolev1(ctx context.Context, client rbacclientv1.RolesGetter, required
return nil, false, nil
}

var original rbacv1.Role
existing.DeepCopyInto(&original)
modified := ptr.To(false)
resourcemerge.EnsureRole(modified, existing, *required)
if !*modified {
return existing, false, nil
}
if reconciling {
klog.V(2).Infof("Updating Role %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required))
klog.V(2).Infof("Updating Role %s/%s due to diff: %v", required.Namespace, required.Name, ManifestDiff(&original, existing))
}

actual, err := client.Roles(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{})
Expand Down
6 changes: 3 additions & 3 deletions lib/resourceapply/security.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ package resourceapply
import (
"context"

"github.com/google/go-cmp/cmp"

apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/klog/v2"
Expand All @@ -31,13 +29,15 @@ func ApplySecurityContextConstraintsv1(ctx context.Context, client securityclien
return nil, false, nil
}

var original securityv1.SecurityContextConstraints
existing.DeepCopyInto(&original)
reconcile := resourcemerge.EnsureSecurityContextConstraints(*existing, *required)
if reconcile == nil {
return existing, false, nil
}

if reconciling {
if diff := cmp.Diff(existing, reconcile); diff != "" {
if diff := ManifestDiff(&original, reconcile); diff != "" {
klog.V(2).Infof("Updating SCC %s/%s due to diff: %v", required.Namespace, required.Name, diff)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
} else {
klog.V(2).Infof("Updating SCC %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name)
Expand Down
8 changes: 4 additions & 4 deletions pkg/cvo/internal/generic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,10 @@ func applyUnstructured(ctx context.Context, client dynamic.ResourceInterface, re
return nil, false, nil
}

skipKeys := sets.New[string]("apiVersion", "kind", "metadata", "status")
skipKeys := sets.New("apiVersion", "kind", "metadata", "status")

// create a copy of required, but copy skipKeys from existing
// this would copy skipKeys data into expected from existing
// create a copy of required, but copy skipKeys -- the parts that don't matter if they differ -- from existing
// so that our comparison only triggers on differences in the remaining fields
expected := required.DeepCopy()
for k, v := range existing.Object {
if skipKeys.Has(k) {
Expand All @@ -90,7 +90,7 @@ func applyUnstructured(ctx context.Context, client dynamic.ResourceInterface, re

objDiff := cmp.Diff(expected, existing)
if objDiff == "" {
// Skip update, as no changes found
// Skip update, as no relevant changes found
return existing, false, nil
}

Expand Down
Loading