chore: Use structured errors in logger (#2136)

Author: jonathan-innis

go.mod

@@ -5,7 +5,7 @@ go 1.24.2
 require (
 	github.com/Pallinder/go-randomdata v1.2.0
 	github.com/avast/retry-go v3.0.0+incompatible
-	github.com/awslabs/operatorpkg v0.0.0-20250320000002-b05af0f15c68
+	github.com/awslabs/operatorpkg v0.0.0-20250415205441-a0c5d2f39e72
 	github.com/docker/docker v28.0.4+incompatible
 	github.com/go-logr/logr v1.4.2
 	github.com/imdario/mergo v0.3.16

go.sum

@@ -2,8 +2,8 @@ github.com/Pallinder/go-randomdata v1.2.0 h1:DZ41wBchNRb/0GfsePLiSwb0PHZmT67XY00
 github.com/Pallinder/go-randomdata v1.2.0/go.mod h1:yHmJgulpD2Nfrm0cR9tI/+oAgRqCQQixsA8HyRZfV9Y=
 github.com/avast/retry-go v3.0.0+incompatible h1:4SOWQ7Qs+oroOTQOYnAHqelpCO0biHSxpiH9JdtuBj0=
 github.com/avast/retry-go v3.0.0+incompatible/go.mod h1:XtSnn+n/sHqQIpZ10K1qAevBhOOCWBLXXy3hyiqqBrY=
-github.com/awslabs/operatorpkg v0.0.0-20250320000002-b05af0f15c68 h1:llLoYu7EeqtFrCGCJzzXIyDxvCwn/Zr+aX+sRyabXgw=
-github.com/awslabs/operatorpkg v0.0.0-20250320000002-b05af0f15c68/go.mod h1:Uu2TsiIC3jUXRxMiDXOsiz3ZuBLTsCj1j4B858r51bs=
+github.com/awslabs/operatorpkg v0.0.0-20250415205441-a0c5d2f39e72 h1:L6TgkVk04OH465tCZsdcPBE69yiIOcMRHwRJz3kltqY=
+github.com/awslabs/operatorpkg v0.0.0-20250415205441-a0c5d2f39e72/go.mod h1:83w70kHuAH/QyhMnKzql71/fs3kSOiTJq+DU64xKq6M=
 github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
 github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
 github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs=

kwok/cloudprovider/cloudprovider.go

@@ -24,6 +24,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/awslabs/operatorpkg/serrors"
 	"github.com/awslabs/operatorpkg/status"
 	"github.com/docker/docker/pkg/namesgenerator"
 	"github.com/samber/lo"
@@ -160,7 +161,7 @@ func (c CloudProvider) getInstanceType(instanceTypeName string) (*cloudprovider.
 		return it.Name == instanceTypeName
 	})
 	if !found {
-		return nil, fmt.Errorf("unable to find instance type %q", instanceTypeName)
+		return nil, serrors.Wrap(fmt.Errorf("unable to find instance type"), "instance-type", instanceTypeName)
 	}
 	return it, nil
 }
@@ -184,7 +185,7 @@ func (c CloudProvider) toNode(nodeClaim *v1.NodeClaim) (*corev1.Node, error) {
 	for _, val := range req.Values {
 		it, err := c.getInstanceType(val)
 		if err != nil {
-			return nil, fmt.Errorf("instance type %s not found", val)
+			return nil, serrors.Wrap(fmt.Errorf("instance type not found"), "instance-type", val)
 		}
 
 		availableOfferings := it.Offerings.Available().Compatible(requirements)

pkg/apis/v1/labels.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/awslabs/operatorpkg/serrors"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -111,7 +112,7 @@ func IsRestrictedLabel(key string) error {
 		return nil
 	}
 	if IsRestrictedNodeLabel(key) {
-		return fmt.Errorf("label %s is restricted; specify a well known label: %v, or a custom label that does not use a restricted domain: %v", key, sets.List(WellKnownLabels), sets.List(RestrictedLabelDomains))
+		return serrors.Wrap(fmt.Errorf("label is restricted; specify a well known label or a custom label that does not use a restricted domain"), "label", key, "well-known-labels", sets.List(WellKnownLabels), "restricted-labels", sets.List(RestrictedLabelDomains))
 	}
 	return nil
 }

