wrap this update call in a retry that ignores conflict errors

[github-actions]

rukpak/internal/source/local.go

Line 67 in 10bea78

// TODO: wrap this update call in a retry that ignores conflict errors

package source

import (
	"context"
	"fmt"
	"io/fs"
	"path/filepath"

	rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
	"github.com/operator-framework/rukpak/internal/util"

	"github.com/go-git/go-billy/v5/memfs"
	corev1 "k8s.io/api/core/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"sigs.k8s.io/controller-runtime/pkg/client"
)

type Local struct {
	client.Client
	// reader queries the API server directly
	reader client.Reader
}

func (o *Local) Unpack(ctx context.Context, bundle *rukpakv1alpha1.Bundle) (*Result, error) {
	if bundle.Spec.Source.Type != rukpakv1alpha1.SourceTypeLocal {
		return nil, fmt.Errorf("bundle source type %q not supported", bundle.Spec.Source.Type)
	}
	if bundle.Spec.Source.Local == nil {
		return nil, fmt.Errorf("bundle source local configuration is unset")
	}

	configMapRef := bundle.Spec.Source.Local.ConfigMapRef

	var cm corev1.ConfigMap
	if err := o.reader.Get(ctx, client.ObjectKey{Name: configMapRef.Name, Namespace: configMapRef.Namespace}, &cm); err != nil {
		return nil, fmt.Errorf("could not find configmap %s/%s on the cluster", configMapRef.Namespace, configMapRef.Name)
	}

	// If the configmap is empty, return early
	if len(cm.Data) == 0 {
		return nil, fmt.Errorf("configmap %s/%s is empty: at least one object is required", configMapRef.Namespace, configMapRef.Name)
	}

	var memFS = memfs.New()
	for filename, contents := range cm.Data {
		file, err := memFS.Create(filepath.Join("manifests", filename))
		if err != nil {
			return nil, fmt.Errorf("creating filesystem from configmap: %s", err)
		}
		_, err = file.Write([]byte(contents))
		if err != nil {
			return nil, fmt.Errorf("creating filesystem from configmap: %s", err)
		}
	}

	// Add an ownerref to the configmap based on the Bundle
	var ownerref = metav1.NewControllerRef(bundle, bundle.GroupVersionKind())
	cm.OwnerReferences = append(cm.OwnerReferences, *ownerref)
	// Update labels to reflect this ConfigMap is now managed by rukpak
	var labels = cm.GetLabels()
	if labels == nil {
		labels = make(map[string]string)
	}
	labels[util.CoreOwnerKindKey] = rukpakv1alpha1.BundleKind
	labels[util.CoreOwnerNameKey] = bundle.GetName()

	// TODO: wrap this update call in a retry that ignores conflict errors
	if err := o.Update(ctx, &cm); err != nil {
		return nil, fmt.Errorf("could not update configmap with bundle ownerreference: %s", err)
	}

	var bundleFS fs.FS = &billyFS{memFS}
	resolvedSource := &rukpakv1alpha1.BundleSource{
		Type:  rukpakv1alpha1.SourceTypeLocal,
		Local: bundle.Spec.Source.Local.DeepCopy(),
	}

	return &Result{Bundle: bundleFS, ResolvedSource: resolvedSource, State: StateUnpacked}, nil
}

[awgreene]

@exdx we reviewed this in the Issue Triage meeting. One concern that was raised focused on the fact that:

• Operators have to be configured to reconcile multiple events in parallel.
• A retry loop within the reconcile function could prevent other events from being reconciled until the retry has finished.
• Most conflict errors occur because a separate controller/entity has updated the resource.

@joelanford recommended:

• Checking the error if it is a conflict error
• Log the error at the debug level
• Return a Result where retry is set to true and a nil error (example: Result{retry:true}, nil)

Could you comment why it makes sense to retry and ignore error conflicts.

[timflannagan]

@exdx Do you remember the context for why this TODO was created? With Joe's #417 PR, will this become obsolete anyways?

[exdx]

I think this was created because updating the configmap could hit conflict errors, so it might be safer to wrap it in a retry (like we do in other places in the codebase). If we delete all this code, sure it will be obsolete.

[timflannagan]

Yeah, I think we can handle that conflict error at any call site though, so adding explicit wrapping logic to retry on conflict outside of the driver code may be the wrong implementation.

[exdx]

Sure. I didn't really expect an issue to be created regarding this, I can go ahead and remove the TODO to close this one.

[timflannagan]

@exdx Sounds good to me. I think automation will close this once the TODO comment is deleted too, which is nice.

[exdx]

Hmm yeah that's cool, let's see if it does.

Opened #465

[exdx]

Didn't auto-close, I'll just clean this up manually.

[timflannagan]

Yeah, it looks like the action silently failed sadly.