From 73e4c791a9e0a5249be67db98a032f013cd4bf22 Mon Sep 17 00:00:00 2001 From: Brian Kane Date: Sun, 29 Mar 2026 05:37:45 +0100 Subject: [PATCH] Fix: rebuild appliedResources from ResourceTracker instead of filtering by name (#7083) appliedResources entries use resource names, not component names, so filtering them against component names incorrectly dropped valid entries. Rebuild directly from the current ResourceTracker each reconcile - already in memory, no extra API calls. Signed-off-by: Brian Kane --- .../application/application_controller.go | 34 +++---- .../v1beta1/application/evalstatus_test.go | 96 ++++--------------- pkg/resourcekeeper/resourcekeeper.go | 18 ++++ pkg/resourcekeeper/resourcekeeper_test.go | 62 ++++++++++++ 4 files changed, 110 insertions(+), 100 deletions(-) diff --git a/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go b/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go index 8774ea7db..b362afbb6 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/application_controller.go @@ -238,20 +238,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } handler.addServiceStatus(false, app.Status.Services...) - handler.addAppliedResource(true, app.Status.AppliedResources...) - app.Status.AppliedResources = handler.appliedResources app.Status.Services = handler.services - // Remove status entries for components that no longer exist in spec - filteredServices, filteredResources, componentsRemoved := filterRemovedComponentsFromStatus( + handler.addAppliedResource(true, app.Status.AppliedResources...) + app.Status.AppliedResources = handler.appliedResources + + // Remove services[] entries for components that no longer exist in spec + filteredServices, componentsRemoved := filterRemovedComponentsFromStatus( app.Spec.Components, app.Status.Services, - app.Status.AppliedResources, ) app.Status.Services = filteredServices - app.Status.AppliedResources = filteredResources handler.services = filteredServices - handler.appliedResources = filteredResources if componentsRemoved { logCtx.Info("Removed deleted components from status") @@ -316,6 +314,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu default: } + // Rebuild appliedResources from the ResourceTracker now that the workflow has finished + // dispatching. The RT is the authoritative source + app.Status.AppliedResources = handler.resourceKeeper.GetAppliedResources() + var phase = common.ApplicationRunning isHealthy := evalStatus(logCtx, handler, appFile, appParser) if !isHealthy { @@ -853,13 +855,12 @@ func setVelaVersion(app *v1beta1.Application) { } } -// filterRemovedComponentsFromStatus removes status entries for components no longer in spec. -// Returns filtered lists and whether any components were removed (used to determine Update vs Patch). +// filterRemovedComponentsFromStatus removes services[] entries for components no longer in spec. +// Returns filtered services and whether any were removed (used to determine Update vs Patch). func filterRemovedComponentsFromStatus( components []common.ApplicationComponent, services []common.ApplicationComponentStatus, - appliedResources []common.ClusterObjectReference, -) (filteredServices []common.ApplicationComponentStatus, filteredResources []common.ClusterObjectReference, removed bool) { +) (filteredServices []common.ApplicationComponentStatus, removed bool) { componentMap := make(map[string]struct{}, len(components)) for _, comp := range components { componentMap[comp.Name] = struct{}{} @@ -874,16 +875,7 @@ func filterRemovedComponentsFromStatus( } } - filteredResources = make([]common.ClusterObjectReference, 0, len(appliedResources)) - for _, res := range appliedResources { - if _, found := componentMap[res.Name]; found { - filteredResources = append(filteredResources, res) - } else { - removed = true - } - } - - return filteredServices, filteredResources, removed + return filteredServices, removed } func evalStatus(ctx monitorContext.Context, handler *AppHandler, appFile *appfile.Appfile, appParser *appfile.Parser) bool { diff --git a/pkg/controller/core.oam.dev/v1beta1/application/evalstatus_test.go b/pkg/controller/core.oam.dev/v1beta1/application/evalstatus_test.go index 3b266440f..b0b817e4f 100644 --- a/pkg/controller/core.oam.dev/v1beta1/application/evalstatus_test.go +++ b/pkg/controller/core.oam.dev/v1beta1/application/evalstatus_test.go @@ -23,7 +23,6 @@ import ( "cuelang.org/go/cue" "github.com/stretchr/testify/assert" - corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -185,16 +184,14 @@ func Test_applyComponentHealthToServices(t *testing.T) { func TestFilterRemovedComponentsFromStatus(t *testing.T) { tests := []struct { - name string - components []common.ApplicationComponent - statusServices []common.ApplicationComponentStatus - statusResources []common.ClusterObjectReference - expectedServices []string - expectedResources []string - componentsRemoved bool + name string + components []common.ApplicationComponent + statusServices []common.ApplicationComponentStatus + expectedServices []string + servicesRemoved bool }{ { - name: "removed components are filtered from status", + name: "removed component is filtered from services", components: []common.ApplicationComponent{ {Name: "backend", Type: "webservice"}, }, @@ -202,55 +199,21 @@ func TestFilterRemovedComponentsFromStatus(t *testing.T) { {Name: "frontend", Namespace: "default"}, {Name: "backend", Namespace: "default"}, }, - statusResources: []common.ClusterObjectReference{ - { - ObjectReference: corev1.ObjectReference{ - Name: "frontend", - Namespace: "default", - Kind: "Deployment", - }, - }, - { - ObjectReference: corev1.ObjectReference{ - Name: "backend", - Namespace: "default", - Kind: "Deployment", - }, - }, - }, - expectedServices: []string{"backend"}, - expectedResources: []string{"backend"}, - componentsRemoved: true, + expectedServices: []string{"backend"}, + servicesRemoved: true, }, { - name: "all components removed results in empty status", + name: "all components removed results in empty services", components: []common.ApplicationComponent{}, statusServices: []common.ApplicationComponentStatus{ {Name: "frontend", Namespace: "default"}, {Name: "backend", Namespace: "default"}, }, - statusResources: []common.ClusterObjectReference{ - { - ObjectReference: corev1.ObjectReference{ - Name: "frontend", - Namespace: "default", - Kind: "Deployment", - }, - }, - { - ObjectReference: corev1.ObjectReference{ - Name: "backend", - Namespace: "default", - Kind: "Deployment", - }, - }, - }, - expectedServices: []string{}, - expectedResources: []string{}, - componentsRemoved: true, + expectedServices: []string{}, + servicesRemoved: true, }, { - name: "no components removed keeps all status entries", + name: "no components removed keeps all services", components: []common.ApplicationComponent{ {Name: "frontend", Type: "webservice"}, {Name: "backend", Type: "webservice"}, @@ -259,38 +222,20 @@ func TestFilterRemovedComponentsFromStatus(t *testing.T) { {Name: "frontend", Namespace: "default"}, {Name: "backend", Namespace: "default"}, }, - statusResources: []common.ClusterObjectReference{ - { - ObjectReference: corev1.ObjectReference{ - Name: "frontend", - Namespace: "default", - Kind: "Deployment", - }, - }, - { - ObjectReference: corev1.ObjectReference{ - Name: "backend", - Namespace: "default", - Kind: "Deployment", - }, - }, - }, - expectedServices: []string{"frontend", "backend"}, - expectedResources: []string{"frontend", "backend"}, - componentsRemoved: false, + expectedServices: []string{"frontend", "backend"}, + servicesRemoved: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - filteredServices, filteredResources, componentsRemoved := filterRemovedComponentsFromStatus( + filteredServices, servicesRemoved := filterRemovedComponentsFromStatus( tt.components, tt.statusServices, - tt.statusResources, ) - assert.Equal(t, tt.componentsRemoved, componentsRemoved, - "componentsRemoved flag should match expected value") + assert.Equal(t, tt.servicesRemoved, servicesRemoved, + "servicesRemoved flag should match expected value") assert.Equal(t, len(tt.expectedServices), len(filteredServices), "filtered services count should match expected") @@ -298,13 +243,6 @@ func TestFilterRemovedComponentsFromStatus(t *testing.T) { assert.Equal(t, expectedName, filteredServices[i].Name, fmt.Sprintf("service at index %d should be %s", i, expectedName)) } - - assert.Equal(t, len(tt.expectedResources), len(filteredResources), - "filtered resources count should match expected") - for i, expectedName := range tt.expectedResources { - assert.Equal(t, expectedName, filteredResources[i].Name, - fmt.Sprintf("resource at index %d should be %s", i, expectedName)) - } }) } } diff --git a/pkg/resourcekeeper/resourcekeeper.go b/pkg/resourcekeeper/resourcekeeper.go index 26fdc4506..9d7ac0361 100644 --- a/pkg/resourcekeeper/resourcekeeper.go +++ b/pkg/resourcekeeper/resourcekeeper.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/oam-dev/kubevela/apis/core.oam.dev/common" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1alpha1" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" "github.com/oam-dev/kubevela/pkg/multicluster" @@ -45,6 +46,9 @@ type ResourceKeeper interface { DispatchComponentRevision(context.Context, *appsv1.ControllerRevision) error DeleteComponentRevision(context.Context, *appsv1.ControllerRevision) error + + // GetAppliedResources returns the current applied resources from the ResourceTracker. + GetAppliedResources() []common.ClusterObjectReference } type resourceKeeper struct { @@ -125,6 +129,20 @@ func (h *resourceKeeper) loadResourceTrackers(ctx context.Context) (err error) { return err } +// GetAppliedResources returns all resources from the current ResourceTracker as ClusterObjectReferences. +// Resources pending deletion (Deleted=true) are included as they still exist in the cluster. +// Returns an empty slice if no current ResourceTracker is loaded. +func (h *resourceKeeper) GetAppliedResources() []common.ClusterObjectReference { + if h._currentRT == nil { + return []common.ClusterObjectReference{} + } + refs := make([]common.ClusterObjectReference, 0, len(h._currentRT.Spec.ManagedResources)) + for _, mr := range h._currentRT.Spec.ManagedResources { + refs = append(refs, mr.ClusterObjectReference) + } + return refs +} + // NewResourceKeeper create a handler for dispatching and deleting resources func NewResourceKeeper(ctx context.Context, cli client.Client, app *v1beta1.Application) (_ ResourceKeeper, err error) { h := &resourceKeeper{ diff --git a/pkg/resourcekeeper/resourcekeeper_test.go b/pkg/resourcekeeper/resourcekeeper_test.go index 4e666c7b8..0a5131a1a 100644 --- a/pkg/resourcekeeper/resourcekeeper_test.go +++ b/pkg/resourcekeeper/resourcekeeper_test.go @@ -21,11 +21,14 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" + oamcommon "github.com/oam-dev/kubevela/apis/core.oam.dev/common" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" "github.com/oam-dev/kubevela/pkg/oam" "github.com/oam-dev/kubevela/pkg/oam/util" @@ -97,3 +100,62 @@ func TestNewResourceKeeper(t *testing.T) { r.NotNil(currentRT) r.Equal(3, len(rk._historyRTs)) } + +func TestGetAppliedResources(t *testing.T) { + ref := func(name, kind, component string, deleted bool) v1beta1.ManagedResource { + return v1beta1.ManagedResource{ + ClusterObjectReference: oamcommon.ClusterObjectReference{ + Creator: oamcommon.WorkflowResourceCreator, + ObjectReference: corev1.ObjectReference{ + Name: name, + Namespace: "default", + Kind: kind, + APIVersion: "v1", + }, + }, + OAMObjectReference: oamcommon.OAMObjectReference{Component: component}, + Deleted: deleted, + } + } + + tests := []struct { + name string + managedRes []v1beta1.ManagedResource + expectedNames []string + }{ + { + name: "returns all resources including pending-delete", + managedRes: []v1beta1.ManagedResource{ + ref("shared-config", "ConfigMap", "my-component", false), + ref("old-config", "ConfigMap", "removed-component", true), + }, + expectedNames: []string{"shared-config", "old-config"}, + }, + { + name: "returns empty when no current RT", + managedRes: nil, + expectedNames: []string{}, + }, + { + name: "returns empty when RT has no resources", + managedRes: []v1beta1.ManagedResource{}, + expectedNames: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rk := &resourceKeeper{} + if tt.managedRes != nil { + rk._currentRT = &v1beta1.ResourceTracker{ + Spec: v1beta1.ResourceTrackerSpec{ManagedResources: tt.managedRes}, + } + } + result := rk.GetAppliedResources() + require.Equal(t, len(tt.expectedNames), len(result)) + for i, name := range tt.expectedNames { + assert.Equal(t, name, result[i].Name) + } + }) + } +}