diff --git a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go index f45ab00420..29a2affcbf 100644 --- a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go +++ b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder.go @@ -21,6 +21,7 @@ import ( "github.com/devtron-labs/devtron/util" "github.com/go-pg/pg" "go.uber.org/zap" + "golang.org/x/exp/slices" "strings" ) @@ -34,6 +35,15 @@ const ( ExternalChartStoreApp AppType = 3 // external helm app ) +const ( + // appStatusHibernating mirrors client/argocdServer/bean.HIBERNATING / appStatus.HealthStatusHibernatingFilter. + // Not imported directly to avoid pulling those packages into the query-builder layer. + appStatusHibernating = "HIBERNATING" + // pipelineDeploymentAppTypeHelm mirrors internal/util.PIPELINE_DEPLOYMENT_TYPE_HELM, kept local for + // the same reason (this file already inlines the sibling "manifest_download" literal below). + pipelineDeploymentAppTypeHelm = "helm" +) + type AppListingRepositoryQueryBuilder struct { logger *zap.SugaredLogger } @@ -318,18 +328,34 @@ func (impl AppListingRepositoryQueryBuilder) buildAppListingWhereCondition(appLi } } } + // AppStatus is only ever persisted to app_status (aps.status) for pipelines whose status has been + // synced there - today that is effectively ArgoCD/GitOps pipelines (see + // appStatus.AppStatusServiceImpl.UpdateStatusWithAppIdEnvId). Helm pipelines' status is instead + // resolved on the fly at listing time (AppListingServiceImpl.updateAppStatusForHelmTypePipelines), + // so a Helm pipeline that hasn't been synced to app_status has aps.status = NULL here, and "aps.status + // IN (...)" alone would wrongly exclude it from a HIBERNATING search. In that case only, let such + // unsynced rows through instead of excluding them outright, so the caller can resolve their status + // on the fly and recheck it against the filter after fetch (see + // AppListingServiceImpl.reconcileAppsWithUnresolvedStatus) - without weakening the filter for any + // other status, which is still matched purely against the persisted column. + statusCondition := " aps.status IN (?) " + statusConditionParams := []interface{}{pg.In(appStatusExcludingNotDeployed)} + if slices.Contains(appStatusExcludingNotDeployed, appStatusHibernating) { + statusCondition = " (aps.status IN (?) or (p.deployment_app_type = ? and aps.status IS NULL)) " + statusConditionParams = []interface{}{pg.In(appStatusExcludingNotDeployed), pipelineDeploymentAppTypeHelm} + } if isNotDeployedFilterApplied { deploymentAppType := "manifest_download" whereCondition += " and (p.deployment_app_created=? and (p.deployment_app_type <> ? or dc.deployment_app_type <> ? ) or a.id NOT IN (SELECT app_id from pipeline) " queryParams = append(queryParams, false, deploymentAppType, deploymentAppType) if len(appStatusExcludingNotDeployed) > 0 { - whereCondition += " or aps.status IN (?) " - queryParams = append(queryParams, pg.In(appStatusExcludingNotDeployed)) + whereCondition += " or " + statusCondition + queryParams = append(queryParams, statusConditionParams...) } whereCondition += " ) " } else if len(appStatusExcludingNotDeployed) > 0 { - whereCondition += " and aps.status IN (?) " - queryParams = append(queryParams, pg.In(appStatusExcludingNotDeployed)) + whereCondition += " and " + statusCondition + queryParams = append(queryParams, statusConditionParams...) } // Tag filters are AND-combined for now as requested by product. // Each row translates to a correlated EXISTS/NOT EXISTS on app_label. diff --git a/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_app_status_test.go b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_app_status_test.go new file mode 100644 index 0000000000..08a3dbad72 --- /dev/null +++ b/internal/sql/repository/helper/AppListingRepositoryQueryBuilder_app_status_test.go @@ -0,0 +1,57 @@ +package helper + +import ( + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +// When HIBERNATING is requested, Helm pipelines - whose status isn't persisted in app_status and is +// instead resolved on the fly at listing time - must not be excluded outright by the db-side predicate +// just because they have no app_status row yet (aps.status IS NULL). See +// AppListingServiceImpl.reconcileAppsWithUnresolvedStatus for how such rows are rechecked after fetch. +func TestBuildAppListingWhereCondition_HibernatingLetsUnresolvedHelmStatusThrough(t *testing.T) { + queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) + + whereClause, queryParams, err := queryBuilder.buildAppListingWhereCondition(AppListingFilter{ + AppStatuses: []string{"HIBERNATING"}, + }) + require.NoError(t, err) + + require.Contains(t, whereClause, "aps.status IN (?) or (p.deployment_app_type = ? and aps.status IS NULL)") + require.Len(t, queryParams, 4) + require.Equal(t, true, queryParams[0]) + require.Equal(t, CustomApp, queryParams[1]) + require.Equal(t, "helm", queryParams[3]) +} + +// A status filter that does not include HIBERNATING keeps matching purely against the persisted +// app_status column, as before - unresolved (NULL) rows are not specially let through for it. +func TestBuildAppListingWhereCondition_NonHibernatingStatusOnlyMatchesPersistedStatus(t *testing.T) { + queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) + + whereClause, queryParams, err := queryBuilder.buildAppListingWhereCondition(AppListingFilter{ + AppStatuses: []string{"Healthy"}, + }) + require.NoError(t, err) + + require.Contains(t, whereClause, "aps.status IN (?)") + require.NotContains(t, whereClause, "deployment_app_type") + require.Len(t, queryParams, 3) +} + +// HIBERNATING combined with NOT DEPLOYED still lets unresolved Helm rows through in the "or aps.status +// IN (...)" branch of the NOT DEPLOYED condition. +func TestBuildAppListingWhereCondition_HibernatingWithNotDeployed(t *testing.T) { + queryBuilder := NewAppListingRepositoryQueryBuilder(zap.NewNop().Sugar()) + + whereClause, queryParams, err := queryBuilder.buildAppListingWhereCondition(AppListingFilter{ + AppStatuses: []string{"HIBERNATING", "NOT DEPLOYED"}, + }) + require.NoError(t, err) + + require.Contains(t, whereClause, "(aps.status IN (?) or (p.deployment_app_type = ? and aps.status IS NULL))") + require.Len(t, queryParams, 7) + require.Equal(t, "helm", queryParams[6]) +} diff --git a/pkg/app/AppListingService.go b/pkg/app/AppListingService.go index 553443d97e..791129b280 100644 --- a/pkg/app/AppListingService.go +++ b/pkg/app/AppListingService.go @@ -438,16 +438,20 @@ func (impl AppListingServiceImpl) FetchAppsByEnvironmentV2(fetchAppListingReques return []*AppView.AppEnvironmentContainer{}, 0, nil } - // Currently AppStatus is available in Db for only ArgoApps - // We fetch AppStatus on the fly for Helm Apps from scoop, So AppStatus filter will be applied in last - // fun to check if "HIBERNATING" exists in fetchAppListingRequest.AppStatuses + // "HIBERNATING" is persisted in app_status (see appStatus.UpdateStatusWithAppIdEnvId) for pipelines + // whose status has been synced there - today that is effectively ArgoCD/GitOps pipelines. For those + // it is filtered+paginated in the db query itself along with the other statuses (see + // buildAppListingWhereCondition), so the count/pagination for that (majority) case is correct. + // + // Helm pipelines don't have their AppStatus persisted in app_status - it is instead fetched on the + // fly at listing time (impl.updateAppStatusForHelmTypePipelines, below), so a Helm pipeline that + // hasn't been synced to app_status has no way to be matched by the db-side "aps.status IN (...)" + // predicate. To avoid silently dropping such apps from a HIBERNATING search, the db query lets rows + // with no persisted app_status through instead of excluding them (buildAppListingWhereCondition), + // and reconcileAppsWithUnresolvedStatus below re-checks exactly those rows, once their status is + // resolved on the fly, against the requested filter - without touching the count/pagination that + // was already correctly computed in sql for the persisted-status rows. isFilteredOnHibernatingStatus := impl.isFilteredOnHibernatingStatus(fetchAppListingRequest) - // remove ""HIBERNATING" from fetchAppListingRequest.AppStatuses - appStatusesFilter := make([]string, 0) - if isFilteredOnHibernatingStatus { - appStatusesFilter = fetchAppListingRequest.AppStatuses - fetchAppListingRequest.AppStatuses = []string{} - } appListingFilter := helper.AppListingFilter{ Environments: fetchAppListingRequest.Environments, @@ -473,6 +477,21 @@ func (impl AppListingServiceImpl) FetchAppsByEnvironmentV2(fetchAppListingReques return []*AppView.AppEnvironmentContainer{}, appSize, err } + // A row whose app-status isn't persisted in db comes back with an empty AppStatus here (aps.status + // was NULL). Record which rows those are before updateAppStatusForHelmTypePipelines below + // potentially resolves an on-the-fly status for them, so that only those rows get rechecked against + // the requested filter afterwards - rows whose status was already persisted (and already correctly + // matched by the db query) are left exactly as sql returned them. + var containersWithUnresolvedStatus map[*AppView.AppEnvironmentContainer]bool + if isFilteredOnHibernatingStatus { + containersWithUnresolvedStatus = make(map[*AppView.AppEnvironmentContainer]bool) + for _, container := range envContainers { + if container.AppStatus == "" { + containersWithUnresolvedStatus[container] = true + } + } + } + envContainersMap := make(map[int][]*AppView.AppEnvironmentContainer) envIds := make([]int, 0) envsSet := make(map[int]bool) @@ -506,20 +525,37 @@ func (impl AppListingServiceImpl) FetchAppsByEnvironmentV2(fetchAppListingReques impl.Logger.Errorw("error, UpdateAppStatusForHelmTypePipelines", "envIds", envIds, "err", err) } - // apply filter for "HIBERNATING" status if isFilteredOnHibernatingStatus { - filteredContainers := make([]*AppView.AppEnvironmentContainer, 0) - for _, container := range envContainers { - if slices.Contains(appStatusesFilter, container.AppStatus) { - filteredContainers = append(filteredContainers, container) - } - } - envContainers = filteredContainers - appSize = len(filteredContainers) + envContainers, appSize = impl.reconcileAppsWithUnresolvedStatus(envContainers, appSize, containersWithUnresolvedStatus, fetchAppListingRequest.AppStatuses) } + return envContainers, appSize, nil } +// reconcileAppsWithUnresolvedStatus drops rows whose app-status was not persisted in db (identified by +// containersWithUnresolvedStatus, captured before the on-the-fly Helm status resolution) and that, once +// resolved, do not actually match the requested app-status filter (appStatusesFilter). appSize is +// decremented by exactly the number of rows dropped, so it is not recomputed wholesale - the count +// contributed by rows whose status IS persisted in db (already correctly filtered and paginated in sql) +// is left untouched. +func (impl AppListingServiceImpl) reconcileAppsWithUnresolvedStatus(envContainers []*AppView.AppEnvironmentContainer, appSize int, containersWithUnresolvedStatus map[*AppView.AppEnvironmentContainer]bool, appStatusesFilter []string) ([]*AppView.AppEnvironmentContainer, int) { + if len(containersWithUnresolvedStatus) == 0 { + return envContainers, appSize + } + filteredContainers := make([]*AppView.AppEnvironmentContainer, 0, len(envContainers)) + dropped := 0 + for _, container := range envContainers { + if containersWithUnresolvedStatus[container] && !slices.Contains(appStatusesFilter, container.AppStatus) { + // unresolved status (e.g. a Helm pipeline not yet synced to app_status) that, even after the + // on-the-fly fetch above, doesn't match what was asked for - exclude it. + dropped++ + continue + } + filteredContainers = append(filteredContainers, container) + } + return filteredContainers, appSize - dropped +} + func (impl AppListingServiceImpl) isFilteredOnHibernatingStatus(fetchAppListingRequest FetchAppListingRequest) bool { if fetchAppListingRequest.AppStatuses != nil && len(fetchAppListingRequest.AppStatuses) > 0 { if slices.Contains(fetchAppListingRequest.AppStatuses, bean2.HIBERNATING) { diff --git a/pkg/app/AppListingService_hibernating_filter_test.go b/pkg/app/AppListingService_hibernating_filter_test.go new file mode 100644 index 0000000000..26ca18735b --- /dev/null +++ b/pkg/app/AppListingService_hibernating_filter_test.go @@ -0,0 +1,65 @@ +package app + +import ( + "testing" + + "github.com/devtron-labs/devtron/api/bean/AppView" + "github.com/stretchr/testify/require" +) + +func TestIsFilteredOnHibernatingStatus(t *testing.T) { + impl := AppListingServiceImpl{} + + require.True(t, impl.isFilteredOnHibernatingStatus(FetchAppListingRequest{AppStatuses: []string{"Healthy", "HIBERNATING"}})) + require.False(t, impl.isFilteredOnHibernatingStatus(FetchAppListingRequest{AppStatuses: []string{"Healthy"}})) + require.False(t, impl.isFilteredOnHibernatingStatus(FetchAppListingRequest{})) +} + +// A container whose status was already persisted in db (and therefore already correctly matched and +// counted by the sql query) must never be touched by the in-memory reconciliation pass, even if its +// resolved AppStatus happens not to be an exact string match (defensive: containersWithUnresolvedStatus +// is the only signal used to decide whether a row is eligible for the recheck). +func TestReconcileAppsWithUnresolvedStatus_LeavesResolvedContainersUntouched(t *testing.T) { + impl := AppListingServiceImpl{} + dbBacked := &AppView.AppEnvironmentContainer{AppId: 1, AppStatus: "HIBERNATING"} + + envContainers := []*AppView.AppEnvironmentContainer{dbBacked} + result, appSize := impl.reconcileAppsWithUnresolvedStatus(envContainers, 10, nil, []string{"HIBERNATING"}) + + require.Equal(t, envContainers, result) + require.Equal(t, 10, appSize) +} + +// A Helm-type row with no persisted app_status (let through by the db query as an unresolved row) is +// dropped, and appSize is decremented by exactly the number of rows dropped, once its on-the-fly status +// turns out not to match the requested filter. +func TestReconcileAppsWithUnresolvedStatus_DropsNonMatchingUnresolvedContainers(t *testing.T) { + impl := AppListingServiceImpl{} + matching := &AppView.AppEnvironmentContainer{AppId: 1, AppStatus: "HIBERNATING"} // resolved on the fly, matches + nonMatching := &AppView.AppEnvironmentContainer{AppId: 2, AppStatus: "Healthy"} // resolved on the fly, doesn't match + unresolvedStillEmpty := &AppView.AppEnvironmentContainer{AppId: 3, AppStatus: ""} // on-the-fly fetch was a no-op / found nothing + dbBackedArgoApp := &AppView.AppEnvironmentContainer{AppId: 4, AppStatus: "HIBERNATING"} // already matched by sql + + envContainers := []*AppView.AppEnvironmentContainer{matching, nonMatching, unresolvedStillEmpty, dbBackedArgoApp} + containersWithUnresolvedStatus := map[*AppView.AppEnvironmentContainer]bool{ + matching: true, + nonMatching: true, + unresolvedStillEmpty: true, + } + + result, appSize := impl.reconcileAppsWithUnresolvedStatus(envContainers, 4, containersWithUnresolvedStatus, []string{"HIBERNATING"}) + + require.Equal(t, []*AppView.AppEnvironmentContainer{matching, dbBackedArgoApp}, result) + require.Equal(t, 2, appSize) +} + +func TestReconcileAppsWithUnresolvedStatus_NoUnresolvedContainersIsNoOp(t *testing.T) { + impl := AppListingServiceImpl{} + container := &AppView.AppEnvironmentContainer{AppId: 1, AppStatus: "HIBERNATING"} + envContainers := []*AppView.AppEnvironmentContainer{container} + + result, appSize := impl.reconcileAppsWithUnresolvedStatus(envContainers, 1, map[*AppView.AppEnvironmentContainer]bool{}, []string{"HIBERNATING"}) + + require.Equal(t, envContainers, result) + require.Equal(t, 1, appSize) +}