Replace custom retry logic with a single retry function

[phillebaba]

Describe what should be investigated or refactored

We have a couple of ways to implement retry logic in Zarf. The most common are use of timers or the retry function form the helpers package. The former respects contexts while the latter does not. It would be better if we could decide on a single retry package to use in this function to simplify things.

A suggestion is to switch to using retry-go. This project has been around for a while and is pretty stable.
https://github.com/avast/retry-go

Links to any relevant code

zarf/src/pkg/cluster/cluster.go

Lines 82 to 120 in 5e77e38

	func waitForHealthyCluster(ctx context.Context, client kubernetes.Interface) error {
	const waitDuration = 1 * time.Second
	timer := time.NewTimer(0)
	defer timer.Stop()
	for {
	select {
	case <-ctx.Done():
	return fmt.Errorf("error waiting for cluster to report healthy: %w", ctx.Err())
	case <-timer.C:
	// Make sure there is at least one running Node
	nodeList, err := client.CoreV1().Nodes().List(ctx, metav1.ListOptions{})
	if err != nil || len(nodeList.Items) < 1 {
	message.Debugf("No nodes reporting healthy yet: %v\n", err)
	timer.Reset(waitDuration)
	continue
	}
	// Get the cluster pod list
	pods, err := client.CoreV1().Pods(corev1.NamespaceAll).List(ctx, metav1.ListOptions{})
	if err != nil {
	message.Debugf("Could not get the pod list: %v", err)
	timer.Reset(waitDuration)
	continue
	}
	// Check that at least one pod is in the 'succeeded' or 'running' state
	for _, pod := range pods.Items {
	if pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodRunning {
	return nil
	}
	}
	message.Debug("No pods reported 'succeeded' or 'running' state yet.")
	timer.Reset(waitDuration)
	}
	}
	}

zarf/src/internal/packager/images/push.go

Line 57 in 5e77e38

if err := helpers.Retry(func() error {

zarf/src/pkg/cluster/namespace.go

Lines 30 to 46 in 5e77e38

	timer := time.NewTimer(0)
	defer timer.Stop()
	for {
	select {
	case <-ctx.Done():
	return ctx.Err()
	case <-timer.C:
	_, err := c.Clientset.CoreV1().Namespaces().Get(ctx, ZarfNamespaceName, metav1.GetOptions{})
	if kerrors.IsNotFound(err) {
	return nil
	}
	if err != nil {
	return err
	}
	timer.Reset(1 * time.Second)
	}
	}

zarf/src/pkg/cluster/state.go

Lines 132 to 154 in 5e77e38

	saCtx, cancel := context.WithTimeout(ctx, 2*time.Minute)
	defer cancel()
	err = func(ctx context.Context, ns, name string) error {
	timer := time.NewTimer(0)
	defer timer.Stop()
	for {
	select {
	case <-ctx.Done():
	return fmt.Errorf("failed to get service account %s/%s: %w", ns, name, ctx.Err())
	case <-timer.C:
	_, err := c.Clientset.CoreV1().ServiceAccounts(ns).Get(ctx, name, metav1.GetOptions{})
	if err != nil && !kerrors.IsNotFound(err) {
	return err
	}
	if kerrors.IsNotFound(err) {
	message.Debug("Service account %s/%s not found, retrying...", ns, name)
	timer.Reset(1 * time.Second)
	continue
	}
	return nil
	}
	}
	}(saCtx, ZarfNamespaceName, "default")

Additional context

N/A

[AustinAbro321]

Small note that there is a helpers.RetryWithContext function we use, though from a glance at the retry-go package it seems like a reasonable substitute for both.

zarf/src/internal/packager/images/pull.go

Line 243 in 5e77e38

if err := helpers.RetryWithContext(ctx, sc, 2, 5*time.Second, message.Warnf); err != nil {

[phillebaba]

Was not aware of that function, that would also be an option. We just need to look at the pros and cons of bringing in a dependency to do this.