From 6065c601ae69ca63f16d12d34c1b2657a1f0d23d Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Wed, 19 Oct 2022 18:47:12 +0200 Subject: [PATCH 1/3] resourceapply: improve diff logging for deployments Previously we logged the diff between `existing` and `required`, but `existing` already contains the result of the merge and therefore all relevant bits from `required` are already identical and will never be logged, so the logged diff is pretty much always just misleading. Instead, save the original existing structure and compare it to the result of the merge. Also, correctly handle the empty diff shown on coding errors in `EnsureDeployment` when there is no relevant difference but somehow `modified` still was set to `true`. --- lib/resourceapply/apps.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/resourceapply/apps.go b/lib/resourceapply/apps.go index 32fcd26eb..0a2ca3113 100644 --- a/lib/resourceapply/apps.go +++ b/lib/resourceapply/apps.go @@ -29,13 +29,19 @@ func ApplyDeploymentv1(ctx context.Context, client appsclientv1.DeploymentsGette return nil, false, nil } + var original appsv1.Deployment + existing.DeepCopyInto(&original) modified := pointer.BoolPtr(false) resourcemerge.EnsureDeployment(modified, existing, *required) if !*modified { return existing, false, nil } if reconciling { - klog.V(2).Infof("Updating Deployment %s/%s due to diff: %v", required.Namespace, required.Name, cmp.Diff(existing, required)) + if diff := cmp.Diff(&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) + } } actual, err := client.Deployments(required.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) From 934b81d80f3aedbc641d53c62d7f3b7e1e2090ba Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Wed, 19 Oct 2022 18:19:57 +0200 Subject: [PATCH 2/3] resourcemerge: fix `SeccompProfile` comparison `ensureSeccompProfile` was comparing a pointer to a struct and hence always considered the two `SeccompProfile`s to be different, set `modified` to true and triggered the update path for possibly unchanged `Deployments` Resolves: OCPBUGS-2592 --- lib/resourcemerge/core.go | 2 +- lib/resourcemerge/core_test.go | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/lib/resourcemerge/core.go b/lib/resourcemerge/core.go index bd24ee462..b5bc8274b 100644 --- a/lib/resourcemerge/core.go +++ b/lib/resourcemerge/core.go @@ -554,7 +554,7 @@ func ensureSeccompProfilePtr(modified *bool, existing **corev1.SeccompProfile, r } func ensureSeccompProfile(modified *bool, existing *corev1.SeccompProfile, required corev1.SeccompProfile) { - if equality.Semantic.DeepEqual(existing, required) { + if equality.Semantic.DeepEqual(*existing, required) { return } diff --git a/lib/resourcemerge/core_test.go b/lib/resourcemerge/core_test.go index 641ef144e..cb1366852 100644 --- a/lib/resourcemerge/core_test.go +++ b/lib/resourcemerge/core_test.go @@ -61,6 +61,28 @@ func TestEnsurePodSpec(t *testing.T) { expected: corev1.PodSpec{ SecurityContext: &corev1.PodSecurityContext{}}, }, + { + name: "PodSecurityContext no changes", + existing: corev1.PodSpec{ + SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(true), + RunAsUser: int64Ptr(int64(1234)), + RunAsGroup: int64Ptr(int64(4321)), + SeccompProfile: &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault}}, + }, + input: corev1.PodSpec{ + SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(true), + RunAsUser: int64Ptr(int64(1234)), + RunAsGroup: int64Ptr(int64(4321)), + SeccompProfile: &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault}}, + }, + expectedModified: false, + expected: corev1.PodSpec{ + SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(true), + RunAsUser: int64Ptr(int64(1234)), + RunAsGroup: int64Ptr(int64(4321)), + SeccompProfile: &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault}}, + }, + }, { name: "PodSecurityContext changes", existing: corev1.PodSpec{ From 60ea731848910a66c29397d97434da56f1497925 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Thu, 20 Oct 2022 14:38:03 +0200 Subject: [PATCH 3/3] resourcemerge: treat empty `PodSecurityContext` as nil For some reason the API/client sometimes returns a pointer to an empty `PodSecurityContext` struct instead of a `nil` pointer. When our desired state is `nil`, this leads to a NOP update and a hotloop. If the desired state is `nil`, do not modify the existing state if it is either `nil` or equal to an empty struct. --- lib/resourcemerge/core.go | 4 ++-- lib/resourcemerge/core_test.go | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/resourcemerge/core.go b/lib/resourcemerge/core.go index b5bc8274b..b6f4df761 100644 --- a/lib/resourcemerge/core.go +++ b/lib/resourcemerge/core.go @@ -646,13 +646,13 @@ func ensureAffinity(modified *bool, existing *corev1.Affinity, required corev1.A } func ensurePodSecurityContextPtr(modified *bool, existing **corev1.PodSecurityContext, required *corev1.PodSecurityContext) { - if *existing == nil && required == nil { + if (*existing == nil || equality.Semantic.DeepEqual(*existing, &corev1.PodSecurityContext{})) && required == nil { return } // Check if we can simply set to required. This can be done if existing is not set or it is set // but required is not set. - if *existing == nil || (required == nil && *existing != nil) { + if *existing == nil || required == nil { *modified = true *existing = required return diff --git a/lib/resourcemerge/core_test.go b/lib/resourcemerge/core_test.go index cb1366852..4ca897854 100644 --- a/lib/resourcemerge/core_test.go +++ b/lib/resourcemerge/core_test.go @@ -61,6 +61,15 @@ func TestEnsurePodSpec(t *testing.T) { expected: corev1.PodSpec{ SecurityContext: &corev1.PodSecurityContext{}}, }, + { + name: "Existing PodSecurityContext empty, desired nil => do not modify", + existing: corev1.PodSpec{ + SecurityContext: &corev1.PodSecurityContext{}}, + input: corev1.PodSpec{SecurityContext: nil}, + expectedModified: false, + expected: corev1.PodSpec{ + SecurityContext: &corev1.PodSecurityContext{}}, + }, { name: "PodSecurityContext no changes", existing: corev1.PodSpec{