Skip to content

🐛 Remove generated names from error messages to reduce reconciliation #5971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 25, 2022
Merged

🐛 Remove generated names from error messages to reduce reconciliation #5971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 45 additions & 19 deletions internal/controllers/topology/cluster/reconcile_state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package cluster
import (
"context"
"fmt"
"strings"

"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -184,27 +185,26 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope)

cpInfraRef, err := contract.ControlPlane().MachineTemplate().InfrastructureRef().Get(s.Desired.ControlPlane.Object)
if err != nil {
return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.InfrastructureMachineTemplate})
return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: s.Desired.ControlPlane.InfrastructureMachineTemplate})
}

// Create or update the MachineInfrastructureTemplate of the control plane.
err = r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
if err = r.reconcileReferencedTemplate(ctx, reconcileReferencedTemplateInput{
cluster: s.Current.Cluster,
ref: cpInfraRef,
current: s.Current.ControlPlane.InfrastructureMachineTemplate,
desired: s.Desired.ControlPlane.InfrastructureMachineTemplate,
compatibilityChecker: check.ObjectsAreCompatible,
templateNamePrefix: controlPlaneInfrastructureMachineTemplateNamePrefix(s.Current.Cluster.Name),
},
)
if err != nil {
return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.InfrastructureMachineTemplate})
); err != nil {
return err
}

// The controlPlaneObject.Spec.machineTemplate.infrastructureRef has to be updated in the desired object
err = contract.ControlPlane().MachineTemplate().InfrastructureRef().Set(s.Desired.ControlPlane.Object, refToUnstructured(cpInfraRef))
if err != nil {
return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object})
return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object})
}
}

Expand All @@ -227,7 +227,7 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope)
},
},
}); err != nil {
return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.Object})
return err
}

// If the ControlPlane has defined a current or desired MachineHealthCheck attempt to reconcile it.
Expand All @@ -242,7 +242,7 @@ func (r *Reconciler) reconcileControlPlane(ctx context.Context, s *scope.Scope)

// Reconcile the current and desired state of the MachineHealthCheck.
if err := r.reconcileMachineHealthCheck(ctx, s.Current.ControlPlane.MachineHealthCheck, s.Desired.ControlPlane.MachineHealthCheck); err != nil {
return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: s.Desired.ControlPlane.MachineHealthCheck})
return err
}
}
return nil
Expand Down Expand Up @@ -403,21 +403,21 @@ func (r *Reconciler) createMachineDeployment(ctx context.Context, cluster *clust
cluster: cluster,
desired: md.InfrastructureMachineTemplate,
}); err != nil {
return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: md.Object})
return errors.Wrapf(err, "failed to create %s", md.Object.Kind)
}

bootstrapCtx, _ := log.WithObject(md.BootstrapTemplate).Into(ctx)
if err := r.reconcileReferencedTemplate(bootstrapCtx, reconcileReferencedTemplateInput{
cluster: cluster,
desired: md.BootstrapTemplate,
}); err != nil {
return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: md.Object})
return errors.Wrapf(err, "failed to create %s", md.Object.Kind)
}

log = log.WithObject(md.Object)
log.Infof(fmt.Sprintf("Creating %s", tlog.KObj{Obj: md.Object}))
if err := r.Client.Create(ctx, md.Object.DeepCopy()); err != nil {
return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: md.Object})
return createErrorWithoutObjectName(err, md.Object)
}
r.recorder.Eventf(cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: md.Object})

Expand All @@ -429,7 +429,7 @@ func (r *Reconciler) createMachineDeployment(ctx context.Context, cluster *clust
*ownerReferenceTo(md.Object),
})
if err := r.reconcileMachineHealthCheck(ctx, nil, md.MachineHealthCheck); err != nil {
return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: md.MachineHealthCheck})
return err
}
}
return nil
Expand All @@ -448,7 +448,7 @@ func (r *Reconciler) updateMachineDeployment(ctx context.Context, cluster *clust
templateNamePrefix: infrastructureMachineTemplateNamePrefix(cluster.Name, mdTopologyName),
compatibilityChecker: check.ObjectsAreCompatible,
}); err != nil {
return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: currentMD.Object})
return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: currentMD.Object})
}

bootstrapCtx, _ := log.WithObject(desiredMD.BootstrapTemplate).Into(ctx)
Expand All @@ -460,13 +460,13 @@ func (r *Reconciler) updateMachineDeployment(ctx context.Context, cluster *clust
templateNamePrefix: bootstrapTemplateNamePrefix(cluster.Name, mdTopologyName),
compatibilityChecker: check.ObjectsAreInTheSameNamespace,
}); err != nil {
return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: currentMD.Object})
return errors.Wrapf(err, "failed to reconcile %s", tlog.KObj{Obj: currentMD.Object})
}

// Patch MachineHealthCheck for the MachineDeployment.
if desiredMD.MachineHealthCheck != nil || currentMD.MachineHealthCheck != nil {
if err := r.reconcileMachineHealthCheck(ctx, currentMD.MachineHealthCheck, desiredMD.MachineHealthCheck); err != nil {
return errors.Wrapf(err, "failed to update %s", tlog.KObj{Obj: desiredMD.MachineHealthCheck})
return err
}
}

