Skip to content

Commit

Permalink
refactor: drop rawObj parameter from logger's methods (#848)
Browse files Browse the repository at this point in the history
  • Loading branch information
@pmalek
pmalek authored Nov 5, 2024
1 parent 90ddb57 commit 52f9b09
Show file tree
Hide file tree
Showing 26 changed files with 295 additions and 277 deletions.
122 changes: 61 additions & 61 deletions controller/controlplane/controller.go
View file

Large diffs are not rendered by default.

5 changes: 2 additions & 3 deletions controller/controlplane/controller_reconciler_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ func (r *Reconciler) ensureDeployment(
return op.Noop, nil, fmt.Errorf("failed creating ControlPlane Deployment %s: %w", generatedDeployment.Name, err)
}

log.Debug(logger, "deployment for ControlPlane created", params.ControlPlane, "deployment", generatedDeployment.Name)
log.Debug(logger, "deployment for ControlPlane created", "deployment", generatedDeployment.Name)
return op.Created, generatedDeployment, nil
}

Expand Down Expand Up @@ -431,7 +431,6 @@ func (r *Reconciler) ensureClusterRoleBinding(
// Delete and re-create ClusterRoleBinding if name of ClusterRole changed because RoleRef is immutable.
if !k8sresources.CompareClusterRoleName(existing, clusterRoleName) {
log.Debug(logger, "ClusterRole name changed, delete and re-create a ClusterRoleBinding",
existing,
"old_cluster_role", existing.RoleRef.Name,
"new_cluster_role", clusterRoleName,
)
Expand Down Expand Up @@ -865,7 +864,7 @@ func (r *Reconciler) ensureValidatingWebhookConfiguration(
}

if updated {
log.Debug(logger, "patching existing ValidatingWebhookConfiguration", webhookConfiguration)
log.Debug(logger, "patching existing ValidatingWebhookConfiguration")
return op.Updated, r.Client.Patch(ctx, &webhookConfiguration, client.MergeFrom(old))
}

Expand Down
50 changes: 25 additions & 25 deletions controller/dataplane/bluegreen_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func (r *BlueGreenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
if err := r.prunePreviewSubresources(ctx, &dataplane); err != nil {
return ctrl.Result{}, fmt.Errorf("failed pruning preview DataPlane subresources: %w", err)
}
log.Trace(logger, "no Rollout with BlueGreen strategy specified, delegating to DataPlaneReconciler", req)
log.Trace(logger, "no Rollout with BlueGreen strategy specified, delegating to DataPlaneReconciler")
return r.DataPlaneController.Reconcile(ctx, req)
}

Expand Down Expand Up @@ -150,7 +150,7 @@ func (r *BlueGreenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
return ctrl.Result{}, nil
}

log.Trace(logger, "ensuring mTLS certificate", dataplane)
log.Trace(logger, "ensuring mTLS certificate")
res, certSecret, err := ensureDataPlaneCertificate(ctx, r.Client, &dataplane,
types.NamespacedName{
Namespace: r.ClusterCASecretNamespace,
Expand All @@ -165,7 +165,7 @@ func (r *BlueGreenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
return ctrl.Result{}, err
}
if res != op.Noop {
log.Debug(logger, "mTLS certificate created/updated", dataplane)
log.Debug(logger, "mTLS certificate created/updated")
return ctrl.Result{}, nil // requeue will be triggered by the creation or update of the owned object
}

Expand Down Expand Up @@ -200,7 +200,7 @@ func (r *BlueGreenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
if deployment.Status.Replicas == 0 ||
deployment.Status.AvailableReplicas != deployment.Status.Replicas ||
deployment.Status.ReadyReplicas != deployment.Status.Replicas {
log.Trace(logger, "preview deployment for DataPlane not ready yet", dataplane)
log.Trace(logger, "preview deployment for DataPlane not ready yet")
err := r.ensureRolledOutCondition(ctx, logger, &dataplane, metav1.ConditionFalse, consts.DataPlaneConditionReasonRolloutProgressing, consts.DataPlaneConditionMessageRolledOutPreviewDeploymentNotYetReady)
return ctrl.Result{}, err
}
Expand All @@ -211,7 +211,7 @@ func (r *BlueGreenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
if proceedWithPromotion, err := canProceedWithPromotion(dataplane); err != nil {
return ctrl.Result{}, fmt.Errorf("failed checking if DataPlane %s/%s can be promoted: %w", dataplane.Namespace, dataplane.Name, err)
} else if !proceedWithPromotion {
log.Debug(logger, "DataPlane preview resources cannot be promoted yet or is awaiting promotion trigger", dataplane,
log.Debug(logger, "DataPlane preview resources cannot be promoted yet or is awaiting promotion trigger",
"promotion_strategy", dataplane.Spec.Deployment.Rollout.Strategy.BlueGreen.Promotion.Strategy)

err := r.ensureRolledOutCondition(ctx, logger, &dataplane, metav1.ConditionFalse, consts.DataPlaneConditionReasonRolloutAwaitingPromotion, "")
Expand All @@ -232,7 +232,7 @@ func (r *BlueGreenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
cErr := r.ensureRolledOutCondition(ctx, logger, &dataplane, metav1.ConditionFalse, consts.DataPlaneConditionReasonRolloutPromotionFailed, "failed to update DataPlane's selector")
return ctrl.Result{}, fmt.Errorf("failed to update DataPlane %s/%s: %w", dataplane.Namespace, dataplane.Name, errors.Join(cErr, err))
} else if updated {
log.Debug(logger, "preview deployment selector assigned to a live selector, promotion in progress", dataplane)
log.Debug(logger, "preview deployment selector assigned to a live selector, promotion in progress")
return ctrl.Result{}, nil
}

Expand All @@ -252,7 +252,7 @@ func (r *BlueGreenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
); err != nil {
return ctrl.Result{}, fmt.Errorf("failed waiting for live %q service to have the expected selector: %w", serviceType, err)
} else if !ok {
log.Debug(logger, fmt.Sprintf("%q live service do not have the expected selector yet, delegating to DP controller", serviceType), dataplane)
log.Debug(logger, fmt.Sprintf("%q live service does not have the expected selector yet, delegating to DP controller", serviceType))
return r.DataPlaneController.Reconcile(ctx, req)
}
}
Expand All @@ -266,7 +266,7 @@ func (r *BlueGreenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
cErr := r.ensureRolledOutCondition(ctx, logger, &dataplane, metav1.ConditionFalse, consts.DataPlaneConditionReasonRolloutPromotionFailed, "failed to label DataPlane's preview Deployment for promotion")
return ctrl.Result{}, fmt.Errorf("failed to ensure preview deployment becomes live %s/%s: %w", dataplane.Namespace, dataplane.Name, errors.Join(cErr, err))
} else if updated {
log.Trace(logger, "preview deployment labeled as live", dataplane)
log.Trace(logger, "preview deployment labeled as live")
}

// We can clear the selector in RolloutStatus which will cause next
Expand All @@ -293,7 +293,7 @@ func (r *BlueGreenReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
return ctrl.Result{}, fmt.Errorf("failed to reduce live deployments: %w", err)
}

log.Debug(logger, "BlueGreen reconciliation complete for DataPlane resource", dataplane)
log.Debug(logger, "BlueGreen reconciliation complete for DataPlane resource")
return ctrl.Result{}, nil
}

Expand Down Expand Up @@ -341,7 +341,7 @@ func shouldDelegateToDataPlaneController(
cReady.ObservedGeneration == cRolledOut.ObservedGeneration &&
cReady.ObservedGeneration == dataplane.Generation {
if cRolledOut.Reason == string(consts.DataPlaneConditionReasonRolloutWaitingForChange) {
log.Debug(logger, "DataPlane is up to date, waiting for changes, delegating to DataPlaneReconciler", dataplane)
log.Debug(logger, "DataPlane is up to date, waiting for changes, delegating to DataPlaneReconciler")
return true
}
}
Expand All @@ -352,7 +352,7 @@ func shouldDelegateToDataPlaneController(
// the DataPlane with the current spec which would cause the rollout to progress
// which is what the promotion does.
if cReady.Status == metav1.ConditionFalse && cReady.ObservedGeneration == dataplane.Generation {
log.Debug(logger, "DataPlane is not ready yet to proceed with BlueGreen rollout, delegating to DataPlaneReconciler", dataplane)
log.Debug(logger, "DataPlane is not ready yet to proceed with BlueGreen rollout, delegating to DataPlaneReconciler")
return true
}

Expand Down Expand Up @@ -385,7 +385,7 @@ func (r *BlueGreenReconciler) prunePreviewSubresources(
return err
}
if len(deployments) > 0 {
log.Debug(logger, "removing preview Deployments", dataplane)
log.Debug(logger, "removing preview Deployments")
if err := removeObjectSliceWithDataPlaneOwnedFinalizer(ctx, r.Client, deployments); err != nil {
return err
}
Expand Down Expand Up @@ -419,13 +419,13 @@ func (r *BlueGreenReconciler) prunePreviewSubresources(
return err
}

log.Debug(logger, "removing preview Secrets", dataplane)
log.Debug(logger, "removing preview Secrets")
if err := removeObjectSliceWithDataPlaneOwnedFinalizer(ctx, r.Client, secrets); err != nil {
return err
}
}
if len(services) > 0 {
log.Debug(logger, "removing preview Services", dataplane)
log.Debug(logger, "removing preview Services")
if err := removeObjectSliceWithDataPlaneOwnedFinalizer(ctx, r.Client, services); err != nil {
return err
}
Expand Down Expand Up @@ -510,11 +510,11 @@ func (r *BlueGreenReconciler) ensureDeploymentForDataPlane(

switch res {
case op.Created, op.Updated:
log.Debug(logger, "deployment modified", dataplane, "reason", res)
log.Debug(logger, "deployment modified", "reason")
// requeue will be triggered by the creation or update of the owned object
return deployment, res, nil
default:
log.Debug(logger, "no need for deployment update", dataplane)
log.Debug(logger, "no need for deployment update")
return deployment, op.Noop, nil
}
}
Expand Down Expand Up @@ -567,8 +567,9 @@ func (r *BlueGreenReconciler) reduceLiveDeployments(
})
// Delete all but the last deployment.
for _, deployment := range deployments[:len(deployments)-1] {
log.Debug(logger, "reducing live deployment", dataPlane,
"deployment", fmt.Sprintf("%s/%s", deployment.Namespace, deployment.Name))
log.Debug(logger, "reducing live deployment",
"deployment", client.ObjectKeyFromObject(&deployment),
)

if err := dataplane.OwnedObjectPreDeleteHook(ctx, r.Client, &deployment); err != nil {
return fmt.Errorf("failed executing pre delete hook: %w", err)
Expand Down Expand Up @@ -700,7 +701,7 @@ func (r *BlueGreenReconciler) ensureDataPlaneAdminAPIInRolloutStatus(
// It returns a bool flag indicating that the status has been patched and an error.
func (r *BlueGreenReconciler) patchRolloutStatus(ctx context.Context, logger logr.Logger, old, updated *operatorv1beta1.DataPlane) (bool, error) {
if rolloutStatusChanged(old, updated) {
log.Debug(logger, "patching DataPlane status", updated, "status", updated.Status)
log.Debug(logger, "patching DataPlane status", "status", updated.Status)
return true, r.Client.Status().Patch(ctx, updated, client.MergeFrom(old))
}

Expand Down Expand Up @@ -752,9 +753,9 @@ func (r *BlueGreenReconciler) ensurePreviewAdminAPIService(

switch res {
case op.Created, op.Updated:
log.Debug(logger, "preview admin service modified", dataplane, "service", svc.Name, "reason", res)
log.Debug(logger, "preview admin service modified", "service", svc.Name, "reason", res)
case op.Noop:
log.Trace(logger, "no need for preview Admin API service update", dataplane)
log.Trace(logger, "no need for preview Admin API service update")
case op.Deleted:
}
return res, svc, nil // dataplane admin service creation/update will trigger reconciliation
Expand Down Expand Up @@ -785,9 +786,9 @@ func (r *BlueGreenReconciler) ensurePreviewIngressService(

switch res {
case op.Created, op.Updated:
log.Debug(logger, "preview ingress service modified", dataplane, "service", svc.Name, "reason", res)
log.Debug(logger, "preview ingress service modified", "service", svc.Name, "reason", res)
case op.Noop:
log.Trace(logger, "no need for preview ingress service update", dataplane)
log.Trace(logger, "no need for preview ingress service update")
case op.Deleted:
}

Expand Down Expand Up @@ -912,8 +913,7 @@ func (r *BlueGreenReconciler) ensurePreviewDeploymentLabeledLive(
// It shouldn't happen in practice, but we'll log it just in case.
if len(deployments) > 1 {
ds := lo.Map(deployments, func(d appsv1.Deployment, _ int) string { return fmt.Sprintf("%s/%s", d.Namespace, d.Name) })
log.Info(logger, "found multiple preview deployments, expected one, will label live only the first one",
dataplane, "deployments", ds)
log.Info(logger, "found multiple preview deployments, expected one, will label live only the first one", "deployments", ds)
}

deployment := deployments[0]
Expand Down
36 changes: 18 additions & 18 deletions controller/dataplane/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
ctx = r.ContextInjector.InjectKeyValues(ctx)
logger := log.GetLogger(ctx, "dataplane", r.DevelopmentMode)

log.Trace(logger, "reconciling DataPlane resource", req)
log.Trace(logger, "reconciling DataPlane resource")
dpNn := req.NamespacedName
dataplane := new(operatorv1beta1.DataPlane)
if err := r.Client.Get(ctx, dpNn, dataplane); err != nil {
Expand All @@ -82,20 +82,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{}, fmt.Errorf("failed updating DataPlane with selector in Status: %w", err)
}

log.Trace(logger, "validating DataPlane configuration", dataplane)
log.Trace(logger, "validating DataPlane configuration")
err := r.Validator.Validate(dataplane)
if err != nil {
log.Info(logger, "failed to validate dataplane: "+err.Error(), dataplane)
log.Info(logger, "failed to validate dataplane: "+err.Error())
r.eventRecorder.Event(dataplane, "Warning", "ValidationFailed", err.Error())
markErr := ensureDataPlaneIsMarkedNotReady(ctx, logger, r.Client, dataplane, DataPlaneConditionValidationFailed, err.Error())
return ctrl.Result{}, markErr
}

log.Trace(logger, "applying extensions", dataplane)
log.Trace(logger, "applying extensions")
patched, requeue, err := applyExtensions(ctx, r.Client, logger, dataplane, r.KonnectEnabled)
if err != nil {
if !requeue {
log.Debug(logger, "failed to apply extensions", dataplane, "error:", err)
log.Debug(logger, "failed to apply extensions", "err", err)
return ctrl.Result{}, nil
}
return ctrl.Result{}, err
Expand All @@ -104,7 +104,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{}, nil
}

log.Trace(logger, "exposing DataPlane deployment admin API via headless service", dataplane)
log.Trace(logger, "exposing DataPlane deployment admin API via headless service")
res, dataplaneAdminService, err := ensureAdminServiceForDataPlane(ctx, r.Client, dataplane,
client.MatchingLabels{
consts.DataPlaneServiceStateLabel: consts.DataPlaneStateLabelValueLive,
Expand All @@ -116,13 +116,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
}
switch res {
case op.Created, op.Updated:
log.Debug(logger, "DataPlane admin service modified", dataplane, "service", dataplaneAdminService.Name, "reason", res)
log.Debug(logger, "DataPlane admin service modified", "service", dataplaneAdminService.Name, "reason", res)
return ctrl.Result{}, nil // dataplane admin service creation/update will trigger reconciliation
case op.Noop:
case op.Deleted: // This should not happen.
}

log.Trace(logger, "exposing DataPlane deployment via service", dataplane)
log.Trace(logger, "exposing DataPlane deployment via service")
additionalServiceLabels := map[string]string{
consts.DataPlaneServiceStateLabel: consts.DataPlaneStateLabelValueLive,
}
Expand All @@ -139,7 +139,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{}, err
}
if serviceRes == op.Created || serviceRes == op.Updated {
log.Debug(logger, "DataPlane ingress service created/updated", dataplane, "service", dataplaneIngressService.Name)
log.Debug(logger, "DataPlane ingress service created/updated", "service", dataplaneIngressService.Name)
return ctrl.Result{}, nil
}

Expand All @@ -148,11 +148,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{}, err
}
if dataplaneServiceChanged {
log.Debug(logger, "ingress service updated in the dataplane status", dataplane)
log.Debug(logger, "ingress service updated in the dataplane status")
return ctrl.Result{}, nil // dataplane status update will trigger reconciliation
}

log.Trace(logger, "ensuring mTLS certificate", dataplane)
log.Trace(logger, "ensuring mTLS certificate")
res, certSecret, err := ensureDataPlaneCertificate(ctx, r.Client, dataplane,
types.NamespacedName{
Namespace: r.ClusterCASecretNamespace,
Expand All @@ -167,20 +167,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{}, err
}
if res != op.Noop {
log.Debug(logger, "mTLS certificate created/updated", dataplane)
log.Debug(logger, "mTLS certificate created/updated")
return ctrl.Result{}, nil // requeue will be triggered by the creation or update of the owned object
}

log.Trace(logger, "checking readiness of DataPlane service", dataplaneIngressService)
log.Trace(logger, "checking readiness of DataPlane service", "service", dataplaneIngressService.Name)
if dataplaneIngressService.Spec.ClusterIP == "" {
return ctrl.Result{}, nil // no need to requeue, the update will trigger.
}

log.Trace(logger, "ensuring DataPlane has service addresses in status", dataplaneIngressService)
log.Trace(logger, "ensuring DataPlane has service addresses in status", "service", dataplaneIngressService.Name)
if updated, err := r.ensureDataPlaneAddressesStatus(ctx, logger, dataplane, dataplaneIngressService); err != nil {
return ctrl.Result{}, err
} else if updated {
log.Debug(logger, "dataplane status.Addresses updated", dataplane)
log.Debug(logger, "dataplane status.Addresses updated")
return ctrl.Result{}, nil // no need to requeue, the update will trigger.
}

Expand All @@ -191,7 +191,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
labelSelectorFromDataPlaneStatusSelectorDeploymentOpt(dataplane),
}

log.Trace(logger, "ensuring generation of deployment configuration for KongPluginInstallations configured for DataPlane", dataplane)
log.Trace(logger, "ensuring generation of deployment configuration for KongPluginInstallations configured for DataPlane")
kpisForDeployment, requeue, err := ensureMappedConfigMapToKongPluginInstallationForDataPlane(ctx, logger, r.Client, dataplane)
if err != nil {
return ctrl.Result{}, fmt.Errorf("cannot ensure KongPluginInstallation for DataPlane: %w", err)
Expand Down Expand Up @@ -230,7 +230,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{}, fmt.Errorf("could not ensure PodDisruptionBudget for DataPlane %s: %w", dpNn, err)
}
if res != op.Noop {
log.Debug(logger, "PodDisruptionBudget created/updated", dataplane)
log.Debug(logger, "PodDisruptionBudget created/updated")
return ctrl.Result{}, nil
}

Expand All @@ -240,7 +240,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return res, nil
}

log.Debug(logger, "reconciliation complete for DataPlane resource", dataplane)
log.Debug(logger, "reconciliation complete for DataPlane resource")
return ctrl.Result{}, nil
}

Expand Down
Loading

0 comments on commit 52f9b09

Please sign in to comment.