Skip to content
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.

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

handle conflict error in instrumentation config updates #1737

Merged
merged 10 commits into from
Nov 21, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"

odigosv1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
"github.com/odigos-io/odigos/k8sutils/pkg/utils"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
Expand Down Expand Up @@ -55,19 +56,18 @@ func (r *InstrumentedApplicationReconciler) Reconcile(ctx context.Context, req c

instrumentationRules := &odigosv1.InstrumentationRuleList{}
err = r.Client.List(ctx, instrumentationRules)
if client.IgnoreNotFound(err) != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can a list operation return not found? or is it only for safety?

If it's not found, don't we want to return and not cause any updates?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thich check is for errors which are not NotFound

return ctrl.Result{}, err
}

err = updateInstrumentationConfigForWorkload(&ic, &ia, instrumentationRules)
if err != nil {
return ctrl.Result{}, err
}

err = r.Client.Update(ctx, &ic)
if client.IgnoreNotFound(err) != nil {
logger.Error(err, "error updating instrumentation config", "workload", ia.Name)
return ctrl.Result{}, err
if err == nil {
logger.V(0).Info("Updated instrumentation config", "workload", ia.Name)
}

logger.V(0).Info("Updated instrumentation config", "workload", ia.Name)

return ctrl.Result{}, nil
return utils.K8SUpdateErrorHandler(err)
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@ func (r *InstrumentedApplicationReconciler) Reconcile(ctx context.Context, req c
return ctrl.Result{}, err
}
err = removeInstrumentationDeviceFromWorkload(ctx, r.Client, req.Namespace, workloadKind, workloadName, ApplyInstrumentationDeviceReasonNoRuntimeDetails)
return utils.RetryOnConflict(err)
return utils.K8SUpdateErrorHandler(err)
}

isNodeCollectorReady := isDataCollectionReady(ctx, r.Client)
err = reconcileSingleWorkload(ctx, r.Client, &runtimeDetails, isNodeCollectorReady)
return utils.RetryOnConflict(err)
return utils.K8SUpdateErrorHandler(err)
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type DeploymentReconciler struct {
func (r *DeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
instrumentedAppName := workload.CalculateWorkloadRuntimeObjectName(req.Name, workload.WorkloadKindDeployment)
err := reconcileSingleInstrumentedApplicationByName(ctx, r.Client, instrumentedAppName, req.Namespace)
return utils.RetryOnConflict(err)
return utils.K8SUpdateErrorHandler(err)
}

type DaemonSetReconciler struct {
Expand All @@ -29,7 +29,7 @@ type DaemonSetReconciler struct {
func (r *DaemonSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
instrumentedAppName := workload.CalculateWorkloadRuntimeObjectName(req.Name, workload.WorkloadKindDaemonSet)
err := reconcileSingleInstrumentedApplicationByName(ctx, r.Client, instrumentedAppName, req.Namespace)
return utils.RetryOnConflict(err)
return utils.K8SUpdateErrorHandler(err)
}

type StatefulSetReconciler struct {
Expand All @@ -39,7 +39,7 @@ type StatefulSetReconciler struct {
func (r *StatefulSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
instrumentedAppName := workload.CalculateWorkloadRuntimeObjectName(req.Name, workload.WorkloadKindStatefulSet)
err := reconcileSingleInstrumentedApplicationByName(ctx, r.Client, instrumentedAppName, req.Namespace)
return utils.RetryOnConflict(err)
return utils.K8SUpdateErrorHandler(err)
}

func reconcileSingleInstrumentedApplicationByName(ctx context.Context, k8sClient client.Client, instrumentedAppName string, namespace string) error {
Expand Down
11 changes: 10 additions & 1 deletion k8sutils/pkg/utils/retryonconflict.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,21 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

func RetryOnConflict(err error) (reconcile.Result, error) {
// K8SUpdateErrorHandler is a helper function to handle k8s update errors.
// It returns a reconcile.Result and an error
// If the error is a conflict error, it returns a requeue result without an error
// If the error is a not found error, it returns an empty result without an error
// For other errors, it returns the error as is - which will cause a requeue
RonFed marked this conversation as resolved.
Show resolved Hide resolved
func K8SUpdateErrorHandler(err error) (reconcile.Result, error) {
if errors.IsConflict(err) {
// For conflict errors, requeue without returning an error.
// this is so that we don't have errors and stack traces in the logs for valid scenario
return reconcile.Result{Requeue: true}, nil
}
if errors.IsNotFound(err) {
// For not found errors, ignore
return reconcile.Result{}, nil
}
// For other errors, return as is (will log the stack trace)
return reconcile.Result{}, err
}
Loading