diff --git a/pkg/pkgrepository/crd_app.go b/pkg/pkgrepository/crd_app.go index f23981e0f..02a064982 100644 --- a/pkg/pkgrepository/crd_app.go +++ b/pkg/pkgrepository/crd_app.go @@ -17,6 +17,7 @@ import ( "carvel.dev/kapp-controller/pkg/template" "github.com/go-logr/logr" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -107,14 +108,19 @@ func (a *CRDApp) updateStatusOnce() error { func (a *CRDApp) updatePackageRepository(updateFunc func(*pkgingv1alpha1.PackageRepository)) error { a.log.Info("Updating PackageRepository") - existingRepo, err := a.appClient.PackagingV1alpha1().PackageRepositories(a.pkgrModel.Namespace).Get(context.Background(), a.pkgrModel.Name, metav1.GetOptions{}) - if err != nil { - return fmt.Errorf("Updating PackageRepository: %s", err) - } + // PackageRepository may be modified concurrently (e.g. by other controllers), + // so refetch and reapply changes when update is rejected due to a conflict + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + existingRepo, err := a.appClient.PackagingV1alpha1().PackageRepositories(a.pkgrModel.Namespace).Get(context.Background(), a.pkgrModel.Name, metav1.GetOptions{}) + if err != nil { + return err + } - updateFunc(existingRepo) + updateFunc(existingRepo) - _, err = a.appClient.PackagingV1alpha1().PackageRepositories(existingRepo.Namespace).Update(context.Background(), existingRepo, metav1.UpdateOptions{}) + _, err = a.appClient.PackagingV1alpha1().PackageRepositories(existingRepo.Namespace).Update(context.Background(), existingRepo, metav1.UpdateOptions{}) + return err + }) if err != nil { return fmt.Errorf("Updating PackageRepository: %s", err) } diff --git a/pkg/pkgrepository/crd_app_test.go b/pkg/pkgrepository/crd_app_test.go new file mode 100644 index 000000000..2a1a02463 --- /dev/null +++ b/pkg/pkgrepository/crd_app_test.go @@ -0,0 +1,100 @@ +// Copyright 2026 The Carvel Authors. +// SPDX-License-Identifier: Apache-2.0 + +package pkgrepository + +import ( + "context" + "fmt" + "testing" + + kcv1alpha1 "carvel.dev/kapp-controller/pkg/apis/kappctrl/v1alpha1" + pkgingv1alpha1 "carvel.dev/kapp-controller/pkg/apis/packaging/v1alpha1" + "carvel.dev/kapp-controller/pkg/client/clientset/versioned/fake" + "carvel.dev/kapp-controller/pkg/deploy" + "carvel.dev/kapp-controller/pkg/exec" + "carvel.dev/kapp-controller/pkg/fetch" + "carvel.dev/kapp-controller/pkg/kubeconfig" + "carvel.dev/kapp-controller/pkg/metrics" + "carvel.dev/kapp-controller/pkg/template" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + k8sfake "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" + logf "sigs.k8s.io/controller-runtime/pkg/log" +) + +func TestUnblockDeletionRetriesOnConflict(t *testing.T) { + pkgr := pkgrWithFinalizers() + kappcs := fake.NewSimpleClientset(&pkgr) + + updateAttempts := 0 + kappcs.PrependReactor("update", "packagerepositories", func(_ k8stesting.Action) (bool, runtime.Object, error) { + updateAttempts++ + if updateAttempts <= 2 { + return true, nil, errors.NewConflict(schema.GroupResource{Group: "packaging.carvel.dev", Resource: "packagerepositories"}, + pkgr.Name, fmt.Errorf("the object has been modified; please apply your changes to the latest version and try again")) + } + // Fall through to the default reactor to persist the update + return false, nil, nil + }) + + err := newTestCRDApp(t, &pkgr, kappcs).unblockDeletion() + require.NoError(t, err) + assert.Equal(t, 3, updateAttempts) + + updatedPkgr, err := kappcs.PackagingV1alpha1().PackageRepositories(pkgr.Namespace).Get(context.Background(), pkgr.Name, metav1.GetOptions{}) + require.NoError(t, err) + assert.Empty(t, updatedPkgr.Finalizers) +} + +func TestUnblockDeletionDoesNotRetryOnNonConflictError(t *testing.T) { + pkgr := pkgrWithFinalizers() + kappcs := fake.NewSimpleClientset(&pkgr) + + updateAttempts := 0 + kappcs.PrependReactor("update", "packagerepositories", func(_ k8stesting.Action) (bool, runtime.Object, error) { + updateAttempts++ + return true, nil, errors.NewInternalError(fmt.Errorf("etcd unavailable")) + }) + + err := newTestCRDApp(t, &pkgr, kappcs).unblockDeletion() + require.Error(t, err) + assert.Contains(t, err.Error(), "Updating PackageRepository: Internal error occurred: etcd unavailable") + assert.Equal(t, 1, updateAttempts) +} + +func pkgrWithFinalizers() pkgingv1alpha1.PackageRepository { + now := metav1.Now() + return pkgingv1alpha1.PackageRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "repo", + Namespace: "default", + DeletionTimestamp: &now, + Finalizers: []string{deleteFinalizerName, deletePrevFinalizerName}, + }, + Spec: pkgingv1alpha1.PackageRepositorySpec{ + Fetch: &pkgingv1alpha1.PackageRepositoryFetch{ + Inline: &kcv1alpha1.AppFetchInline{}, + }, + }, + } +} + +func newTestCRDApp(t *testing.T, pkgr *pkgingv1alpha1.PackageRepository, kappcs *fake.Clientset) *CRDApp { + log := logf.Log.WithName("kc") + + app, err := NewPackageRepoApp(pkgr) + require.NoError(t, err) + + k8scs := k8sfake.NewSimpleClientset() + fetchFac := fetch.NewFactory(k8scs, fetch.VendirOpts{}, exec.NewPlainCmdRunner()) + tmpFac := template.NewFactory(k8scs, fetchFac, false, exec.NewPlainCmdRunner()) + deployFac := deploy.NewFactory(k8scs, kubeconfig.NewKubeconfig(k8scs, log), nil, exec.NewPlainCmdRunner(), log) + + return NewCRDApp(app, pkgr, log, kappcs, metrics.NewMetrics(), fetchFac, tmpFac, deployFac) +}