From 24e3a7e06aba2bcf5aebb4bf39f075eb6ca8165e Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Tue, 2 Jan 2024 19:08:26 +0100 Subject: [PATCH] Improve patch helper error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- .../controllers/kubeadmconfig_controller.go | 2 +- cmd/clusterctl/client/cluster/mover.go | 4 +- .../kubeadm/internal/control_plane.go | 4 +- .../internal/controllers/controller.go | 13 +++-- .../kubeadm/internal/controllers/helpers.go | 4 +- .../internal/controllers/remediation.go | 4 +- .../clusterresourceset_controller.go | 1 - .../clusterresourcesetbinding_controller.go | 2 +- .../machinepool_controller_noderef.go | 2 +- .../controllers/extensionconfig_controller.go | 8 +-- .../clusterclass/clusterclass_controller.go | 8 +-- .../machinehealthcheck_controller.go | 1 - .../machinehealthcheck_controller_test.go | 6 ++- .../machinehealthcheck_targets.go | 2 +- .../machineset/machineset_controller.go | 4 +- .../topology/cluster/cluster_controller.go | 2 +- .../machinedeployment_controller.go | 4 +- .../machineset/machineset_controller.go | 4 +- internal/hooks/tracking.go | 12 ++--- test/framework/autoscaler_helpers.go | 4 +- .../docker/internal/docker/machine.go | 4 +- .../controllers/inmemorycluster_controller.go | 6 +-- .../controllers/inmemorymachine_controller.go | 5 +- util/patch/patch.go | 54 +++++++++++-------- 24 files changed, 80 insertions(+), 80 deletions(-) diff --git a/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go b/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go index ee75f52c4db0..39138ce78f75 100644 --- a/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go +++ b/bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go @@ -226,7 +226,7 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{}) } if err := patchHelper.Patch(ctx, config, patchOpts...); err != nil { - rerr = kerrors.NewAggregate([]error{rerr, errors.Wrapf(err, "failed to patch %s", klog.KObj(config))}) + rerr = kerrors.NewAggregate([]error{rerr, err}) } }() diff --git a/cmd/clusterctl/client/cluster/mover.go b/cmd/clusterctl/client/cluster/mover.go index a93bf2f86f60..31e493171254 100644 --- a/cmd/clusterctl/client/cluster/mover.go +++ b/cmd/clusterctl/client/cluster/mover.go @@ -741,7 +741,7 @@ func pauseClusterClass(ctx context.Context, proxy Proxy, n *node, pause bool, mu patchHelper, err := patch.NewHelper(clusterClass, cFrom) if err != nil { - return errors.Wrapf(err, "error creating patcher for ClusterClass %s/%s", n.identity.Namespace, n.identity.Name) + return err } // Update the annotation to the desired state @@ -760,7 +760,7 @@ func pauseClusterClass(ctx context.Context, proxy Proxy, n *node, pause bool, mu // Update the ClusterClass with the new annotations. clusterClass.SetAnnotations(ccAnnotations) if err := patchHelper.Patch(ctx, clusterClass); err != nil { - return errors.Wrapf(err, "error patching ClusterClass %s/%s", n.identity.Namespace, n.identity.Name) + return err } return nil diff --git a/controlplane/kubeadm/internal/control_plane.go b/controlplane/kubeadm/internal/control_plane.go index 1a3297637932..376e3ceff05f 100644 --- a/controlplane/kubeadm/internal/control_plane.go +++ b/controlplane/kubeadm/internal/control_plane.go @@ -70,7 +70,7 @@ func NewControlPlane(ctx context.Context, managementCluster ManagementCluster, c for _, machine := range ownedMachines { patchHelper, err := patch.NewHelper(machine, client) if err != nil { - return nil, errors.Wrapf(err, "failed to create patch helper for machine %s", machine.Name) + return nil, err } patchHelpers[machine.Name] = patchHelper } @@ -271,7 +271,7 @@ func (c *ControlPlane) PatchMachines(ctx context.Context) error { controlplanev1.MachineEtcdPodHealthyCondition, controlplanev1.MachineEtcdMemberHealthyCondition, }}); err != nil { - errList = append(errList, errors.Wrapf(err, "failed to patch machine %s", machine.Name)) + errList = append(errList, err) } continue } diff --git a/controlplane/kubeadm/internal/controllers/controller.go b/controlplane/kubeadm/internal/controllers/controller.go index 91d9e806069a..d58125fc9972 100644 --- a/controlplane/kubeadm/internal/controllers/controller.go +++ b/controlplane/kubeadm/internal/controllers/controller.go @@ -179,8 +179,7 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl. // Patch ObservedGeneration only if the reconciliation completed successfully patchOpts := []patch.Option{patch.WithStatusObservedGeneration{}} if err := patchHelper.Patch(ctx, kcp, patchOpts...); err != nil { - log.Error(err, "Failed to patch KubeadmControlPlane to add finalizer") - return ctrl.Result{}, err + return ctrl.Result{}, errors.Wrapf(err, "failed to add finalizer") } return ctrl.Result{}, nil @@ -627,7 +626,7 @@ func (r *KubeadmControlPlaneReconciler) syncMachines(ctx context.Context, contro // TODO: This should be cleaned-up to have a more streamline way of constructing and using patchHelpers. patchHelper, err := patch.NewHelper(updatedMachine, r.Client) if err != nil { - return errors.Wrapf(err, "failed to create patch helper for Machine %s", klog.KObj(updatedMachine)) + return err } patchHelpers[machineName] = patchHelper @@ -812,7 +811,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileCertificateExpiries(ctx context log.V(2).Info(fmt.Sprintf("Setting certificate expiry to %s", expiry)) patchHelper, err := patch.NewHelper(kubeadmConfig, r.Client) if err != nil { - return errors.Wrapf(err, "failed to reconcile certificate expiry for Machine/%s: failed to create PatchHelper for KubeadmConfig/%s", m.Name, kubeadmConfig.Name) + return errors.Wrapf(err, "failed to reconcile certificate expiry for Machine/%s", m.Name) } if annotations == nil { @@ -822,7 +821,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileCertificateExpiries(ctx context kubeadmConfig.SetAnnotations(annotations) if err := patchHelper.Patch(ctx, kubeadmConfig); err != nil { - return errors.Wrapf(err, "failed to reconcile certificate expiry for Machine/%s: failed to patch KubeadmConfig/%s", m.Name, kubeadmConfig.Name) + return errors.Wrapf(err, "failed to reconcile certificate expiry for Machine/%s", m.Name) } } @@ -950,7 +949,7 @@ func (r *KubeadmControlPlaneReconciler) ensureCertificatesOwnerRef(ctx context.C patchHelper, err := patch.NewHelper(c.Secret, r.Client) if err != nil { - return errors.Wrapf(err, "failed to create patchHelper for Secret %s", klog.KObj(c.Secret)) + return err } controller := metav1.GetControllerOf(c.Secret) @@ -971,7 +970,7 @@ func (r *KubeadmControlPlaneReconciler) ensureCertificatesOwnerRef(ctx context.C c.Secret.SetOwnerReferences(util.EnsureOwnerRef(c.Secret.GetOwnerReferences(), owner)) } if err := patchHelper.Patch(ctx, c.Secret); err != nil { - return errors.Wrapf(err, "failed to patch Secret %s with ownerReference %s", klog.KObj(c.Secret), owner.String()) + return errors.Wrapf(err, "failed to set ownerReference") } } return nil diff --git a/controlplane/kubeadm/internal/controllers/helpers.go b/controlplane/kubeadm/internal/controllers/helpers.go index d03784ea2f38..4d74c70be1c1 100644 --- a/controlplane/kubeadm/internal/controllers/helpers.go +++ b/controlplane/kubeadm/internal/controllers/helpers.go @@ -104,11 +104,11 @@ func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context, func (r *KubeadmControlPlaneReconciler) adoptKubeconfigSecret(ctx context.Context, configSecret *corev1.Secret, kcp *controlplanev1.KubeadmControlPlane) (reterr error) { patchHelper, err := patch.NewHelper(configSecret, r.Client) if err != nil { - return errors.Wrap(err, "failed to create patch helper for the kubeconfig secret") + return err } defer func() { if err := patchHelper.Patch(ctx, configSecret); err != nil { - reterr = errors.Wrap(err, "failed to patch the kubeconfig secret") + reterr = kerrors.NewAggregate([]error{reterr, err}) } }() controller := metav1.GetControllerOf(configSecret) diff --git a/controlplane/kubeadm/internal/controllers/remediation.go b/controlplane/kubeadm/internal/controllers/remediation.go index d920c6481ffd..675dc7d988a7 100644 --- a/controlplane/kubeadm/internal/controllers/remediation.go +++ b/controlplane/kubeadm/internal/controllers/remediation.go @@ -55,7 +55,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileUnhealthyMachines(ctx context.C m.DeletionTimestamp.IsZero() { patchHelper, err := patch.NewHelper(m, r.Client) if err != nil { - errList = append(errList, errors.Wrapf(err, "failed to get PatchHelper for machine %s", m.Name)) + errList = append(errList, err) continue } @@ -64,7 +64,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileUnhealthyMachines(ctx context.C if err := patchHelper.Patch(ctx, m, patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{ clusterv1.MachineOwnerRemediatedCondition, }}); err != nil { - errList = append(errList, errors.Wrapf(err, "failed to patch machine %s", m.Name)) + errList = append(errList, err) } } } diff --git a/exp/addons/internal/controllers/clusterresourceset_controller.go b/exp/addons/internal/controllers/clusterresourceset_controller.go index 0eb41a6b4a5a..2713dba76864 100644 --- a/exp/addons/internal/controllers/clusterresourceset_controller.go +++ b/exp/addons/internal/controllers/clusterresourceset_controller.go @@ -212,7 +212,6 @@ func (r *ClusterResourceSetReconciler) reconcileDelete(ctx context.Context, clus log.Error(err, "failed to delete empty ClusterResourceSetBinding") } } else if err := patchHelper.Patch(ctx, clusterResourceSetBinding); err != nil { - log.Error(err, "failed to patch ClusterResourceSetBinding") return err } } diff --git a/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go b/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go index 3ebdb70d9725..93ac69079ca9 100644 --- a/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go +++ b/exp/addons/internal/controllers/clusterresourcesetbinding_controller.go @@ -123,7 +123,7 @@ func (r *ClusterResourceSetBindingReconciler) clusterToClusterResourceSetBinding func (r *ClusterResourceSetBindingReconciler) updateClusterReference(ctx context.Context, binding *addonsv1.ClusterResourceSetBinding) error { patchHelper, err := patch.NewHelper(binding, r.Client) if err != nil { - return errors.Wrap(err, "failed to configure the patch helper") + return err } // If the `.spec.clusterName` is not set, take the value from the ownerReference. diff --git a/exp/internal/controllers/machinepool_controller_noderef.go b/exp/internal/controllers/machinepool_controller_noderef.go index f46d1b77e718..15791b2cdb85 100644 --- a/exp/internal/controllers/machinepool_controller_noderef.go +++ b/exp/internal/controllers/machinepool_controller_noderef.go @@ -228,7 +228,7 @@ func (r *MachinePoolReconciler) patchNodes(ctx context.Context, c client.Client, // Patch the node if needed. if hasAnnotationChanges || hasTaintChanges { if err := patchHelper.Patch(ctx, node); err != nil { - log.V(2).Info("Failed patch node to set annotations and drop taints", "err", err, "node name", node.Name) + log.V(2).Info("Failed patch Node to set annotations and drop taints", "err", err, "node name", node.Name) return err } } diff --git a/exp/runtime/internal/controllers/extensionconfig_controller.go b/exp/runtime/internal/controllers/extensionconfig_controller.go index 95770ba053cc..ffbe2ab3d793 100644 --- a/exp/runtime/internal/controllers/extensionconfig_controller.go +++ b/exp/runtime/internal/controllers/extensionconfig_controller.go @@ -159,17 +159,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu func patchExtensionConfig(ctx context.Context, client client.Client, original, modified *runtimev1.ExtensionConfig, options ...patch.Option) error { patchHelper, err := patch.NewHelper(original, client) if err != nil { - return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: modified}) + return err } options = append(options, patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{ runtimev1.RuntimeExtensionDiscoveredCondition, }}) - err = patchHelper.Patch(ctx, modified, options...) - if err != nil { - return errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: modified}) - } - return nil + return patchHelper.Patch(ctx, modified, options...) } // reconcileDelete will remove the ExtensionConfig from the registry on deletion of the object. Note this is a best diff --git a/internal/controllers/clusterclass/clusterclass_controller.go b/internal/controllers/clusterclass/clusterclass_controller.go index a2823ae6a9fb..7dba27c739b9 100644 --- a/internal/controllers/clusterclass/clusterclass_controller.go +++ b/internal/controllers/clusterclass/clusterclass_controller.go @@ -114,7 +114,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re patchHelper, err := patch.NewHelper(clusterClass, r.Client) if err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: clusterClass}) + return ctrl.Result{}, err } defer func() { @@ -124,7 +124,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{}) } if err := patchHelper.Patch(ctx, clusterClass, patchOpts...); err != nil { - reterr = kerrors.NewAggregate([]error{reterr, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: clusterClass})}) + reterr = kerrors.NewAggregate([]error{reterr, err}) return } }() @@ -374,7 +374,7 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *cluste // Initialize the patch helper. patchHelper, err := patch.NewHelper(obj, r.Client) if err != nil { - return errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: obj}) + return err } // Set external object ControllerReference to the ClusterClass. @@ -384,7 +384,7 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *cluste // Patch the external object. if err := patchHelper.Patch(ctx, obj); err != nil { - return errors.Wrapf(err, "failed to patch object %s", tlog.KObj{Obj: obj}) + return err } return nil diff --git a/internal/controllers/machinehealthcheck/machinehealthcheck_controller.go b/internal/controllers/machinehealthcheck/machinehealthcheck_controller.go index c62411a07bc8..08879720d839 100644 --- a/internal/controllers/machinehealthcheck/machinehealthcheck_controller.go +++ b/internal/controllers/machinehealthcheck/machinehealthcheck_controller.go @@ -148,7 +148,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re // Initialize the patch helper patchHelper, err := patch.NewHelper(m, r.Client) if err != nil { - log.Error(err, "Failed to build patch helper") return ctrl.Result{}, err } diff --git a/internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go b/internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go index fab1d0e34077..93fa8332cab0 100644 --- a/internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go +++ b/internal/controllers/machinehealthcheck/machinehealthcheck_controller_test.go @@ -2607,7 +2607,8 @@ func TestPatchTargets(t *testing.T) { // To make the patch fail, create patchHelper with a different client. fakeMachine := machine1.DeepCopy() fakeMachine.Name = "fake" - patchHelper, _ := patch.NewHelper(fakeMachine, fake.NewClientBuilder().WithObjects(fakeMachine).Build()) + patchHelper, err := patch.NewHelper(fakeMachine, fake.NewClientBuilder().WithObjects(fakeMachine).Build()) + g.Expect(err).ToNot(HaveOccurred()) // healthCheckTarget with fake patchHelper, patch should fail on this target. target1 := healthCheckTarget{ MHC: mhc, @@ -2617,7 +2618,8 @@ func TestPatchTargets(t *testing.T) { } // healthCheckTarget with correct patchHelper. - patchHelper2, _ := patch.NewHelper(machine2, cl) + patchHelper2, err := patch.NewHelper(machine2, cl) + g.Expect(err).ToNot(HaveOccurred()) target3 := healthCheckTarget{ MHC: mhc, Machine: machine2, diff --git a/internal/controllers/machinehealthcheck/machinehealthcheck_targets.go b/internal/controllers/machinehealthcheck/machinehealthcheck_targets.go index 9b1ff909aa96..b9ee058279f1 100644 --- a/internal/controllers/machinehealthcheck/machinehealthcheck_targets.go +++ b/internal/controllers/machinehealthcheck/machinehealthcheck_targets.go @@ -212,7 +212,7 @@ func (r *Reconciler) getTargetsFromMHC(ctx context.Context, logger logr.Logger, patchHelper, err := patch.NewHelper(&machines[k], r.Client) if err != nil { - return nil, errors.Wrap(err, "unable to initialize patch helper") + return nil, err } target := healthCheckTarget{ Cluster: cluster, diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index e2b589749343..58682dba2c7c 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -995,12 +995,12 @@ func (r *Reconciler) reconcileUnhealthyMachines(ctx context.Context, cluster *cl for _, m := range machinesToRemediate { patchHelper, err := patch.NewHelper(m, r.Client) if err != nil { - errs = append(errs, errors.Wrapf(err, "failed to create patch helper for Machine %s", klog.KObj(m))) + errs = append(errs, err) continue } conditions.MarkFalse(m, clusterv1.MachineOwnerRemediatedCondition, clusterv1.WaitingForRemediationReason, clusterv1.ConditionSeverityWarning, preflightCheckErrMessage) if err := patchHelper.Patch(ctx, m); err != nil { - errs = append(errs, errors.Wrapf(err, "failed to patch Machine %s", klog.KObj(m))) + errs = append(errs, err) } } diff --git a/internal/controllers/topology/cluster/cluster_controller.go b/internal/controllers/topology/cluster/cluster_controller.go index 27101dd46d79..af564c089817 100644 --- a/internal/controllers/topology/cluster/cluster_controller.go +++ b/internal/controllers/topology/cluster/cluster_controller.go @@ -191,7 +191,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re patch.WithForceOverwriteConditions{}, } if err := patchHelper.Patch(ctx, cluster, options...); err != nil { - reterr = kerrors.NewAggregate([]error{reterr, errors.Wrap(err, "failed to patch cluster")}) + reterr = kerrors.NewAggregate([]error{reterr, err}) return } }() diff --git a/internal/controllers/topology/machinedeployment/machinedeployment_controller.go b/internal/controllers/topology/machinedeployment/machinedeployment_controller.go index 524c08ac4f4e..6ab485f4c745 100644 --- a/internal/controllers/topology/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/topology/machinedeployment/machinedeployment_controller.go @@ -116,11 +116,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re // Create a patch helper to add or remove the finalizer from the MachineDeployment. patchHelper, err := patch.NewHelper(md, r.Client) if err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: md}) + return ctrl.Result{}, err } defer func() { if err := patchHelper.Patch(ctx, md); err != nil { - reterr = kerrors.NewAggregate([]error{reterr, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: md})}) + reterr = kerrors.NewAggregate([]error{reterr, err}) } }() diff --git a/internal/controllers/topology/machineset/machineset_controller.go b/internal/controllers/topology/machineset/machineset_controller.go index a6476da44b2a..5437b9489415 100644 --- a/internal/controllers/topology/machineset/machineset_controller.go +++ b/internal/controllers/topology/machineset/machineset_controller.go @@ -123,11 +123,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re // Create a patch helper to add or remove the finalizer from the MachineSet. patchHelper, err := patch.NewHelper(ms, r.Client) if err != nil { - return ctrl.Result{}, errors.Wrapf(err, "failed to create patch helper for %s", tlog.KObj{Obj: ms}) + return ctrl.Result{}, err } defer func() { if err := patchHelper.Patch(ctx, ms); err != nil { - reterr = kerrors.NewAggregate([]error{reterr, errors.Wrapf(err, "failed to patch %s", tlog.KObj{Obj: ms})}) + reterr = kerrors.NewAggregate([]error{reterr, err}) } }() diff --git a/internal/hooks/tracking.go b/internal/hooks/tracking.go index 21358ffc0e66..d8d91b1b5b2b 100644 --- a/internal/hooks/tracking.go +++ b/internal/hooks/tracking.go @@ -41,7 +41,7 @@ func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hook patchHelper, err := patch.NewHelper(obj, c) if err != nil { - return errors.Wrapf(err, "failed to mark %q hook(s) as pending: failed to create patch helper for %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj}) + return errors.Wrapf(err, "failed to mark %q hook(s) as pending", strings.Join(hookNames, ",")) } // Read the annotation of the objects and add the hook to the comma separated list @@ -53,7 +53,7 @@ func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hook obj.SetAnnotations(annotations) if err := patchHelper.Patch(ctx, obj); err != nil { - return errors.Wrapf(err, "failed to mark %q hook(s) as pending: failed to patch %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj}) + return errors.Wrapf(err, "failed to mark %q hook(s) as pending", strings.Join(hookNames, ",")) } return nil @@ -80,7 +80,7 @@ func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks . patchHelper, err := patch.NewHelper(obj, c) if err != nil { - return errors.Wrapf(err, "failed to mark %q hook(s) as done: failed to create patch helper for %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj}) + return errors.Wrapf(err, "failed to mark %q hook(s) as done", strings.Join(hookNames, ",")) } // Read the annotation of the objects and add the hook to the comma separated list @@ -95,7 +95,7 @@ func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks . obj.SetAnnotations(annotations) if err := patchHelper.Patch(ctx, obj); err != nil { - return errors.Wrapf(err, "failed to mark %q hook(s) as done: failed to patch %s", strings.Join(hookNames, ","), tlog.KObj{Obj: obj}) + return errors.Wrapf(err, "failed to mark %q hook(s) as done", strings.Join(hookNames, ",")) } return nil @@ -117,7 +117,7 @@ func IsOkToDelete(obj client.Object) bool { func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object) error { patchHelper, err := patch.NewHelper(obj, c) if err != nil { - return errors.Wrapf(err, "failed to mark %s as ok to delete: failed to create patch helper", tlog.KObj{Obj: obj}) + return errors.Wrapf(err, "failed to mark %s as ok to delete", tlog.KObj{Obj: obj}) } annotations := obj.GetAnnotations() @@ -128,7 +128,7 @@ func MarkAsOkToDelete(ctx context.Context, c client.Client, obj client.Object) e obj.SetAnnotations(annotations) if err := patchHelper.Patch(ctx, obj); err != nil { - return errors.Wrapf(err, "failed to mark %s as ok to delete: failed to patch", tlog.KObj{Obj: obj}) + return errors.Wrapf(err, "failed to mark %s as ok to delete", tlog.KObj{Obj: obj}) } return nil diff --git a/test/framework/autoscaler_helpers.go b/test/framework/autoscaler_helpers.go index ba896e9c5c00..067e6504e7b1 100644 --- a/test/framework/autoscaler_helpers.go +++ b/test/framework/autoscaler_helpers.go @@ -277,7 +277,7 @@ func DisableAutoscalerForMachineDeploymentTopologyAndWait(ctx context.Context, i log.Logf("Dropping the %s and %s annotations from the MachineDeployments in ClusterTopology", clusterv1.AutoscalerMinSizeAnnotation, clusterv1.AutoscalerMaxSizeAnnotation) patchHelper, err := patch.NewHelper(input.Cluster, mgmtClient) - Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("failed to create patch helper for Cluster %s", klog.KObj(input.Cluster))) + Expect(err).ToNot(HaveOccurred()) for i := range input.Cluster.Spec.Topology.Workers.MachineDeployments { md := input.Cluster.Spec.Topology.Workers.MachineDeployments[i] delete(md.Metadata.Annotations, clusterv1.AutoscalerMinSizeAnnotation) @@ -321,7 +321,7 @@ func EnableAutoscalerForMachineDeploymentTopologyAndWait(ctx context.Context, in log.Logf("Add the %s and %s annotations to the MachineDeployments in ClusterTopology", clusterv1.AutoscalerMinSizeAnnotation, clusterv1.AutoscalerMaxSizeAnnotation) patchHelper, err := patch.NewHelper(input.Cluster, mgmtClient) - Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("failed to create patch helper for Cluster %s", klog.KObj(input.Cluster))) + Expect(err).ToNot(HaveOccurred()) for i := range input.Cluster.Spec.Topology.Workers.MachineDeployments { md := input.Cluster.Spec.Topology.Workers.MachineDeployments[i] if md.Metadata.Annotations == nil { diff --git a/test/infrastructure/docker/internal/docker/machine.go b/test/infrastructure/docker/internal/docker/machine.go index 96c9df5ed6d8..928fd881c9f8 100644 --- a/test/infrastructure/docker/internal/docker/machine.go +++ b/test/infrastructure/docker/internal/docker/machine.go @@ -441,7 +441,7 @@ func (m *Machine) SetNodeProviderID(ctx context.Context, c client.Client) error node.Spec.ProviderID = m.ProviderID() if err = patchHelper.Patch(ctx, node); err != nil { - return errors.Wrap(err, "failed update providerID") + return errors.Wrap(err, "failed to set providerID") } return nil @@ -509,7 +509,7 @@ func (m *Machine) CloudProviderNodePatch(ctx context.Context, c client.Client, d } if err = patchHelper.Patch(ctx, node); err != nil { - return errors.Wrap(err, "failed to patch node") + return err } return nil } diff --git a/test/infrastructure/inmemory/internal/controllers/inmemorycluster_controller.go b/test/infrastructure/inmemory/internal/controllers/inmemorycluster_controller.go index 62364e4ed918..aae65769c31b 100644 --- a/test/infrastructure/inmemory/internal/controllers/inmemorycluster_controller.go +++ b/test/infrastructure/inmemory/internal/controllers/inmemorycluster_controller.go @@ -23,6 +23,7 @@ import ( "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" + kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -99,10 +100,7 @@ func (r *InMemoryClusterReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Always attempt to Patch the InMemoryCluster object and status after each reconciliation. defer func() { if err := patchHelper.Patch(ctx, inMemoryCluster); err != nil { - log.Error(err, "failed to patch InMemoryCluster") - if rerr == nil { - rerr = err - } + rerr = kerrors.NewAggregate([]error{rerr, err}) } }() diff --git a/test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go b/test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go index 593e7737b5f3..f1624d32376c 100644 --- a/test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go +++ b/test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go @@ -162,10 +162,7 @@ func (r *InMemoryMachineReconciler) Reconcile(ctx context.Context, req ctrl.Requ conditions.WithStepCounterIf(inMemoryMachine.ObjectMeta.DeletionTimestamp.IsZero() && inMemoryMachine.Spec.ProviderID == nil), ) if err := patchHelper.Patch(ctx, inMemoryMachine, patch.WithOwnedConditions{Conditions: inMemoryMachineConditions}); err != nil { - log.Error(err, "failed to patch InMemoryMachine") - if rerr == nil { - rerr = err - } + rerr = kerrors.NewAggregate([]error{rerr, err}) } }() diff --git a/util/patch/patch.go b/util/patch/patch.go index dcf693495edd..3da4b4495b13 100644 --- a/util/patch/patch.go +++ b/util/patch/patch.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" @@ -53,20 +54,20 @@ type Helper struct { func NewHelper(obj client.Object, crClient client.Client) (*Helper, error) { // Return early if the object is nil. if util.IsNil(obj) { - return nil, errors.New("helper could not be created: object is nil") + return nil, errors.New("failed to create patch helper: object is nil") } // Get the GroupVersionKind of the object, // used to validate against later on. gvk, err := apiutil.GVKForObject(obj, crClient.Scheme()) if err != nil { - return nil, err + return nil, errors.Wrapf(err, "failed to create patch helper for object %s", klog.KObj(obj)) } // Convert the object to unstructured to compare against our before copy. unstructuredObj, err := toUnstructured(obj) if err != nil { - return nil, err + return nil, errors.Wrapf(err, "failed to create patch helper for %s %s: failed to convert object to Unstructured", gvk.Kind, klog.KObj(obj)) } // Check if the object satisfies the Cluster API conditions contract. @@ -85,16 +86,16 @@ func NewHelper(obj client.Object, crClient client.Client) (*Helper, error) { func (h *Helper) Patch(ctx context.Context, obj client.Object, opts ...Option) error { // Return early if the object is nil. if util.IsNil(obj) { - return errors.New("Patch could not be completed: object is nil") + return errors.Errorf("failed to patch %s %s: modified object is nil", h.gvk.Kind, klog.KObj(h.beforeObject)) } // Get the GroupVersionKind of the object that we want to patch. gvk, err := apiutil.GVKForObject(obj, h.client.Scheme()) if err != nil { - return err + return errors.Wrapf(err, "failed to patch %s %s", h.gvk.Kind, klog.KObj(h.beforeObject)) } if gvk != h.gvk { - return errors.Errorf("unmatched GroupVersionKind, expected %q got %q", h.gvk, gvk) + return errors.Wrapf(err, "failed to patch %s %s: unmatched GroupVersionKind, expected %q got %q", h.gvk.Kind, klog.KObj(h.beforeObject), h.gvk, gvk) } // Calculate the options. @@ -106,7 +107,7 @@ func (h *Helper) Patch(ctx context.Context, obj client.Object, opts ...Option) e // Convert the object to unstructured to compare against our before copy. h.after, err = toUnstructured(obj) if err != nil { - return err + return errors.Wrapf(err, "failed to patch %s %s: failed to convert object to Unstructured", h.gvk.Kind, klog.KObj(h.beforeObject)) } // Determine if the object has status. @@ -114,12 +115,12 @@ func (h *Helper) Patch(ctx context.Context, obj client.Object, opts ...Option) e if options.IncludeStatusObservedGeneration { // Set status.observedGeneration if we're asked to do so. if err := unstructured.SetNestedField(h.after.Object, h.after.GetGeneration(), "status", "observedGeneration"); err != nil { - return err + return errors.Wrapf(err, "failed to patch %s %s: failed to set .status.observedGeneration", h.gvk.Kind, klog.KObj(h.beforeObject)) } // Restore the changes back to the original object. if err := runtime.DefaultUnstructuredConverter.FromUnstructured(h.after.Object, obj); err != nil { - return err + return errors.Wrapf(err, "failed to patch %s %s: failed to converted object from Unstructured", h.gvk.Kind, klog.KObj(h.beforeObject)) } } } @@ -127,22 +128,31 @@ func (h *Helper) Patch(ctx context.Context, obj client.Object, opts ...Option) e // Calculate and store the top-level field changes (e.g. "metadata", "spec", "status") we have before/after. h.changes, err = h.calculateChanges(obj) if err != nil { - return err + return errors.Wrapf(err, "failed to patch %s %s", h.gvk.Kind, klog.KObj(h.beforeObject)) } // Issue patches and return errors in an aggregate. - return kerrors.NewAggregate([]error{ - // Patch the conditions first. - // - // Given that we pass in metadata.resourceVersion to perform a 3-way-merge conflict resolution, - // patching conditions first avoids an extra loop if spec or status patch succeeds first - // given that causes the resourceVersion to mutate. - h.patchStatusConditions(ctx, obj, options.ForceOverwriteConditions, options.OwnedConditions), - - // Then proceed to patch the rest of the object. - h.patch(ctx, obj), - h.patchStatus(ctx, obj), - }) + var errs []error + // Patch the conditions first. + // + // Given that we pass in metadata.resourceVersion to perform a 3-way-merge conflict resolution, + // patching conditions first avoids an extra loop if spec or status patch succeeds first + // given that causes the resourceVersion to mutate. + if err := h.patchStatusConditions(ctx, obj, options.ForceOverwriteConditions, options.OwnedConditions); err != nil { + errs = append(errs, err) + } + // Then proceed to patch the rest of the object. + if err := h.patch(ctx, obj); err != nil { + errs = append(errs, err) + } + if err := h.patchStatus(ctx, obj); err != nil { + errs = append(errs, err) + } + + if len(errs) > 0 { + return errors.Wrapf(kerrors.NewAggregate(errs), "failed to patch %s %s", h.gvk.Kind, klog.KObj(h.beforeObject)) + } + return nil } // patch issues a patch for metadata and spec.