Skip to content

Commit 367ae9a

Browse files
mdzhigarovMarin Dzhigarov
mdzhigarov
and
Marin Dzhigarov
authored
Fix PKGI reconciliation taking old App status during upgrade (#1751)
* Fix PKGI reconciliation taking old App status during upgrade Signed-off-by: Marin Dzhigarov <[email protected]> * Addresses comments Signed-off-by: Marin Dzhigarov <[email protected]> --------- Signed-off-by: Marin Dzhigarov <[email protected]> Co-authored-by: Marin Dzhigarov <[email protected]>
1 parent bb948f6 commit 367ae9a

File tree

2 files changed

+306
-14
lines changed

2 files changed

+306
-14
lines changed

‎pkg/packageinstall/packageinstall.go‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -181,22 +181,25 @@ func (pi *PackageInstallCR) reconcile(modelStatus *reconciler.Status) (reconcile
181181
return reconcile.Result{Requeue: true}, err
182182
}
183183

184-
if existingApp.Generation != existingApp.Status.ObservedGeneration {
185-
modelStatus.SetReconciling(pi.model.ObjectMeta)
186-
} else {
187-
appStatus := reconciler.Status{S: existingApp.Status.GenericStatus}
188-
switch {
189-
case appStatus.IsReconciling():
184+
// Create a status updater closure that sets PackageInstall status based on App state
185+
statusUpdater := func(app *kcv1alpha1.App) {
186+
if app.Generation != app.Status.ObservedGeneration {
190187
modelStatus.SetReconciling(pi.model.ObjectMeta)
191-
case appStatus.IsReconcileSucceeded():
192-
modelStatus.SetReconcileCompleted(nil)
193-
case appStatus.IsReconcileFailed():
194-
modelStatus.SetUsefulErrorMessage(existingApp.Status.UsefulErrorMessage)
195-
modelStatus.SetReconcileCompleted(fmt.Errorf("Error (see .status.usefulErrorMessage for details)"))
188+
} else {
189+
appStatus := reconciler.Status{S: app.Status.GenericStatus}
190+
switch {
191+
case appStatus.IsReconciling():
192+
modelStatus.SetReconciling(pi.model.ObjectMeta)
193+
case appStatus.IsReconcileSucceeded():
194+
modelStatus.SetReconcileCompleted(nil)
195+
case appStatus.IsReconcileFailed():
196+
modelStatus.SetUsefulErrorMessage(app.Status.UsefulErrorMessage)
197+
modelStatus.SetReconcileCompleted(fmt.Errorf("Error (see .status.usefulErrorMessage for details)"))
198+
}
196199
}
197200
}
198201

199-
return pi.reconcileAppWithPackage(existingApp, pkg)
202+
return pi.reconcileAppWithPackage(existingApp, pkg, statusUpdater)
200203
}
201204

202205
func (pi *PackageInstallCR) createAppFromPackage(pkg datapkgingv1alpha1.Package) (reconcile.Result, error) {
@@ -213,7 +216,7 @@ func (pi *PackageInstallCR) createAppFromPackage(pkg datapkgingv1alpha1.Package)
213216
return reconcile.Result{}, nil
214217
}
215218

216-
func (pi *PackageInstallCR) reconcileAppWithPackage(existingApp *kcv1alpha1.App, pkg datapkgingv1alpha1.Package) (reconcile.Result, error) {
219+
func (pi *PackageInstallCR) reconcileAppWithPackage(existingApp *kcv1alpha1.App, pkg datapkgingv1alpha1.Package, statusUpdater func(*kcv1alpha1.App)) (reconcile.Result, error) {
217220
pkgWithPlaceholderSecrets, err := pi.reconcileFetchPlaceholderSecrets(pkg)
218221
if err != nil {
219222
return reconcile.Result{}, err
@@ -224,14 +227,20 @@ func (pi *PackageInstallCR) reconcileAppWithPackage(existingApp *kcv1alpha1.App,
224227
return reconcile.Result{Requeue: true}, err
225228
}
226229

230+
var appToCheckStatus *kcv1alpha1.App = existingApp
231+
227232
if !equality.Semantic.DeepEqual(desiredApp, existingApp) {
228-
_, err = pi.kcclient.KappctrlV1alpha1().Apps(desiredApp.Namespace).Update(
233+
updatedApp, err := pi.kcclient.KappctrlV1alpha1().Apps(desiredApp.Namespace).Update(
229234
context.Background(), desiredApp, metav1.UpdateOptions{})
230235
if err != nil {
231236
return reconcile.Result{Requeue: true}, err
232237
}
238+
appToCheckStatus = updatedApp
233239
}
234240

241+
// Call the status updater with the current app state (either existing or updated)
242+
statusUpdater(appToCheckStatus)
243+
235244
return reconcile.Result{}, nil
236245
}
237246

‎pkg/packageinstall/packageinstall_test.go‎

Lines changed: 283 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,12 @@ import (
2020
"github.com/stretchr/testify/require"
2121
corev1 "k8s.io/api/core/v1"
2222
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
23+
"k8s.io/apimachinery/pkg/runtime"
2324
"k8s.io/apimachinery/pkg/runtime/schema"
2425
"k8s.io/apimachinery/pkg/version"
2526
fakediscovery "k8s.io/client-go/discovery/fake"
2627
"k8s.io/client-go/kubernetes/fake"
28+
k8stesting "k8s.io/client-go/testing"
2729
logf "sigs.k8s.io/controller-runtime/pkg/log"
2830
)
2931

@@ -688,6 +690,287 @@ func Test_PlaceHolderSecretCreated_WhenPackageInstallUpdated(t *testing.T) {
688690
assert.Equal(t, "instl-pkg-fetch-0", app.Spec.Fetch[0].ImgpkgBundle.SecretRef.Name)
689691
}
690692

693+
// Test_StatusUpdaterClosureWithAppUpdateFromReconcileFailed tests this exact scenario:
694+
// 1. App exists in ReconcileFailed state (generation == observedGeneration)
695+
// 2. PackageInstall gets updated to reference a new package version
696+
// 3. This updates the App spec (and in real K8s would increment generation)
697+
// 4. PackageInstall status must now reflect the updated App state
698+
func Test_StatusUpdaterClosureWithAppUpdateFromReconcileFailed(t *testing.T) {
699+
log := logf.Log.WithName("kc")
700+
701+
// Create a package
702+
pkg := datapkgingv1alpha1.Package{
703+
ObjectMeta: metav1.ObjectMeta{
704+
Name: "test-pkg.2.0.0",
705+
},
706+
Spec: datapkgingv1alpha1.PackageSpec{
707+
RefName: "test-pkg",
708+
Version: "2.0.0",
709+
Template: datapkgingv1alpha1.AppTemplateSpec{
710+
Spec: &v1alpha1.AppSpec{
711+
Fetch: []v1alpha1.AppFetch{
712+
{
713+
ImgpkgBundle: &v1alpha1.AppFetchImgpkgBundle{
714+
Image: "test-pkg:2.0.0",
715+
},
716+
},
717+
},
718+
Template: []v1alpha1.AppTemplate{
719+
{
720+
Ytt: &v1alpha1.AppTemplateYtt{},
721+
},
722+
},
723+
Deploy: []v1alpha1.AppDeploy{
724+
{
725+
Kapp: &v1alpha1.AppDeployKapp{},
726+
},
727+
},
728+
},
729+
},
730+
},
731+
}
732+
733+
fakePkgClient := fakeapiserver.NewSimpleClientset(&pkg)
734+
735+
model := &pkgingv1alpha1.PackageInstall{
736+
ObjectMeta: metav1.ObjectMeta{
737+
Name: "test-pkg-install",
738+
},
739+
Spec: pkgingv1alpha1.PackageInstallSpec{
740+
PackageRef: &pkgingv1alpha1.PackageRef{
741+
RefName: "test-pkg",
742+
VersionSelection: &versions.VersionSelectionSemver{
743+
Constraints: "2.0.0",
744+
},
745+
},
746+
ServiceAccountName: "use-local-cluster-sa",
747+
},
748+
}
749+
750+
// Create an existing App that will be updated
751+
existingApp := &v1alpha1.App{
752+
ObjectMeta: metav1.ObjectMeta{
753+
Name: "test-pkg-install",
754+
Generation: 3,
755+
Annotations: map[string]string{
756+
"packaging.carvel.dev/package-ref-name": "test-pkg",
757+
"packaging.carvel.dev/package-version": "1.0.0", // Old version
758+
},
759+
},
760+
Spec: v1alpha1.AppSpec{
761+
Fetch: []v1alpha1.AppFetch{
762+
{
763+
ImgpkgBundle: &v1alpha1.AppFetchImgpkgBundle{
764+
Image: "test-pkg:1.0.0", // Old image
765+
},
766+
},
767+
},
768+
Template: []v1alpha1.AppTemplate{
769+
{
770+
Ytt: &v1alpha1.AppTemplateYtt{},
771+
},
772+
},
773+
Deploy: []v1alpha1.AppDeploy{
774+
{
775+
Kapp: &v1alpha1.AppDeployKapp{},
776+
},
777+
},
778+
ServiceAccountName: "use-local-cluster-sa",
779+
},
780+
Status: v1alpha1.AppStatus{
781+
GenericStatus: v1alpha1.GenericStatus{
782+
ObservedGeneration: 3, // Generation == ObservedGeneration
783+
Conditions: []v1alpha1.Condition{
784+
{
785+
Type: v1alpha1.ReconcileFailed,
786+
Status: corev1.ConditionTrue,
787+
},
788+
},
789+
FriendlyDescription: "Reconcile failed",
790+
UsefulErrorMessage: "Original error from v1.0.0",
791+
},
792+
},
793+
}
794+
795+
// Create a custom fake client that increments generation on update (like real K8s)
796+
fakekctrl := fakekappctrl.NewSimpleClientset(model, existingApp)
797+
798+
// Add a reactor to simulate generation increment on App updates
799+
fakekctrl.PrependReactor("update", "apps", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
800+
updateAction := action.(k8stesting.UpdateAction)
801+
if app, ok := updateAction.GetObject().(*v1alpha1.App); ok {
802+
// Simulate generation increment when App is updated (like real K8s)
803+
app.Generation = app.Generation + 1
804+
}
805+
return false, nil, nil // Let the default handler process the update
806+
})
807+
808+
fakek8s := fake.NewSimpleClientset()
809+
fakeDiscovery, _ := fakek8s.Discovery().(*fakediscovery.FakeDiscovery)
810+
fakeDiscovery.FakedServerVersion = &version.Info{
811+
GitVersion: "v0.20.0",
812+
}
813+
814+
ip := NewPackageInstallCR(model, log, fakekctrl, fakePkgClient, fakek8s,
815+
FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")}, Opts{},
816+
metrics.NewMetrics())
817+
818+
// Reconcile should update the App and set PackageInstall status
819+
_, err := ip.Reconcile()
820+
assert.Nil(t, err)
821+
822+
// Verify the App was updated
823+
gvr := schema.GroupVersionResource{"kappctrl.k14s.io", "v1alpha1", "apps"}
824+
obj, err := fakekctrl.Tracker().Get(gvr, "", "test-pkg-install")
825+
assert.Nil(t, err)
826+
require.NotNil(t, obj)
827+
updatedApp := obj.(*v1alpha1.App)
828+
829+
// Verify App was updated with new package content
830+
assert.Equal(t, "test-pkg:2.0.0", updatedApp.Spec.Fetch[0].ImgpkgBundle.Image)
831+
assert.Equal(t, "2.0.0", updatedApp.Annotations["packaging.carvel.dev/package-version"])
832+
assert.Equal(t, int64(4), updatedApp.Generation, "Generation should have incremented from 3 to 4")
833+
834+
// Verify PackageInstall status reflects the updated App state
835+
assert.Len(t, ip.model.Status.Conditions, 1)
836+
assert.Equal(t, v1alpha1.Reconciling, ip.model.Status.Conditions[0].Type)
837+
assert.Equal(t, corev1.ConditionTrue, ip.model.Status.Conditions[0].Status)
838+
assert.Equal(t, "Reconciling", ip.model.Status.FriendlyDescription)
839+
}
840+
841+
// Test_StatusUpdaterClosureWithNoAppUpdate verifies the closure works when App doesn't need updating:
842+
// 1. App exists in ReconcileFailed state (generation == observedGeneration)
843+
// 2. PackageInstall points to same package version as App
844+
// 3. No App update needed, so PackageInstall status should reflect existing App state
845+
func Test_StatusUpdaterClosureWithNoAppUpdate(t *testing.T) {
846+
log := logf.Log.WithName("kc")
847+
848+
// Create a package
849+
pkg := datapkgingv1alpha1.Package{
850+
ObjectMeta: metav1.ObjectMeta{
851+
Name: "test-pkg.1.0.0",
852+
},
853+
Spec: datapkgingv1alpha1.PackageSpec{
854+
RefName: "test-pkg",
855+
Version: "1.0.0",
856+
Template: datapkgingv1alpha1.AppTemplateSpec{
857+
Spec: &v1alpha1.AppSpec{
858+
Fetch: []v1alpha1.AppFetch{
859+
{
860+
ImgpkgBundle: &v1alpha1.AppFetchImgpkgBundle{
861+
Image: "test-pkg:1.0.0",
862+
},
863+
},
864+
},
865+
Template: []v1alpha1.AppTemplate{
866+
{
867+
Ytt: &v1alpha1.AppTemplateYtt{},
868+
},
869+
},
870+
Deploy: []v1alpha1.AppDeploy{
871+
{
872+
Kapp: &v1alpha1.AppDeployKapp{},
873+
},
874+
},
875+
},
876+
},
877+
},
878+
}
879+
880+
fakePkgClient := fakeapiserver.NewSimpleClientset(&pkg)
881+
882+
// Create a PackageInstall pointing to v1.0.0
883+
model := &pkgingv1alpha1.PackageInstall{
884+
ObjectMeta: metav1.ObjectMeta{
885+
Name: "test-pkg-install",
886+
},
887+
Spec: pkgingv1alpha1.PackageInstallSpec{
888+
PackageRef: &pkgingv1alpha1.PackageRef{
889+
RefName: "test-pkg",
890+
VersionSelection: &versions.VersionSelectionSemver{
891+
Constraints: "1.0.0",
892+
},
893+
},
894+
ServiceAccountName: "use-local-cluster-sa",
895+
},
896+
}
897+
898+
// Create an existing App in ReconcileFailed state with matching spec (no update needed)
899+
existingApp := &v1alpha1.App{
900+
ObjectMeta: metav1.ObjectMeta{
901+
Name: "test-pkg-install",
902+
Generation: 3,
903+
},
904+
Spec: v1alpha1.AppSpec{
905+
Fetch: []v1alpha1.AppFetch{
906+
{
907+
ImgpkgBundle: &v1alpha1.AppFetchImgpkgBundle{
908+
Image: "test-pkg:1.0.0", // Same as package spec
909+
},
910+
},
911+
},
912+
Template: []v1alpha1.AppTemplate{
913+
{
914+
Ytt: &v1alpha1.AppTemplateYtt{},
915+
},
916+
},
917+
Deploy: []v1alpha1.AppDeploy{
918+
{
919+
Kapp: &v1alpha1.AppDeployKapp{},
920+
},
921+
},
922+
ServiceAccountName: "use-local-cluster-sa",
923+
},
924+
Status: v1alpha1.AppStatus{
925+
GenericStatus: v1alpha1.GenericStatus{
926+
ObservedGeneration: 3, // Generation == ObservedGeneration (stable failed state)
927+
Conditions: []v1alpha1.Condition{
928+
{
929+
Type: v1alpha1.ReconcileFailed,
930+
Status: corev1.ConditionTrue,
931+
},
932+
},
933+
FriendlyDescription: "Reconcile failed",
934+
UsefulErrorMessage: "Deploy failed for v1.0.0",
935+
},
936+
},
937+
}
938+
939+
fakekctrl := fakekappctrl.NewSimpleClientset(model, existingApp)
940+
fakek8s := fake.NewSimpleClientset()
941+
942+
// mock the kubernetes server version
943+
fakeDiscovery, _ := fakek8s.Discovery().(*fakediscovery.FakeDiscovery)
944+
fakeDiscovery.FakedServerVersion = &version.Info{
945+
GitVersion: "v0.20.0",
946+
}
947+
948+
ip := NewPackageInstallCR(model, log, fakekctrl, fakePkgClient, fakek8s,
949+
FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")}, Opts{},
950+
metrics.NewMetrics())
951+
952+
// Reconcile should NOT update the App since specs match
953+
_, err := ip.Reconcile()
954+
assert.Nil(t, err)
955+
956+
// Verify the App was NOT updated (generation should remain the same)
957+
gvr := schema.GroupVersionResource{"kappctrl.k14s.io", "v1alpha1", "apps"}
958+
obj, err := fakekctrl.Tracker().Get(gvr, "", "test-pkg-install")
959+
assert.Nil(t, err)
960+
require.NotNil(t, obj)
961+
appAfterReconcile := obj.(*v1alpha1.App)
962+
963+
// App should be unchanged
964+
assert.Equal(t, int64(3), appAfterReconcile.Generation, "App generation should not have changed")
965+
assert.Equal(t, "test-pkg:1.0.0", appAfterReconcile.Spec.Fetch[0].ImgpkgBundle.Image)
966+
967+
// PackageInstall status should reflect the existing App state (ReconcileFailed)
968+
assert.Len(t, ip.model.Status.Conditions, 1)
969+
assert.Equal(t, v1alpha1.ReconcileFailed, ip.model.Status.Conditions[0].Type)
970+
assert.Equal(t, corev1.ConditionTrue, ip.model.Status.Conditions[0].Status)
971+
assert.Equal(t, "Deploy failed for v1.0.0", ip.model.Status.UsefulErrorMessage)
972+
}
973+
691974
func generatePackageWithConstraints(name, version, kcConstraint string, k8sConstraint string) datapkgingv1alpha1.Package {
692975
return datapkgingv1alpha1.Package{
693976
ObjectMeta: metav1.ObjectMeta{

0 commit comments

Comments
 (0)