From d3bd71ce79f9c0127f6707de4b0ce8872d8e664d Mon Sep 17 00:00:00 2001 From: NopeCharon <115794254+NopeCharon@users.noreply.github.com> Date: Sat, 24 Oct 2026 03:37:29 +0000 Subject: [PATCH] Retry PackageRepository updates on conflict Adding and removing the PackageRepository deletion finalizer is done by fetching the latest PackageRepository and immediately updating it. If the resource is modified by someone else between those two calls (e.g. a status update or another controller touching it), the API server rejects the update with "the object has been modified; please apply your changes to the latest version and try again". When that happened while removing the finalizer, deletion failed with "Unblocking for deploy: Updating PackageRepository: ..." and the PackageRepository stayed stuck until the next reconcile. App CRs already retry such updates (see pkg/app/crd_app.go). Wrap the PackageRepository get-and-update in retry.RetryOnConflict so that it is refetched and the change is reapplied on conflicts, while other errors are still returned immediately. Add unit tests verifying that the finalizer is removed after transient conflicts and that non-conflict errors are not retried. Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com> --- pkg/pkgrepository/crd_app.go | 18 ++++-- pkg/pkgrepository/crd_app_test.go | 100 ++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 6 deletions(-) create mode 100644 pkg/pkgrepository/crd_app_test.go diff --git a/pkg/pkgrepository/crd_app.go b/pkg/pkgrepository/crd_app.go index f23981e0ff..02a064982b 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 0000000000..2a1a024635 --- /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) +}