From 05e1af7fbaaeb5e7fbe2c502d3a29bfdea3b51d9 Mon Sep 17 00:00:00 2001 From: Arjun Naik Date: Tue, 13 Jul 2021 13:09:03 +0200 Subject: [PATCH] Log resource diffs on update only in reconcile mode Logging of resource diffs was introduced to detect hotlooping in the CVO[1]. But this increases the verbosity of logging during updates when the resources have changed which is not useful. Hence logging of the diffs is more useful in the reconcile mode. Also the diff is generated with ObjectDiff[2] which is deprecated. The ObjectDiff function now just uses cmp.Diff[3] internally and all instances of ObjectDiff have been replaced with cmp.Diff. [1] - https://github.com/openshift/cluster-version-operator/pull/561 [2] - https://github.com/kubernetes/apimachinery/blob/235edae7dd90601011bbe3bcd6f84f7dc857b034/pkg/util/diff/diff.go#L57 [3] - https://pkg.go.dev/github.com/google/go-cmp/cmp#Diff --- lib/resourceapply/apiext.go | 8 +-- lib/resourceapply/apps.go | 68 +++----------------------- lib/resourceapply/batch.go | 8 +-- lib/resourceapply/core.go | 26 ++++++---- lib/resourceapply/cv.go | 30 +----------- lib/resourceapply/imagestream.go | 7 ++- lib/resourceapply/rbac.go | 26 ++++++---- lib/resourceapply/security.go | 8 +-- lib/resourcebuilder/podspec_test.go | 7 ++- lib/resourcebuilder/resourcebuilder.go | 31 ++++++------ pkg/cvo/internal/generic.go | 15 ++++-- pkg/cvo/internal/generic_test.go | 10 +--- 12 files changed, 95 insertions(+), 149 deletions(-) diff --git a/lib/resourceapply/apiext.go b/lib/resourceapply/apiext.go index 1649c488e..93e0068df 100644 --- a/lib/resourceapply/apiext.go +++ b/lib/resourceapply/apiext.go @@ -3,17 +3,17 @@ package resourceapply import ( "context" + "github.com/google/go-cmp/cmp" "github.com/openshift/cluster-version-operator/lib/resourcemerge" 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" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/diff" "k8s.io/klog/v2" "k8s.io/utils/pointer" ) -func ApplyCustomResourceDefinitionv1(ctx context.Context, client apiextclientv1.CustomResourceDefinitionsGetter, required *apiextv1.CustomResourceDefinition) (*apiextv1.CustomResourceDefinition, bool, error) { +func ApplyCustomResourceDefinitionv1(ctx context.Context, client apiextclientv1.CustomResourceDefinitionsGetter, required *apiextv1.CustomResourceDefinition, reconciling bool) (*apiextv1.CustomResourceDefinition, bool, error) { existing, err := client.CustomResourceDefinitions().Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("CRD %s not found, creating", required.Name) @@ -34,7 +34,9 @@ func ApplyCustomResourceDefinitionv1(ctx context.Context, client apiextclientv1. return existing, false, nil } - klog.V(2).Infof("Updating CRD %s due to diff: %v", required.Name, diff.ObjectDiff(existing, required)) + if reconciling { + klog.V(4).Infof("Updating CRD %s due to diff: %v", required.Name, cmp.Diff(existing, required)) + } actual, err := client.CustomResourceDefinitions().Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err diff --git a/lib/resourceapply/apps.go b/lib/resourceapply/apps.go index b337f463b..a4ec9dc45 100644 --- a/lib/resourceapply/apps.go +++ b/lib/resourceapply/apps.go @@ -3,19 +3,18 @@ package resourceapply import ( "context" + "github.com/google/go-cmp/cmp" "github.com/openshift/cluster-version-operator/lib/resourcemerge" appsv1 "k8s.io/api/apps/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/diff" appsclientv1 "k8s.io/client-go/kubernetes/typed/apps/v1" - appslisterv1 "k8s.io/client-go/listers/apps/v1" "k8s.io/klog/v2" "k8s.io/utils/pointer" ) // ApplyDeploymentv1 applies the required deployment to the cluster. -func ApplyDeploymentv1(ctx context.Context, client appsclientv1.DeploymentsGetter, required *appsv1.Deployment) (*appsv1.Deployment, bool, error) { +func ApplyDeploymentv1(ctx context.Context, client appsclientv1.DeploymentsGetter, required *appsv1.Deployment, reconciling bool) (*appsv1.Deployment, bool, error) { existing, err := client.Deployments(required.Namespace).Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("Deployment %s/%s not found, creating", required.Namespace, required.Name) @@ -35,42 +34,16 @@ func ApplyDeploymentv1(ctx context.Context, client appsclientv1.DeploymentsGette if !*modified { return existing, false, nil } - klog.V(2).Infof("Updating Deployment %s/%s due to diff: %v", required.Namespace, required.Name, diff.ObjectDiff(existing, required)) - - actual, err := client.Deployments(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) - return actual, true, err -} - -// ApplyDeploymentFromCache applies the required deployment to the cluster. -func ApplyDeploymentFromCache(ctx context.Context, lister appslisterv1.DeploymentLister, client appsclientv1.DeploymentsGetter, required *appsv1.Deployment) (*appsv1.Deployment, bool, error) { - existing, err := lister.Deployments(required.Namespace).Get(required.Name) - if apierrors.IsNotFound(err) { - klog.V(2).Infof("Deployment %s/%s not found, creating", required.Namespace, required.Name) - actual, err := client.Deployments(required.Namespace).Create(ctx, required, metav1.CreateOptions{}) - return actual, true, err - } - if err != nil { - return nil, false, err - } - // if we only create this resource, we have no need to continue further - if IsCreateOnly(required) { - return nil, false, nil + if reconciling { + klog.V(4).Infof("Updating Deployment %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) } - existing = existing.DeepCopy() - modified := pointer.BoolPtr(false) - resourcemerge.EnsureDeployment(modified, existing, *required) - if !*modified { - return existing, false, nil - } - klog.V(2).Infof("Updating Deployment %s/%s due to diff: %v", required.Namespace, required.Name, diff.ObjectDiff(existing, required)) - actual, err := client.Deployments(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err } // ApplyDaemonSetv1 applies the required daemonset to the cluster. -func ApplyDaemonSetv1(ctx context.Context, client appsclientv1.DaemonSetsGetter, required *appsv1.DaemonSet) (*appsv1.DaemonSet, bool, error) { +func ApplyDaemonSetv1(ctx context.Context, client appsclientv1.DaemonSetsGetter, required *appsv1.DaemonSet, reconciling bool) (*appsv1.DaemonSet, bool, error) { existing, err := client.DaemonSets(required.Namespace).Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("DaemonSet %s/%s not found, creating", required.Namespace, required.Name) @@ -91,36 +64,9 @@ func ApplyDaemonSetv1(ctx context.Context, client appsclientv1.DaemonSetsGetter, return existing, false, nil } - klog.V(2).Infof("Updating DaemonSet %s/%s due to diff: %v", required.Namespace, required.Name, diff.ObjectDiff(existing, required)) - - actual, err := client.DaemonSets(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) - return actual, true, err -} - -// ApplyDaemonSetFromCache applies the required deployment to the cluster. -func ApplyDaemonSetFromCache(ctx context.Context, lister appslisterv1.DaemonSetLister, client appsclientv1.DaemonSetsGetter, required *appsv1.DaemonSet) (*appsv1.DaemonSet, bool, error) { - existing, err := lister.DaemonSets(required.Namespace).Get(required.Name) - if apierrors.IsNotFound(err) { - klog.V(2).Infof("DaemonSet %s/%s not found, creating", required.Namespace, required.Name) - actual, err := client.DaemonSets(required.Namespace).Create(ctx, required, metav1.CreateOptions{}) - return actual, true, err - } - if err != nil { - return nil, false, err + if reconciling { + klog.V(4).Infof("Updating DaemonSet %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) } - // if we only create this resource, we have no need to continue further - if IsCreateOnly(required) { - return nil, false, nil - } - - existing = existing.DeepCopy() - modified := pointer.BoolPtr(false) - resourcemerge.EnsureDaemonSet(modified, existing, *required) - if !*modified { - return existing, false, nil - } - - klog.V(2).Infof("Updating DaemonSet %s/%s due to diff: %v", required.Namespace, required.Name, diff.ObjectDiff(existing, required)) actual, err := client.DaemonSets(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err diff --git a/lib/resourceapply/batch.go b/lib/resourceapply/batch.go index 964e34968..16d378eec 100644 --- a/lib/resourceapply/batch.go +++ b/lib/resourceapply/batch.go @@ -3,18 +3,18 @@ package resourceapply import ( "context" + "github.com/google/go-cmp/cmp" "github.com/openshift/cluster-version-operator/lib/resourcemerge" batchv1 "k8s.io/api/batch/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/diff" batchclientv1 "k8s.io/client-go/kubernetes/typed/batch/v1" "k8s.io/klog/v2" "k8s.io/utils/pointer" ) // ApplyJobv1 applies the required Job to the cluster. -func ApplyJobv1(ctx context.Context, client batchclientv1.JobsGetter, required *batchv1.Job) (*batchv1.Job, bool, error) { +func ApplyJobv1(ctx context.Context, client batchclientv1.JobsGetter, required *batchv1.Job, reconciling bool) (*batchv1.Job, bool, error) { existing, err := client.Jobs(required.Namespace).Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("Job %s/%s not found, creating", required.Namespace, required.Name) @@ -35,7 +35,9 @@ func ApplyJobv1(ctx context.Context, client batchclientv1.JobsGetter, required * return existing, false, nil } - klog.V(2).Infof("Updating Job %s/%s due to diff: %v", required.Namespace, required.Name, diff.ObjectDiff(existing, required)) + if reconciling { + klog.V(4).Infof("Updating Job %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + } actual, err := client.Jobs(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err diff --git a/lib/resourceapply/core.go b/lib/resourceapply/core.go index dc772e8b0..2ed75c31b 100644 --- a/lib/resourceapply/core.go +++ b/lib/resourceapply/core.go @@ -3,13 +3,13 @@ package resourceapply import ( "context" + "github.com/google/go-cmp/cmp" "k8s.io/klog/v2" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/diff" coreclientv1 "k8s.io/client-go/kubernetes/typed/core/v1" "k8s.io/utils/pointer" @@ -17,7 +17,7 @@ import ( ) // ApplyNamespacev1 merges objectmeta, does not worry about anything else -func ApplyNamespacev1(ctx context.Context, client coreclientv1.NamespacesGetter, required *corev1.Namespace) (*corev1.Namespace, bool, error) { +func ApplyNamespacev1(ctx context.Context, client coreclientv1.NamespacesGetter, required *corev1.Namespace, reconciling bool) (*corev1.Namespace, bool, error) { existing, err := client.Namespaces().Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("Namespace %s not found, creating", required.Name) @@ -37,7 +37,9 @@ func ApplyNamespacev1(ctx context.Context, client coreclientv1.NamespacesGetter, if !*modified { return existing, false, nil } - klog.V(2).Infof("Updating Namespace %s due to diff: %v", required.Name, diff.ObjectDiff(existing, required)) + if reconciling { + klog.V(4).Infof("Updating Namespace %s due to diff: %v", required.Name, cmp.Diff(existing, required)) + } actual, err := client.Namespaces().Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err @@ -46,7 +48,7 @@ func ApplyNamespacev1(ctx context.Context, client coreclientv1.NamespacesGetter, // ApplyServicev1 merges objectmeta and requires // TODO, since this cannot determine whether changes are due to legitimate actors (api server) or illegitimate ones (users), we cannot update // TODO I've special cased the selector for now -func ApplyServicev1(ctx context.Context, client coreclientv1.ServicesGetter, required *corev1.Service) (*corev1.Service, bool, error) { +func ApplyServicev1(ctx context.Context, client coreclientv1.ServicesGetter, required *corev1.Service, reconciling bool) (*corev1.Service, bool, error) { existing, err := client.Services(required.Namespace).Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("Service %s/%s not found, creating", required.Namespace, required.Name) @@ -72,14 +74,16 @@ func ApplyServicev1(ctx context.Context, client coreclientv1.ServicesGetter, req } existing.Spec.Selector = required.Spec.Selector - klog.V(2).Infof("Updating Service %s/%s due to diff: %v", required.Namespace, required.Name, diff.ObjectDiff(existing, required)) + if reconciling { + klog.V(4).Infof("Updating Service %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + } actual, err := client.Services(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err } // ApplyServiceAccountv1 applies the required serviceaccount to the cluster. -func ApplyServiceAccountv1(ctx context.Context, client coreclientv1.ServiceAccountsGetter, required *corev1.ServiceAccount) (*corev1.ServiceAccount, bool, error) { +func ApplyServiceAccountv1(ctx context.Context, client coreclientv1.ServiceAccountsGetter, required *corev1.ServiceAccount, reconciling bool) (*corev1.ServiceAccount, bool, error) { existing, err := client.ServiceAccounts(required.Namespace).Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("ServiceAccount %s/%s not found, creating", required.Namespace, required.Name) @@ -100,14 +104,16 @@ func ApplyServiceAccountv1(ctx context.Context, client coreclientv1.ServiceAccou return existing, false, nil } - klog.V(2).Infof("Updating ServiceAccount %s/%s due to diff: %v", required.Namespace, required.Name, diff.ObjectDiff(existing, required)) + if reconciling { + klog.V(4).Infof("Updating ServiceAccount %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + } actual, err := client.ServiceAccounts(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err } // ApplyConfigMapv1 applies the required serviceaccount to the cluster. -func ApplyConfigMapv1(ctx context.Context, client coreclientv1.ConfigMapsGetter, required *corev1.ConfigMap) (*corev1.ConfigMap, bool, error) { +func ApplyConfigMapv1(ctx context.Context, client coreclientv1.ConfigMapsGetter, required *corev1.ConfigMap, reconciling bool) (*corev1.ConfigMap, bool, error) { existing, err := client.ConfigMaps(required.Namespace).Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("ConfigMap %s/%s not found, creating", required.Namespace, required.Name) @@ -128,7 +134,9 @@ func ApplyConfigMapv1(ctx context.Context, client coreclientv1.ConfigMapsGetter, return existing, false, nil } - klog.V(2).Infof("Updating ConfigMap %s/%s due to diff: %v", required.Namespace, required.Name, diff.ObjectDiff(existing, required)) + if reconciling { + klog.V(4).Infof("Updating ConfigMap %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + } actual, err := client.ConfigMaps(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err diff --git a/lib/resourceapply/cv.go b/lib/resourceapply/cv.go index 357d67e3f..8f4edb6af 100644 --- a/lib/resourceapply/cv.go +++ b/lib/resourceapply/cv.go @@ -3,43 +3,17 @@ package resourceapply import ( "context" + "github.com/google/go-cmp/cmp" configv1 "github.com/openshift/api/config/v1" configclientv1 "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1" configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" "github.com/openshift/cluster-version-operator/lib/resourcemerge" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/diff" "k8s.io/klog/v2" "k8s.io/utils/pointer" ) -func ApplyClusterVersionv1(ctx context.Context, client configclientv1.ClusterVersionsGetter, required *configv1.ClusterVersion) (*configv1.ClusterVersion, bool, error) { - existing, err := client.ClusterVersions().Get(ctx, required.Name, metav1.GetOptions{}) - if errors.IsNotFound(err) { - actual, err := client.ClusterVersions().Create(ctx, required, metav1.CreateOptions{}) - return actual, true, err - } - if err != nil { - return nil, false, err - } - // if we only create this resource, we have no need to continue further - if IsCreateOnly(required) { - return nil, false, nil - } - - modified := pointer.BoolPtr(false) - resourcemerge.EnsureClusterVersion(modified, existing, *required) - if !*modified { - return existing, false, nil - } - - klog.V(2).Infof("Updating ClusterVersion %s due to diff: %v", required.Name, diff.ObjectDiff(existing, required)) - - actual, err := client.ClusterVersions().Update(ctx, existing, metav1.UpdateOptions{}) - return actual, true, err -} - func ApplyClusterVersionFromCache(ctx context.Context, lister configlistersv1.ClusterVersionLister, client configclientv1.ClusterVersionsGetter, required *configv1.ClusterVersion) (*configv1.ClusterVersion, bool, error) { obj, err := lister.Get(required.Name) if errors.IsNotFound(err) { @@ -62,7 +36,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, diff.ObjectDiff(existing, required)) + klog.V(4).Infof("Updating ClusterVersion %s due to diff: %v", required.Name, cmp.Diff(existing, required)) actual, err := client.ClusterVersions().Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err diff --git a/lib/resourceapply/imagestream.go b/lib/resourceapply/imagestream.go index 732982ece..9a5113c07 100644 --- a/lib/resourceapply/imagestream.go +++ b/lib/resourceapply/imagestream.go @@ -3,15 +3,17 @@ package resourceapply import ( "context" + "github.com/google/go-cmp/cmp" imagev1 "github.com/openshift/api/image/v1" imageclientv1 "github.com/openshift/client-go/image/clientset/versioned/typed/image/v1" "github.com/openshift/cluster-version-operator/lib/resourcemerge" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/klog/v2" "k8s.io/utils/pointer" ) -func ApplyImageStreamv1(ctx context.Context, client imageclientv1.ImageStreamsGetter, required *imagev1.ImageStream) (*imagev1.ImageStream, bool, error) { +func ApplyImageStreamv1(ctx context.Context, client imageclientv1.ImageStreamsGetter, required *imagev1.ImageStream, reconciling bool) (*imagev1.ImageStream, bool, error) { existing, err := client.ImageStreams(required.Namespace).Get(ctx, required.Name, metav1.GetOptions{}) if errors.IsNotFound(err) { actual, err := client.ImageStreams(required.Namespace).Create(ctx, required, metav1.CreateOptions{}) @@ -31,6 +33,9 @@ func ApplyImageStreamv1(ctx context.Context, client imageclientv1.ImageStreamsGe return existing, false, nil } + if reconciling { + klog.V(4).Infof("Updating Namespace %s due to diff: %v", required.Name, cmp.Diff(existing, required)) + } actual, err := client.ImageStreams(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err } diff --git a/lib/resourceapply/rbac.go b/lib/resourceapply/rbac.go index 7b5c3bcd8..4f7fd184f 100644 --- a/lib/resourceapply/rbac.go +++ b/lib/resourceapply/rbac.go @@ -3,18 +3,18 @@ package resourceapply import ( "context" + "github.com/google/go-cmp/cmp" "github.com/openshift/cluster-version-operator/lib/resourcemerge" rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/diff" rbacclientv1 "k8s.io/client-go/kubernetes/typed/rbac/v1" "k8s.io/klog/v2" "k8s.io/utils/pointer" ) // ApplyClusterRoleBindingv1 applies the required clusterrolebinding to the cluster. -func ApplyClusterRoleBindingv1(ctx context.Context, client rbacclientv1.ClusterRoleBindingsGetter, required *rbacv1.ClusterRoleBinding) (*rbacv1.ClusterRoleBinding, bool, error) { +func ApplyClusterRoleBindingv1(ctx context.Context, client rbacclientv1.ClusterRoleBindingsGetter, required *rbacv1.ClusterRoleBinding, reconciling bool) (*rbacv1.ClusterRoleBinding, bool, error) { existing, err := client.ClusterRoleBindings().Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("ClusterRoleBinding %s not found, creating", required.Name) @@ -34,14 +34,16 @@ func ApplyClusterRoleBindingv1(ctx context.Context, client rbacclientv1.ClusterR if !*modified { return existing, false, nil } - klog.V(2).Infof("Updating ClusterRoleBinding %s due to diff: %v", required.Name, diff.ObjectDiff(existing, required)) + if reconciling { + klog.V(4).Infof("Updating ClusterRoleBinding %s due to diff: %v", required.Name, cmp.Diff(existing, required)) + } actual, err := client.ClusterRoleBindings().Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err } // ApplyClusterRolev1 applies the required clusterrole to the cluster. -func ApplyClusterRolev1(ctx context.Context, client rbacclientv1.ClusterRolesGetter, required *rbacv1.ClusterRole) (*rbacv1.ClusterRole, bool, error) { +func ApplyClusterRolev1(ctx context.Context, client rbacclientv1.ClusterRolesGetter, required *rbacv1.ClusterRole, reconciling bool) (*rbacv1.ClusterRole, bool, error) { existing, err := client.ClusterRoles().Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("ClusterRole %s not found, creating", required.Name) @@ -61,14 +63,16 @@ func ApplyClusterRolev1(ctx context.Context, client rbacclientv1.ClusterRolesGet if !*modified { return existing, false, nil } - klog.V(2).Infof("Updating ClusterRole %s due to diff: %v", required.Name, diff.ObjectDiff(existing, required)) + if reconciling { + klog.V(4).Infof("Updating ClusterRole %s due to diff: %v", required.Name, cmp.Diff(existing, required)) + } actual, err := client.ClusterRoles().Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err } // ApplyRoleBindingv1 applies the required clusterrolebinding to the cluster. -func ApplyRoleBindingv1(ctx context.Context, client rbacclientv1.RoleBindingsGetter, required *rbacv1.RoleBinding) (*rbacv1.RoleBinding, bool, error) { +func ApplyRoleBindingv1(ctx context.Context, client rbacclientv1.RoleBindingsGetter, required *rbacv1.RoleBinding, reconciling bool) (*rbacv1.RoleBinding, bool, error) { existing, err := client.RoleBindings(required.Namespace).Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("RoleBinding %s/%s not found, creating", required.Namespace, required.Name) @@ -88,14 +92,16 @@ func ApplyRoleBindingv1(ctx context.Context, client rbacclientv1.RoleBindingsGet if !*modified { return existing, false, nil } - klog.V(2).Infof("Updating RoleBinding %s/%s due to diff: %v", required.Namespace, required.Name, diff.ObjectDiff(existing, required)) + if reconciling { + klog.V(4).Infof("Updating RoleBinding %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + } actual, err := client.RoleBindings(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err } // ApplyRolev1 applies the required clusterrole to the cluster. -func ApplyRolev1(ctx context.Context, client rbacclientv1.RolesGetter, required *rbacv1.Role) (*rbacv1.Role, bool, error) { +func ApplyRolev1(ctx context.Context, client rbacclientv1.RolesGetter, required *rbacv1.Role, reconciling bool) (*rbacv1.Role, bool, error) { existing, err := client.Roles(required.Namespace).Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("Role %s/%s not found, creating", required.Namespace, required.Name) @@ -115,7 +121,9 @@ func ApplyRolev1(ctx context.Context, client rbacclientv1.RolesGetter, required if !*modified { return existing, false, nil } - klog.V(2).Infof("Updating Role %s/%s due to diff: %v", required.Namespace, required.Name, diff.ObjectDiff(existing, required)) + if reconciling { + klog.V(4).Infof("Updating Role %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + } actual, err := client.Roles(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err diff --git a/lib/resourceapply/security.go b/lib/resourceapply/security.go index 1ab9c44ac..c4b2b54cc 100644 --- a/lib/resourceapply/security.go +++ b/lib/resourceapply/security.go @@ -3,18 +3,18 @@ package resourceapply import ( "context" + "github.com/google/go-cmp/cmp" securityv1 "github.com/openshift/api/security/v1" securityclientv1 "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1" "github.com/openshift/cluster-version-operator/lib/resourcemerge" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/diff" "k8s.io/klog/v2" "k8s.io/utils/pointer" ) // ApplySecurityContextConstraintsv1 applies the required SecurityContextConstraints to the cluster. -func ApplySecurityContextConstraintsv1(ctx context.Context, client securityclientv1.SecurityContextConstraintsGetter, required *securityv1.SecurityContextConstraints) (*securityv1.SecurityContextConstraints, bool, error) { +func ApplySecurityContextConstraintsv1(ctx context.Context, client securityclientv1.SecurityContextConstraintsGetter, required *securityv1.SecurityContextConstraints, reconciling bool) (*securityv1.SecurityContextConstraints, bool, error) { existing, err := client.SecurityContextConstraints().Get(ctx, required.Name, metav1.GetOptions{}) if apierrors.IsNotFound(err) { klog.V(2).Infof("SCC %s not found, creating", required.Name) @@ -35,7 +35,9 @@ func ApplySecurityContextConstraintsv1(ctx context.Context, client securityclien return existing, false, nil } - klog.V(2).Infof("Updating SCC %s due to diff: %v", required.Name, diff.ObjectDiff(existing, required)) + if reconciling { + klog.V(4).Infof("Updating SCC %s due to diff: %v", required.Name, cmp.Diff(existing, required)) + } actual, err := client.SecurityContextConstraints().Update(ctx, existing, metav1.UpdateOptions{}) return actual, true, err diff --git a/lib/resourcebuilder/podspec_test.go b/lib/resourcebuilder/podspec_test.go index ea462ebaf..a3ce778ef 100644 --- a/lib/resourcebuilder/podspec_test.go +++ b/lib/resourcebuilder/podspec_test.go @@ -4,8 +4,7 @@ import ( "reflect" "testing" - "k8s.io/apimachinery/pkg/util/diff" - + "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" ) @@ -105,7 +104,7 @@ func TestUpdatePodSpecWithProxy(t *testing.T) { } if !reflect.DeepEqual(test.input, test.expected) { - t.Error(diff.ObjectDiff(test.input, test.expected)) + t.Error(cmp.Diff(test.input, test.expected)) } }) } @@ -264,7 +263,7 @@ func TestUpdatePodSpecWithInternalLoadBalancerKubeService(t *testing.T) { } if !reflect.DeepEqual(test.input, test.expected) { - t.Error(diff.ObjectDiff(test.input, test.expected)) + t.Error(cmp.Diff(test.input, test.expected)) } }) } diff --git a/lib/resourcebuilder/resourcebuilder.go b/lib/resourcebuilder/resourcebuilder.go index 29aad4906..f95e00e9a 100644 --- a/lib/resourcebuilder/resourcebuilder.go +++ b/lib/resourcebuilder/resourcebuilder.go @@ -75,7 +75,8 @@ func (b *builder) WithModifier(f MetaV1ObjectModifierFunc) Interface { func (b *builder) Do(ctx context.Context) error { obj := resourceread.ReadOrDie(b.raw) - updatingMode := (b.mode == UpdatingMode) + updatingMode := b.mode == UpdatingMode + reconcilingMode := b.mode == ReconcilingMode switch typedObject := obj.(type) { case *imagev1.ImageStream: @@ -86,7 +87,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplyImageStreamv1(ctx, b.imageClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyImageStreamv1(ctx, b.imageClientv1, typedObject, reconcilingMode); err != nil { return err } } @@ -98,7 +99,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplySecurityContextConstraintsv1(ctx, b.securityClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplySecurityContextConstraintsv1(ctx, b.securityClientv1, typedObject, reconcilingMode); err != nil { return err } } @@ -113,7 +114,7 @@ func (b *builder) Do(ctx context.Context) error { if err := b.modifyDaemonSet(ctx, typedObject); err != nil { return err } - if _, _, err := resourceapply.ApplyDaemonSetv1(ctx, b.appsClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyDaemonSetv1(ctx, b.appsClientv1, typedObject, reconcilingMode); err != nil { return err } return b.checkDaemonSetHealth(ctx, typedObject) @@ -129,7 +130,7 @@ func (b *builder) Do(ctx context.Context) error { if err := b.modifyDeployment(ctx, typedObject); err != nil { return err } - if _, _, err := resourceapply.ApplyDeploymentv1(ctx, b.appsClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyDeploymentv1(ctx, b.appsClientv1, typedObject, reconcilingMode); err != nil { return err } return b.checkDeploymentHealth(ctx, typedObject) @@ -142,7 +143,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplyJobv1(ctx, b.batchClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyJobv1(ctx, b.batchClientv1, typedObject, reconcilingMode); err != nil { return err } return b.checkJobHealth(ctx, typedObject) @@ -155,7 +156,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplyConfigMapv1(ctx, b.coreClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyConfigMapv1(ctx, b.coreClientv1, typedObject, reconcilingMode); err != nil { return err } } @@ -167,7 +168,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplyNamespacev1(ctx, b.coreClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyNamespacev1(ctx, b.coreClientv1, typedObject, reconcilingMode); err != nil { return err } } @@ -179,7 +180,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplyServicev1(ctx, b.coreClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyServicev1(ctx, b.coreClientv1, typedObject, reconcilingMode); err != nil { return err } } @@ -191,7 +192,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplyServiceAccountv1(ctx, b.coreClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyServiceAccountv1(ctx, b.coreClientv1, typedObject, reconcilingMode); err != nil { return err } } @@ -203,7 +204,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplyClusterRolev1(ctx, b.rbacClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyClusterRolev1(ctx, b.rbacClientv1, typedObject, reconcilingMode); err != nil { return err } } @@ -215,7 +216,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplyClusterRoleBindingv1(ctx, b.rbacClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyClusterRoleBindingv1(ctx, b.rbacClientv1, typedObject, reconcilingMode); err != nil { return err } } @@ -227,7 +228,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplyRolev1(ctx, b.rbacClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyRolev1(ctx, b.rbacClientv1, typedObject, reconcilingMode); err != nil { return err } } @@ -239,7 +240,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplyRoleBindingv1(ctx, b.rbacClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyRoleBindingv1(ctx, b.rbacClientv1, typedObject, reconcilingMode); err != nil { return err } } @@ -251,7 +252,7 @@ func (b *builder) Do(ctx context.Context) error { updatingMode); err != nil { return err } else if !deleteReq { - if _, _, err := resourceapply.ApplyCustomResourceDefinitionv1(ctx, b.apiextensionsClientv1, typedObject); err != nil { + if _, _, err := resourceapply.ApplyCustomResourceDefinitionv1(ctx, b.apiextensionsClientv1, typedObject, reconcilingMode); err != nil { return err } } diff --git a/pkg/cvo/internal/generic.go b/pkg/cvo/internal/generic.go index 40d145ab1..111f888f5 100644 --- a/pkg/cvo/internal/generic.go +++ b/pkg/cvo/internal/generic.go @@ -4,12 +4,12 @@ import ( "context" "fmt" + "github.com/google/go-cmp/cmp" "github.com/openshift/cluster-version-operator/lib/resourceapply" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/util/diff" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/dynamic" "k8s.io/klog/v2" @@ -55,7 +55,7 @@ func deleteUnstructured(ctx context.Context, client dynamic.ResourceInterface, r return true, nil } -func applyUnstructured(ctx context.Context, client dynamic.ResourceInterface, required *unstructured.Unstructured) (*unstructured.Unstructured, bool, error) { +func applyUnstructured(ctx context.Context, client dynamic.ResourceInterface, required *unstructured.Unstructured, reconciling bool) (*unstructured.Unstructured, bool, error) { if required.GetName() == "" { return nil, false, fmt.Errorf("invalid object: name cannot be empty") } @@ -84,7 +84,7 @@ func applyUnstructured(ctx context.Context, client dynamic.ResourceInterface, re } } - objDiff := diff.ObjectDiff(expected, existing) + objDiff := cmp.Diff(expected, existing) if objDiff == "" { // Skip update, as no changes found return existing, false, nil @@ -102,7 +102,9 @@ func applyUnstructured(ctx context.Context, client dynamic.ResourceInterface, re existing.SetLabels(required.GetLabels()) existing.SetOwnerReferences(required.GetOwnerReferences()) - klog.V(2).Infof("Updating %s %s/%s due to diff: %v", required.GetKind(), required.GetNamespace(), required.GetName(), objDiff) + if reconciling { + klog.V(4).Infof("Updating %s %s/%s due to diff: %v", required.GetKind(), required.GetNamespace(), required.GetName(), objDiff) + } actual, err := client.Update(ctx, existing, metav1.UpdateOptions{}) if err != nil { @@ -115,6 +117,7 @@ type genericBuilder struct { client dynamic.ResourceInterface raw []byte modifier resourcebuilder.MetaV1ObjectModifierFunc + mode resourcebuilder.Mode } // NewGenericBuilder returns an implementation of resourcebuilder.Interface that @@ -127,6 +130,7 @@ func NewGenericBuilder(client dynamic.ResourceInterface, m manifest.Manifest) (r } func (b *genericBuilder) WithMode(m resourcebuilder.Mode) resourcebuilder.Interface { + b.mode = m return b } @@ -136,6 +140,7 @@ func (b *genericBuilder) WithModifier(f resourcebuilder.MetaV1ObjectModifierFunc } func (b *genericBuilder) Do(ctx context.Context) error { + reconciling := b.mode == resourcebuilder.ReconcilingMode ud := readUnstructuredV1OrDie(b.raw) if b.modifier != nil { b.modifier(ud) @@ -145,7 +150,7 @@ func (b *genericBuilder) Do(ctx context.Context) error { if err != nil { return err } else if !deleteReq { - _, _, err = applyUnstructured(ctx, b.client, ud) + _, _, err = applyUnstructured(ctx, b.client, ud, reconciling) } return err } diff --git a/pkg/cvo/internal/generic_test.go b/pkg/cvo/internal/generic_test.go index 325743c86..1fd7c7e44 100644 --- a/pkg/cvo/internal/generic_test.go +++ b/pkg/cvo/internal/generic_test.go @@ -31,10 +31,7 @@ func TestCreateOnlyCreate(t *testing.T) { } fakeClient := fake.NewSimpleDynamicClient(runtime.NewScheme()) - _, modified, err := applyUnstructured( - ctx, - fakeClient.Resource(schema.GroupVersionResource{Group: "config.openshift.io", Version: "v1", Resource: "featuregates"}), - obj.(*unstructured.Unstructured)) + _, modified, err := applyUnstructured(ctx, fakeClient.Resource(schema.GroupVersionResource{Group: "config.openshift.io", Version: "v1", Resource: "featuregates"}), obj.(*unstructured.Unstructured), false) if err != nil { t.Fatal(err) } @@ -76,10 +73,7 @@ func TestCreateOnlyUpdate(t *testing.T) { } fakeClient := fake.NewSimpleDynamicClient(runtime.NewScheme(), existingObj) - _, modified, err := applyUnstructured( - ctx, - fakeClient.Resource(schema.GroupVersionResource{Group: "config.openshift.io", Version: "v1", Resource: "featuregates"}), - obj.(*unstructured.Unstructured)) + _, modified, err := applyUnstructured(ctx, fakeClient.Resource(schema.GroupVersionResource{Group: "config.openshift.io", Version: "v1", Resource: "featuregates"}), obj.(*unstructured.Unstructured), false) if err != nil { t.Fatal(err) }