Expand Down Expand Up @@ -525,7 +525,7 @@ func (r *Reconciler) deleteMachineDeployment(ctx context.Context, cluster *clust
// delete MachineHealthCheck for the MachineDeployment.
if md.MachineHealthCheck != nil {
if err := r.reconcileMachineHealthCheck(ctx, md.MachineHealthCheck, nil); err != nil {
return errors.Wrapf(err, "failed to delete %s", tlog.KObj{Obj: md.MachineHealthCheck})
return err
}
}
log.Infof("Deleting %s", tlog.KObj{Obj: md.Object})
Expand Down Expand Up @@ -587,7 +587,7 @@ func (r *Reconciler) reconcileReferencedObject(ctx context.Context, in reconcile
return errors.Wrapf(err, "failed to create a copy of %s with the managed field annotation", tlog.KObj{Obj: in.desired})
}
if err := r.Client.Create(ctx, desiredWithManagedFieldAnnotation); err != nil {
return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: desiredWithManagedFieldAnnotation})
return createErrorWithoutObjectName(err, desiredWithManagedFieldAnnotation)
}
r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: in.desired})
return nil
Expand Down Expand Up @@ -664,7 +664,7 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci
return errors.Wrapf(err, "failed to create a copy of %s with the managed field annotation", tlog.KObj{Obj: in.desired})
}
if err := r.Client.Create(ctx, desiredWithManagedFieldAnnotation); err != nil {
return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: desiredWithManagedFieldAnnotation})
return createErrorWithoutObjectName(err, desiredWithManagedFieldAnnotation)
}
r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q", tlog.KObj{Obj: in.desired})
return nil
Expand Down Expand Up @@ -716,7 +716,7 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci
return errors.Wrapf(err, "failed to create a copy of %s with the managed field annotation", tlog.KObj{Obj: in.desired})
}
if err := r.Client.Create(ctx, desiredWithManagedFieldAnnotation); err != nil {
return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: desiredWithManagedFieldAnnotation})
return createErrorWithoutObjectName(err, desiredWithManagedFieldAnnotation)
}
r.recorder.Eventf(in.cluster, corev1.EventTypeNormal, createEventReason, "Created %q as a replacement for %q (template rotation)", tlog.KObj{Obj: in.desired}, in.ref.Name)

Expand All @@ -727,3 +727,29 @@ func (r *Reconciler) reconcileReferencedTemplate(ctx context.Context, in reconci

return nil
}

// createErrorWithoutObjectName removes the name of the object from the error message. As each new Create call involves an
// object with a unique generated name each error appears to be a different error. As the errors are being surfaced in a condition
// on the Cluster, the name is removed here to prevent each creation error from triggering a new reconciliation.
func createErrorWithoutObjectName(err error, obj client.Object) error {
var statusError *apierrors.StatusError
if errors.As(err, &statusError) {
if statusError.Status().Details != nil {
var causes []string
for _, cause := range statusError.Status().Details.Causes {
causes = append(causes, fmt.Sprintf("%s: %s: %s", cause.Type, cause.Field, cause.Message))
}
var msg string
if len(causes) > 0 {
msg = fmt.Sprintf("failed to create %s.%s: %s", statusError.Status().Details.Kind, statusError.Status().Details.Group, strings.Join(causes, " "))
} else {
msg = fmt.Sprintf("failed to create %s.%s", statusError.Status().Details.Kind, statusError.Status().Details.Group)
}
// Replace the statusError message with the constructed message.
statusError.ErrStatus.Message = msg
return statusError
}
}
// If this isn't a StatusError return a more generic error with the object details.
return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: obj})
}
49 changes: 49 additions & 0 deletions internal/controllers/topology/cluster/reconcile_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package cluster
import (
"context"
"fmt"
"net/http"
"regexp"
"testing"
"time"
Expand Down Expand Up @@ -2081,3 +2082,51 @@ func TestReconciler_reconcileMachineHealthCheck(t *testing.T) {
})
}
}

func Test_createErrorWithoutObjectName(t *testing.T) {
originalError := &apierrors.StatusError{
ErrStatus: metav1.Status{
Status: metav1.StatusFailure,
Code: http.StatusUnprocessableEntity,
Reason: metav1.StatusReasonInvalid,
Message: "DockerMachineTemplate.infrastructure.cluster.x-k8s.io \"docker-template-one\" is invalid: spec.template.spec.preLoadImages: Invalid value: \"array\": spec.template.spec.preLoadImages in body must be of type string: \"array\"",
Details: &metav1.StatusDetails{
Group: "infrastructure.cluster.x-k8s.io",
Kind: "DockerMachineTemplate",
Name: "docker-template-one",
Causes: []metav1.StatusCause{
{
Type: "FieldValueInvalid",
Message: "Invalid value: \"array\": spec.template.spec.preLoadImages in body must be of type string: \"array\"",
Field: "spec.template.spec.preLoadImages",
},
},
},
}}
wantError := &apierrors.StatusError{
ErrStatus: metav1.Status{
Status: metav1.StatusFailure,
Code: http.StatusUnprocessableEntity,
Reason: metav1.StatusReasonInvalid,
// The only difference between the two objects should be in the Message section.
Message: "failed to create DockerMachineTemplate.infrastructure.cluster.x-k8s.io: FieldValueInvalid: spec.template.spec.preLoadImages: Invalid value: \"array\": spec.template.spec.preLoadImages in body must be of type string: \"array\"",
Details: &metav1.StatusDetails{
Group: "infrastructure.cluster.x-k8s.io",
Kind: "DockerMachineTemplate",
Name: "docker-template-one",
Causes: []metav1.StatusCause{
{
Type: "FieldValueInvalid",
Message: "Invalid value: \"array\": spec.template.spec.preLoadImages in body must be of type string: \"array\"",
Field: "spec.template.spec.preLoadImages",
},
},
},
},
}
t.Run("Transform a create error correctly", func(t *testing.T) {
g := NewWithT(t)
err := createErrorWithoutObjectName(originalError, nil)
g.Expect(err).To(Equal(wantError), cmp.Diff(err, wantError))
})
}