From fa34e15e689dd0e1cd67592fbc20fa2d67a57331 Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Fri, 5 Mar 2021 19:42:33 -0500 Subject: [PATCH 1/2] Log diffs when reconciling resources This commit improves log output when reconciling managed resources to include the diff between the current and updated resources when updating resources. * pkg/operator/controller/canary/route.go (updateCanaryRoute): Log the diff between the current and updated resources when updating the route. * pkg/operator/controller/ingress/cluster-role.go (ensureClusterRole): Move logging of updates from here... (updateClusterRole): ...to here. Log diff. * pkg/operator/controller/ingress/dns.go (ensureWildcardDNSRecord): Move logging of updates from here... (updateDNSRecord): ...to here. Log diff. * pkg/operator/controller/ingress/load_balancer_service.go (ensureLoadBalancerService): Move logging of updates from here... (updateLoadBalancerService): ...to here. Log diff. * pkg/operator/controller/ingress/monitoring.go (ensureServiceMonitor): Move logging of updates from here... (updateServiceMonitor): ...to here. Log diff. * pkg/operator/controller/ingress/nodeport_service.go (ensureNodePortService): Move logging of updates from here... (updateNodePortService): ...to here. Log diff. * pkg/operator/controller/ingress/poddisruptionbudget.go (ensureRouterPodDisruptionBudget): Move logging of updates from here... (updateRouterPodDisruptionBudget): ...to here. Log diff. * pkg/operator/controller/ingress/rsyslog_configmap.go (ensureRsyslogConfigMap): Move logging of updates from here... (updateRouterPodDisruptionBudget): ...to here. Log diff. * pkg/operator/controller/ingress/serviceca_configmap.go (ensureServiceCAConfigMap): Move logging of updates from here... (updateServiceCAConfigMap): ...to here. Log diff. --- pkg/operator/controller/canary/route.go | 4 +++- pkg/operator/controller/ingress/cluster-role.go | 7 ++++++- pkg/operator/controller/ingress/dns.go | 4 +++- pkg/operator/controller/ingress/load_balancer_service.go | 7 ++++++- pkg/operator/controller/ingress/monitoring.go | 7 ++++++- pkg/operator/controller/ingress/nodeport_service.go | 4 +++- pkg/operator/controller/ingress/poddisruptionbudget.go | 4 +++- pkg/operator/controller/ingress/rsyslog_configmap.go | 7 ++++++- pkg/operator/controller/ingress/serviceca_configmap.go | 7 ++++++- 9 files changed, 42 insertions(+), 9 deletions(-) diff --git a/pkg/operator/controller/canary/route.go b/pkg/operator/controller/canary/route.go index f5c698f950..806e81458a 100644 --- a/pkg/operator/controller/canary/route.go +++ b/pkg/operator/controller/canary/route.go @@ -75,10 +75,12 @@ func (r *reconciler) updateCanaryRoute(current, desired *routev1.Route) (bool, e return false, nil } + // Diff before updating because the client may mutate the object. + diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) if err := r.client.Update(context.TODO(), updated); err != nil { return false, fmt.Errorf("failed to update canary route %s/%s: %v", updated.Namespace, updated.Name, err) } - log.Info("updated canary route", "namespace", updated.Namespace, "name", updated.Name) + log.Info("updated canary route", "namespace", updated.Namespace, "name", updated.Name, "diff", diff) return true, nil } diff --git a/pkg/operator/controller/ingress/cluster-role.go b/pkg/operator/controller/ingress/cluster-role.go index a25f4e917c..888cef489e 100644 --- a/pkg/operator/controller/ingress/cluster-role.go +++ b/pkg/operator/controller/ingress/cluster-role.go @@ -5,6 +5,9 @@ import ( "fmt" "reflect" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" @@ -45,7 +48,6 @@ func (r *reconciler) ensureClusterRole() (bool, *rbacv1.ClusterRole, error) { if updated, err := r.updateClusterRole(current, desired); err != nil { return true, current, fmt.Errorf("failed to update cluster role: %v", err) } else if updated { - log.Info("updated cluster role", "desired", desired) return r.currentClusterRole() } } @@ -77,11 +79,14 @@ func (r *reconciler) updateClusterRole(current, desired *rbacv1.ClusterRole) (bo } updated := current.DeepCopy() updated.Rules = desired.Rules + // Diff before updating because the client may mutate the object. + diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) if err := r.client.Update(context.TODO(), updated); err != nil { if errors.IsAlreadyExists(err) { return false, nil } return false, err } + log.Info("updated cluster role", "namespace", updated.Namespace, "name", updated.Name, "diff", diff) return true, nil } diff --git a/pkg/operator/controller/ingress/dns.go b/pkg/operator/controller/ingress/dns.go index 62a0ce3b43..78e72691f6 100644 --- a/pkg/operator/controller/ingress/dns.go +++ b/pkg/operator/controller/ingress/dns.go @@ -51,7 +51,6 @@ func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, s if updated, err := r.updateDNSRecord(current, desired); err != nil { return true, current, fmt.Errorf("failed to update dnsrecord %s/%s: %v", desired.Namespace, desired.Name, err) } else if updated { - log.Info("updated dnsrecord", "dnsrecord", desired) return r.currentWildcardDNSRecord(ic) } } @@ -170,9 +169,12 @@ func (r *reconciler) updateDNSRecord(current, desired *iov1.DNSRecord) (bool, er return false, nil } + // Diff before updating because the client may mutate the object. + diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) if err := r.client.Update(context.TODO(), updated); err != nil { return false, err } + log.Info("updated dnsrecord", "namespace", updated.Namespace, "name", updated.Name, "diff", diff) return true, nil } diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index cc718b6930..0326af9e1e 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -4,6 +4,9 @@ import ( "context" "fmt" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + operatorv1 "github.com/openshift/api/operator/v1" "github.com/openshift/cluster-ingress-operator/pkg/manifests" "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" @@ -160,7 +163,6 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, if updated, err := r.updateLoadBalancerService(currentLBService, desiredLBService, platform); err != nil { return true, currentLBService, fmt.Errorf("failed to update load balancer service: %v", err) } else if updated { - log.Info("updated load balancer service", "namespace", desiredLBService.Namespace, "name", desiredLBService.Name) return r.currentLoadBalancerService(ci) } } @@ -299,9 +301,12 @@ func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service, if !changed { return false, nil } + // Diff before updating because the client may mutate the object. + diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) if err := r.client.Update(context.TODO(), updated); err != nil { return false, err } + log.Info("updated load balancer service", "namespace", updated.Namespace, "name", updated.Name, "diff", diff) return true, nil } diff --git a/pkg/operator/controller/ingress/monitoring.go b/pkg/operator/controller/ingress/monitoring.go index 18d5b0e6e4..93175d535b 100644 --- a/pkg/operator/controller/ingress/monitoring.go +++ b/pkg/operator/controller/ingress/monitoring.go @@ -5,6 +5,9 @@ import ( "fmt" "reflect" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" operatorv1 "github.com/openshift/api/operator/v1" @@ -41,7 +44,6 @@ func (r *reconciler) ensureServiceMonitor(ic *operatorv1.IngressController, svc if updated, err := r.updateServiceMonitor(current, desired); err != nil { return true, current, fmt.Errorf("failed to update servicemonitor %s/%s: %v", desired.GetNamespace(), desired.GetName(), err) } else if updated { - log.Info("updated servicemonitor", "namespace", desired.GetNamespace(), "name", desired.GetName()) return r.currentServiceMonitor(ic) } } @@ -131,9 +133,12 @@ func (r *reconciler) updateServiceMonitor(current, desired *unstructured.Unstruc return false, nil } + // Diff before updating because the client may mutate the object. + diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) if err := r.client.Update(context.TODO(), updated); err != nil { return false, err } + log.Info("updated servicemonitor", "namespace", updated.GetNamespace(), "name", updated.GetName(), "diff", diff) return true, nil } diff --git a/pkg/operator/controller/ingress/nodeport_service.go b/pkg/operator/controller/ingress/nodeport_service.go index 13413d8a94..30e7a619e6 100644 --- a/pkg/operator/controller/ingress/nodeport_service.go +++ b/pkg/operator/controller/ingress/nodeport_service.go @@ -53,7 +53,6 @@ func (r *reconciler) ensureNodePortService(ic *operatorv1.IngressController, dep if updated, err := r.updateNodePortService(current, desired); err != nil { return true, current, fmt.Errorf("failed to update NodePort service: %v", err) } else if updated { - log.Info("updated NodePort service", "service", desired) return r.currentNodePortService(ic) } } @@ -126,9 +125,12 @@ func (r *reconciler) updateNodePortService(current, desired *corev1.Service) (bo return false, nil } + // Diff before updating because the client may mutate the object. + diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) if err := r.client.Update(context.TODO(), updated); err != nil { return false, err } + log.Info("updated NodePort service", "namespace", updated.Namespace, "name", updated.Name, "diff", diff) return true, nil } diff --git a/pkg/operator/controller/ingress/poddisruptionbudget.go b/pkg/operator/controller/ingress/poddisruptionbudget.go index 975662b2cc..f75b4d9ebe 100644 --- a/pkg/operator/controller/ingress/poddisruptionbudget.go +++ b/pkg/operator/controller/ingress/poddisruptionbudget.go @@ -53,7 +53,6 @@ func (r *reconciler) ensureRouterPodDisruptionBudget(ic *operatorv1.IngressContr if updated, err := r.updateRouterPodDisruptionBudget(current, desired); err != nil { return true, current, fmt.Errorf("failed to update pod disruption budget: %v", err) } else if updated { - log.Info("updated pod disruption budget", "poddisruptionbudget", desired) return r.currentRouterPodDisruptionBudget(ic) } } @@ -115,9 +114,12 @@ func (r *reconciler) updateRouterPodDisruptionBudget(current, desired *policyv1b return false, nil } + // Diff before updating because the client may mutate the object. + diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) if err := r.client.Update(context.TODO(), updated); err != nil { return false, err } + log.Info("updated pod disruption budget", "namespace", updated.Namespace, "name", updated.Name, "diff", diff) return true, nil } diff --git a/pkg/operator/controller/ingress/rsyslog_configmap.go b/pkg/operator/controller/ingress/rsyslog_configmap.go index 41f0eddf61..b1f691f9b6 100644 --- a/pkg/operator/controller/ingress/rsyslog_configmap.go +++ b/pkg/operator/controller/ingress/rsyslog_configmap.go @@ -5,6 +5,9 @@ import ( "fmt" "reflect" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" @@ -60,7 +63,6 @@ func (r *reconciler) ensureRsyslogConfigMap(ic *operatorv1.IngressController, de if updated, err := r.updateRsyslogConfigMap(current, desired); err != nil { return true, current, fmt.Errorf("failed to update configmap: %v", err) } else if updated { - log.Info("updated configmap", "configmap", desired) return r.currentRsyslogConfigMap(ic) } } @@ -114,12 +116,15 @@ func (r *reconciler) updateRsyslogConfigMap(current, desired *corev1.ConfigMap) } updated := current.DeepCopy() updated.Data = desired.Data + // Diff before updating because the client may mutate the object. + diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) if err := r.client.Update(context.TODO(), updated); err != nil { if errors.IsAlreadyExists(err) { return false, nil } return false, err } + log.Info("updated configmap", "namespace", updated.Namespace, "name", updated.Name, "diff", diff) return true, nil } diff --git a/pkg/operator/controller/ingress/serviceca_configmap.go b/pkg/operator/controller/ingress/serviceca_configmap.go index dd451a1fa6..dc537825a1 100644 --- a/pkg/operator/controller/ingress/serviceca_configmap.go +++ b/pkg/operator/controller/ingress/serviceca_configmap.go @@ -4,6 +4,9 @@ import ( "context" "fmt" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" corev1 "k8s.io/api/core/v1" @@ -47,7 +50,6 @@ func (r *reconciler) ensureServiceCAConfigMap() (bool, *corev1.ConfigMap, error) if updated, err := r.updateServiceCAConfigMap(current, desired); err != nil { return true, current, fmt.Errorf("failed to update configmap: %v", err) } else if updated { - log.Info("updated configmap", "configmap", desired) return r.currentServiceCAConfigMap() } } @@ -101,11 +103,14 @@ func (r *reconciler) updateServiceCAConfigMap(current, desired *corev1.ConfigMap updated := current.DeepCopy() updated.Annotations["service.beta.openshift.io/inject-cabundle"] = "true" + // Diff before updating because the client may mutate the object. + diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) if err := r.client.Update(context.TODO(), updated); err != nil { if errors.IsAlreadyExists(err) { return false, nil } return false, err } + log.Info("updated configmap", "namespace", updated.Namespace, "name", updated.Name, "diff", diff) return true, nil } From c6cc79f54087f7b1fb8c5b0b77d5366537374ec8 Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Fri, 5 Mar 2021 19:50:24 -0500 Subject: [PATCH 2/2] nodePortServiceChanged: Fix for clusterIPs Ignore the clusterIPs field when comparing NodePort services to determine whether an update is required. Before this commit, the update logic would keep trying to revert the default value that the API set for the service's clusterIPs field. The clusterIPs field is new in Kubernetes 1.20 (OpenShift 4.7). This commit fixes bug 1936030. https://bugzilla.redhat.com/show_bug.cgi?id=1936030 * pkg/operator/controller/ingress/nodeport_service.go (nodePortServiceChanged): Ignore the service's clusterIPs field. * pkg/operator/controller/ingress/nodeport_service_test.go (TestNodePortServiceChanged): Verify that nodePortServiceChanged returns false if the only mutation is that the clusterIPs field's value has changed. --- pkg/operator/controller/ingress/nodeport_service.go | 2 +- pkg/operator/controller/ingress/nodeport_service_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/operator/controller/ingress/nodeport_service.go b/pkg/operator/controller/ingress/nodeport_service.go index 30e7a619e6..f85b4fca5b 100644 --- a/pkg/operator/controller/ingress/nodeport_service.go +++ b/pkg/operator/controller/ingress/nodeport_service.go @@ -141,7 +141,7 @@ func nodePortServiceChanged(current, expected *corev1.Service) (bool, *corev1.Se // Ignore fields that the API, other controllers, or user may // have modified. cmpopts.IgnoreFields(corev1.ServicePort{}, "NodePort"), - cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "ExternalIPs", "HealthCheckNodePort"), + cmpopts.IgnoreFields(corev1.ServiceSpec{}, "ClusterIP", "ClusterIPs", "ExternalIPs", "HealthCheckNodePort"), cmp.Comparer(cmpServiceAffinity), cmpopts.EquateEmpty(), } diff --git a/pkg/operator/controller/ingress/nodeport_service_test.go b/pkg/operator/controller/ingress/nodeport_service_test.go index 6448d8c881..ebdea019d8 100644 --- a/pkg/operator/controller/ingress/nodeport_service_test.go +++ b/pkg/operator/controller/ingress/nodeport_service_test.go @@ -75,6 +75,7 @@ func TestNodePortServiceChanged(t *testing.T) { description: "if .spec.clusterIP changes", mutate: func(svc *corev1.Service) { svc.Spec.ClusterIP = "2.3.4.5" + svc.Spec.ClusterIPs = []string{"2.3.4.5"} }, expect: false, },