Skip to content

Commit

Permalink
make client calls retriable when requeue is not possible
Browse files Browse the repository at this point in the history
  • Loading branch information
pixelsoccupied committed May 31, 2024
1 parent de386c0 commit 6841803
Show file tree
Hide file tree
Showing 12 changed files with 101 additions and 48 deletions.
2 changes: 1 addition & 1 deletion controllers/ibu_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func (r *ImageBasedUpgradeReconciler) Reconcile(ctx context.Context, req ctrl.Re

// Use a non-cached query to Get the IBU CR, to ensure we aren't running against a stale cached CR
ibu := &ibuv1.ImageBasedUpgrade{}
err = common.RetryOnRetriable(common.RetryBackoffTwoMinutes, func() error {
err = common.CallWithRetry(func() error {
return r.NoncachedClient.Get(ctx, req.NamespacedName, ibu) //nolint:wrapcheck
})
if err != nil {
Expand Down
4 changes: 3 additions & 1 deletion controllers/idle_handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ func (r *ImageBasedUpgradeReconciler) handleAbortFailure(ctx context.Context, ib
func (r *ImageBasedUpgradeReconciler) checkManualCleanup(ctx context.Context, ibu *ibuv1.ImageBasedUpgrade) (bool, error) {
if _, ok := ibu.Annotations[utils.ManualCleanupAnnotation]; ok {
delete(ibu.Annotations, utils.ManualCleanupAnnotation)
if err := r.Client.Update(ctx, ibu); err != nil {
if err := common.CallWithRetry(func() error {
return r.Client.Update(ctx, ibu) //nolint:wrapcheck
}); err != nil {
return false, fmt.Errorf("failed to remove manual cleanup annotation from ibu: %w", err)
}
return true, nil
Expand Down
4 changes: 2 additions & 2 deletions controllers/seedgen_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,7 @@ func (r *SeedGeneratorReconciler) Reconcile(ctx context.Context, req ctrl.Reques

// Use a non-cached query to Get the SeedGen CR, to ensure we aren't running against a stale cached CR
seedgen := &seedgenv1.SeedGenerator{}
err = common.RetryOnRetriable(common.RetryBackoffTwoMinutes, func() error {
err = common.CallWithRetry(func() error {
return r.NoncachedClient.Get(ctx, req.NamespacedName, seedgen) //nolint:wrapcheck
})
if err != nil {
Expand Down Expand Up @@ -1006,7 +1006,7 @@ func setSeedGenStatusCompleted(seedgen *seedgenv1.SeedGenerator) {

func (r *SeedGeneratorReconciler) updateStatus(ctx context.Context, seedgen *seedgenv1.SeedGenerator) error {
seedgen.Status.ObservedGeneration = seedgen.ObjectMeta.Generation
err := common.RetryOnRetriable(common.RetryBackoffTwoMinutes, func() error {
err := common.CallWithRetry(func() error {
return r.Status().Update(ctx, seedgen) //nolint:wrapcheck
})

Expand Down
2 changes: 1 addition & 1 deletion controllers/utils/conditions.go
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ func UpdateIBUStatus(ctx context.Context, c client.Client, ibu *ibuv1.ImageBased
}
}

err := common.RetryOnRetriable(common.RetryBackoffTwoMinutes, func() error {
err := common.CallWithRetry(func() error {
return c.Status().Update(ctx, ibu) //nolint:wrapcheck
})

Expand Down
30 changes: 20 additions & 10 deletions internal/backuprestore/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,9 @@ func (h *BRHandler) cleanupBackupLabels(ctx context.Context, backup *velerov1.Ba
for _, obj := range objs {
patchedObj := obj

if err := patchObj(ctx, h.DynamicClient, &patchedObj, false, payload); err != nil {
if err := common.CallWithRetry(func() error {
return patchObj(ctx, h.DynamicClient, &patchedObj, false, payload)
}); err != nil {
if k8serrors.IsNotFound(err) || k8serrors.IsInvalid(err) {
h.Log.Info("backup label doesn't exist, no patching needed, ignoring", "name", obj.Name,
"namespace", obj.Namespace, "resource", obj.Resource, "group", obj.Group, "version", obj.Version)
Expand Down Expand Up @@ -304,7 +306,9 @@ func (h *BRHandler) PatchPVsReclaimPolicy(ctx context.Context) error {
// in PVs created by LVMS (this field was updated during pre-pivot by LCA).
func (h *BRHandler) RestorePVsReclaimPolicy(ctx context.Context) error {
pvList := &corev1.PersistentVolumeList{}
if err := h.List(ctx, pvList); err != nil {
if err := common.CallWithRetry(func() error {
return h.List(ctx, pvList) //nolint:wrapcheck
}); err != nil {
return fmt.Errorf("failed to list PersistentVolumes: %w", err)
}

Expand All @@ -323,7 +327,9 @@ func (h *BRHandler) RestorePVsReclaimPolicy(ctx context.Context) error {
pvPatched := pv
pvPatched.Spec.PersistentVolumeReclaimPolicy = "Delete"
delete(pvPatched.Annotations, updatedReclaimPolicyAnnotation)
if err := h.Client.Update(ctx, &pvPatched); err != nil {
if err := common.CallWithRetry(func() error {
return h.Client.Update(ctx, &pvPatched) //nolint:wrapcheck
}); err != nil {
return fmt.Errorf("failed to update PersistentVolume %s: %w", pv.Name, err)
}
}
Expand Down Expand Up @@ -437,8 +443,8 @@ func (h *BRHandler) CleanupBackups(ctx context.Context) error {

// List all backups created for this cluster
backupList := &velerov1.BackupList{}
if err := h.List(ctx, backupList, client.MatchingLabels{
clusterIDLabel: clusterID,
if err := common.CallWithRetry(func() error {
return h.List(ctx, backupList, client.MatchingLabels{clusterIDLabel: clusterID}) //nolint:wrapcheck
}); err != nil {
var groupDiscoveryErr *discovery.ErrGroupDiscoveryFailed
if errors.As(err, &groupDiscoveryErr) || meta.IsNoMatchError(err) {
Expand Down Expand Up @@ -467,7 +473,9 @@ func (h *BRHandler) CleanupBackups(ctx context.Context) error {
},
}

if err := h.Create(ctx, deleteBackupRequest); err != nil {
if err := common.CallWithRetry(func() error {
return h.Create(ctx, deleteBackupRequest) //nolint:wrapcheck
}); err != nil {
return fmt.Errorf("could not apply deleteBackupRequest CR: %w", err)
}
h.Log.Info("Backup deletion request has sent", "backup", backup.Name)
Expand Down Expand Up @@ -570,7 +578,7 @@ func (h *BRHandler) ensureBackupsDeleted(ctx context.Context, backups []velerov1
}

for _, backup := range backups {
err := common.RetryOnRetriable(common.RetryBackoffTwoMinutes, func() error {
err := common.CallWithRetry(func() error {
return h.Get(ctx, types.NamespacedName{ //nolint:wrapcheck
Name: backup.Name,
Namespace: backup.Namespace,
Expand Down Expand Up @@ -599,8 +607,8 @@ func (h *BRHandler) CleanupDeleteBackupRequests(ctx context.Context) error {

// List all DeleteBackupRequest CRs created for this cluster
deleteBackupRequestList := &velerov1.DeleteBackupRequestList{}
if err := h.List(ctx, deleteBackupRequestList, client.MatchingLabels{
clusterIDLabel: clusterID,
if err := common.CallWithRetry(func() error {
return h.List(ctx, deleteBackupRequestList, client.MatchingLabels{clusterIDLabel: clusterID}) //nolint:wrapcheck
}); err != nil {
var groupDiscoveryErr *discovery.ErrGroupDiscoveryFailed
if errors.As(err, &groupDiscoveryErr) || meta.IsNoMatchError(err) {
Expand All @@ -620,7 +628,9 @@ func (h *BRHandler) CleanupDeleteBackupRequests(ctx context.Context) error {
deleteBackupRequestName := deleteBackupRequest.GetName()

h.Log.Info(fmt.Sprintf("Deleting DeleteBackupRequest CR %s", deleteBackupRequestName))
if err := h.Delete(ctx, deleteBackupRequest.DeepCopy()); err != nil {
if err := common.CallWithRetry(func() error {
return h.Delete(ctx, deleteBackupRequest.DeepCopy()) //nolint:wrapcheck
}); err != nil {
return fmt.Errorf("failed to delete DeleteBackupRequest CR %s: %w", deleteBackupRequestName, err)
}
}
Expand Down
17 changes: 12 additions & 5 deletions internal/backuprestore/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -378,11 +378,16 @@ func patchObj(ctx context.Context, client dynamic.Interface, obj *ObjMetadata, i
}

if obj.Namespace != "" {
_, err = resourceClient.Namespace(obj.Namespace).Patch(
ctx, obj.Name, types.JSONPatchType, payload, patchOptions,
)
err = common.CallWithRetry(func() error {
_, err = resourceClient.Namespace(obj.Namespace).Patch(ctx, obj.Name, types.JSONPatchType, payload, patchOptions)
return err //nolint:wrapcheck
})

} else {
_, err = resourceClient.Patch(ctx, obj.Name, types.JSONPatchType, payload, patchOptions)
err = common.CallWithRetry(func() error {
_, err = resourceClient.Patch(ctx, obj.Name, types.JSONPatchType, payload, patchOptions)
return err //nolint:wrapcheck
})
}
if err != nil {
return fmt.Errorf("failed to patch object: %w", err)
Expand Down Expand Up @@ -536,7 +541,9 @@ func (h *BRHandler) CheckOadpOperatorAvailability(ctx context.Context) error {
// IsOadpInstalled a simple function to determine if OADP is present by checking the presence of at least one OADP defined CRD
func (h *BRHandler) IsOadpInstalled(ctx context.Context) bool {
crds := &apiextensionsv1.CustomResourceDefinitionList{}
if err := h.Client.List(ctx, crds); err != nil {
if err := common.CallWithRetry(func() error {
return h.Client.List(ctx, crds) //nolint:wrapcheck
}); err != nil {
h.Log.Error(err, "could not list CRDs to verify if OADP is installed")
return false
}
Expand Down
44 changes: 27 additions & 17 deletions internal/common/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/client-go/kubernetes"
ctrl "sigs.k8s.io/controller-runtime"

"github.com/go-logr/logr"
cp "github.com/otiai10/copy"
Expand All @@ -47,13 +48,6 @@ import (
// TODO: Need a better way to change this but will require relatively big refactoring
var OstreeDeployPathPrefix = ""

var RetryBackoffTwoMinutes = wait.Backoff{
Steps: 120,
Duration: time.Second,
Factor: 1.0,
Jitter: 0.1,
}

// GetConfigMap retrieves the configmap from cluster
func GetConfigMap(ctx context.Context, c client.Client, configMap ibuv1.ConfigMapRef) (*corev1.ConfigMap, error) {

Expand Down Expand Up @@ -129,12 +123,6 @@ func GetStaterootOptOpenshift(staterootPath string) string {
return filepath.Join(staterootPath, "var", OptOpenshift)
}

// FuncTimer check execution time
func FuncTimer(start time.Time, name string, r logr.Logger) {
elapsed := time.Since(start)
r.Info(fmt.Sprintf("%s took %s", name, elapsed))
}

func IsConflictOrRetriable(err error) bool {
return apierrors.IsConflict(err) || apierrors.IsInternalError(err) || apierrors.IsServiceUnavailable(err) || net.IsConnectionRefused(err)
}
Expand All @@ -147,10 +135,6 @@ func IsRetriable(err error) bool {
return apierrors.IsInternalError(err) || apierrors.IsServiceUnavailable(err) || net.IsConnectionRefused(err)
}

func RetryOnRetriable(backoff wait.Backoff, fn func() error) error {
return retry.OnError(backoff, IsRetriable, fn) //nolint:wrapcheck
}

func GetDesiredStaterootName(ibu *ibuv1.ImageBasedUpgrade) string {
return GetStaterootName(ibu.Spec.SeedImageRef.Version)
}
Expand Down Expand Up @@ -243,3 +227,29 @@ func GenerateDeleteOptions() *client.DeleteOptions {
}
return &delOpt
}

// CallWithRetry use this function when requeue is not possible by controller but client https calls must complete.
func CallWithRetry(fn func() error) error {
if err := retry.OnError(getBackoff(), IsRetriable, func() error {
if err := fn(); err != nil {
if IsRetriable(err) {
ctrl.Log.WithName("CallWithRetry").Info(err.Error())
}
return err
}
return nil
}); err != nil {
return fmt.Errorf("failed to make client call: %w", err)
}
return nil
}

// getBackoff configured for API outages where we require additional time (about 5 mins) to recover before being served
func getBackoff() wait.Backoff {
return wait.Backoff{
Steps: 40,
Duration: 5 * time.Millisecond,
Factor: 2.0,
Jitter: 0.1,
}
}
4 changes: 3 additions & 1 deletion internal/extramanifest/extramanifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,9 @@ func RemoveAnnotationEMWarningValidation(c client.Client, log logr.Logger, ibu *
delete(ann, ValidationWarningAnnotation)
ibu.SetAnnotations(ann)

if err := c.Patch(context.Background(), ibu, patch); err != nil {
if err := common.CallWithRetry(func() error {
return c.Patch(context.Background(), ibu, patch) //nolint:wrapcheck
}); err != nil {
return fmt.Errorf("failed to update annotations: %w", err)
}
log.Info("Successfully removed annotation", "annotations", ValidationWarningAnnotation)
Expand Down
13 changes: 10 additions & 3 deletions internal/precache/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,9 @@ func deleteConfigMap(ctx context.Context, c client.Client) error {
},
}

if err := c.Delete(ctx, &cm, common.GenerateDeleteOptions()); err != nil {
if err := common.CallWithRetry(func() error {
return c.Delete(ctx, &cm, common.GenerateDeleteOptions()) //nolint:wrapcheck
}); err != nil {
if !k8serrors.IsNotFound(err) {
return fmt.Errorf("failed to delete configMaps: %w", err)
}
Expand All @@ -259,7 +261,10 @@ func deleteJob(ctx context.Context, c client.Client) error {
Namespace: common.LcaNamespace,
},
}
if err := c.Delete(ctx, &precache, common.GenerateDeleteOptions()); err != nil {

if err := common.CallWithRetry(func() error {
return c.Delete(ctx, &precache, common.GenerateDeleteOptions()) //nolint:wrapcheck
}); err != nil {
if !k8serrors.IsNotFound(err) {
return fmt.Errorf("failed to job: %w", err)
}
Expand All @@ -281,7 +286,9 @@ func removePrecacheFinalizer(ctx context.Context, c client.Client) error {
if controllerutil.ContainsFinalizer(precache, LcaPrecacheFinalizer) {
finalizersUpdated := controllerutil.RemoveFinalizer(precache, LcaPrecacheFinalizer)
if finalizersUpdated {
if err := c.Update(ctx, precache); err != nil {
if err := common.CallWithRetry(func() error {
return c.Update(ctx, precache) //nolint:wrapcheck
}); err != nil {
return fmt.Errorf("failed to remove finalizer during update: %w", err)
}
}
Expand Down
17 changes: 13 additions & 4 deletions internal/prep/prep_stateroot_setup_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ var StaterootSetupTerminationGracePeriodSeconds int64 = 1800

func GetStaterootSetupJob(ctx context.Context, c client.Client, log logr.Logger) (*batchv1.Job, error) {
job := &batchv1.Job{}
if err := c.Get(ctx, types.NamespacedName{Name: StaterootSetupJobName, Namespace: common.LcaNamespace}, job); err != nil {
if err := common.CallWithRetry(func() error {
return c.Get(ctx, types.NamespacedName{Name: StaterootSetupJobName, Namespace: common.LcaNamespace}, job) //nolint:wrapcheck
}); err != nil {
return job, err //nolint:wrapcheck
}

Expand Down Expand Up @@ -142,7 +144,10 @@ func DeleteStaterootSetupJob(ctx context.Context, c client.Client, log logr.Logg
Namespace: common.LcaNamespace,
},
}
if err := c.Delete(ctx, &stateroot, common.GenerateDeleteOptions()); err != nil {

if err := common.CallWithRetry(func() error {
return c.Delete(ctx, &stateroot, common.GenerateDeleteOptions()) //nolint:wrapcheck
}); err != nil {
if !k8serrors.IsNotFound(err) {
return fmt.Errorf("failed to delete stateroot setup job: %w", err)
}
Expand All @@ -166,7 +171,9 @@ func waitUntilStaterootSetupPodIsRemoved(ctx context.Context, c client.Client) e
client.MatchingLabels{"job-name": StaterootSetupJobName},
}
podList := &corev1.PodList{}
if err := c.List(ctx, podList, opts...); err != nil {
if err := common.CallWithRetry(func() error {
return c.List(ctx, podList, opts...) //nolint:wrapcheck
}); err != nil {
return false, fmt.Errorf("failed to list pods: %w", err)
}

Expand All @@ -187,7 +194,9 @@ func removeStaterootSetupJobFinalizer(ctx context.Context, c client.Client, log
if controllerutil.ContainsFinalizer(staterootjob, staterootSetupJobFinalizer) {
finalizerRemoved := controllerutil.RemoveFinalizer(staterootjob, staterootSetupJobFinalizer)
if finalizerRemoved {
if err := c.Update(ctx, staterootjob); err != nil {
if err := common.CallWithRetry(func() error {
return c.Update(ctx, staterootjob) //nolint:wrapcheck
}); err != nil {
return fmt.Errorf("failed to remove finalizer during update: %w", err)
}
}
Expand Down
4 changes: 3 additions & 1 deletion lca-cli/cmd/ibuStaterootSetup.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ func ibuStaterootSetupRun() error {

logger.Info("Fetching the latest IBU cr")
ibu := &ibuv1.ImageBasedUpgrade{}
if err := c.Get(ctx, types.NamespacedName{Namespace: common.LcaNamespace, Name: utils.IBUName}, ibu); err != nil {
if err := common.CallWithRetry(func() error {
return c.Get(ctx, types.NamespacedName{Namespace: common.LcaNamespace, Name: utils.IBUName}, ibu) //nolint:wrapcheck
}); err != nil {
return fmt.Errorf("failed get IBU cr: %w", err)
}

Expand Down
8 changes: 6 additions & 2 deletions utils/client_helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ import (

func GetSecretData(ctx context.Context, name, namespace, key string, client runtimeclient.Client) (string, error) {
secret := &corev1.Secret{}
if err := client.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, secret); err != nil {
if err := common.CallWithRetry(func() error {
return client.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, secret) //nolint:wrapcheck
}); err != nil {
// NOTE: The error is intentionally left unwrapped here, so the caller
// can check client.IgnoreNotFound on it
return "", err //nolint:wrapcheck
Expand All @@ -42,7 +44,9 @@ func GetSecretData(ctx context.Context, name, namespace, key string, client runt

func GetConfigMapData(ctx context.Context, name, namespace, key string, client runtimeclient.Client) (string, error) {
cm := &corev1.ConfigMap{}
if err := client.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, cm); err != nil {
if err := common.CallWithRetry(func() error {
return client.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, cm) //nolint:wrapcheck
}); err != nil {
return "", fmt.Errorf("failed to get get configMap: %w", err)
}

Expand Down

0 comments on commit 6841803

Please sign in to comment.