pkg/apis/v1/nodepool.go

@@ -21,6 +21,7 @@ import (
 	"math"
 	"strconv"
 
+	"github.com/awslabs/operatorpkg/serrors"
 	"github.com/mitchellh/hashstructure/v2"
 	"github.com/robfig/cron/v3"
 	"github.com/samber/lo"
@@ -146,7 +147,7 @@ func (l Limits) ExceededBy(resources v1.ResourceList) error {
 	for resourceName, usage := range resources {
 		if limit, ok := l[resourceName]; ok {
 			if usage.Cmp(limit) > 0 {
-				return fmt.Errorf("%s resource usage of %v exceeds limit of %v", resourceName, usage.AsDec(), limit.AsDec())
+				return serrors.Wrap(fmt.Errorf("resource usage exceeds limit"), "resource-name", resourceName, "usage", usage.AsDec(), "limit", limit.AsDec())
 			}
 		}
 	}
@@ -358,7 +359,7 @@ func (in *Budget) IsActive(c clock.Clock) (bool, error) {
 	if err != nil {
 		// Should only occur if there's a discrepancy
 		// with the validation regex and the cron package.
-		return false, fmt.Errorf("invariant violated, invalid cron %s", schedule)
+		return false, serrors.Wrap(fmt.Errorf("invariant violated, invalid cron"), "cron", schedule)
 	}
 	// Walk back in time for the duration associated with the schedule
 	checkPoint := c.Now().UTC().Add(-lo.FromPtr(in.Duration).Duration)

pkg/cloudprovider/fake/cloudprovider.go

@@ -24,6 +24,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/awslabs/operatorpkg/serrors"
 	"github.com/awslabs/operatorpkg/status"
 	"github.com/samber/lo"
 	corev1 "k8s.io/api/core/v1"
@@ -204,7 +205,7 @@ func (c *CloudProvider) Get(_ context.Context, id string) (*v1.NodeClaim, error)
 	if nodeClaim, ok := c.CreatedNodeClaims[id]; ok {
 		return nodeClaim.DeepCopy(), nil
 	}
-	return nil, cloudprovider.NewNodeClaimNotFoundError(fmt.Errorf("no nodeclaim exists with id '%s'", id))
+	return nil, cloudprovider.NewNodeClaimNotFoundError(serrors.Wrap(fmt.Errorf("no nodeclaim exists with id"), "id", id))
 }
 
 func (c *CloudProvider) List(_ context.Context) ([]*v1.NodeClaim, error) {
@@ -284,7 +285,7 @@ func (c *CloudProvider) Delete(_ context.Context, nc *v1.NodeClaim) error {
 		delete(c.CreatedNodeClaims, nc.Status.ProviderID)
 		return nil
 	}
-	return cloudprovider.NewNodeClaimNotFoundError(fmt.Errorf("no nodeclaim exists with provider id '%s'", nc.Status.ProviderID))
+	return cloudprovider.NewNodeClaimNotFoundError(serrors.Wrap(fmt.Errorf("no nodeclaim exists with provider id"), "provider-id", nc.Status.ProviderID))
 }
 
 func (c *CloudProvider) IsDrifted(context.Context, *v1.NodeClaim) (cloudprovider.DriftReason, error) {

pkg/cloudprovider/types.go

@@ -25,6 +25,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/awslabs/operatorpkg/serrors"
 	"github.com/awslabs/operatorpkg/status"
 	"github.com/samber/lo"
 	corev1 "k8s.io/api/core/v1"
@@ -214,7 +215,7 @@ func (its InstanceTypes) SatisfiesMinValues(requirements scheduling.Requirements
 		}
 	}
 	if incompatibleKey != "" {
-		return len(its), fmt.Errorf("minValues requirement is not met for %q", incompatibleKey)
+		return len(its), serrors.Wrap(fmt.Errorf("minValues requirement is not met for label"), "label", incompatibleKey)
 	}
 	return len(its), nil
 }

pkg/controllers/disruption/consolidation.go

@@ -23,6 +23,7 @@ import (
 	"sort"
 	"time"
 
+	"github.com/awslabs/operatorpkg/serrors"
 	"github.com/samber/lo"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/utils/clock"
@@ -318,7 +319,7 @@ func getCandidatePrices(candidates []*Candidate) (float64, error) {
 			if reqs.Get(v1.CapacityTypeLabelKey).Has(v1.CapacityTypeReserved) {
 				return 0.0, nil
 			}
-			return 0.0, fmt.Errorf("unable to determine offering for %s/%s/%s", c.instanceType.Name, c.capacityType, c.zone)
+			return 0.0, serrors.Wrap(fmt.Errorf("unable to determine offering"), "instance-type", c.instanceType.Name, "capacity-type", c.capacityType, "zone", c.zone)
 		}
 		price += compatibleOfferings.Cheapest().Price
 	}

pkg/controllers/disruption/controller.go

@@ -25,6 +25,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/awslabs/operatorpkg/serrors"
 	"github.com/awslabs/operatorpkg/singleton"
 	"github.com/samber/lo"
 	"go.uber.org/multierr"
@@ -131,13 +132,13 @@ func (c *Controller) Reconcile(ctx context.Context) (reconcile.Result, error) {
 		if errors.IsConflict(err) {
 			return reconcile.Result{Requeue: true}, nil
 		}
-		return reconcile.Result{}, fmt.Errorf("removing taint %s from nodes, %w", pretty.Taint(v1.DisruptedNoScheduleTaint), err)
+		return reconcile.Result{}, serrors.Wrap(fmt.Errorf("removing taint from nodes, %w", err), "taint", pretty.Taint(v1.DisruptedNoScheduleTaint))
 	}
 	if err := state.ClearNodeClaimsCondition(ctx, c.kubeClient, v1.ConditionTypeDisruptionReason, outdatedNodes...); err != nil {
 		if errors.IsConflict(err) {
 			return reconcile.Result{Requeue: true}, nil
 		}
-		return reconcile.Result{}, fmt.Errorf("removing %s condition from nodeclaims, %w", v1.ConditionTypeDisruptionReason, err)
+		return reconcile.Result{}, serrors.Wrap(fmt.Errorf("removing condition from nodeclaims, %w", err), "condition", v1.ConditionTypeDisruptionReason)
 	}
 
 	// Attempt different disruption methods. We'll only let one method perform an action
@@ -148,7 +149,7 @@ func (c *Controller) Reconcile(ctx context.Context) (reconcile.Result, error) {
 			if errors.IsConflict(err) {
 				return reconcile.Result{Requeue: true}, nil
 			}
-			return reconcile.Result{}, fmt.Errorf("disrupting via reason=%q, %w", strings.ToLower(string(m.Reason())), err)
+			return reconcile.Result{}, serrors.Wrap(fmt.Errorf("disrupting, %w", err), strings.ToLower(string(m.Reason())), "reason")
 		}
 		if success {
 			return reconcile.Result{RequeueAfter: singleton.RequeueImmediately}, nil
@@ -206,7 +207,7 @@ func (c *Controller) executeCommand(ctx context.Context, m Method, cmd Command,
 
 	// Cordon the old nodes before we launch the replacements to prevent new pods from scheduling to the old nodes
 	if err := c.MarkDisrupted(ctx, m, cmd.candidates...); err != nil {
-		return fmt.Errorf("marking disrupted (command-id: %s), %w", commandID, err)
+		return serrors.Wrap(fmt.Errorf("marking disrupted, %w", err), "command-id", commandID)
 	}
 
 	var nodeClaimNames []string
@@ -215,7 +216,7 @@ func (c *Controller) executeCommand(ctx context.Context, m Method, cmd Command,
 		if nodeClaimNames, err = c.createReplacementNodeClaims(ctx, m, cmd); err != nil {
 			// If we failed to launch the replacement, don't disrupt.  If this is some permanent failure,
 			// we don't want to disrupt workloads with no way to provision new nodes for them.
-			return fmt.Errorf("launching replacement nodeclaim (command-id: %s), %w", commandID, err)
+			return serrors.Wrap(fmt.Errorf("launching replacement nodeclaim, %w", err), "command-id", commandID)
 		}
 	}
 
@@ -234,7 +235,7 @@ func (c *Controller) executeCommand(ctx context.Context, m Method, cmd Command,
 	if err := c.queue.Add(orchestration.NewCommand(nodeClaimNames, statenodes, commandID, m.Reason(), m.ConsolidationType())); err != nil {
 		providerIDs := lo.Map(cmd.candidates, func(c *Candidate, _ int) string { return c.ProviderID() })
 		c.cluster.UnmarkForDeletion(providerIDs...)
-		return fmt.Errorf("adding command to queue (command-id: %s), %w", commandID, err)
+		return serrors.Wrap(fmt.Errorf("adding command to queue, %w", err), "command-id", commandID)
 	}
 
 	// An action is only performed and pods/nodes are only disrupted after a successful add to the queue
@@ -254,7 +255,7 @@ func (c *Controller) createReplacementNodeClaims(ctx context.Context, m Method,
 	}
 	if len(nodeClaimNames) != len(cmd.replacements) {
 		// shouldn't ever occur since a partially failed CreateNodeClaims should return an error
-		return nil, fmt.Errorf("expected %d replacements, got %d", len(cmd.replacements), len(nodeClaimNames))
+		return nil, serrors.Wrap(fmt.Errorf("expected replacement count did not equal actual replacement count"), "expected-count", len(cmd.replacements), "actual-count", len(nodeClaimNames))
 	}
 	return nodeClaimNames, nil
 }
@@ -264,7 +265,7 @@ func (c *Controller) MarkDisrupted(ctx context.Context, m Method, candidates ...
 		return c.StateNode
 	})
 	if err := state.RequireNoScheduleTaint(ctx, c.kubeClient, true, stateNodes...); err != nil {
-		return fmt.Errorf("tainting nodes with %s: %w", pretty.Taint(v1.DisruptedNoScheduleTaint), err)
+		return serrors.Wrap(fmt.Errorf("tainting nodes, %w", err), "taint", pretty.Taint(v1.DisruptedNoScheduleTaint))
 	}
 
 	providerIDs := lo.Map(candidates, func(c *Candidate, _ int) string { return c.ProviderID() })

pkg/controllers/disruption/orchestration/queue.go

@@ -24,6 +24,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/awslabs/operatorpkg/serrors"
 	"github.com/awslabs/operatorpkg/singleton"
 	"github.com/samber/lo"
 	"go.uber.org/multierr"
@@ -243,7 +244,7 @@ func (q *Queue) Reconcile(ctx context.Context) (reconcile.Result, error) {
 // nolint:gocyclo
 func (q *Queue) waitOrTerminate(ctx context.Context, cmd *Command) error {
 	if q.clock.Since(cmd.timeAdded) > maxRetryDuration {
-		return NewUnrecoverableError(fmt.Errorf("command reached timeout after %s", q.clock.Since(cmd.timeAdded)))
+		return NewUnrecoverableError(serrors.Wrap(fmt.Errorf("command reached timeout"), "duration", q.clock.Since(cmd.timeAdded)))
 	}
 	waitErrs := make([]error, len(cmd.Replacements))
 	for i := range cmd.Replacements {
@@ -269,7 +270,7 @@ func (q *Queue) waitOrTerminate(ctx context.Context, cmd *Command) error {
 		initializedStatus := nodeClaim.StatusConditions().Get(v1.ConditionTypeInitialized)
 		if !initializedStatus.IsTrue() {
 			q.recorder.Publish(disruptionevents.WaitingOnReadiness(nodeClaim))
-			waitErrs[i] = fmt.Errorf("nodeclaim %s not initialized", nodeClaim.Name)
+			waitErrs[i] = serrors.Wrap(fmt.Errorf("nodeclaim not initialized"), "NodeClaim", klog.KRef("", nodeClaim.Name))
 			continue
 		}
 		cmd.Replacements[i].Initialized = true