From e2113cde0901d58ff4ee66b6dfc2f03bcce24711 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Thu, 17 Sep 2026 17:41:11 +0200 Subject: [PATCH 01/16] RHINENG-26122: remove aggregator DriftCheck --- aggregator/drift_check.go | 67 ----------------------- aggregator/drift_check_test.go | 61 --------------------- aggregator/events.go | 2 - tasks/caches/backfill_account_advisory.go | 2 - 4 files changed, 132 deletions(-) delete mode 100644 aggregator/drift_check.go delete mode 100644 aggregator/drift_check_test.go diff --git a/aggregator/drift_check.go b/aggregator/drift_check.go deleted file mode 100644 index 54ff8aa9c..000000000 --- a/aggregator/drift_check.go +++ /dev/null @@ -1,67 +0,0 @@ -package aggregator - -import ( - "app/base/database" - "app/base/utils" -) - -type advisoryCounts struct { - AdvisoryID int64 - SystemsApplicable int - SystemsInstallable int -} - -func CheckAdvisoryDrift(rhAccountID int, advisoryIDs []int64) { - if len(advisoryIDs) == 0 { - return - } - - var newCounts []advisoryCounts - err := database.DB.Table("account_advisory aa"). - Select(`aa.advisory_id, - SUM(aa.systems_installable) as systems_installable, - SUM(aa.systems_applicable) as systems_applicable`). - Where("aa.rh_account_id = ? AND aa.advisory_id IN (?)", rhAccountID, advisoryIDs). - Group("aa.advisory_id"). - Find(&newCounts).Error - if err != nil { - utils.LogError("err", err, "rh_account_id", rhAccountID, "drift check: failed to query account_advisory") - return - } - - newCountsMap := make(map[int64]advisoryCounts, len(newCounts)) - for _, c := range newCounts { - newCountsMap[c.AdvisoryID] = c - } - - var legacyCounts []advisoryCounts - err = database.DB.Table("advisory_account_data"). - Select("advisory_id, systems_applicable, systems_installable"). - Where("rh_account_id = ? AND advisory_id IN (?)", rhAccountID, advisoryIDs). - Find(&legacyCounts).Error - if err != nil { - utils.LogError("err", err, "rh_account_id", rhAccountID, "drift check: failed to query advisory_account_data") - return - } - - for _, legacy := range legacyCounts { - newVal, ok := newCountsMap[legacy.AdvisoryID] - if !ok { - utils.LogWarn("rh_account_id", rhAccountID, "advisory_id", legacy.AdvisoryID, - "drift check: advisory present in legacy table but missing from account_advisory") - continue - } - if legacy.SystemsApplicable != newVal.SystemsApplicable || legacy.SystemsInstallable != newVal.SystemsInstallable { - utils.LogWarn("rh_account_id", rhAccountID, "advisory_id", legacy.AdvisoryID, - "legacy_applicable", legacy.SystemsApplicable, "new_applicable", newVal.SystemsApplicable, - "legacy_installable", legacy.SystemsInstallable, "new_installable", newVal.SystemsInstallable, - "drift check: count mismatch between legacy and new table") - } - delete(newCountsMap, legacy.AdvisoryID) - } - - for _, newVal := range newCountsMap { - utils.LogWarn("rh_account_id", rhAccountID, "advisory_id", newVal.AdvisoryID, - "drift check: advisory present in account_advisory but missing from legacy table") - } -} diff --git a/aggregator/drift_check_test.go b/aggregator/drift_check_test.go deleted file mode 100644 index 6eec2d99c..000000000 --- a/aggregator/drift_check_test.go +++ /dev/null @@ -1,61 +0,0 @@ -package aggregator - -import ( - "app/base/core" - "app/base/database" - "app/base/models" - "app/base/utils" - "testing" - - log "github.com/sirupsen/logrus" - "github.com/stretchr/testify/assert" -) - -func TestCheckAdvisoryDriftCountMismatch(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - - assert.Nil(t, database.DB.Exec("SELECT refresh_advisory_caches(NULL, 1)").Error) - assert.Nil(t, database.DB.Exec("SELECT backfill_account_advisory(1)").Error) - defer database.DeleteAccountAdvisoryByAccount(t, 1) - - assert.Nil(t, database.DB.Model(&models.AccountAdvisory{}). - Where("advisory_id = 1 AND rh_account_id = 1"). - Update("systems_installable", 999).Error) - - hook := utils.NewTestLogHook(log.WarnLevel) - log.AddHook(hook) - - CheckAdvisoryDrift(1, []int64{1}) - - found := false - for _, entry := range hook.LogEntries { - if entry.Message == "drift check: count mismatch between legacy and new table" { - found = true - break - } - } - assert.True(t, found, "expected count mismatch warning") -} - -func TestCheckAdvisoryDriftMissingFromNew(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - - assert.Nil(t, database.DB.Exec("SELECT refresh_advisory_caches(NULL, 1)").Error) - defer database.DeleteAccountAdvisoryByAccount(t, 1) - - hook := utils.NewTestLogHook(log.WarnLevel) - log.AddHook(hook) - - CheckAdvisoryDrift(1, []int64{1, 2}) - - found := false - for _, entry := range hook.LogEntries { - if entry.Message == "drift check: advisory present in legacy table but missing from account_advisory" { - found = true - break - } - } - assert.True(t, found, "expected missing from new table warning") -} diff --git a/aggregator/events.go b/aggregator/events.go index 30930b827..51493cd1b 100644 --- a/aggregator/events.go +++ b/aggregator/events.go @@ -94,8 +94,6 @@ func processAdvisoryBatch(grouped map[int][]int64) { continue } - CheckAdvisoryDrift(rhAccountID, advisoryIDs) - if err := publishNewAdvisoryNotification(rhAccountID, advisoryIDs); err != nil { utils.LogError("err", err, "rh_account_id", rhAccountID, "failed to publish new advisory notification") } diff --git a/tasks/caches/backfill_account_advisory.go b/tasks/caches/backfill_account_advisory.go index ff762783e..479b723bb 100644 --- a/tasks/caches/backfill_account_advisory.go +++ b/tasks/caches/backfill_account_advisory.go @@ -1,7 +1,6 @@ package caches import ( - "app/aggregator" "app/base/database" "app/base/utils" "app/tasks" @@ -59,7 +58,6 @@ func backfillAccountAdvisoryPerAccounts(wg *sync.WaitGroup) { utils.LogError("err", err, "rh_account_id", rhAccountID, "failed to load advisory IDs for drift check") return } - aggregator.CheckAdvisoryDrift(rhAccountID, advisoryIDs) }(i, rhAccountID) } } From b036fc8d09f2fa6eec208725417453860e143266 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Thu, 24 Sep 2026 11:11:50 +0200 Subject: [PATCH 02/16] RHINENG-26122: revert (#2289) --- base/mqueue/platform_event.go | 51 ++++------- base/mqueue/platform_event_test.go | 53 ------------ deploy/clowdapp.yaml | 53 +----------- evaluator/evaluate.go | 8 +- evaluator/notifications.go | 35 ++++---- evaluator/notifications_test.go | 84 +------------------ main.go | 3 - tasks/config.go | 2 - .../system_advisories_0_recovery/recovery.go | 77 ----------------- .../recovery_test.go | 60 ------------- 10 files changed, 41 insertions(+), 385 deletions(-) delete mode 100644 tasks/system_advisories_0_recovery/recovery.go delete mode 100644 tasks/system_advisories_0_recovery/recovery_test.go diff --git a/base/mqueue/platform_event.go b/base/mqueue/platform_event.go index b138c1358..c7ad07c7b 100644 --- a/base/mqueue/platform_event.go +++ b/base/mqueue/platform_event.go @@ -22,10 +22,6 @@ type PlatformEvent struct { URL *string `json:"url"` SystemIDs []uuid.UUID `json:"system_ids,omitempty"` RequestIDs []string `json:"request_ids,omitempty"` - // SkipNotifications suppresses instant advisory notification publish for this event. - // Evaluator still marks matching advisory_account_data.notified so later evals do not flood. - // Used by recovery recalc; omit/false for normal upload/recalc traffic. - SkipNotifications bool `json:"skip_notifications,omitempty"` } type EvalData struct { @@ -76,18 +72,16 @@ func writePlatformEvents(ctx context.Context, w Writer, events ...PlatformEvent) return w.WriteMessages(ctx, msgs...) } -func batchCount(grouped map[int][]uuid.UUID, size int) int { - if size <= 0 { - size = BatchSize - } +func batchSize(grouped map[int][]uuid.UUID) int { + // compute how many batches we will create var batches = 0 for _, ev := range grouped { - batches += (len(ev) + size - 1) / size + batches += len(ev)/BatchSize + 1 } return batches } -func (evals EvalDataSlice) getAccountEvalData(size int) (int, accountInventories, accountRequests, orgIDs) { +func (evals EvalDataSlice) getAccountEvalData() (int, accountInventories, accountRequests, orgIDs) { // group systems by account invs := accountInventories{} reqs := accountRequests{} @@ -99,41 +93,30 @@ func (evals EvalDataSlice) getAccountEvalData(size int) (int, accountInventories orgs[e.RhAccountID] = e.OrgID } } - return batchCount(invs, size), invs, reqs, orgs + return batchSize(invs), invs, reqs, orgs } func (evals EvalDataSlice) WriteEvents(ctx context.Context, w Writer) error { - return evals.writeEvents(ctx, w, BatchSize, false) -} - -// WriteEventsSkipNotifications publishes recalc events in batches of batchSize systems -// per account with SkipNotifications set. Used by one-off recovery jobs. -func (evals EvalDataSlice) WriteEventsSkipNotifications(ctx context.Context, w Writer, batchSize int) error { - return evals.writeEvents(ctx, w, batchSize, true) -} - -func (evals EvalDataSlice) writeEvents(ctx context.Context, w Writer, size int, skipNotifications bool) error { - if size <= 0 { - size = BatchSize - } - batches, accInvs, reqs, orgs := evals.getAccountEvalData(size) + batches, accInvs, reqs, orgs := evals.getAccountEvalData() + // create events, per BatchSize of systems from one account now := types.Rfc3339Timestamp(time.Now()) events := make(PlatformEvents, 0, batches) for acc, invs := range accInvs { - for start := 0; start < len(invs); start += size { - end := start + size + for start := 0; start < len(invs); start += BatchSize { + end := start + BatchSize if end > len(invs) { end = len(invs) } events = append(events, PlatformEvent{ - Timestamp: &now, - AccountID: acc, - SystemIDs: invs[start:end], - RequestIDs: reqs[acc][start:end], - OrgID: orgs[acc], - SkipNotifications: skipNotifications, + Timestamp: &now, + AccountID: acc, + SystemIDs: invs[start:end], + RequestIDs: reqs[acc][start:end], + OrgID: orgs[acc], }) } } - return writePlatformEvents(ctx, w, events...) + // write events to queue + err := writePlatformEvents(ctx, w, events...) + return err } diff --git a/base/mqueue/platform_event_test.go b/base/mqueue/platform_event_test.go index 7f9cf3923..dc0f8b10e 100644 --- a/base/mqueue/platform_event_test.go +++ b/base/mqueue/platform_event_test.go @@ -9,31 +9,6 @@ import ( "github.com/stretchr/testify/assert" ) -func TestPlatformEventSkipNotificationsJSON(t *testing.T) { - orgID := "org_1" - event := PlatformEvent{ - AccountID: 1, - OrgID: &orgID, - SkipNotifications: true, - } - data, err := sonic.Marshal(event) - assert.NoError(t, err) - - var parsed PlatformEvent - assert.NoError(t, sonic.Unmarshal(data, &parsed)) - assert.True(t, parsed.SkipNotifications) - - // omitempty: false must not appear in JSON; fresh unmarshal defaults to false - event.SkipNotifications = false - data, err = sonic.Marshal(event) - assert.NoError(t, err) - assert.NotContains(t, string(data), "skip_notifications") - - var parsedFalse PlatformEvent - assert.NoError(t, sonic.Unmarshal(data, &parsedFalse)) - assert.False(t, parsedFalse.SkipNotifications) -} - func TestWriteEventsOfInventoryAccounts(t *testing.T) { var ( acc = 1 @@ -60,32 +35,4 @@ func TestWriteEventsOfInventoryAccounts(t *testing.T) { assert.True(t, len(event.SystemIDs) == 2) assert.Equal(t, inv2, event.SystemIDs[0]) assert.Equal(t, inv3, event.SystemIDs[1]) - assert.False(t, event.SkipNotifications) -} - -func TestWriteEventsSkipNotificationsChunking(t *testing.T) { - acc := 7 - orgID := "org_recovery" - invs := make(EvalDataSlice, 0, 501) - for i := 0; i < 501; i++ { - invs = append(invs, EvalData{ - InventoryID: uuid.New(), - RhAccountID: acc, - OrgID: &orgID, - }) - } - - writer := &MockKafkaWriter{} - assert.NoError(t, invs.WriteEventsSkipNotifications(context.Background(), writer, 500)) - assert.Equal(t, 2, len(writer.Messages)) - - var first, second PlatformEvent - assert.NoError(t, sonic.Unmarshal(writer.Messages[0].Value, &first)) - assert.NoError(t, sonic.Unmarshal(writer.Messages[1].Value, &second)) - assert.True(t, first.SkipNotifications) - assert.True(t, second.SkipNotifications) - assert.Equal(t, 500, len(first.SystemIDs)) - assert.Equal(t, 1, len(second.SystemIDs)) - assert.Equal(t, acc, first.AccountID) - assert.Equal(t, orgID, first.GetOrgID()) } diff --git a/deploy/clowdapp.yaml b/deploy/clowdapp.yaml index 706c3516d..df05d3497 100644 --- a/deploy/clowdapp.yaml +++ b/deploy/clowdapp.yaml @@ -622,39 +622,6 @@ objects: key: vmaas-sync-database-password}}} - {name: POD_CONFIG, value: '${JOBS_CONFIG}'} - - name: system-advisories-0-recovery - # One-shot Job (no schedule): runs on deploy / CJI like db-migration. - # No-op unless JOBS_CONFIG includes system_advisories_0_recovery=true. - completions: 1 - parallelism: 1 - activeDeadlineSeconds: ${{JOBS_TIMEOUT}} - podSpec: - image: ${IMAGE}:${IMAGE_TAG} - initContainers: - - name: check-for-db - image: ${IMAGE}:${IMAGE_TAG} - command: - - ./database_admin/check-upgraded.sh - env: - - {name: POD_CONFIG, value: '${DATABASE_ADMIN_CONFIG}'} - command: - - ./scripts/entrypoint.sh - - job - - system_advisories_0_recovery - env: - - {name: LOG_LEVEL, value: '${LOG_LEVEL_JOBS}'} - - {name: GIN_MODE, value: '${GIN_MODE}'} - - {name: SENTRY_DSN, valueFrom: {secretKeyRef: {name: patchman-sentry, key: sentry-dsn}}} - - {name: DB_DEBUG, value: '${DB_DEBUG_JOBS}'} - - {name: DB_USER, value: vmaas_sync} - - {name: DB_PASSWD, valueFrom: {secretKeyRef: {name: patchman-engine-database-passwords, - key: vmaas-sync-database-password}}} - - {name: KAFKA_GROUP, value: patchman} - - {name: KAFKA_WRITER_MAX_ATTEMPTS, value: '${KAFKA_WRITER_MAX_ATTEMPTS}'} - - {name: EVAL_TOPIC, value: patchman.evaluator.recalc} - - {name: SSL_CERT_DIR, value: '${SSL_CERT_DIR}'} - - {name: POD_CONFIG, value: '${JOBS_CONFIG}'} # set system_advisories_0_recovery=true for cutover - database: name: patchman version: 16 @@ -698,22 +665,6 @@ objects: jobs: - db-migration -# One-off system_advisories_0 recovery recalc. Keep disabled; enable for cutover deploy with -# JOBS_CONFIG=system_advisories_0_recovery=true, then disable again. -- apiVersion: cloud.redhat.com/v1alpha1 - kind: ClowdJobInvocation - metadata: - annotations: - clowder.redhat.com/expected-image-tag: ${IMAGE_TAG} - labels: - app: patchman - name: system-advisories-0-recovery-${IMAGE_TAG}-${CJI_UID} - spec: - appName: patchman - disabled: ${{SA0_RECOVERY_DISABLED}} - jobs: - - system-advisories-0-recovery - - apiVersion: metrics.console.redhat.com/v1alpha1 kind: FloorPlan metadata: @@ -927,7 +878,7 @@ parameters: - {name: JOBS_TIMEOUT, value: '1800'} # 30 min timeout for jobs - {name: PROMETHEUS_PUSHGATEWAY, required: true, value: "pushgateway"} - {name: DB_READ_REPLICA_ENABLED_JOBS, value: 'TRUE'} -- {name: JOBS_CONFIG, value: ''} # e.g. system_advisories_0_recovery=true for SA0 recovery job cutover +- {name: JOBS_CONFIG, value: ''} # DB migration - {name: DB_MIGRATION_DISABLED, value: 'false'} # Disable db-migration job execution @@ -935,8 +886,6 @@ parameters: description: Unique db-migration CJI name suffix generate: expression from: '[a-z0-9]{6}' -# system_advisories_0 recovery recalc CJI (one-off; keep disabled unless cutover) -- {name: SA0_RECOVERY_DISABLED, value: 'true'} # set false + JOBS_CONFIG=system_advisories_0_recovery=true for cutover # VMaaS sync - {name: VMAAS_SYNC_SCHEDULE, value: '*/5 * * * *'} # Cronjob schedule definition - {name: VMAAS_SYNC_SUSPEND, value: 'false'} # Disable cronjob execution diff --git a/evaluator/evaluate.go b/evaluator/evaluate.go index 24ad47102..e4d953480 100644 --- a/evaluator/evaluate.go +++ b/evaluator/evaluate.go @@ -523,11 +523,9 @@ func evaluateAndStore(system *models.SystemPlatformV2, } } - // Instant notifications, or mark-notified only when the event opts out of publishing - // (e.g. recovery recalc with skip_notifications). - if event.SkipNotifications || enableInstantNotifications { - err = publishNewAdvisoriesNotification(tx, system, event.GetOrgID(), systemAdvisoriesNew, - event.SkipNotifications) + // Send instant notification with new advisories + if enableInstantNotifications { + err = publishNewAdvisoriesNotification(tx, system, event.GetOrgID(), systemAdvisoriesNew) if err != nil { evaluationCnt.WithLabelValues("error-advisory-notification").Inc() utils.LogError("orgID", event.GetOrgID(), "inventoryID", system.GetInventoryID(), "err", err, diff --git a/evaluator/notifications.go b/evaluator/notifications.go index 2791c9b0a..9186fe291 100644 --- a/evaluator/notifications.go +++ b/evaluator/notifications.go @@ -90,7 +90,11 @@ func markAdvisoriesNotified(tx *gorm.DB, accountID int, advisoryIDs []int64) err // skipPublish is true. In both cases, matching advisory_account_data rows are marked notified // when there is something to notify about (so skipPublish still prevents later flood). func publishNewAdvisoriesNotification(tx *gorm.DB, system *models.SystemPlatformV2, orgID string, - newAdvisories SystemAdvisoryMap, skipPublish bool) error { + newAdvisories SystemAdvisoryMap) error { + if notificationsPublisher == nil { + return nil + } + defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("advisory-notification-publish")) advisories, err := getUnnotifiedAdvisories(tx, system.Inventory.RhAccountID, newAdvisories) @@ -101,21 +105,6 @@ func publishNewAdvisoriesNotification(tx *gorm.DB, system *models.SystemPlatform return nil } - advisoryIDs := make([]int64, 0, len(advisories)) - for _, a := range advisories { - advisoryIDs = append(advisoryIDs, a.AdvisoryID) - } - - if skipPublish { - utils.LogInfo("inventoryID", system.GetInventoryID(), "advisoryIDs", advisoryIDs, "orgID", orgID, - "skipping advisory notification publish") - return markAdvisoriesNotified(tx, system.Inventory.RhAccountID, advisoryIDs) - } - - if notificationsPublisher == nil { - return nil - } - events := make([]ntf.Event, 0, len(advisories)) for _, advisory := range advisories { // At least empty metadata required to avoid NPE further on at the time of writing. @@ -142,8 +131,20 @@ func publishNewAdvisoriesNotification(tx *gorm.DB, system *models.SystemPlatform return errors.Wrap(err, "writing message to notifications publisher failed") } + advisoryIDs := make([]int64, 0, len(advisories)) + for _, a := range advisories { + advisoryIDs = append(advisoryIDs, a.AdvisoryID) + } + utils.LogInfo("inventoryID", system.GetInventoryID(), "advisoryIDs", advisoryIDs, "orgID", orgID, "notification sent successfully") - return markAdvisoriesNotified(tx, system.Inventory.RhAccountID, advisoryIDs) + err = tx.Table("advisory_account_data"). + Where("rh_account_id = ? AND advisory_id IN (?)", system.Inventory.RhAccountID, advisoryIDs). + Update("notified", time.Now()).Error + if err != nil { + return errors.Wrap(err, "updating notified column failed") + } + + return nil } diff --git a/evaluator/notifications_test.go b/evaluator/notifications_test.go index fce75c4a3..5f0bdc8d7 100644 --- a/evaluator/notifications_test.go +++ b/evaluator/notifications_test.go @@ -86,50 +86,6 @@ func TestAdvisoriesNotificationPublish(t *testing.T) { database.DeleteAdvisoryAccountData(t, rhAccountID, oldSystemAdvisoryIDs) } -// TestAdvisoriesNotificationSkipPublishViaEvaluate runs a full evaluateHandler path with -// skip_notifications set: advisories are stored and marked notified, but no Kafka notify is sent. -func TestAdvisoriesNotificationSkipPublishViaEvaluate(t *testing.T) { - utils.SkipWithoutDB(t) - utils.SkipWithoutPlatform(t) - core.SetupTestEnvironment() - - configure() - loadCache() - mockWriter := mqueue.MockKafkaWriter{} - notificationsPublisher = &mockWriter - - expectedAddedAdvisories := []string{"RH-1", "RH-2", "RH-100"} - expectedAdvisoryIDs := []int64{1, 2} - oldSystemAdvisoryIDs := []int64{1, 3, 4} - - database.DeleteSystemAdvisories(t, testDBID, expectedAdvisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, expectedAdvisoryIDs) - database.CreateSystemAdvisories(t, rhAccountID, testDBID, oldSystemAdvisoryIDs) - database.CreateAdvisoryAccountData(t, rhAccountID, oldSystemAdvisoryIDs, 1) - database.CheckCachesValid(t) - database.CheckAdvisoriesAccountDataNotified(t, rhAccountID, oldSystemAdvisoryIDs, false) - - orgID := "1234567" - data, err := sonic.Marshal(mqueue.PlatformEvent{ - SystemIDs: []uuid.UUID{testInventoryID}, - RequestIDs: []string{"request-skip-notif"}, - AccountID: rhAccountID, - OrgID: &orgID, - SkipNotifications: true, - }) - assert.NoError(t, err) - err = evaluateHandler(mqueue.KafkaMessage{Value: data}) - assert.NoError(t, err) - - advisoryIDs := database.CheckAdvisoriesInDB(t, expectedAddedAdvisories) - database.CheckAdvisoriesAccountDataNotified(t, rhAccountID, expectedAdvisoryIDs, true) - assert.Empty(t, mockWriter.Messages, "no notification should be sent when skip_notifications is set") - - database.DeleteSystemAdvisories(t, testDBID, advisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, advisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, oldSystemAdvisoryIDs) -} - func TestAdvisoriesNotificationMessage(t *testing.T) { events := make([]ntf.Event, 1) events[0] = ntf.Event{ @@ -232,7 +188,7 @@ func TestAdvisoriesNotificationAlreadyNotified(t *testing.T) { "RH-2": {AdvisoryID: 2}, } - err := publishNewAdvisoriesNotification(database.DB, system, orgID, newAdvs, false) + err := publishNewAdvisoriesNotification(database.DB, system, orgID, newAdvs) assert.NoError(t, err) assert.Empty(t, mockWriter.Messages, "no notification should be sent when all advisories are already notified") } @@ -259,46 +215,10 @@ func TestAdvisoriesNotificationEmptyAdvisoryMap(t *testing.T) { } // An empty map means there is nothing to query — no messages should be produced regardless. - publishNewAdvisoriesNotification(database.DB, system, orgID, SystemAdvisoryMap{}, false) //nolint:errcheck + publishNewAdvisoriesNotification(database.DB, system, orgID, SystemAdvisoryMap{}) //nolint:errcheck assert.Empty(t, mockWriter.Messages, "no notification should be sent when the advisory map is empty") } -// TestAdvisoriesNotificationSkipPublish verifies recovery-style skip_notifications: no Kafka -// message is sent, but advisory_account_data.notified is still set. -func TestAdvisoriesNotificationSkipPublish(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - configure() - - mockWriter := mqueue.MockKafkaWriter{} - notificationsPublisher = &mockWriter - - advisoryIDs := []int64{1, 2} - database.DeleteAdvisoryAccountData(t, rhAccountID, advisoryIDs) - database.CreateAdvisoryAccountData(t, rhAccountID, advisoryIDs, 1) - database.CheckAdvisoriesAccountDataNotified(t, rhAccountID, advisoryIDs, false) - defer database.DeleteAdvisoryAccountData(t, rhAccountID, advisoryIDs) - - system := &models.SystemPlatformV2{ - Inventory: models.SystemInventory{ - ID: 1, - RhAccountID: rhAccountID, - InventoryID: uuid.MustParse("00000000-0000-0000-0000-000000000001"), - DisplayName: "display name", - }, - Patch: models.SystemPatch{}, - } - newAdvs := SystemAdvisoryMap{ - "RH-1": {AdvisoryID: 1}, - "RH-2": {AdvisoryID: 2}, - } - - err := publishNewAdvisoriesNotification(database.DB, system, orgID, newAdvs, true) - assert.NoError(t, err) - assert.Empty(t, mockWriter.Messages, "no notification should be sent when skipPublish is true") - database.CheckAdvisoriesAccountDataNotified(t, rhAccountID, advisoryIDs, true) -} - // TestGetUnnotifiedAdvisoriesReturnsEmpty documents the return-type contract of // getUnnotifiedAdvisories: when all candidate advisories are already notified the function // must return a non-nil empty slice (not nil). This prevents a future nil-vs-empty regression diff --git a/main.go b/main.go index 87786c96f..feb8e84e3 100644 --- a/main.go +++ b/main.go @@ -12,7 +12,6 @@ import ( "app/tasks/caches" "app/tasks/cleaning" "app/tasks/repack" - "app/tasks/system_advisories_0_recovery" "app/tasks/system_culling" "app/tasks/vmaas_sync" "app/turnpike" @@ -81,7 +80,5 @@ func runJob(name string) { caches.RunAccountAdvisoryBackfill() case "clean_advisory_account_data": cleaning.RunCleanAdvisoryAccountData() - case "system_advisories_0_recovery": - system_advisories_0_recovery.Run() } } diff --git a/tasks/config.go b/tasks/config.go index 8735c36f0..d059dc58c 100644 --- a/tasks/config.go +++ b/tasks/config.go @@ -42,6 +42,4 @@ var ( MaxChangedPackages = utils.PodConfig.GetInt("max_changed_packages", 30000) // prune deleted_system table records older than threshold DeletedSystemsThreshold = time.Hour * time.Duration(utils.PodConfig.GetInt("system_delete_hrs", 4)) - // One-off: publish recalc for non-stale system_advisories hash remainder 0 (default off) - EnableSystemAdvisories0Recovery = utils.PodConfig.GetBool("system_advisories_0_recovery", false) ) diff --git a/tasks/system_advisories_0_recovery/recovery.go b/tasks/system_advisories_0_recovery/recovery.go deleted file mode 100644 index ef28a071f..000000000 --- a/tasks/system_advisories_0_recovery/recovery.go +++ /dev/null @@ -1,77 +0,0 @@ -package system_advisories_0_recovery - -import ( - "app/base" - "app/base/core" - "app/base/mqueue" - "app/base/utils" - "app/tasks" - "time" -) - -const ( - systemAdvisoriesPartitions = 32 - systemAdvisoriesRemainder = 0 - // recoveryBatchSize matches the cutover plan: 500 systems per Kafka message. - recoveryBatchSize = 500 -) - -var evalWriter mqueue.Writer - -func Configure() { - core.ConfigureApp() - evalTopic := utils.FailIfEmpty(utils.CoreCfg.EvalTopic, "EVAL_TOPIC") - evalWriter = mqueue.NewKafkaWriterFromEnv(evalTopic) -} - -func Run() { - tasks.HandleContextCancel(tasks.WaitAndExit) - Configure() - defer utils.LogPanics(true) - - if !tasks.EnableSystemAdvisories0Recovery { - utils.LogInfo("system_advisories_0_recovery disabled (set system_advisories_0_recovery=true in JOBS_CONFIG), skipping") //nolint:lll - return - } - - utils.LogInfo("Starting system_advisories_0 recovery recalc publish") - if err := publishBucket0Recalc(); err != nil { - utils.LogError("err", err, "system_advisories_0 recovery failed") - return - } - utils.LogInfo("system_advisories_0 recovery recalc publish finished") -} - -func publishBucket0Recalc() error { - inventoryAIDs, err := getNonStaleBucket0InventoryIDs() - if err != nil { - return err - } - utils.LogInfo("count", len(inventoryAIDs), "non-stale bucket-0 systems selected for recovery recalc") - - start := time.Now() - err = mqueue.EvalDataSlice(inventoryAIDs).WriteEventsSkipNotifications(base.Context, evalWriter, recoveryBatchSize) - if err != nil { - utils.LogError("err", err, "sending recovery recalc messages failed") - return err - } - utils.LogInfo("count", len(inventoryAIDs), "seconds", time.Since(start).Seconds(), - "systems sent to recovery recalc with skip_notifications") - return nil -} - -func getNonStaleBucket0InventoryIDs() ([]mqueue.EvalData, error) { - var inventoryAIDs []mqueue.EvalData - err := tasks.CancelableDB().Table("system_inventory si"). - Select("si.inventory_id, si.rh_account_id, ra.org_id"). - Joins("JOIN rh_account ra ON ra.id = si.rh_account_id"). - Where("si.stale = false"). - Where("satisfies_hash_partition('system_advisories'::regclass, ?, ?, si.rh_account_id)", - systemAdvisoriesPartitions, systemAdvisoriesRemainder). - Order("si.rh_account_id, si.id"). - Scan(&inventoryAIDs).Error - if err != nil { - return nil, err - } - return inventoryAIDs, nil -} diff --git a/tasks/system_advisories_0_recovery/recovery_test.go b/tasks/system_advisories_0_recovery/recovery_test.go deleted file mode 100644 index 72350b962..000000000 --- a/tasks/system_advisories_0_recovery/recovery_test.go +++ /dev/null @@ -1,60 +0,0 @@ -package system_advisories_0_recovery - -import ( - "app/base/core" - "app/base/database" - "app/base/mqueue" - "app/base/utils" - "context" - "testing" - - "github.com/bytedance/sonic" - "github.com/google/uuid" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestGetNonStaleBucket0InventoryIDs(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - - ids, err := getNonStaleBucket0InventoryIDs() - require.NoError(t, err) - - var expected int64 - err = database.DB.Raw(` - SELECT count(*) - FROM system_inventory si - WHERE si.stale = false - AND satisfies_hash_partition('system_advisories'::regclass, ?, ?, si.rh_account_id) - `, systemAdvisoriesPartitions, systemAdvisoriesRemainder).Scan(&expected).Error - require.NoError(t, err) - assert.Equal(t, int(expected), len(ids)) - - for _, row := range ids { - var inBucket bool - err = database.DB.Raw( - `SELECT satisfies_hash_partition('system_advisories'::regclass, ?, ?, ?)`, - systemAdvisoriesPartitions, systemAdvisoriesRemainder, row.RhAccountID, - ).Scan(&inBucket).Error - require.NoError(t, err) - assert.True(t, inBucket) - assert.NotEqual(t, uuid.Nil, row.InventoryID) - } -} - -func TestPublishSetsSkipNotifications(t *testing.T) { - orgID := "org_1" - evals := mqueue.EvalDataSlice{ - {InventoryID: uuid.MustParse("00000000-0000-0000-0000-000000000001"), RhAccountID: 1, OrgID: &orgID}, - {InventoryID: uuid.MustParse("00000000-0000-0000-0000-000000000002"), RhAccountID: 1, OrgID: &orgID}, - } - writer := &mqueue.MockKafkaWriter{} - require.NoError(t, evals.WriteEventsSkipNotifications(context.Background(), writer, recoveryBatchSize)) - require.Len(t, writer.Messages, 1) - - var event mqueue.PlatformEvent - require.NoError(t, sonic.Unmarshal(writer.Messages[0].Value, &event)) - assert.True(t, event.SkipNotifications) - assert.Equal(t, 2, len(event.SystemIDs)) -} From f80ea345fafaa39cd8b05256947299f0b6da94e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Fri, 18 Sep 2026 10:03:26 +0200 Subject: [PATCH 03/16] RHINENG-26122: remove notifications from evaluator --- aggregator/notifications_test.go | 1 - base/database/testing.go | 23 --- base/notification/notification.go | 49 +----- evaluator/evaluate.go | 11 -- evaluator/notifications.go | 150 ----------------- evaluator/notifications_test.go | 256 ------------------------------ 6 files changed, 1 insertion(+), 489 deletions(-) delete mode 100644 evaluator/notifications.go delete mode 100644 evaluator/notifications_test.go diff --git a/aggregator/notifications_test.go b/aggregator/notifications_test.go index 435758c02..923ae3f33 100644 --- a/aggregator/notifications_test.go +++ b/aggregator/notifications_test.go @@ -67,7 +67,6 @@ func TestPublishNewAdvisoryNotificationSuccess(t *testing.T) { var notif ntf.Notification assert.Nil(t, sonic.Unmarshal(mockWriter.Messages[0].Value, ¬if)) assert.Equal(t, "org_1", notif.OrgID) - assert.Nil(t, notif.Context) assert.NotEmpty(t, notif.Events) // Verify advisories were marked as notified (count varies by workspace) diff --git a/base/database/testing.go b/base/database/testing.go index 95cb34117..031abda34 100644 --- a/base/database/testing.go +++ b/base/database/testing.go @@ -197,29 +197,6 @@ func CheckAdvisoriesAccountData(t *testing.T, rhAccountID int, advisoryIDs []int assert.Equal(t, systemsInstallable*len(advisoryIDs), sum, "sum of systems_installable does not match") } -func CheckAdvisoriesAccountDataNotified(t *testing.T, rhAccountID int, advisoryIDs []int64, notified bool) { - var advisoryAccountData []models.AdvisoryAccountData - err := DB.Where("rh_account_id = ? AND advisory_id IN (?)", rhAccountID, advisoryIDs). - Find(&advisoryAccountData).Error - assert.Nil(t, err) - - for _, item := range advisoryAccountData { - if notified { - assert.NotNil(t, item.Notified) - } else { - assert.Nil(t, item.Notified) - } - } -} - -func CreateReportedAdvisories(reportedAdvisories []string, status []int) map[string]int { - reportedAdvisoriesMap := make(map[string]int, len(reportedAdvisories)) - for i, adv := range reportedAdvisories { - reportedAdvisoriesMap[adv] = status[i] - } - return reportedAdvisoriesMap -} - func CreateStoredAdvisories(advisoryPatched []int64) map[string]models.SystemAdvisories { systemAdvisoriesMap := make(map[string]models.SystemAdvisories, len(advisoryPatched)) for _, advisoryID := range advisoryPatched { diff --git a/base/notification/notification.go b/base/notification/notification.go index b45f5d4ff..cabf3885c 100644 --- a/base/notification/notification.go +++ b/base/notification/notification.go @@ -1,12 +1,8 @@ package notification import ( - "app/base/models" - "app/base/utils" - "fmt" "time" - "github.com/google/uuid" "github.com/pkg/errors" ) @@ -17,16 +13,6 @@ const ( NewAdvisoryEvent = "new-advisory" ) -// TODO: Remove Context, MakeNotification and *Context field on Notification after fully migrating to the aggregator -// See: https://redhat.atlassian.net/browse/RHINENG-26543 - -type Context struct { - InventoryID uuid.UUID `json:"inventory_id"` - DisplayName string `json:"display_name"` - HostURL string `json:"host_url"` - Tags []SystemTag `json:"tags"` -} - type Metadata struct{} type Event struct { @@ -64,8 +50,7 @@ type Notification struct { // ISO-8601 formatted date (per platform convention when the message was sent). Timestamp string `json:"timestamp"` // Extra information that are common to all the events that are sent in this message. - Context *Context `json:"context,omitempty"` - Events []Event `json:"events"` + Events []Event `json:"events"` // Recipients settings - Applications can add extra email recipients by adding entries to this array. // This setting extends whatever the Administrators configured in their Notifications settings (since v1.1.0). Recipients []Recipient `json:"recipients,omitempty"` @@ -80,38 +65,6 @@ type Advisory struct { Synopsis string `json:"synopsis"` } -type SystemTag struct { - Key string `json:"key,omitempty"` - Namespace string `json:"namespace,omitempty"` - Value string `json:"value,omitempty"` -} - -func MakeNotification(inv *models.SystemInventory, systemTags []SystemTag, orgID string, - eventType string, events []Event) (*Notification, error) { - if orgID == "" || orgID == "null" { - return nil, errors.New("invalid orgID") - } - - hostURL := fmt.Sprintf("https://%s/insights/inventory/%s", utils.CoreCfg.ConsoledotHostname, inv.InventoryID) - - return &Notification{ - Version: Version, - Bundle: Bundle, - Application: Application, - EventType: eventType, - // ISO-8601 formatted time - Timestamp: time.Now().Format(time.RFC3339), - Context: &Context{ - InventoryID: inv.InventoryID, - DisplayName: inv.DisplayName, - HostURL: hostURL, - Tags: systemTags, - }, - Events: events, - OrgID: orgID, - }, nil -} - func MakeAccountNotification(orgID string, eventType string, events []Event) (*Notification, error) { if orgID == "" || orgID == "null" { return nil, errors.New("invalid orgID") diff --git a/evaluator/evaluate.go b/evaluator/evaluate.go index e4d953480..568aafc93 100644 --- a/evaluator/evaluate.go +++ b/evaluator/evaluate.go @@ -83,7 +83,6 @@ func configure() { } vmaasUpdatesURL = utils.FailIfEmpty(utils.CoreCfg.VmaasAddress, "VMAAS_ADDRESS") + base.VMaaSAPIPrefix + "/updates" configureRemediations() - configureNotifications() configureInventoryViews() configureAdvisoryUpdates() configureStatus() @@ -523,16 +522,6 @@ func evaluateAndStore(system *models.SystemPlatformV2, } } - // Send instant notification with new advisories - if enableInstantNotifications { - err = publishNewAdvisoriesNotification(tx, system, event.GetOrgID(), systemAdvisoriesNew) - if err != nil { - evaluationCnt.WithLabelValues("error-advisory-notification").Inc() - utils.LogError("orgID", event.GetOrgID(), "inventoryID", system.GetInventoryID(), "err", err, - "publishing new advisories notification failed") - } - } - if enableAdvisoryUpdates { err = publishAdvisoryUpdates(system, advisoriesByName) if err != nil { diff --git a/evaluator/notifications.go b/evaluator/notifications.go deleted file mode 100644 index 9186fe291..000000000 --- a/evaluator/notifications.go +++ /dev/null @@ -1,150 +0,0 @@ -package evaluator - -import ( - "app/base" - "app/base/models" - "app/base/mqueue" - ntf "app/base/notification" - "app/base/utils" - "time" - - "github.com/bytedance/sonic" - "github.com/pkg/errors" - "gorm.io/gorm" -) - -var notificationsPublisher mqueue.Writer - -func configureNotifications() { - if topic := utils.CoreCfg.NotificationsTopic; topic != "" { - notificationsPublisher = mqueue.NewKafkaWriterFromEnv(topic) - } -} - -func getUnnotifiedAdvisories(tx *gorm.DB, accountID int, newAdvs SystemAdvisoryMap) ([]ntf.Advisory, error) { - unAdvs := make([]ntf.Advisory, 0, len(newAdvs)) - - advIDs := make([]int64, 0, len(newAdvs)) - for _, a := range newAdvs { - advIDs = append(advIDs, a.AdvisoryID) - } - - err := tx.Table("advisory_account_data as acd"). - Select("am.id as advisory_id, am.name as advisory_name, at.name as advisory_type, am.synopsis"). - Joins("inner join advisory_metadata am on am.id = acd.advisory_id"). - Joins("inner join advisory_type at on at.id = am.advisory_type_id"). - Where("acd.rh_account_id = ? AND acd.advisory_id IN (?)"+ - "AND acd.notified IS NULL AND acd.systems_installable > 0", accountID, advIDs). - Order("am.name ASC"). - Scan(&unAdvs).Error - if err != nil { - return nil, errors.Wrap(err, "querying unnotified advisories from DB failed") - } - - return unAdvs, nil -} - -func getSystemTags(tx *gorm.DB, system *models.SystemPlatformV2) ([]ntf.SystemTag, error) { - if system == nil { - return nil, nil - } - - var tags []ntf.SystemTag - var tagsJSON string - err := tx.Table("system_inventory"). - Select("tags"). - Where("rh_account_id = ?", system.Inventory.RhAccountID). - Where("id = ?", system.InternalSystemID()). - Scan(&tagsJSON).Error - if err != nil { - return nil, errors.Wrap(err, "system tags query failed") - } - if err = sonic.Unmarshal([]byte(tagsJSON), &tags); err != nil { - return nil, errors.Wrap(err, "system tags unmarshal failed") - } - - return tags, nil -} - -func markAdvisoriesNotified(tx *gorm.DB, accountID int, advisoryIDs []int64) error { - if len(advisoryIDs) == 0 { - return nil - } - err := tx.Table("advisory_account_data"). - Where("rh_account_id = ? AND advisory_id IN (?)", accountID, advisoryIDs). - Update("notified", time.Now()).Error - if err != nil { - return errors.Wrap(err, "updating notified column failed") - } - // Ensure notifications are in sync between aad and aa, while we transition - err = tx.Table("account_advisory"). - Where("rh_account_id = ? AND advisory_id IN (?)", accountID, advisoryIDs). - Update("notified", time.Now()).Error - if err != nil { - return errors.Wrap(err, "updating notified column in account_advisory failed") - } - return nil -} - -// publishNewAdvisoriesNotification publishes instant new-advisory notifications unless -// skipPublish is true. In both cases, matching advisory_account_data rows are marked notified -// when there is something to notify about (so skipPublish still prevents later flood). -func publishNewAdvisoriesNotification(tx *gorm.DB, system *models.SystemPlatformV2, orgID string, - newAdvisories SystemAdvisoryMap) error { - if notificationsPublisher == nil { - return nil - } - - defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("advisory-notification-publish")) - - advisories, err := getUnnotifiedAdvisories(tx, system.Inventory.RhAccountID, newAdvisories) - if err != nil { - return errors.Wrap(err, "getting unnotified advisories failed") - } - if len(advisories) == 0 { - return nil - } - - events := make([]ntf.Event, 0, len(advisories)) - for _, advisory := range advisories { - // At least empty metadata required to avoid NPE further on at the time of writing. - events = append(events, ntf.Event{Payload: advisory, Metadata: ntf.Metadata{}}) - } - - tags, err := getSystemTags(tx, system) - if err != nil { - return errors.Wrap(err, "getting system tags failed") - } - - notif, err := ntf.MakeNotification(&system.Inventory, tags, orgID, ntf.NewAdvisoryEvent, events) - if err != nil { - return errors.Wrap(err, "creating notification failed") - } - - msg, err := mqueue.MessageFromJSON(system.GetInventoryID().String(), notif, nil) - if err != nil { - return errors.Wrap(err, "creating message from notification failed") - } - - err = notificationsPublisher.WriteMessages(base.Context, msg) - if err != nil { - return errors.Wrap(err, "writing message to notifications publisher failed") - } - - advisoryIDs := make([]int64, 0, len(advisories)) - for _, a := range advisories { - advisoryIDs = append(advisoryIDs, a.AdvisoryID) - } - - utils.LogInfo("inventoryID", system.GetInventoryID(), "advisoryIDs", advisoryIDs, "orgID", orgID, - "notification sent successfully") - - err = tx.Table("advisory_account_data"). - Where("rh_account_id = ? AND advisory_id IN (?)", system.Inventory.RhAccountID, advisoryIDs). - Update("notified", time.Now()).Error - if err != nil { - return errors.Wrap(err, "updating notified column failed") - } - - return nil -} diff --git a/evaluator/notifications_test.go b/evaluator/notifications_test.go deleted file mode 100644 index 5f0bdc8d7..000000000 --- a/evaluator/notifications_test.go +++ /dev/null @@ -1,256 +0,0 @@ -package evaluator - -import ( - "app/base/core" - "app/base/database" - "app/base/models" - "app/base/mqueue" - ntf "app/base/notification" - "app/base/utils" - "fmt" - "testing" - "time" - - "github.com/bytedance/sonic" - "github.com/google/uuid" - "github.com/stretchr/testify/assert" -) - -func checkNotificationPayload(t *testing.T, notification ntf.Notification, name, advType, synopsis string) { - for _, event := range notification.Events { - payload, ok := event.Payload.(map[string]interface{}) - assert.True(t, ok) - - if payload["advisory_name"] != name { - continue - } - if payload["advisory_type"] != advType { - continue - } - if payload["synopsis"] != synopsis { - continue - } - return - } - t.Fatal("such payload does not exist") -} - -func TestAdvisoriesNotificationPublish(t *testing.T) { - utils.SkipWithoutDB(t) - utils.SkipWithoutPlatform(t) - core.SetupTestEnvironment() - - configure() - loadCache() - mockWriter := mqueue.MockKafkaWriter{} - notificationsPublisher = &mockWriter - - expectedAddedAdvisories := []string{"RH-1", "RH-2", "RH-100"} - expectedAdvisoryIDs := []int64{1, 2} // advisories expected to be paired to the system after evaluation - oldSystemAdvisoryIDs := []int64{1, 3, 4} // old advisories paired with the system - - database.DeleteSystemAdvisories(t, testDBID, expectedAdvisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, expectedAdvisoryIDs) - database.CreateSystemAdvisories(t, rhAccountID, testDBID, oldSystemAdvisoryIDs) - database.CreateAdvisoryAccountData(t, rhAccountID, oldSystemAdvisoryIDs, 1) - database.CheckCachesValid(t) - database.CheckAdvisoriesAccountDataNotified(t, rhAccountID, oldSystemAdvisoryIDs, false) - - orgID := "1234567" - // do evaluate the system - data, err := sonic.Marshal(mqueue.PlatformEvent{ - SystemIDs: []uuid.UUID{testInventoryID}, - RequestIDs: []string{"request-2"}, - AccountID: rhAccountID, - OrgID: &orgID}) - assert.NoError(t, err) - err = evaluateHandler(mqueue.KafkaMessage{Value: data}) - assert.NoError(t, err) - advisoryIDs := database.CheckAdvisoriesInDB(t, expectedAddedAdvisories) - database.CheckAdvisoriesAccountDataNotified(t, rhAccountID, expectedAdvisoryIDs, true) - - assert.Equal(t, 1, len(mockWriter.Messages)) - - var notificationSent ntf.Notification - assert.Nil(t, sonic.Unmarshal(mockWriter.Messages[0].Value, ¬ificationSent)) - checkNotificationPayload(t, notificationSent, "RH-1", "enhancement", "adv-1-syn") - checkNotificationPayload(t, notificationSent, "RH-2", "bugfix", "adv-2-syn") - - events := notificationSent.Events - // Assert is sorted ASC - assert.True(t, events[0].Payload.(map[string]interface{})["advisory_name"].(string) < - events[1].Payload.(map[string]interface{})["advisory_name"].(string)) - - database.DeleteSystemAdvisories(t, testDBID, advisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, advisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, oldSystemAdvisoryIDs) -} - -func TestAdvisoriesNotificationMessage(t *testing.T) { - events := make([]ntf.Event, 1) - events[0] = ntf.Event{ - Payload: ntf.Advisory{ - AdvisoryName: "RH-1", - AdvisoryType: "bugfix", - Synopsis: "Resolves some bug", - }, - } - - displayName := "display-name" - inv := &models.SystemInventory{ - InventoryID: testInventoryID, - DisplayName: displayName, - } - tags := []ntf.SystemTag{{Key: "key", Namespace: "namespace", Value: "value"}} - - orgID := "1234567" - url := fmt.Sprintf("https://localhost/insights/inventory/%s", testInventoryID.String()) - - notification, err := ntf.MakeNotification(inv, tags, orgID, ntf.NewAdvisoryEvent, events) - assert.Nil(t, err) - assert.Equal(t, orgID, notification.OrgID) - assert.Equal(t, url, notification.Context.HostURL) - assert.Equal(t, testInventoryID, notification.Context.InventoryID) - assert.Equal(t, displayName, notification.Context.DisplayName) - assert.Equal(t, tags, notification.Context.Tags) - - msg, err := mqueue.MessageFromJSON(testInventoryID.String(), notification, nil) - assert.Nil(t, err) - assert.Equal(t, testInventoryID.String(), string(msg.Key)) - - notificationJSON, err := sonic.Marshal(notification) - assert.Nil(t, err) - assert.Equal(t, notificationJSON, msg.Value) -} - -func TestGetSystemTags(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - configure() - - system := &models.SystemPlatformV2{ - Inventory: models.SystemInventory{ - ID: 1, - RhAccountID: 1, - InventoryID: uuid.MustParse("00000000-0000-0000-0000-000000000001"), - DisplayName: "display name", - }, - Patch: models.SystemPatch{}, - } - tags, err := getSystemTags(database.DB, system) - expected := []ntf.SystemTag{ - {Key: "k1", Value: "val1", Namespace: "ns1"}, - {Key: "k2", Value: "val2", Namespace: "ns1"}, - } - if assert.NoError(t, err) { - assert.Equal(t, expected, tags) - } -} - -// TestAdvisoriesNotificationAlreadyNotified verifies that no Kafka message is sent when all -// advisories on the system have already been notified (notified IS NOT NULL). This is the -// exact scenario that caused the blank-email production bug introduced in RHINENG-21786. -func TestAdvisoriesNotificationAlreadyNotified(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - configure() - - mockWriter := mqueue.MockKafkaWriter{} - notificationsPublisher = &mockWriter - - advisoryIDs := []int64{1, 2} - database.DeleteAdvisoryAccountData(t, rhAccountID, advisoryIDs) - - now := time.Now() - for _, id := range advisoryIDs { - err := database.DB.Create(&models.AdvisoryAccountData{ - AdvisoryID: id, - RhAccountID: rhAccountID, - SystemsInstallable: 1, - SystemsApplicable: 1, - Notified: &now, - }).Error - assert.NoError(t, err) - } - defer database.DeleteAdvisoryAccountData(t, rhAccountID, advisoryIDs) - - system := &models.SystemPlatformV2{ - Inventory: models.SystemInventory{ - ID: 1, - RhAccountID: rhAccountID, - InventoryID: uuid.MustParse("00000000-0000-0000-0000-000000000001"), - DisplayName: "display name", - }, - Patch: models.SystemPatch{}, - } - newAdvs := SystemAdvisoryMap{ - "RH-1": {AdvisoryID: 1}, - "RH-2": {AdvisoryID: 2}, - } - - err := publishNewAdvisoriesNotification(database.DB, system, orgID, newAdvs) - assert.NoError(t, err) - assert.Empty(t, mockWriter.Messages, "no notification should be sent when all advisories are already notified") -} - -// TestAdvisoriesNotificationEmptyAdvisoryMap verifies that no Kafka message is sent when -// publishNewAdvisoriesNotification is called with an empty SystemAdvisoryMap (no advisories -// on the system at all). -func TestAdvisoriesNotificationEmptyAdvisoryMap(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - configure() - - mockWriter := mqueue.MockKafkaWriter{} - notificationsPublisher = &mockWriter - - system := &models.SystemPlatformV2{ - Inventory: models.SystemInventory{ - ID: 1, - RhAccountID: rhAccountID, - InventoryID: uuid.MustParse("00000000-0000-0000-0000-000000000001"), - DisplayName: "display name", - }, - Patch: models.SystemPatch{}, - } - - // An empty map means there is nothing to query — no messages should be produced regardless. - publishNewAdvisoriesNotification(database.DB, system, orgID, SystemAdvisoryMap{}) //nolint:errcheck - assert.Empty(t, mockWriter.Messages, "no notification should be sent when the advisory map is empty") -} - -// TestGetUnnotifiedAdvisoriesReturnsEmpty documents the return-type contract of -// getUnnotifiedAdvisories: when all candidate advisories are already notified the function -// must return a non-nil empty slice (not nil). This prevents a future nil-vs-empty regression -// from silently bypassing the len == 0 guard in publishNewAdvisoriesNotification. -func TestGetUnnotifiedAdvisoriesReturnsEmpty(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - configure() - - advisoryIDs := []int64{1, 2} - database.DeleteAdvisoryAccountData(t, rhAccountID, advisoryIDs) - - now := time.Now() - for _, id := range advisoryIDs { - err := database.DB.Create(&models.AdvisoryAccountData{ - AdvisoryID: id, - RhAccountID: rhAccountID, - SystemsInstallable: 1, - SystemsApplicable: 1, - Notified: &now, - }).Error - assert.NoError(t, err) - } - defer database.DeleteAdvisoryAccountData(t, rhAccountID, advisoryIDs) - - newAdvs := SystemAdvisoryMap{ - "RH-1": {AdvisoryID: 1}, - "RH-2": {AdvisoryID: 2}, - } - - result, err := getUnnotifiedAdvisories(database.DB, rhAccountID, newAdvs) - assert.NoError(t, err) - assert.NotNil(t, result, "result must be a non-nil slice so callers can use len() safely") - assert.Empty(t, result, "no advisories should be returned when all are already notified") -} From 449210f25bfe2368a0063a600b2da68dad2cd873 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Thu, 17 Sep 2026 17:41:11 +0200 Subject: [PATCH 04/16] RHINENG-26122: remove advisory_account_data update --- base/database/database.go | 23 ---- base/database/testing.go | 18 --- evaluator/advisory_update_test.go | 1 - evaluator/evaluate.go | 3 - evaluator/evaluate_advisories.go | 95 --------------- evaluator/evaluate_advisories_test.go | 126 -------------------- evaluator/evaluate_test.go | 3 - tasks/system_culling/system_culling_test.go | 88 +++++++------- 8 files changed, 41 insertions(+), 316 deletions(-) diff --git a/base/database/database.go b/base/database/database.go index 050c0c565..fb8b0f1bd 100644 --- a/base/database/database.go +++ b/base/database/database.go @@ -24,26 +24,3 @@ func OnConflictUpdateMulti(db *gorm.DB, keys []string, updateCols ...string) *go } return db.Clauses(onConflict) } - -type UpExpr struct { - Name string - Expr string -} - -func OnConflictDoUpdateExpr(db *gorm.DB, keys []string, updateExprs ...UpExpr) *gorm.DB { - updateColsValues := make(map[string]interface{}, len(updateExprs)) - for _, v := range updateExprs { - updateColsValues[v.Name] = v.Expr - } - conflictColumns := make([]clause.Column, len(keys)) - for i, key := range keys { - conflictColumns[i] = clause.Column{Name: key} - } - if len(updateColsValues) > 0 { - return db.Clauses(clause.OnConflict{ - Columns: conflictColumns, - DoUpdates: clause.Assignments(updateColsValues), - }) - } - return db -} diff --git a/base/database/testing.go b/base/database/testing.go index 031abda34..9d0bba250 100644 --- a/base/database/testing.go +++ b/base/database/testing.go @@ -27,18 +27,6 @@ func TestWorkspace1NamePtr() *string { return &name } -func DebugWithCachesCheck(part string, fun func()) { - fun() - validAfter, err := CheckCachesValidRet() - if err != nil { - utils.LogPanic("error", err, "Could not check validity of caches") - } - - if !validAfter { - utils.LogPanic("part", part, "Cache mismatch created") - } -} - type key struct { AccountID int AdvisoryID int64 @@ -110,12 +98,6 @@ func CheckCachesValidRet() (bool, error) { return valid, nil } -func CheckCachesValid(t *testing.T) { - valid, err := CheckCachesValidRet() - assert.Nil(t, err) - assert.True(t, valid) -} - func CheckAdvisoriesInDB(t *testing.T, advisories []string) []int64 { var advisoryIDs []int64 err := DB.Model(models.AdvisoryMetadata{}).Where("name IN (?)", advisories). diff --git a/evaluator/advisory_update_test.go b/evaluator/advisory_update_test.go index 66a2b68d6..5083c84ed 100644 --- a/evaluator/advisory_update_test.go +++ b/evaluator/advisory_update_test.go @@ -157,7 +157,6 @@ func TestAdvisoryUpdateKafkaRoundTrip(t *testing.T) { oldAdvisoryIDs := []int64{1, 3, 4} database.CreateSystemAdvisories(t, rhAccountID, testDBID, oldAdvisoryIDs) database.CreateAdvisoryAccountData(t, rhAccountID, oldAdvisoryIDs, 1) - database.CheckCachesValid(t) // Run evaluation data, err := sonic.Marshal(mqueue.PlatformEvent{ diff --git a/evaluator/evaluate.go b/evaluator/evaluate.go index 568aafc93..fe24527ec 100644 --- a/evaluator/evaluate.go +++ b/evaluator/evaluate.go @@ -64,7 +64,6 @@ var ( enableAdvisoryUpdates bool enableSatelliteFunctionality bool enableTemplateAdvisoryEval bool - enableAdvisoryAccountData bool errVmaasBadRequest = errors.New("vmaas bad request") ) @@ -96,8 +95,6 @@ func configureEvaluator() { disableCompression = !utils.PodConfig.GetBool("vmaas_call_compression", true) // Evaluate advisories enableAdvisoryAnalysis = utils.PodConfig.GetBool("advisory_analysis", true) - // Update legacy advisory_account_data counts during evaluation - enableAdvisoryAccountData = utils.PodConfig.GetBool("advisory_account_data", true) // evaluate packages enablePackageAnalysis = utils.PodConfig.GetBool("package_analysis", true) // Look for third party repos diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index 06b89b580..1e81d41d3 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -5,10 +5,8 @@ import ( "app/base/models" "app/base/utils" "app/base/vmaas" - "cmp" "fmt" "regexp" - "slices" "time" "github.com/google/uuid" @@ -254,77 +252,9 @@ func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatformV2, advisoriesB return nil, err } - err = updateAdvisoryAccountData(tx, system, advisoriesByName) - if err != nil { - return nil, errors.Wrap(err, "Unable to update advisory_account_data caches") - } return systemAdvisoriesNew, nil } -func calcAdvisoryChanges(system *models.SystemPlatformV2, //nolint: funlen - advisoriesByName extendedAdvisoryMap) []models.AdvisoryAccountData { - // If system is stale, we won't change any rows in advisory_account_data - if system.Inventory.Stale { - return []models.AdvisoryAccountData{} - } - - aadMap := make(map[int64]models.AdvisoryAccountData, len(advisoriesByName)) - for _, advisory := range advisoriesByName { - switch advisory.change { - case Remove: - aadMap[advisory.AdvisoryID] = models.AdvisoryAccountData{ - AdvisoryID: advisory.AdvisoryID, - RhAccountID: system.Inventory.RhAccountID, - SystemsInstallable: -1, - } - if advisory.StatusID != APPLICABLE { // advisory is no longer applicable - aad := aadMap[advisory.AdvisoryID] - aad.SystemsApplicable = -1 - aadMap[advisory.AdvisoryID] = aad - } - case Keep: - continue - case Add: - fallthrough - case Update: - if advisory.StatusID == INSTALLABLE { - aadMap[advisory.AdvisoryID] = models.AdvisoryAccountData{ - AdvisoryID: advisory.AdvisoryID, - RhAccountID: system.Inventory.RhAccountID, - SystemsInstallable: 1, - // every installable advisory is also applicable advisory - SystemsApplicable: 1, - } - } else { // APPLICABLE - // add advisories which are only applicable and not installable to `aadMap` - if _, ok := aadMap[advisory.AdvisoryID]; !ok { - // FIXME: this check can be removed if advisories don't repeat. - // Is it possible that there would be 2 advisories with the same AdvisoryID \ - // where one would be one INSTALLABLE and the other APPLICABLE? - aadMap[advisory.AdvisoryID] = models.AdvisoryAccountData{ - AdvisoryID: advisory.AdvisoryID, - RhAccountID: system.Inventory.RhAccountID, - SystemsApplicable: 1, - } - } - } - } - } - - // aadMap into aadSlice - aadSlice := make([]models.AdvisoryAccountData, 0, len(advisoriesByName)) - for _, aad := range aadMap { - aadSlice = append(aadSlice, aad) - } - slices.SortStableFunc(aadSlice, func(x, y models.AdvisoryAccountData) int { - if n := cmp.Compare(x.RhAccountID, y.RhAccountID); n != 0 { - return n - } - return cmp.Compare(x.AdvisoryID, y.AdvisoryID) - }) - return aadSlice -} - func deleteOldSystemAdvisories(tx *gorm.DB, accountID int, systemID int64, patched []int64) error { err := tx.Where("rh_account_id = ? ", accountID). Where("system_id = ?", systemID). @@ -411,28 +341,3 @@ func loadSystemAdvisories(tx *gorm.DB, accountID int, systemID int64) (SystemAdv } return systemAdvisories, nil } - -func updateAdvisoryAccountData( - tx *gorm.DB, - system *models.SystemPlatformV2, - advisoriesByName extendedAdvisoryMap, -) error { - if !enableAdvisoryAccountData { - utils.LogInfo("inventoryID", system.GetInventoryID(), "advisory_account_data updates disabled, skipping") - return nil - } - - changes := calcAdvisoryChanges(system, advisoriesByName) - - if len(changes) == 0 { - return nil - } - - txOnConflict := database.OnConflictDoUpdateExpr(tx, []string{"rh_account_id", "advisory_id"}, - database.UpExpr{Name: "systems_installable", - Expr: "advisory_account_data.systems_installable + excluded.systems_installable"}, - database.UpExpr{Name: "systems_applicable", - Expr: "advisory_account_data.systems_applicable + excluded.systems_applicable"}) - - return database.BulkInsert(txOnConflict, changes) -} diff --git a/evaluator/evaluate_advisories_test.go b/evaluator/evaluate_advisories_test.go index 9f28e2e31..72f4a5ba0 100644 --- a/evaluator/evaluate_advisories_test.go +++ b/evaluator/evaluate_advisories_test.go @@ -148,93 +148,6 @@ func TestIncrementAdvisoryTypeCounts(t *testing.T) { assert.Equal(t, 1, secCount) } -func TestUpdateAdvisoryAccountData(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - - system := &models.SystemPlatformV2{ - Inventory: models.SystemInventory{ID: 12, RhAccountID: 3}, - Patch: models.SystemPatch{}, - } - advisoryIDs := []int64{2, 3, 4} - database.CreateSystemAdvisories(t, system.Inventory.RhAccountID, system.InternalSystemID(), advisoryIDs) - database.CreateAdvisoryAccountData(t, system.Inventory.RhAccountID, advisoryIDs, 1) - advisoriesByName := extendedAdvisoryMap{ - "ER-2": { - change: Remove, - SystemAdvisories: models.SystemAdvisories{ - AdvisoryID: 2, SystemID: system.InternalSystemID(), RhAccountID: system.Inventory.RhAccountID}, - }, - "ER-3": { - change: Remove, - SystemAdvisories: models.SystemAdvisories{ - AdvisoryID: 3, SystemID: system.InternalSystemID(), RhAccountID: system.Inventory.RhAccountID}, - }, - "ER-4": { - change: Remove, - SystemAdvisories: models.SystemAdvisories{ - AdvisoryID: 4, SystemID: system.InternalSystemID(), RhAccountID: system.Inventory.RhAccountID}, - }, - } - - // Update as if the advisories became patched - err := updateAdvisoryAccountData(database.DB, system, advisoriesByName) - assert.NoError(t, err) - database.CheckSystemAdvisories(t, system.InternalSystemID(), advisoryIDs) - database.CheckAdvisoriesAccountData(t, system.Inventory.RhAccountID, advisoryIDs, 0) - - // Update as if the advisories became unpatched - for name, ea := range advisoriesByName { - ea.change = Add - advisoriesByName[name] = ea - } - err = updateAdvisoryAccountData(database.DB, system, advisoriesByName) - assert.NoError(t, err) - database.CheckAdvisoriesAccountData(t, system.Inventory.RhAccountID, advisoryIDs, 1) - - database.DeleteSystemAdvisories(t, system.InternalSystemID(), advisoryIDs) - database.DeleteAdvisoryAccountData(t, system.Inventory.RhAccountID, advisoryIDs) -} - -func TestUpdateAdvisoryAccountDataDisabled(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - - prev := enableAdvisoryAccountData - enableAdvisoryAccountData = false - defer func() { enableAdvisoryAccountData = prev }() - - system := &models.SystemPlatformV2{ - Inventory: models.SystemInventory{ID: 12, RhAccountID: 3}, - Patch: models.SystemPatch{}, - } - advisoryIDs := []int64{2, 3, 4} - database.CreateAdvisoryAccountData(t, system.Inventory.RhAccountID, advisoryIDs, 1) - defer database.DeleteAdvisoryAccountData(t, system.Inventory.RhAccountID, advisoryIDs) - - advisoriesByName := extendedAdvisoryMap{ - "ER-2": { - change: Remove, - SystemAdvisories: models.SystemAdvisories{ - AdvisoryID: 2, SystemID: system.InternalSystemID(), RhAccountID: system.Inventory.RhAccountID}, - }, - "ER-3": { - change: Remove, - SystemAdvisories: models.SystemAdvisories{ - AdvisoryID: 3, SystemID: system.InternalSystemID(), RhAccountID: system.Inventory.RhAccountID}, - }, - "ER-4": { - change: Remove, - SystemAdvisories: models.SystemAdvisories{ - AdvisoryID: 4, SystemID: system.InternalSystemID(), RhAccountID: system.Inventory.RhAccountID}, - }, - } - - err := updateAdvisoryAccountData(database.DB, system, advisoriesByName) - assert.NoError(t, err) - database.CheckAdvisoriesAccountData(t, system.Inventory.RhAccountID, advisoryIDs, 1) -} - func TestGetMissingAdvisories(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() @@ -321,45 +234,6 @@ func TestUpsertSystemAdvisories(t *testing.T) { database.DeleteSystemAdvisories(t, testDBID, []int64{3, 4}) } -func TestCalcAdvisoryChanges(t *testing.T) { - system := &models.SystemPlatformV2{ - Inventory: models.SystemInventory{ID: testDBID, RhAccountID: rhAccountID}, - Patch: models.SystemPatch{}, - } - advisoriesByName := extendedAdvisoryMap{ - "ER-102": { - change: Update, - SystemAdvisories: models.SystemAdvisories{AdvisoryID: int64(102), StatusID: INSTALLABLE}, - }, - "ER-103": { - change: Remove, - SystemAdvisories: models.SystemAdvisories{AdvisoryID: int64(103), StatusID: INSTALLABLE}, - }, - "ER-104": { - change: Remove, - SystemAdvisories: models.SystemAdvisories{AdvisoryID: int64(104), StatusID: APPLICABLE}, - }, - "ER-105": { - change: Add, - SystemAdvisories: models.SystemAdvisories{AdvisoryID: int64(105), StatusID: APPLICABLE}, - }, - } - - changes := calcAdvisoryChanges(system, advisoriesByName) - expected := map[int64]models.AdvisoryAccountData{ - 102: {SystemsApplicable: 1, SystemsInstallable: 1}, - 103: {SystemsApplicable: -1, SystemsInstallable: -1}, - 104: {SystemsInstallable: -1}, - 105: {SystemsApplicable: 1}, - } - assert.Equal(t, len(expected), len(changes)) - for _, change := range changes { - advisoryID := change.AdvisoryID - assert.Equal(t, change.SystemsApplicable, expected[advisoryID].SystemsApplicable) - assert.Equal(t, change.SystemsInstallable, expected[advisoryID].SystemsInstallable) - } -} - func TestStoreMissingAdvisories(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() diff --git a/evaluator/evaluate_test.go b/evaluator/evaluate_test.go index acb5b819e..bbabba90d 100644 --- a/evaluator/evaluate_test.go +++ b/evaluator/evaluate_test.go @@ -59,7 +59,6 @@ func TestEvaluate(t *testing.T) { database.CreateSystemAdvisories(t, rhAccountID, testDBID, oldSystemAdvisoryIDs) database.CreateAdvisoryAccountData(t, rhAccountID, oldSystemAdvisoryIDs, 1) database.CreateSystemRepos(t, rhAccountID, testDBID, systemRepoIDs) - database.CheckCachesValid(t) // do evaluate the system data, err := sonic.Marshal(mqueue.PlatformEvent{ @@ -76,7 +75,6 @@ func TestEvaluate(t *testing.T) { database.CheckSystemPackages(t, rhAccountID, testDBID, len(expectedPackageIDs), expectedPackageIDs...) database.CheckSystemJustEvaluated(t, testInventoryID, 3, 1, 1, 0, 3, 1, 1, 0, 2, 2, 2, false) - database.CheckCachesValid(t) // test evaluation with third party repos thirdPartySystemRepoIDs := []int64{1, 2, 4} @@ -126,7 +124,6 @@ func TestEvaluateYum(t *testing.T) { database.DeleteAdvisoryAccountData(t, rhAccountID, expectedAdvisoryIDs) database.CreateSystemAdvisories(t, rhAccountID, testDBID, oldSystemAdvisoryIDs) database.CreateAdvisoryAccountData(t, rhAccountID, oldSystemAdvisoryIDs, 1) - database.CheckCachesValid(t) data, err := sonic.Marshal(mqueue.PlatformEvent{ SystemIDs: []uuid.UUID{testInventoryID}, diff --git a/tasks/system_culling/system_culling_test.go b/tasks/system_culling/system_culling_test.go index 7bef40137..5dda6b51f 100644 --- a/tasks/system_culling/system_culling_test.go +++ b/tasks/system_culling/system_culling_test.go @@ -59,38 +59,34 @@ func TestSingleSystemStale(t *testing.T) { var inv models.SystemInventory var accountData []models.AdvisoryAccountData - database.DebugWithCachesCheck("stale-trigger", func() { - assert.NotNil(t, staleDate) - assert.NoError(t, database.DB.Find(&accountData, "systems_installable > 1 "). - Order("systems_installable DESC").Error) - inv = loadFirstInstallableNonStaleInventory(t, database.DB, accountData[0].RhAccountID) - - updateInventoryStaleFields(t, database.DB, &inv, &staleDate, &staleDate, inv.Stale) - - nMarked, err := markSystemsStale(database.DB, 0) - assert.Nil(t, err) - assert.Equal(t, int64(0), nMarked) - - nMarked, err = markSystemsStale(database.DB, 1) - assert.Nil(t, err) - assert.Equal(t, int64(1), nMarked) - - oldAffected = accountData[0].SystemsInstallable - assert.NoError(t, database.DB.Find(&accountData, "rh_account_id = ? AND advisory_id = ?", - accountData[0].RhAccountID, accountData[0].AdvisoryID).Error) - - assert.Equal(t, oldAffected-1, accountData[0].SystemsInstallable, - "Systems affected should be decremented by one") - }) - - database.DebugWithCachesCheck("stale-trigger", func() { - updateInventoryStaleFields(t, database.DB, &inv, nil, nil, false) - assert.NoError(t, database.DB.Find(&accountData, "rh_account_id = ? AND advisory_id = ?", - accountData[0].RhAccountID, accountData[0].AdvisoryID).Error) - - assert.Equal(t, oldAffected, accountData[0].SystemsInstallable, - "Systems affected should be changed to match value at the start of the test case") - }) + assert.NotNil(t, staleDate) + assert.NoError(t, database.DB.Find(&accountData, "systems_installable > 1 "). + Order("systems_installable DESC").Error) + inv = loadFirstInstallableNonStaleInventory(t, database.DB, accountData[0].RhAccountID) + + updateInventoryStaleFields(t, database.DB, &inv, &staleDate, &staleDate, inv.Stale) + + nMarked, err := markSystemsStale(database.DB, 0) + assert.Nil(t, err) + assert.Equal(t, int64(0), nMarked) + + nMarked, err = markSystemsStale(database.DB, 1) + assert.Nil(t, err) + assert.Equal(t, int64(1), nMarked) + + oldAffected = accountData[0].SystemsInstallable + assert.NoError(t, database.DB.Find(&accountData, "rh_account_id = ? AND advisory_id = ?", + accountData[0].RhAccountID, accountData[0].AdvisoryID).Error) + + assert.Equal(t, oldAffected-1, accountData[0].SystemsInstallable, + "Systems affected should be decremented by one") + + updateInventoryStaleFields(t, database.DB, &inv, nil, nil, false) + assert.NoError(t, database.DB.Find(&accountData, "rh_account_id = ? AND advisory_id = ?", + accountData[0].RhAccountID, accountData[0].AdvisoryID).Error) + + assert.Equal(t, oldAffected, accountData[0].SystemsInstallable, + "Systems affected should be changed to match value at the start of the test case") } // Test for making sure system culling works @@ -190,21 +186,19 @@ func TestCullSystems(t *testing.T) { var cnt int64 var cntAfter int64 - database.DebugWithCachesCheck("delete-culled", func() { - assert.NoError(t, database.DB.Model(&models.SystemInventory{}).Count(&cnt).Error) - // first batch - nDeleted, err := deleteCulledSystems(database.DB, 3) - assert.Nil(t, err) - assert.Equal(t, int64(3), nDeleted) - - // second batch - nDeleted, err = deleteCulledSystems(database.DB, 3) - assert.Nil(t, err) - assert.Equal(t, int64(1), nDeleted) - - assert.NoError(t, database.DB.Model(&models.SystemInventory{}).Count(&cntAfter).Error) - assert.Equal(t, cnt-int64(nToDelete), cntAfter) - }) + assert.NoError(t, database.DB.Model(&models.SystemInventory{}).Count(&cnt).Error) + // first batch + nDeleted, err := deleteCulledSystems(database.DB, 3) + assert.Nil(t, err) + assert.Equal(t, int64(3), nDeleted) + + // second batch + nDeleted, err = deleteCulledSystems(database.DB, 3) + assert.Nil(t, err) + assert.Equal(t, int64(1), nDeleted) + + assert.NoError(t, database.DB.Model(&models.SystemInventory{}).Count(&cntAfter).Error) + assert.Equal(t, cnt-int64(nToDelete), cntAfter) } func TestPruneDeletedSystems(t *testing.T) { From 50b3d80cd9e83e2e1e70c8623508a6c045474c2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Fri, 18 Sep 2026 10:52:05 +0200 Subject: [PATCH 05/16] RHINENG-26122: remove CheckCaches admin endpoint --- base/database/testing.go | 60 ----------------------------------- docs/admin/openapi.json | 45 -------------------------- manager/routes/routes.go | 1 - turnpike/controllers/admin.go | 28 ---------------- 4 files changed, 134 deletions(-) diff --git a/base/database/testing.go b/base/database/testing.go index 9d0bba250..2f504d1d7 100644 --- a/base/database/testing.go +++ b/base/database/testing.go @@ -1,7 +1,6 @@ package database import ( - "app/base" "app/base/models" "app/base/utils" "fmt" @@ -39,65 +38,6 @@ type advisoryCount struct { SystemsApplicable int } -func CheckCachesValidRet() (bool, error) { - valid := true - var aad []models.AdvisoryAccountData - - tx := DB.WithContext(base.Context).Begin() - defer tx.Rollback() - err := tx.Set("gorm:query_option", "FOR SHARE OF advisory_account_data"). - Order("rh_account_id, advisory_id").Find(&aad).Error - if err != nil { - return false, err - } - var counts []advisoryCount - - err = tx.Select("si.rh_account_id, sa.advisory_id," + - "count(*) filter (where sa.status_id = 0) as systems_installable," + - "count(*) as systems_applicable"). - Table("system_advisories sa"). - Joins("JOIN system_inventory si ON sa.rh_account_id = si.rh_account_id AND sa.system_id = si.id"). - Joins("JOIN system_patch spatch ON si.id = spatch.system_id AND si.rh_account_id = spatch.rh_account_id"). - Where("si.stale = false AND spatch.last_evaluation IS NOT NULL"). - Order("si.rh_account_id, sa.advisory_id"). - Group("si.rh_account_id, sa.advisory_id"). - Find(&counts).Error - if err != nil { - return false, err - } - - cached := make(map[key][]int, len(aad)) - calculated := make(map[key][]int, len(counts)) - - for _, val := range aad { - cached[key{val.RhAccountID, val.AdvisoryID}] = []int{val.SystemsInstallable, val.SystemsApplicable} - } - for _, val := range counts { - calculated[key{val.RhAccountID, val.AdvisoryID}] = []int{val.SystemsInstallable, val.SystemsApplicable} - } - - crossCheckCache := func(a, b map[key][]int) { - for key, aCounts := range a { - bCounts := b[key] - if len(bCounts) == 0 { - bCounts = []int{0, 0} - } - for i, msg := range []string{"installable", "applicable"} { - if aCounts[i] != bCounts[i] { - utils.LogError("advisory_id", key.AdvisoryID, "account_id", key.AccountID, - "cached", aCounts[i], "calculated", bCounts[i], fmt.Sprintf("Cached %s counts mismatch", msg)) - valid = false - } - } - } - } - crossCheckCache(cached, calculated) - crossCheckCache(calculated, cached) - - tx.Commit() - return valid, nil -} - func CheckAdvisoriesInDB(t *testing.T, advisories []string) []int64 { var advisoryIDs []int64 err := DB.Model(models.AdvisoryMetadata{}).Where("name IN (?)", advisories). diff --git a/docs/admin/openapi.json b/docs/admin/openapi.json index 580a54c7d..3b52604cb 100644 --- a/docs/admin/openapi.json +++ b/docs/admin/openapi.json @@ -16,51 +16,6 @@ } ], "paths": { - "/check-caches": { - "get": { - "summary": "Check cached counts", - "description": "Check cached counts", - "operationId": "checkCaches", - "responses": { - "200": { - "description": "OK", - "content": { - "application/json": { - "schema": { - "type": "string" - } - } - } - }, - "409": { - "description": "Conflict", - "content": { - "application/json": { - "schema": { - "type": "string" - } - } - } - }, - "500": { - "description": "Internal Server Error", - "content": { - "application/json": { - "schema": { - "type": "object", - "additionalProperties": true - } - } - } - } - }, - "security": [ - { - "RhIdentity": [] - } - ] - } - }, "/clean-advisory-account-data": { "put": { "summary": "Clean advisory_account_data", diff --git a/manager/routes/routes.go b/manager/routes/routes.go index 346c8a2fd..4b8edbae9 100644 --- a/manager/routes/routes.go +++ b/manager/routes/routes.go @@ -102,7 +102,6 @@ func InitAdmin(app *gin.Engine, enableTurnpikeAuth bool) { api.GET("/sync", admin.Syncapi) api.GET("/re-calc", admin.Recalc) - api.GET("/check-caches", admin.CheckCaches) api.PUT("/refresh-packages", admin.RefreshPackagesHandler) api.PUT("/refresh-packages/:account", admin.RefreshPackagesAccountHandler) api.GET("/repack/:table_name", admin.RepackHandler) diff --git a/turnpike/controllers/admin.go b/turnpike/controllers/admin.go index 7d36df73d..f23b59f40 100644 --- a/turnpike/controllers/admin.go +++ b/turnpike/controllers/admin.go @@ -1,7 +1,6 @@ package controllers import ( - "app/base/database" "app/base/utils" "app/manager/middlewares" "app/tasks/caches" @@ -61,33 +60,6 @@ func Recalc(c *gin.Context) { c.JSON(http.StatusOK, "OK") } -// @Summary Check cached counts -// @Description Check cached counts -// @ID checkCaches -// @Security RhIdentity -// @Accept json -// @Produce json -// @Success 200 {object} string -// @Failure 409 {object} string -// @Failure 500 {object} map[string]interface{} -// @Router /check-caches [get] -func CheckCaches(c *gin.Context) { - valid, err := database.CheckCachesValidRet() - if err != nil { - utils.LogError("error", err, "Could not check validity of caches") - c.JSON(http.StatusInternalServerError, gin.H{"err": err.Error()}) - return - } - - if !valid { - utils.LogError("Cache mismatch found") - c.JSON(http.StatusConflict, "conflict") - return - } - - c.JSON(http.StatusOK, "caches counts OK") -} - // @Summary Refresh package caches // @Description Refresh package caches for all accounts with invalidated cache // @ID refreshPackagesCaches From f6b55e6e512bed90c6e2fd245030b9df3e404b90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Mon, 21 Sep 2026 12:01:22 +0200 Subject: [PATCH 06/16] RHINENG-26122: remove account_advisory backfill --- aggregator/notifications_test.go | 4 +- .../169_drop_advisory_account_data.down.sql | 16 +++++ .../169_drop_advisory_account_data.up.sql | 1 + database_admin/schema/create_schema.sql | 19 +----- deploy/clowdapp.yaml | 31 --------- main.go | 2 - tasks/caches/backfill_account_advisory.go | 63 ------------------- tasks/caches/caches.go | 8 --- .../refresh_account_advisory_caches_test.go | 6 +- 9 files changed, 23 insertions(+), 127 deletions(-) create mode 100644 database_admin/migrations/169_drop_advisory_account_data.down.sql create mode 100644 database_admin/migrations/169_drop_advisory_account_data.up.sql delete mode 100644 tasks/caches/backfill_account_advisory.go diff --git a/aggregator/notifications_test.go b/aggregator/notifications_test.go index 923ae3f33..c6431d76a 100644 --- a/aggregator/notifications_test.go +++ b/aggregator/notifications_test.go @@ -51,8 +51,8 @@ func TestPublishNewAdvisoryNotificationSuccess(t *testing.T) { notificationsPublisher = nil }() - // Backfill to populate account_advisory from system_advisories - assert.Nil(t, database.DB.Exec("SELECT backfill_account_advisory(1)").Error) + // populate account_advisory from system_advisories + assert.Nil(t, database.DB.Exec("SELECT refresh_account_advisory_caches_multi(NULL, 1)").Error) defer database.DeleteAccountAdvisoryByAccount(t, 1) // Advisory IDs 1-8 exist for rh_account_id=1 in test data diff --git a/database_admin/migrations/169_drop_advisory_account_data.down.sql b/database_admin/migrations/169_drop_advisory_account_data.down.sql new file mode 100644 index 000000000..1e44e1470 --- /dev/null +++ b/database_admin/migrations/169_drop_advisory_account_data.down.sql @@ -0,0 +1,16 @@ +CREATE OR REPLACE FUNCTION backfill_account_advisory(rh_account_id_in INTEGER) + RETURNS VOID AS +$backfill$ +BEGIN + PERFORM refresh_account_advisory_caches_multi(NULL, rh_account_id_in); + + -- copy `notified` for all `workspace_id`s per account + UPDATE account_advisory aa + SET notified = aad.notified + FROM advisory_account_data aad + WHERE aa.advisory_id = aad.advisory_id + AND aa.rh_account_id = aad.rh_account_id + AND aa.rh_account_id = rh_account_id_in + AND aad.notified IS NOT NULL; +END; +$backfill$ LANGUAGE plpgsql; diff --git a/database_admin/migrations/169_drop_advisory_account_data.up.sql b/database_admin/migrations/169_drop_advisory_account_data.up.sql new file mode 100644 index 000000000..70a918bb0 --- /dev/null +++ b/database_admin/migrations/169_drop_advisory_account_data.up.sql @@ -0,0 +1 @@ +DROP FUNCTION IF EXISTS backfill_account_advisory(rh_account_id_in INTEGER); diff --git a/database_admin/schema/create_schema.sql b/database_admin/schema/create_schema.sql index 0f1eb8f9f..3a44192f5 100644 --- a/database_admin/schema/create_schema.sql +++ b/database_admin/schema/create_schema.sql @@ -7,7 +7,7 @@ CREATE TABLE IF NOT EXISTS schema_migrations INSERT INTO schema_migrations -VALUES (168, false); +VALUES (169, false); -- --------------------------------------------------------------------------- -- Functions @@ -224,23 +224,6 @@ BEGIN END; $refresh_account_advisory$ LANGUAGE plpgsql; -CREATE OR REPLACE FUNCTION backfill_account_advisory(rh_account_id_in INTEGER) - RETURNS VOID AS -$backfill$ -BEGIN - PERFORM refresh_account_advisory_caches_multi(NULL, rh_account_id_in); - - -- copy `notified` for all `workspace_id`s per account - UPDATE account_advisory aa - SET notified = aad.notified - FROM advisory_account_data aad - WHERE aa.advisory_id = aad.advisory_id - AND aa.rh_account_id = aad.rh_account_id - AND aa.rh_account_id = rh_account_id_in - AND aad.notified IS NOT NULL; -END; -$backfill$ LANGUAGE plpgsql; - -- handle a new workspace with already notified advisory CREATE OR REPLACE FUNCTION sync_account_advisory_notified_on_insert() RETURNS TRIGGER AS diff --git a/deploy/clowdapp.yaml b/deploy/clowdapp.yaml index df05d3497..619e2c150 100644 --- a/deploy/clowdapp.yaml +++ b/deploy/clowdapp.yaml @@ -594,34 +594,6 @@ objects: key: vmaas-sync-database-password}}} - {name: POD_CONFIG, value: '${JOBS_CONFIG}'} - - name: account-advisory-backfill - activeDeadlineSeconds: ${{JOBS_TIMEOUT}} - schedule: ${ACCOUNT_ADVISORY_BACKFILL_SCHEDULE} - suspend: ${{ACCOUNT_ADVISORY_BACKFILL_SUSPEND}} - concurrencyPolicy: Forbid - podSpec: - image: ${IMAGE}:${IMAGE_TAG} - initContainers: - - name: check-for-db - image: ${IMAGE}:${IMAGE_TAG} - command: - - ./database_admin/check-upgraded.sh - env: - - {name: POD_CONFIG, value: '${DATABASE_ADMIN_CONFIG}'} - command: - - ./scripts/entrypoint.sh - - job - - account_advisory_backfill - env: - - {name: LOG_LEVEL, value: '${LOG_LEVEL_JOBS}'} - - {name: GIN_MODE, value: '${GIN_MODE}'} - - {name: SENTRY_DSN, valueFrom: {secretKeyRef: {name: patchman-sentry, key: sentry-dsn}}} - - {name: DB_DEBUG, value: '${DB_DEBUG_JOBS}'} - - {name: DB_USER, value: vmaas_sync} - - {name: DB_PASSWD, valueFrom: {secretKeyRef: {name: patchman-engine-database-passwords, - key: vmaas-sync-database-password}}} - - {name: POD_CONFIG, value: '${JOBS_CONFIG}'} - database: name: patchman version: 16 @@ -911,9 +883,6 @@ parameters: # Clean advisory_account_data - {name: CLEAN_AAD_SCHEDULE, value: '0 12 * * *'} # Cronjob schedule definition - {name: CLEAN_AAD_SUSPEND, value: 'false'} # Disable cronjob execution -# Backfill account_advisory -- {name: ACCOUNT_ADVISORY_BACKFILL_SCHEDULE, value: '0 3 * * *'} # Cronjob schedule definition -- {name: ACCOUNT_ADVISORY_BACKFILL_SUSPEND, value: 'true'} # Suspended until ready to run # Database admin - {name: MIGRATION_TIMEOUT, value: '7200'} # 2h timeout for db-migration job diff --git a/main.go b/main.go index feb8e84e3..6bc9d5965 100644 --- a/main.go +++ b/main.go @@ -76,8 +76,6 @@ func runJob(name string) { caches.RunPackageRefresh() case "repack": repack.RunRepack() - case "account_advisory_backfill": - caches.RunAccountAdvisoryBackfill() case "clean_advisory_account_data": cleaning.RunCleanAdvisoryAccountData() } diff --git a/tasks/caches/backfill_account_advisory.go b/tasks/caches/backfill_account_advisory.go deleted file mode 100644 index 479b723bb..000000000 --- a/tasks/caches/backfill_account_advisory.go +++ /dev/null @@ -1,63 +0,0 @@ -package caches - -import ( - "app/base/database" - "app/base/utils" - "app/tasks" - "sync" - - "gorm.io/gorm" -) - -func BackfillAccountAdvisory() { - var wg sync.WaitGroup - backfillAccountAdvisoryPerAccounts(&wg) - wg.Wait() -} - -func backfillAccountAdvisoryPerAccounts(wg *sync.WaitGroup) { - var rhAccountIDs []int - err := tasks.WithReadReplicaTx(func(tx *gorm.DB) error { - return tx.Table("rh_account"). - Order("hash_partition_id(id, 128), id"). - Pluck("id", &rhAccountIDs).Error - }) - if err != nil { - utils.LogError("err", err, "unable to load rh_account IDs for account_advisory backfill") - return - } - - utils.LogInfo("accounts", len(rhAccountIDs), "starting account_advisory backfill") - - guard := make(chan struct{}, 4) - - for i, rhAccountID := range rhAccountIDs { - guard <- struct{}{} - wg.Add(1) - go func(i, rhAccountID int) { - defer func() { - <-guard - wg.Done() - }() - - err := tasks.WithTx(func(tx *gorm.DB) error { - utils.LogInfo("i", i, "rh_account_id", rhAccountID, "backfilling account_advisory") - return tx.Exec("SELECT backfill_account_advisory(?)", rhAccountID).Error - }) - if err != nil { - utils.LogError("err", err, "rh_account_id", rhAccountID, "failed to backfill account_advisory") - return - } - utils.LogInfo("i", i, "rh_account_id", rhAccountID, "backfilled account_advisory") - - var advisoryIDs []int64 - if err := database.DB.Table("account_advisory"). - Where("rh_account_id = ?", rhAccountID). - Distinct("advisory_id"). - Pluck("advisory_id", &advisoryIDs).Error; err != nil { - utils.LogError("err", err, "rh_account_id", rhAccountID, "failed to load advisory IDs for drift check") - return - } - }(i, rhAccountID) - } -} diff --git a/tasks/caches/caches.go b/tasks/caches/caches.go index 39384439b..51175aa35 100644 --- a/tasks/caches/caches.go +++ b/tasks/caches/caches.go @@ -21,14 +21,6 @@ func RunAdvisoryRefresh() { RefreshAdvisoryCaches() } -func RunAccountAdvisoryBackfill() { - tasks.HandleContextCancel(tasks.WaitAndExit) - configure() - utils.LogInfo("Starting account_advisory backfill") - BackfillAccountAdvisory() - utils.LogInfo("Finished account_advisory backfill") -} - func RunPackageRefresh() { tasks.HandleContextCancel(tasks.WaitAndExit) configure() diff --git a/tasks/caches/refresh_account_advisory_caches_test.go b/tasks/caches/refresh_account_advisory_caches_test.go index 6bdd23985..a7c26bb7d 100644 --- a/tasks/caches/refresh_account_advisory_caches_test.go +++ b/tasks/caches/refresh_account_advisory_caches_test.go @@ -19,8 +19,8 @@ func TestRefreshAccountAdvisoryCaches(t *testing.T) { workspace := testWorkspace - // populate account_advisory using backfill - assert.Nil(t, database.DB.Exec("SELECT backfill_account_advisory(1)").Error) + // populate account_advisory + assert.Nil(t, database.DB.Exec("SELECT refresh_account_advisory_caches_multi(NULL, 1)").Error) // capture correct counts before corrupting countAdv1 := database.PluckInt(database.DB.Table("account_advisory"). @@ -58,7 +58,7 @@ func TestRefreshAccountAdvisoryCachesRemovesOrphanedRows(t *testing.T) { configure() workspace := testWorkspace - assert.Nil(t, database.DB.Exec("SELECT backfill_account_advisory(1)").Error) + assert.Nil(t, database.DB.Exec("SELECT refresh_account_advisory_caches_multi(NULL, 1)").Error) // mark all systems in this workspace as stale assert.Nil(t, database.DB.Exec( From 5fcc3af342c01ce4a339f3d4eed03914c83f09a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Thu, 17 Sep 2026 17:41:11 +0200 Subject: [PATCH 07/16] RHINENG-26122: remove clean advisory_account_data --- deploy/clowdapp.yaml | 31 ------------- docs/admin/openapi.json | 45 ------------------- main.go | 2 - tasks/cleaning/clean_advisory_account_data.go | 34 -------------- turnpike/controllers/admin.go | 22 --------- 5 files changed, 134 deletions(-) delete mode 100644 tasks/cleaning/clean_advisory_account_data.go diff --git a/deploy/clowdapp.yaml b/deploy/clowdapp.yaml index 619e2c150..16863e29f 100644 --- a/deploy/clowdapp.yaml +++ b/deploy/clowdapp.yaml @@ -566,34 +566,6 @@ objects: - {name: DB_DEBUG, value: '${DB_DEBUG_JOBS}'} - {name: POD_CONFIG, value: '${JOBS_CONFIG}'} - - name: clean-advisory-account-data - activeDeadlineSeconds: ${{JOBS_TIMEOUT}} - schedule: ${CLEAN_AAD_SCHEDULE} - suspend: ${{CLEAN_AAD_SUSPEND}} - concurrencyPolicy: Forbid - podSpec: - image: ${IMAGE}:${IMAGE_TAG} - initContainers: - - name: check-for-db - image: ${IMAGE}:${IMAGE_TAG} - command: - - ./database_admin/check-upgraded.sh - env: - - {name: POD_CONFIG, value: '${DATABASE_ADMIN_CONFIG}'} - command: - - ./scripts/entrypoint.sh - - job - - clean_advisory_account_data - env: - - {name: LOG_LEVEL, value: '${LOG_LEVEL_JOBS}'} - - {name: GIN_MODE, value: '${GIN_MODE}'} - - {name: SENTRY_DSN, valueFrom: {secretKeyRef: {name: patchman-sentry, key: sentry-dsn}}} - - {name: DB_DEBUG, value: '${DB_DEBUG_JOBS}'} - - {name: DB_USER, value: vmaas_sync} - - {name: DB_PASSWD, valueFrom: {secretKeyRef: {name: patchman-engine-database-passwords, - key: vmaas-sync-database-password}}} - - {name: POD_CONFIG, value: '${JOBS_CONFIG}'} - database: name: patchman version: 16 @@ -880,9 +852,6 @@ parameters: # Repack - {name: REPACK_SCHEDULE, value: '0 11 * * 5'} # Cronjob schedule definition - {name: REPACK_SUSPEND, value: 'false'} # Disable cronjob execution -# Clean advisory_account_data -- {name: CLEAN_AAD_SCHEDULE, value: '0 12 * * *'} # Cronjob schedule definition -- {name: CLEAN_AAD_SUSPEND, value: 'false'} # Disable cronjob execution # Database admin - {name: MIGRATION_TIMEOUT, value: '7200'} # 2h timeout for db-migration job diff --git a/docs/admin/openapi.json b/docs/admin/openapi.json index 3b52604cb..64fa2ca43 100644 --- a/docs/admin/openapi.json +++ b/docs/admin/openapi.json @@ -16,51 +16,6 @@ } ], "paths": { - "/clean-advisory-account-data": { - "put": { - "summary": "Clean advisory_account_data", - "description": "Delete rows with no installable and applicable systems", - "operationId": "cleanAdvisoryAccountData", - "responses": { - "200": { - "description": "OK", - "content": { - "application/json": { - "schema": { - "type": "string" - } - } - } - }, - "409": { - "description": "Conflict", - "content": { - "application/json": { - "schema": { - "type": "string" - } - } - } - }, - "500": { - "description": "Internal Server Error", - "content": { - "application/json": { - "schema": { - "type": "object", - "additionalProperties": true - } - } - } - } - }, - "security": [ - { - "RhIdentity": [] - } - ] - } - }, "/database/pg_repack/recreate": { "put": { "summary": "Recreate pg_repack database extension", diff --git a/main.go b/main.go index 6bc9d5965..e5578b528 100644 --- a/main.go +++ b/main.go @@ -76,7 +76,5 @@ func runJob(name string) { caches.RunPackageRefresh() case "repack": repack.RunRepack() - case "clean_advisory_account_data": - cleaning.RunCleanAdvisoryAccountData() } } diff --git a/tasks/cleaning/clean_advisory_account_data.go b/tasks/cleaning/clean_advisory_account_data.go deleted file mode 100644 index a5b12d00d..000000000 --- a/tasks/cleaning/clean_advisory_account_data.go +++ /dev/null @@ -1,34 +0,0 @@ -package cleaning - -import ( - "app/base/core" - "app/base/models" - "app/base/utils" - "app/tasks" -) - -func RunCleanAdvisoryAccountData() { - tasks.HandleContextCancel(tasks.WaitAndExit) - core.ConfigureApp() - defer utils.LogPanics(true) - utils.LogInfo("Deleting advisory rows with 0 applicable systems from advisory_account_data") - - if err := CleanAdvisoryAccountData(); err != nil { - utils.LogError("err", err, "Cleaning advisory account data") - return - } - utils.LogInfo("CleanAdvisoryAccountData task performed successfully") -} - -func CleanAdvisoryAccountData() error { - tx := tasks.CancelableDB().Begin() - defer tx.Rollback() - - err := tx.Delete(&models.AdvisoryAccountData{}, "systems_installable <= 0 AND systems_applicable <= 0").Error - if err != nil { - return err - } - - tx.Commit() - return nil -} diff --git a/turnpike/controllers/admin.go b/turnpike/controllers/admin.go index f23b59f40..64f30ccf8 100644 --- a/turnpike/controllers/admin.go +++ b/turnpike/controllers/admin.go @@ -4,7 +4,6 @@ import ( "app/base/utils" "app/manager/middlewares" "app/tasks/caches" - "app/tasks/cleaning" "app/tasks/repack" sync "app/tasks/vmaas_sync" "errors" @@ -151,27 +150,6 @@ func RepackHandler(c *gin.Context) { c.JSON(http.StatusOK, "OK") } -// @Summary Clean advisory_account_data -// @Description Delete rows with no installable and applicable systems -// @ID cleanAdvisoryAccountData -// @Security RhIdentity -// @Accept json -// @Produce json -// @Success 200 {object} string -// @Failure 409 {object} string -// @Failure 500 {object} map[string]interface{} -// @Router /clean-advisory-account-data [put] -func CleanAADHandler(c *gin.Context) { - err := cleaning.CleanAdvisoryAccountData() - if err != nil { - utils.LogError("error", err, "Could not clean advisory account data") - c.JSON(http.StatusInternalServerError, gin.H{"err": err.Error()}) - return - } - - c.JSON(http.StatusOK, "cleaning advisory account data") -} - // @Summary Delete system by inventory id // @Description Delete system by inventory id // @ID deletesystem From 66ead1fdae8c3302bb8fbd3a2ee291b54e3fca9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Thu, 17 Sep 2026 17:41:11 +0200 Subject: [PATCH 08/16] RHINENG-26122: remove advisory cahces refresh Co-authored-by: Gemini --- aggregator/events_test.go | 1 - .../169_drop_advisory_account_data.down.sql | 72 +++++++++++ .../169_drop_advisory_account_data.up.sql | 14 +++ database_admin/schema/create_schema.sql | 114 ------------------ deploy/clowdapp.yaml | 30 ----- dev/test_data.sql | 2 +- evaluator/evaluate.go | 8 +- main.go | 2 - tasks/caches/caches.go | 7 -- tasks/caches/refresh_advisory_caches.go | 75 ------------ tasks/caches/refresh_advisory_caches_test.go | 37 ------ tasks/config.go | 2 - tasks/system_culling/system_culling_test.go | 96 +++++---------- tasks/vmaas_sync/vmaas_sync.go | 8 -- 14 files changed, 124 insertions(+), 344 deletions(-) delete mode 100644 tasks/caches/refresh_advisory_caches.go delete mode 100644 tasks/caches/refresh_advisory_caches_test.go diff --git a/aggregator/events_test.go b/aggregator/events_test.go index 692c98fa0..b9bc2c3d9 100644 --- a/aggregator/events_test.go +++ b/aggregator/events_test.go @@ -65,7 +65,6 @@ func TestBufferedEventsProcessedOnBatchThreshold(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() - assert.Nil(t, database.DB.Exec("SELECT refresh_advisory_caches(NULL, 1)").Error) defer database.DeleteAccountAdvisoryByAccount(t, 1) batchSize = 3 diff --git a/database_admin/migrations/169_drop_advisory_account_data.down.sql b/database_admin/migrations/169_drop_advisory_account_data.down.sql index 1e44e1470..34f5fb147 100644 --- a/database_admin/migrations/169_drop_advisory_account_data.down.sql +++ b/database_admin/migrations/169_drop_advisory_account_data.down.sql @@ -14,3 +14,75 @@ BEGIN AND aad.notified IS NOT NULL; END; $backfill$ LANGUAGE plpgsql; + +ALTER TABLE rh_account ADD COLUMN valid_advisory_cache BOOLEAN NOT NULL DEFAULT FALSE; + +CREATE OR REPLACE FUNCTION refresh_advisory_caches_multi(advisory_ids_in INTEGER[] DEFAULT NULL, + rh_account_id_in INTEGER DEFAULT NULL) + RETURNS VOID AS +$refresh_advisory$ +BEGIN + -- Lock rows + PERFORM aad.rh_account_id, aad.advisory_id + FROM advisory_account_data aad + WHERE (aad.advisory_id = ANY (advisory_ids_in) OR advisory_ids_in IS NULL) + AND (aad.rh_account_id = rh_account_id_in OR rh_account_id_in IS NULL) + FOR UPDATE OF aad; + + WITH current_counts AS ( + SELECT sa.advisory_id, sa.rh_account_id, + count(sa.*) filter (where sa.status_id = 0) as systems_installable, + count(sa.*) as systems_applicable + FROM system_advisories sa + JOIN system_inventory si + ON sa.rh_account_id = si.rh_account_id AND sa.system_id = si.id + JOIN system_patch sp + ON si.id = sp.system_id AND sp.rh_account_id = si.rh_account_id + WHERE sp.last_evaluation IS NOT NULL + AND si.stale = FALSE + AND (sa.advisory_id = ANY (advisory_ids_in) OR advisory_ids_in IS NULL) + AND (si.rh_account_id = rh_account_id_in OR rh_account_id_in IS NULL) + GROUP BY sa.advisory_id, sa.rh_account_id + ), + upserted AS ( + INSERT INTO advisory_account_data (advisory_id, rh_account_id, systems_installable, systems_applicable) + SELECT advisory_id, rh_account_id, systems_installable, systems_applicable + FROM current_counts + ON CONFLICT (advisory_id, rh_account_id) DO UPDATE SET + systems_installable = EXCLUDED.systems_installable, + systems_applicable = EXCLUDED.systems_applicable + ) + DELETE FROM advisory_account_data + WHERE (advisory_id, rh_account_id) NOT IN (SELECT advisory_id, rh_account_id FROM current_counts) + AND (advisory_id = ANY (advisory_ids_in) OR advisory_ids_in IS NULL) + AND (rh_account_id = rh_account_id_in OR rh_account_id_in IS NULL); +END; +$refresh_advisory$ language plpgsql; + +CREATE OR REPLACE FUNCTION refresh_advisory_caches(advisory_id_in INTEGER DEFAULT NULL, + rh_account_id_in INTEGER DEFAULT NULL) + RETURNS VOID AS +$refresh_advisory$ +BEGIN + IF advisory_id_in IS NOT NULL THEN + PERFORM refresh_advisory_caches_multi(ARRAY [advisory_id_in], rh_account_id_in); + ELSE + PERFORM refresh_advisory_caches_multi(NULL, rh_account_id_in); + END IF; +END; +$refresh_advisory$ language plpgsql; + +-- Not adding the following functions, because it was not used anywhere: +-- - refresh_advisory_cached_counts(advisory_name varchar) +-- - refresh_advisory_account_cached_counts(advisory_name varchar, rh_account_name varchar) +-- - refresh_account_cached_counts(rh_account_in varchar) + +CREATE OR REPLACE FUNCTION refresh_all_cached_counts() + RETURNS void AS +$refresh_all_cached_counts$ +BEGIN + PERFORM refresh_system_caches(NULL, NULL); + PERFORM refresh_advisory_caches(NULL, NULL); +END; +$refresh_all_cached_counts$ + LANGUAGE 'plpgsql'; diff --git a/database_admin/migrations/169_drop_advisory_account_data.up.sql b/database_admin/migrations/169_drop_advisory_account_data.up.sql index 70a918bb0..2459021e4 100644 --- a/database_admin/migrations/169_drop_advisory_account_data.up.sql +++ b/database_admin/migrations/169_drop_advisory_account_data.up.sql @@ -1 +1,15 @@ DROP FUNCTION IF EXISTS backfill_account_advisory(rh_account_id_in INTEGER); + +ALTER TABLE rh_account DROP COLUMN IF EXISTS valid_advisory_cache; + +DROP FUNCTION IF EXISTS refresh_advisory_caches(advisory_id_in INTEGER, rh_account_id_in INTEGER); + +DROP FUNCTION IF EXISTS refresh_advisory_cached_counts(advisory_name varchar); + +DROP FUNCTION IF EXISTS refresh_advisory_account_cached_counts(advisory_name varchar, rh_account_name varchar); + +DROP FUNCTION IF EXISTS refresh_account_cached_counts(rh_account_in varchar); + +DROP FUNCTION IF EXISTS refresh_all_cached_counts(); + +DROP FUNCTION IF EXISTS refresh_advisory_caches_multi(advisory_ids_in INTEGER[], rh_account_id_in INTEGER); diff --git a/database_admin/schema/create_schema.sql b/database_admin/schema/create_schema.sql index 3a44192f5..5c5f8c0ff 100644 --- a/database_admin/schema/create_schema.sql +++ b/database_admin/schema/create_schema.sql @@ -115,61 +115,6 @@ BEGIN END; $system_update$ LANGUAGE plpgsql; -CREATE OR REPLACE FUNCTION refresh_advisory_caches_multi(advisory_ids_in INTEGER[] DEFAULT NULL, - rh_account_id_in INTEGER DEFAULT NULL) - RETURNS VOID AS -$refresh_advisory$ -BEGIN - -- Lock rows - PERFORM aad.rh_account_id, aad.advisory_id - FROM advisory_account_data aad - WHERE (aad.advisory_id = ANY (advisory_ids_in) OR advisory_ids_in IS NULL) - AND (aad.rh_account_id = rh_account_id_in OR rh_account_id_in IS NULL) - FOR UPDATE OF aad; - - WITH current_counts AS ( - SELECT sa.advisory_id, sa.rh_account_id, - count(sa.*) filter (where sa.status_id = 0) as systems_installable, - count(sa.*) as systems_applicable - FROM system_advisories sa - JOIN system_inventory si - ON sa.rh_account_id = si.rh_account_id AND sa.system_id = si.id - JOIN system_patch sp - ON si.id = sp.system_id AND sp.rh_account_id = si.rh_account_id - WHERE sp.last_evaluation IS NOT NULL - AND si.stale = FALSE - AND (sa.advisory_id = ANY (advisory_ids_in) OR advisory_ids_in IS NULL) - AND (si.rh_account_id = rh_account_id_in OR rh_account_id_in IS NULL) - GROUP BY sa.advisory_id, sa.rh_account_id - ), - upserted AS ( - INSERT INTO advisory_account_data (advisory_id, rh_account_id, systems_installable, systems_applicable) - SELECT advisory_id, rh_account_id, systems_installable, systems_applicable - FROM current_counts - ON CONFLICT (advisory_id, rh_account_id) DO UPDATE SET - systems_installable = EXCLUDED.systems_installable, - systems_applicable = EXCLUDED.systems_applicable - ) - DELETE FROM advisory_account_data - WHERE (advisory_id, rh_account_id) NOT IN (SELECT advisory_id, rh_account_id FROM current_counts) - AND (advisory_id = ANY (advisory_ids_in) OR advisory_ids_in IS NULL) - AND (rh_account_id = rh_account_id_in OR rh_account_id_in IS NULL); -END; -$refresh_advisory$ language plpgsql; - -CREATE OR REPLACE FUNCTION refresh_advisory_caches(advisory_id_in INTEGER DEFAULT NULL, - rh_account_id_in INTEGER DEFAULT NULL) - RETURNS VOID AS -$refresh_advisory$ -BEGIN - IF advisory_id_in IS NOT NULL THEN - PERFORM refresh_advisory_caches_multi(ARRAY [advisory_id_in], rh_account_id_in); - ELSE - PERFORM refresh_advisory_caches_multi(NULL, rh_account_id_in); - END IF; -END; -$refresh_advisory$ language plpgsql; - CREATE OR REPLACE FUNCTION refresh_account_advisory_caches_multi(advisory_ids_in INTEGER[] DEFAULT NULL, rh_account_id_in INTEGER DEFAULT NULL) RETURNS VOID AS @@ -297,64 +242,6 @@ END; $update_system_caches$ LANGUAGE 'plpgsql'; --- refresh_all_cached_counts --- WARNING: executing this procedure takes long time, --- use only when necessary, e.g. during upgrade to populate initial caches -CREATE OR REPLACE FUNCTION refresh_all_cached_counts() - RETURNS void AS -$refresh_all_cached_counts$ -BEGIN - PERFORM refresh_system_caches(NULL, NULL); - PERFORM refresh_advisory_caches(NULL, NULL); -END; -$refresh_all_cached_counts$ - LANGUAGE 'plpgsql'; - -CREATE OR REPLACE FUNCTION refresh_account_cached_counts(rh_account_in varchar) - RETURNS void AS -$refresh_account_cached_counts$ -DECLARE - rh_account_id_in INT; -BEGIN - -- update advisory count for ordered systems - SELECT id FROM rh_account WHERE name = rh_account_in INTO rh_account_id_in; - - PERFORM refresh_system_caches(NULL, rh_account_id_in); - PERFORM refresh_advisory_caches(NULL, rh_account_id_in); -END; -$refresh_account_cached_counts$ - LANGUAGE 'plpgsql'; - -CREATE OR REPLACE FUNCTION refresh_advisory_cached_counts(advisory_name varchar) - RETURNS void AS -$refresh_advisory_cached_counts$ -DECLARE - advisory_id_id BIGINT; -BEGIN - -- update system count for advisory - SELECT id FROM advisory_metadata WHERE name = advisory_name INTO advisory_id_id; - - PERFORM refresh_advisory_caches(advisory_id_id, NULL); -END; -$refresh_advisory_cached_counts$ - LANGUAGE 'plpgsql'; - -CREATE OR REPLACE FUNCTION refresh_advisory_account_cached_counts(advisory_name varchar, rh_account_name varchar) - RETURNS void AS -$refresh_advisory_account_cached_counts$ -DECLARE - advisory_md_id BIGINT; - rh_account_id_in INT; -BEGIN - -- update system count for ordered advisories - SELECT id FROM advisory_metadata WHERE name = advisory_name INTO advisory_md_id; - SELECT id FROM rh_account WHERE name = rh_account_name INTO rh_account_id_in; - - PERFORM refresh_advisory_caches(advisory_md_id, rh_account_id_in); -END; -$refresh_advisory_account_cached_counts$ - LANGUAGE 'plpgsql'; - CREATE OR REPLACE FUNCTION refresh_system_cached_counts(inventory_id_in varchar) RETURNS void AS $refresh_system_cached_counts$ @@ -573,7 +460,6 @@ CREATE TABLE IF NOT EXISTS rh_account name TEXT UNIQUE CHECK (NOT empty(name)), org_id TEXT UNIQUE CHECK (NOT empty(org_id)), valid_package_cache BOOLEAN NOT NULL DEFAULT FALSE, - valid_advisory_cache BOOLEAN NOT NULL DEFAULT FALSE, CHECK (name IS NOT NULL OR org_id IS NOT NULL), PRIMARY KEY (id) ) TABLESPACE pg_default; diff --git a/deploy/clowdapp.yaml b/deploy/clowdapp.yaml index 16863e29f..af1bf6978 100644 --- a/deploy/clowdapp.yaml +++ b/deploy/clowdapp.yaml @@ -485,34 +485,6 @@ objects: - {name: PROMETHEUS_PUSHGATEWAY,value: '${PROMETHEUS_PUSHGATEWAY}'} - {name: POD_CONFIG, value: '${JOBS_CONFIG}'} - - name: advisory-refresh - activeDeadlineSeconds: ${{JOBS_TIMEOUT}} - schedule: ${ADVISORY_REFRESH_SCHEDULE} - suspend: ${{ADVISORY_REFRESH_SUSPEND}} - concurrencyPolicy: Forbid - podSpec: - image: ${IMAGE}:${IMAGE_TAG} - initContainers: - - name: check-for-db - image: ${IMAGE}:${IMAGE_TAG} - command: - - ./database_admin/check-upgraded.sh - env: - - {name: POD_CONFIG, value: '${DATABASE_ADMIN_CONFIG}'} - command: - - ./scripts/entrypoint.sh - - job - - advisory_cache_refresh - env: - - {name: LOG_LEVEL, value: '${LOG_LEVEL_JOBS}'} - - {name: GIN_MODE, value: '${GIN_MODE}'} - - {name: SENTRY_DSN, valueFrom: {secretKeyRef: {name: patchman-sentry, key: sentry-dsn}}} - - {name: DB_DEBUG, value: '${DB_DEBUG_JOBS}'} - - {name: DB_USER, value: vmaas_sync} - - {name: DB_PASSWD, valueFrom: {secretKeyRef: {name: patchman-engine-database-passwords, - key: vmaas-sync-database-password}}} - - {name: POD_CONFIG, value: '${JOBS_CONFIG}'} - - name: delete-unused activeDeadlineSeconds: ${{JOBS_TIMEOUT}} schedule: ${DELETE_UNUSED_SCHEDULE} @@ -847,8 +819,6 @@ parameters: # Cache refresh - {name: PKG_REFRESH_SCHEDULE, value: '5 11-20/2 * * *'} # Cronjob schedule definition - {name: PKG_REFRESH_SUSPEND, value: 'false'} # Disable cronjob execution -- {name: ADVISORY_REFRESH_SCHEDULE, value: '*/15 * * * *'} # Cronjob schedule definition -- {name: ADVISORY_REFRESH_SUSPEND, value: 'true'} # Disable cronjob execution # Repack - {name: REPACK_SCHEDULE, value: '0 11 * * 5'} # Cronjob schedule definition - {name: REPACK_SUSPEND, value: 'false'} # Disable cronjob execution diff --git a/dev/test_data.sql b/dev/test_data.sql index ceb152bdf..7aa71e238 100644 --- a/dev/test_data.sql +++ b/dev/test_data.sql @@ -203,7 +203,7 @@ INSERT INTO system_package2 (rh_account_id, system_id, name_id, package_id, inst INSERT INTO timestamp_kv (name, value) VALUES ('last_eval_repo_based', '2018-04-05T01:23:45+02:00'); -SELECT refresh_all_cached_counts(); +SELECT refresh_system_caches(NULL, NULL); SELECT refresh_account_advisory_caches_multi(NULL, NULL); ALTER TABLE advisory_metadata ALTER COLUMN id RESTART WITH 100; diff --git a/evaluator/evaluate.go b/evaluator/evaluate.go index fe24527ec..0ba010878 100644 --- a/evaluator/evaluate.go +++ b/evaluator/evaluate.go @@ -719,12 +719,12 @@ func parseVmaasJSON(inv *models.SystemInventory) (vmaas.UpdatesV3Request, error) return utils.ParseVmaasJSON(inv) } -func invalidateCaches(orgID string) error { +func invalidatePackageCache(orgID string) error { err := database.DB.Model(models.RhAccount{}). Where("org_id = ?", orgID). - Where("valid_package_cache = true OR valid_advisory_cache = true"). + Where("valid_package_cache = true"). // use map because struct updates only non-zero values and we need to update it to `false` - Updates(map[string]interface{}{"valid_package_cache": false, "valid_advisory_cache": false}). + Updates(map[string]interface{}{"valid_package_cache": false}). Error return err } @@ -768,7 +768,7 @@ func evaluateHandler(m mqueue.KafkaMessage) error { } wg.Wait() - if cacheErr := invalidateCaches(event.GetOrgID()); cacheErr != nil { + if cacheErr := invalidatePackageCache(event.GetOrgID()); cacheErr != nil { utils.LogError("err", cacheErr, "org_id", event.GetOrgID(), "Couldn't invalidate caches") } diff --git a/main.go b/main.go index e5578b528..db3836525 100644 --- a/main.go +++ b/main.go @@ -68,8 +68,6 @@ func runJob(name string) { vmaas_sync.RunVmaasSync() case "system_culling": system_culling.RunSystemCulling() - case "advisory_cache_refresh": - caches.RunAdvisoryRefresh() case "delete_unused": cleaning.RunDeleteUnusedData() case "packages_cache_refresh": diff --git a/tasks/caches/caches.go b/tasks/caches/caches.go index 51175aa35..af0f172c3 100644 --- a/tasks/caches/caches.go +++ b/tasks/caches/caches.go @@ -14,13 +14,6 @@ func configure() { core.ConfigureApp() } -func RunAdvisoryRefresh() { - tasks.HandleContextCancel(tasks.WaitAndExit) - configure() - utils.LogInfo("Refreshing advisory cache") - RefreshAdvisoryCaches() -} - func RunPackageRefresh() { tasks.HandleContextCancel(tasks.WaitAndExit) configure() diff --git a/tasks/caches/refresh_advisory_caches.go b/tasks/caches/refresh_advisory_caches.go deleted file mode 100644 index cdec369fe..000000000 --- a/tasks/caches/refresh_advisory_caches.go +++ /dev/null @@ -1,75 +0,0 @@ -package caches - -import ( - "app/base/utils" - "app/tasks" - "sync" - - "github.com/pkg/errors" - "gorm.io/gorm" -) - -func RefreshAdvisoryCaches() { - var wg sync.WaitGroup - refreshAdvisoryCachesPerAccounts(&wg) - wg.Wait() -} - -func refreshAdvisoryCachesPerAccounts(wg *sync.WaitGroup) { - var rhAccountIDs []int - err := tasks.WithReadReplicaTx(func(tx *gorm.DB) error { - return tx.Table("rh_account"). - Where("valid_advisory_cache = FALSE"). - Order("hash_partition_id(id, 128), id"). - Pluck("id", &rhAccountIDs).Error - }) - if skipNAccountsRefresh > 0 { - utils.LogInfo("n", skipNAccountsRefresh, "Skipping refresh of first N accounts") - rhAccountIDs = rhAccountIDs[skipNAccountsRefresh:] - } - utils.LogInfo("accounts", len(rhAccountIDs), "Starting advisory cache refresh for accounts") - if err != nil { - utils.LogError("err", err, "Unable to load rh_account table ids to refresh caches") - return - } - - // use max 4 goroutines for cache refresh - guard := make(chan struct{}, 4) - - for i, rhAccountID := range rhAccountIDs { - guard <- struct{}{} - wg.Add(1) - go func(i, rhAccountID int) { - defer func() { - <-guard - wg.Done() - }() - - err = tasks.WithTx(func(tx *gorm.DB) error { - utils.LogInfo("i", i, "rh_account_id", rhAccountID, "Refreshing account advisory cache") - return tx.Exec("select refresh_advisory_caches(NULL, ?)", rhAccountID).Error - }) - if err != nil { - utils.LogError("err", err, "rh_account_id", rhAccountID, - "Refreshed account advisory caches") - return - } - if err := updateAdvisoryCacheValidity(rhAccountID); err != nil { - utils.LogError("err", err, "rh_account_id", rhAccountID, "Refresh failed") - return - } - utils.LogInfo("i", i, "rh_account_id", rhAccountID, "Refreshed account advisory cache") - }(i, rhAccountID) - } -} - -func updateAdvisoryCacheValidity(accID int) error { - utils.LogDebug("Updating cache validity") - err := tasks.WithTx(func(tx *gorm.DB) error { - return tx.Table("rh_account"). - Where("valid_advisory_cache = ?", false). - Where("id = ?", accID). - Update("valid_advisory_cache", true).Error - }) - return errors.Wrap(err, "failed to update valid_advisory_cache") -} diff --git a/tasks/caches/refresh_advisory_caches_test.go b/tasks/caches/refresh_advisory_caches_test.go deleted file mode 100644 index 951fc0e51..000000000 --- a/tasks/caches/refresh_advisory_caches_test.go +++ /dev/null @@ -1,37 +0,0 @@ -package caches - -import ( - "app/base/core" - "app/base/database" - "app/base/models" - "app/base/utils" - "sync" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestRefreshAdvisoryCachesPerAccounts(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - configure() - - // set wrong numbers of caches - assert.Nil(t, database.DB.Model(&models.AdvisoryAccountData{}). - Where("advisory_id = 1 AND rh_account_id = 2").Update("systems_installable", 5).Error) - assert.Nil(t, database.DB.Model(&models.AdvisoryAccountData{}). - Where("advisory_id = 2 AND rh_account_id = 1").Update("systems_installable", 3).Error) - assert.Nil(t, database.DB.Model(&models.AdvisoryAccountData{}). - Where("advisory_id = 3 AND rh_account_id = 1").Update("systems_installable", 8).Error) - - var wg sync.WaitGroup - refreshAdvisoryCachesPerAccounts(&wg) - wg.Wait() - - assert.Equal(t, 1, database.PluckInt(database.DB.Table("advisory_account_data"). - Where("advisory_id = 1 AND rh_account_id = 2"), "systems_installable")) - assert.Equal(t, 0, database.PluckInt(database.DB.Table("advisory_account_data"). - Where("advisory_id = 2 AND rh_account_id = 1"), "systems_installable")) - assert.Equal(t, 1, database.PluckInt(database.DB.Table("advisory_account_data"). - Where("advisory_id = 3 AND rh_account_id = 1"), "systems_installable")) -} diff --git a/tasks/config.go b/tasks/config.go index d059dc58c..9f440a599 100644 --- a/tasks/config.go +++ b/tasks/config.go @@ -25,8 +25,6 @@ var ( EnablePackagesSync = utils.PodConfig.GetBool("packages_sync", true) // Toggle repo sync in vmaas_sync EnableReposSync = utils.PodConfig.GetBool("repos_sync", true) - // Toggle advisory cache refresh in vmaas_sync - EnableAdvisoryCacheRefresh = utils.PodConfig.GetBool("advisory_cache_refresh", true) // Sync data in vnass_sync based on timestamp EnableModifiedSinceSync = utils.PodConfig.GetBool("modified_since_sync", true) // Page size for /errata vmass API call diff --git a/tasks/system_culling/system_culling_test.go b/tasks/system_culling/system_culling_test.go index 5dda6b51f..af085df22 100644 --- a/tasks/system_culling/system_culling_test.go +++ b/tasks/system_culling/system_culling_test.go @@ -24,20 +24,6 @@ func loadAllSystemInventories(t *testing.T, db *gorm.DB) []models.SystemInventor return rows } -func loadFirstInstallableNonStaleInventory(t *testing.T, db *gorm.DB, rhAccountID int) models.SystemInventory { - t.Helper() - var inv models.SystemInventory - err := db.Table("system_inventory AS si"). - Select("si.*"). - Joins("JOIN system_patch sp ON sp.system_id = si.id AND sp.rh_account_id = si.rh_account_id"). - Where("si.rh_account_id = ? AND si.stale = ? AND sp.installable_advisory_count_cache > ?", - rhAccountID, false, 0). - Order("si.id"). - First(&inv).Error - assert.NoError(t, err) - return inv -} - func updateInventoryStaleFields(t *testing.T, db *gorm.DB, inv *models.SystemInventory, staleTS, staleWarnTS *time.Time, stale bool, ) { @@ -55,87 +41,68 @@ func TestSingleSystemStale(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() - var oldAffected int var inv models.SystemInventory - var accountData []models.AdvisoryAccountData - assert.NotNil(t, staleDate) - assert.NoError(t, database.DB.Find(&accountData, "systems_installable > 1 "). - Order("systems_installable DESC").Error) - inv = loadFirstInstallableNonStaleInventory(t, database.DB, accountData[0].RhAccountID) + assert.NoError(t, database.DB.Where("stale = ?", false).Order("rh_account_id, id").First(&inv).Error) - updateInventoryStaleFields(t, database.DB, &inv, &staleDate, &staleDate, inv.Stale) + updateInventoryStaleFields(t, database.DB, &inv, &staleDate, &staleDate, false) nMarked, err := markSystemsStale(database.DB, 0) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, int64(0), nMarked) nMarked, err = markSystemsStale(database.DB, 1) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, int64(1), nMarked) - oldAffected = accountData[0].SystemsInstallable - assert.NoError(t, database.DB.Find(&accountData, "rh_account_id = ? AND advisory_id = ?", - accountData[0].RhAccountID, accountData[0].AdvisoryID).Error) + var updated models.SystemInventory + assert.NoError(t, database.DB.First(&updated, "rh_account_id = ? AND id = ?", inv.RhAccountID, inv.ID).Error) + assert.True(t, updated.Stale, "System should be marked stale") - assert.Equal(t, oldAffected-1, accountData[0].SystemsInstallable, - "Systems affected should be decremented by one") + futureDate := time.Now().Add(24 * time.Hour) + updateInventoryStaleFields(t, database.DB, &inv, &futureDate, &futureDate, true) - updateInventoryStaleFields(t, database.DB, &inv, nil, nil, false) - assert.NoError(t, database.DB.Find(&accountData, "rh_account_id = ? AND advisory_id = ?", - accountData[0].RhAccountID, accountData[0].AdvisoryID).Error) + nMarked, err = markSystemsStale(database.DB, 1) + assert.NoError(t, err) + assert.Equal(t, int64(1), nMarked) + + assert.NoError(t, database.DB.First(&updated, "rh_account_id = ? AND id = ?", inv.RhAccountID, inv.ID).Error) + assert.False(t, updated.Stale, "System should be marked not stale") - assert.Equal(t, oldAffected, accountData[0].SystemsInstallable, - "Systems affected should be changed to match value at the start of the test case") + // Cleanup fixture state + updateInventoryStaleFields(t, database.DB, &inv, nil, nil, false) } -// Test for making sure system culling works func TestMarkSystemsStale(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() - inventories := loadAllSystemInventories(t, database.DB) - var accountData []models.AdvisoryAccountData assert.NotNil(t, staleDate) - assert.NoError(t, database.DB.Find(&accountData).Error) + inventories := loadAllSystemInventories(t, database.DB) for i := range inventories { assert.NotEqual(t, 0, inventories[i].ID) - assert.Equal(t, false, inventories[i].Stale, "No systems should be stale") - updateInventoryStaleFields(t, database.DB, &inventories[i], &staleDate, &staleDate, inventories[i].Stale) + assert.False(t, inventories[i].Stale, "No systems should start stale") + updateInventoryStaleFields(t, database.DB, &inventories[i], &staleDate, &staleDate, false) } - assert.True(t, len(accountData) > 0, "We should have some systems affected by advisories") - for _, a := range accountData { - assert.True(t, a.SystemsInstallable+a.SystemsApplicable > 0, "We should have some systems affected") - } nMarked, err := markSystemsStale(database.DB, 500) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, int64(18), nMarked) inventories = loadAllSystemInventories(t, database.DB) for i := range inventories { - assert.Equal(t, true, inventories[i].Stale, "All systems should be stale") + assert.True(t, inventories[i].Stale, "All systems should be marked stale") + // Clean up fixture updateInventoryStaleFields(t, database.DB, &inventories[i], nil, nil, false) } - - assert.NoError(t, database.DB.Find(&accountData).Error) - assert.True(t, len(accountData) > 0, "advisory_account_data should still exist after unstale") - sumAffected := 0 - for _, a := range accountData { - sumAffected += a.SystemsInstallable + a.SystemsApplicable - } - assert.True(t, sumAffected > 0, - "after clearing stale, caches should show systems again (installable+applicable > 0)", sumAffected) } func TestMarkSystemsNotStale(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() - // This test runs before TestMarkSystemsStale by name order; the DB fixture is not stale. - // Match TestMarkSystemsStale setup so every host is stale, then verify clearing stale restores counts. - var accountData []models.AdvisoryAccountData assert.NotNil(t, staleDate) + futureDate := time.Now().Add(24 * time.Hour) inventories := loadAllSystemInventories(t, database.DB) for i := range inventories { @@ -148,14 +115,17 @@ func TestMarkSystemsNotStale(t *testing.T) { inventories = loadAllSystemInventories(t, database.DB) for i := range inventories { - assert.True(t, inventories[i].Stale, "all systems should be stale after markSystemsStale") - updateInventoryStaleFields(t, database.DB, &inventories[i], nil, nil, false) + assert.True(t, inventories[i].Stale, "all systems should be stale before un-staling") + updateInventoryStaleFields(t, database.DB, &inventories[i], &futureDate, &futureDate, true) } - assert.NoError(t, database.DB.Find(&accountData).Error) - assert.True(t, len(accountData) > 0, "We should have some systems affected by advisories") - for _, a := range accountData { - assert.True(t, a.SystemsInstallable+a.SystemsApplicable > 0, "We should have some systems affected") + nMarked, err = markSystemsStale(database.DB, 500) + assert.NoError(t, err) + assert.Equal(t, int64(18), nMarked) + + inventories = loadAllSystemInventories(t, database.DB) + for i := range inventories { + assert.False(t, inventories[i].Stale, "all systems should be marked not stale") } } diff --git a/tasks/vmaas_sync/vmaas_sync.go b/tasks/vmaas_sync/vmaas_sync.go index 773b44ce2..bafca438b 100644 --- a/tasks/vmaas_sync/vmaas_sync.go +++ b/tasks/vmaas_sync/vmaas_sync.go @@ -9,7 +9,6 @@ import ( "app/base/types" "app/base/utils" "app/tasks" - "app/tasks/caches" "net/http" "time" @@ -112,13 +111,6 @@ func SyncData(lastModifiedTS *types.Rfc3339TimestampWithZ, vmaasExportedTS *type database.UpdateTimestampKVValue(VmaasExported, *vmaasExportedTS.Time()) } - // refresh caches - if tasks.EnableAdvisoryCacheRefresh { - caches.RefreshAdvisoryCaches() - } else { - utils.LogInfo("Advisory cache refresh is disabled") - } - utils.LogInfo("Data sync finished successfully") return nil } From da326d6250e0fbfa3210f90603da4fc8763dcaf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Thu, 17 Sep 2026 17:41:11 +0200 Subject: [PATCH 09/16] RHINENG-26122: remove AdvisoryAccountData from tests --- base/database/testing.go | 38 ------------------------- evaluator/advisory_update_test.go | 4 --- evaluator/evaluate_test.go | 9 ------ evaluator/template_advisory_e2e_test.go | 1 - 4 files changed, 52 deletions(-) diff --git a/base/database/testing.go b/base/database/testing.go index 2f504d1d7..53f9ae12c 100644 --- a/base/database/testing.go +++ b/base/database/testing.go @@ -105,20 +105,6 @@ func CheckSystemJustEvaluated(t *testing.T, inventoryID uuid.UUID, nIAll, nIEnh, assert.Equal(t, thirdParty, patch.ThirdParty) } -func CheckAdvisoriesAccountData(t *testing.T, rhAccountID int, advisoryIDs []int64, systemsInstallable int) { - var advisoryAccountData []models.AdvisoryAccountData - err := DB.Where("rh_account_id = ? AND advisory_id IN (?)", rhAccountID, advisoryIDs). - Find(&advisoryAccountData).Error - assert.Nil(t, err) - - sum := 0 - for _, item := range advisoryAccountData { - sum += item.SystemsInstallable - } - // covers both cases, when we have advisory_account_data stored with 0 systems_installable, and when we delete it - assert.Equal(t, systemsInstallable*len(advisoryIDs), sum, "sum of systems_installable does not match") -} - func CreateStoredAdvisories(advisoryPatched []int64) map[string]models.SystemAdvisories { systemAdvisoriesMap := make(map[string]models.SystemAdvisories, len(advisoryPatched)) for _, advisoryID := range advisoryPatched { @@ -137,18 +123,6 @@ func CreateSystemAdvisories(t *testing.T, rhAccountID int, systemID int64, advis CheckSystemAdvisories(t, systemID, advisoryIDs) } -func CreateAdvisoryAccountData(t *testing.T, rhAccountID int, advisoryIDs []int64, - systemsInstallable int) { - for _, advisoryID := range advisoryIDs { - err := DB.Create(&models.AdvisoryAccountData{ - AdvisoryID: advisoryID, RhAccountID: rhAccountID, SystemsInstallable: systemsInstallable, - // create same number of applicable and installable systems because installable is subset of applicable - SystemsApplicable: systemsInstallable}).Error - assert.Nil(t, err) - } - CheckAdvisoriesAccountData(t, rhAccountID, advisoryIDs, systemsInstallable) -} - func CreateSystemRepos(t *testing.T, rhAccountID int, systemID int64, repoIDs []int64) { for _, repoID := range repoIDs { assert.Nil(t, DB.Create(&models.SystemRepo{RhAccountID: int64(rhAccountID), @@ -252,16 +226,6 @@ func DeleteAccountAdvisoryByAccount(t *testing.T, rhAccountID int) { Delete(&models.AccountAdvisory{}).Error) } -func DeleteAdvisoryAccountData(t *testing.T, rhAccountID int, advisoryIDs []int64) { - query := DB.Model(&models.AdvisoryAccountData{}).Where("rh_account_id = ? AND advisory_id IN (?)", - rhAccountID, advisoryIDs) - assert.Nil(t, query.Delete(&models.AdvisoryAccountData{}).Error) - - var cnt int64 - assert.Nil(t, query.Count(&cnt).Error) - assert.Equal(t, int64(0), cnt) -} - func DeleteSystemPackages(t *testing.T, accountID int, systemID int64, pkgIDs ...int64) { query := DB.Model(&models.SystemPackage{}).Where("rh_account_id = ? AND system_id = ?", accountID, systemID) if len(pkgIDs) > 0 { @@ -294,9 +258,7 @@ func DeleteNewlyAddedPackages(t *testing.T) { func DeleteNewlyAddedAdvisories(t *testing.T) { query := DB.Model(models.AdvisoryMetadata{}).Where("id >= 100") querySa := DB.Model(models.SystemAdvisories{}).Where("advisory_id >= 100") - queryAad := DB.Model(models.AdvisoryAccountData{}).Where("advisory_id >= 100") assert.Nil(t, querySa.Delete(models.SystemAdvisories{}).Error) - assert.Nil(t, queryAad.Delete(models.AdvisoryAccountData{}).Error) assert.Nil(t, query.Delete(models.AdvisoryMetadata{}).Error) var cnt int64 assert.Nil(t, query.Count(&cnt).Error) diff --git a/evaluator/advisory_update_test.go b/evaluator/advisory_update_test.go index 5083c84ed..9a09f22f5 100644 --- a/evaluator/advisory_update_test.go +++ b/evaluator/advisory_update_test.go @@ -151,12 +151,10 @@ func TestAdvisoryUpdateKafkaRoundTrip(t *testing.T) { // Remove stale rows from previous test runs database.DeleteSystemAdvisories(t, testDBID, []int64{1, 2}) - database.DeleteAdvisoryAccountData(t, rhAccountID, []int64{1, 2}) // Pair system with advisories before evaluation oldAdvisoryIDs := []int64{1, 3, 4} database.CreateSystemAdvisories(t, rhAccountID, testDBID, oldAdvisoryIDs) - database.CreateAdvisoryAccountData(t, rhAccountID, oldAdvisoryIDs, 1) // Run evaluation data, err := sonic.Marshal(mqueue.PlatformEvent{ @@ -182,6 +180,4 @@ func TestAdvisoryUpdateKafkaRoundTrip(t *testing.T) { evaluatedAdvisoryNames := []string{"RH-1", "RH-2", "RH-100"} evaluatedAdvisoryIDs := database.CheckAdvisoriesInDB(t, evaluatedAdvisoryNames) database.DeleteSystemAdvisories(t, testDBID, evaluatedAdvisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, evaluatedAdvisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, oldAdvisoryIDs) } diff --git a/evaluator/evaluate_test.go b/evaluator/evaluate_test.go index bbabba90d..6417d7c7e 100644 --- a/evaluator/evaluate_test.go +++ b/evaluator/evaluate_test.go @@ -52,12 +52,9 @@ func TestEvaluate(t *testing.T) { database.DeleteSystemAdvisories(t, testDBID, expectedAdvisoryIDs) database.DeleteSystemAdvisories(t, testDBID, patchingSystemAdvisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, expectedAdvisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, patchingSystemAdvisoryIDs) database.DeleteSystemPackages(t, rhAccountID, testDBID, expectedPackageIDs...) database.DeleteSystemRepos(t, rhAccountID, testDBID, systemRepoIDs) database.CreateSystemAdvisories(t, rhAccountID, testDBID, oldSystemAdvisoryIDs) - database.CreateAdvisoryAccountData(t, rhAccountID, oldSystemAdvisoryIDs, 1) database.CreateSystemRepos(t, rhAccountID, testDBID, systemRepoIDs) // do evaluate the system @@ -92,8 +89,6 @@ func TestEvaluate(t *testing.T) { 3, 1, 1, 0, 2, 2, 2, true) database.DeleteSystemAdvisories(t, testDBID, advisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, advisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, oldSystemAdvisoryIDs) database.DeleteSystemRepos(t, rhAccountID, testDBID, thirdPartySystemRepoIDs) assert.Equal(t, 2, len(mockWriter.Messages)) @@ -121,9 +116,7 @@ func TestEvaluateYum(t *testing.T) { } database.DeleteSystemAdvisories(t, testDBID, expectedAdvisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, expectedAdvisoryIDs) database.CreateSystemAdvisories(t, rhAccountID, testDBID, oldSystemAdvisoryIDs) - database.CreateAdvisoryAccountData(t, rhAccountID, oldSystemAdvisoryIDs, 1) data, err := sonic.Marshal(mqueue.PlatformEvent{ SystemIDs: []uuid.UUID{testInventoryID}, @@ -140,8 +133,6 @@ func TestEvaluateYum(t *testing.T) { database.DeleteSystemPackages(t, rhAccountID, testDBID, expectedPackageIDs...) database.DeleteSystemAdvisories(t, testDBID, advisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, advisoryIDs) - database.DeleteAdvisoryAccountData(t, rhAccountID, oldSystemAdvisoryIDs) assert.Equal(t, 1, len(mockWriter.Messages)) } diff --git a/evaluator/template_advisory_e2e_test.go b/evaluator/template_advisory_e2e_test.go index 6aaf9250e..7d97ead8c 100644 --- a/evaluator/template_advisory_e2e_test.go +++ b/evaluator/template_advisory_e2e_test.go @@ -66,7 +66,6 @@ func TestTemplateAdvisoryEvalE2E(t *testing.T) { defer database.DeleteTemplateAdvisories(t, template.ID, []int64{1, 2, 3}) database.DeleteSystemAdvisories(t, systemInv.ID, []int64{1, 2, 3, 100}) - database.DeleteAdvisoryAccountData(t, accountID, []int64{1, 2, 3, 100}) description := "e2e template" updateEvent := mqueue.TemplateEvent{ From 247ae178e34eb4cd9b7ce93201dacff18937a8eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Wed, 23 Sep 2026 17:53:30 +0200 Subject: [PATCH 10/16] RHINENG-26122: remove aad metrics --- ...hts-patchman-engine-general.configmap.yaml | 139 ------------------ manager/controllers/advisories.go | 3 - manager/middlewares/prometheus.go | 9 +- 3 files changed, 1 insertion(+), 150 deletions(-) diff --git a/dashboards/app-sre/grafana-dashboard-insights-patchman-engine-general.configmap.yaml b/dashboards/app-sre/grafana-dashboard-insights-patchman-engine-general.configmap.yaml index e45fddaee..d74663a54 100644 --- a/dashboards/app-sre/grafana-dashboard-insights-patchman-engine-general.configmap.yaml +++ b/dashboards/app-sre/grafana-dashboard-insights-patchman-engine-general.configmap.yaml @@ -2655,145 +2655,6 @@ data: } ] }, - "gridPos": { - "h": 6, - "w": 12, - "x": 12, - "y": 34 - }, - "id": 96, - "options": { - "legend": { - "calcs": [ - - ], - "displayMode": "list", - "placement": "bottom", - "showLegend": true - }, - "tooltip": { - "hideZeros": false, - "mode": "multi", - "sort": "desc" - } - }, - "pluginVersion": "11.6.3", - "targets": [ - { - "datasource": { - "type": "prometheus", - "uid": "$datasource" - }, - "editorMode": "code", - "expr": "sum(patchman_engine_manager_advisory_account_data_cache)", - "hide": false, - "legendFormat": "items", - "range": true, - "refId": "A" - }, - { - "datasource": { - "type": "prometheus", - "uid": "$datasource" - }, - "editorMode": "code", - "expr": "sum(patchman_engine_manager_advisory_account_data_cache{type=\"hit\"})/sum(patchman_engine_manager_advisory_account_data_cache)", - "hide": false, - "legendFormat": "hit ratio", - "range": true, - "refId": "B" - } - ], - "title": "Advisory account data usage", - "type": "timeseries" - }, - { - "datasource": { - "type": "prometheus", - "uid": "$datasource" - }, - "description": "", - "fieldConfig": { - "defaults": { - "color": { - "mode": "palette-classic" - }, - "custom": { - "axisBorderShow": false, - "axisCenteredZero": false, - "axisColorMode": "text", - "axisLabel": "Items", - "axisPlacement": "auto", - "barAlignment": 0, - "barWidthFactor": 0.6, - "drawStyle": "line", - "fillOpacity": 0, - "gradientMode": "none", - "hideFrom": { - "legend": false, - "tooltip": false, - "viz": false - }, - "insertNulls": false, - "lineInterpolation": "linear", - "lineWidth": 1, - "pointSize": 5, - "scaleDistribution": { - "type": "linear" - }, - "showPoints": "never", - "spanNulls": false, - "stacking": { - "group": "A", - "mode": "none" - }, - "thresholdsStyle": { - "mode": "off" - } - }, - "links": [ - - ], - "mappings": [ - - ], - "thresholds": { - "mode": "absolute", - "steps": [ - { - "color": "green" - }, - { - "color": "red", - "value": 80 - } - ] - }, - "unit": "none" - }, - "overrides": [ - { - "matcher": { - "id": "byName", - "options": "hit ratio" - }, - "properties": [ - { - "id": "custom.axisLabel", - "value": "Hit ratio" - }, - { - "id": "unit", - "value": "percentunit" - }, - { - "id": "custom.axisPlacement", - "value": "right" - } - ] - } - ] - }, "gridPos": { "h": 6, "w": 12, diff --git a/manager/controllers/advisories.go b/manager/controllers/advisories.go index ca8cee13e..74b57ffe4 100644 --- a/manager/controllers/advisories.go +++ b/manager/controllers/advisories.go @@ -233,7 +233,6 @@ func resolveAdvisoriesQuery(db *gorm.DB, account int, workspaceIDs []string, fil return nil, err } } - middlewares.AdvisoryAccountDataCnt.WithLabelValues("hit").Inc() if len(effectiveWorkspaceIDs) == 0 { query := buildQueryAdvisoriesFromAccountAdvisory(db, account, effectiveWorkspaceIDs) return query.Where("FALSE"), nil @@ -246,10 +245,8 @@ func resolveAdvisoriesQuery(db *gorm.DB, account int, workspaceIDs []string, fil // leaving until RBAC cannot be re-enabled if !config.EnableAccountAdvisoryReadPath && !config.DisableCachedCounts && !HasInventoryFilter(filters) && len(workspaceIDs) == 0 { - middlewares.AdvisoryAccountDataCnt.WithLabelValues("hit").Inc() return buildQueryAdvisories(db, account), nil } - middlewares.AdvisoryAccountDataCnt.WithLabelValues("miss").Inc() return buildQueryAdvisoriesTagged(db, filters, account, workspaceIDs), nil } diff --git a/manager/middlewares/prometheus.go b/manager/middlewares/prometheus.go index f7cca17e0..6c5774d2d 100644 --- a/manager/middlewares/prometheus.go +++ b/manager/middlewares/prometheus.go @@ -43,13 +43,6 @@ var AdvisoryDetailGauge = prometheus.NewGauge(prometheus.GaugeOpts{ Name: "advisory_detail_cache_size", }) -var AdvisoryAccountDataCnt = prometheus.NewCounterVec(prometheus.CounterOpts{ - Help: "How many requests hit/miss advisory_account_data", - Namespace: "patchman_engine", - Subsystem: "manager", - Name: "advisory_account_data_cache", -}, []string{"type"}) - var PackageAccountDataCnt = prometheus.NewCounterVec(prometheus.CounterOpts{ Help: "How many requests hit/miss package_account_data", Namespace: "patchman_engine", @@ -60,7 +53,7 @@ var PackageAccountDataCnt = prometheus.NewCounterVec(prometheus.CounterOpts{ // Create and configure Prometheus middleware to expose metrics func Prometheus() *ginprometheus.Prometheus { prometheus.MustRegister(serviceErrorCnt, requestDurations, callerSourceCnt, - AdvisoryDetailCnt, AdvisoryDetailGauge, AdvisoryAccountDataCnt, PackageAccountDataCnt) + AdvisoryDetailCnt, AdvisoryDetailGauge, PackageAccountDataCnt) p := ginprometheus.NewPrometheus("patchman_engine") p.MetricsPath = utils.CoreCfg.MetricsPath From e6bc0ef9787e0a04a56601139982663020c98e5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Wed, 23 Sep 2026 18:09:15 +0200 Subject: [PATCH 11/16] RHINENG-26122: remove unused aad table read --- manager/controllers/advisories.go | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/manager/controllers/advisories.go b/manager/controllers/advisories.go index 74b57ffe4..13516f77b 100644 --- a/manager/controllers/advisories.go +++ b/manager/controllers/advisories.go @@ -213,16 +213,6 @@ func AdvisoriesListIDsHandler(c *gin.Context) { c.JSON(http.StatusOK, &resp) } -func buildQueryAdvisories(db *gorm.DB, account int) *gorm.DB { - query := database.AdvisoryMetadata(db). - Select(AdvisoriesSelect). - Joins("JOIN advisory_account_data aad ON am.id = aad.advisory_id"). - Joins("LEFT JOIN advisory_severity sev ON am.severity_id = sev.id"). - Where("aad.rh_account_id = ?", account). - Where("aad.systems_applicable > 0") - return query -} - func resolveAdvisoriesQuery(db *gorm.DB, account int, workspaceIDs []string, filters Filters) (*gorm.DB, error) { if config.EnableAccountAdvisoryReadPath && !hasNonGroupInventoryFilter(filters) { effectiveWorkspaceIDs := workspaceIDs @@ -239,14 +229,6 @@ func resolveAdvisoriesQuery(db *gorm.DB, account int, workspaceIDs []string, fil } return buildQueryAdvisoriesFromAccountAdvisory(db, account, effectiveWorkspaceIDs), nil } - // TODO: fix below; - // the condition is always true since moving from groups to workspaces, - // there will always be at least root workspace - // leaving until RBAC cannot be re-enabled - if !config.EnableAccountAdvisoryReadPath && !config.DisableCachedCounts && - !HasInventoryFilter(filters) && len(workspaceIDs) == 0 { - return buildQueryAdvisories(db, account), nil - } return buildQueryAdvisoriesTagged(db, filters, account, workspaceIDs), nil } From 7f5ba152a35aac943f094d72f7ee4fd188d4c726 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Thu, 17 Sep 2026 17:41:11 +0200 Subject: [PATCH 12/16] RHINENG-26122: remove AdvisoryAccountData model --- base/models/models.go | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/base/models/models.go b/base/models/models.go index 273f14274..8e616284f 100644 --- a/base/models/models.go +++ b/base/models/models.go @@ -287,20 +287,6 @@ func (SystemAdvisories) TableName() string { type SystemAdvisoriesSlice []SystemAdvisories -type AdvisoryAccountData struct { - AdvisoryID int64 `gorm:"primaryKey"` - RhAccountID int `gorm:"primaryKey"` - SystemsApplicable int - SystemsInstallable int - Notified *time.Time -} - -func (AdvisoryAccountData) TableName() string { - return "advisory_account_data" -} - -type AdvisoryAccountDataSlice []AdvisoryAccountData - type AccountAdvisory struct { AdvisoryID int64 `gorm:"primaryKey"` RhAccountID int `gorm:"primaryKey"` From c7062064dce93cdac47d79b7dac7e1f63214ab8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Wed, 23 Sep 2026 18:36:56 +0200 Subject: [PATCH 13/16] RHINENG-26122: resolve remaining aad uses in go --- listener/common_test.go | 4 ++-- tasks/cleaning/clean_unused_data.go | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/listener/common_test.go b/listener/common_test.go index d84773850..2d19c5bbe 100644 --- a/listener/common_test.go +++ b/listener/common_test.go @@ -32,8 +32,8 @@ func TestInit(_ *testing.T) { func deleteData(t *testing.T) { // Delete test data from previous run - assert.Nil(t, database.DB.Unscoped().Exec("DELETE FROM advisory_account_data aad "+ - "USING rh_account ra WHERE ra.id = aad.rh_account_id AND ra.name = ?", testOrgID).Error) + assert.Nil(t, database.DB.Unscoped().Exec("DELETE FROM account_advisory aa "+ + "USING rh_account ra WHERE ra.id = aa.rh_account_id AND ra.name = ?", testOrgID).Error) assert.Nil(t, database.DB.Unscoped().Where("first_reported > timestamp '2020-01-01'"). Delete(&models.SystemAdvisories{}).Error) assert.Nil(t, database.DB.Unscoped().Where("repo_id NOT IN (1, 2) OR system_id NOT IN (2, 3, 17)"). diff --git a/tasks/cleaning/clean_unused_data.go b/tasks/cleaning/clean_unused_data.go index 5161c48de..49aa96542 100644 --- a/tasks/cleaning/clean_unused_data.go +++ b/tasks/cleaning/clean_unused_data.go @@ -45,13 +45,11 @@ func deleteUnusedAdvisories() { // remove unused advisories not synced from vmaas // before changing the query below test its performance on big data otherwise it can lock database // Time: 18988.223 ms (00:18.988) for 50k advisories, 75M system_advisories, 1.6M package and 50k rh_account - // both advisory_account_data and account_advisory are checked temporarily until the legacy table is dropped subq := tx.Select("id").Table("advisory_metadata am"). Where("am.synced = ?", false). Where("NOT EXISTS (SELECT 1 FROM system_advisories sa WHERE am.id = sa.advisory_id)"). Where("NOT EXISTS (SELECT 1 FROM template_advisory ta WHERE am.id = ta.advisory_id)"). Where("NOT EXISTS (SELECT 1 FROM package p WHERE am.id = p.advisory_id)"). - Where("NOT EXISTS (SELECT 1 FROM advisory_account_data aad WHERE am.id = aad.advisory_id)"). Where("NOT EXISTS (SELECT 1 FROM account_advisory aa WHERE am.id = aa.advisory_id)"). Limit(tasks.DeleteUnusedDataLimit) From 68569e83552b3df0991e699ff5acd26fb98796d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Wed, 23 Sep 2026 18:48:46 +0200 Subject: [PATCH 14/16] RHINENG-26122: drop advisory_account_data table --- .../169_drop_advisory_account_data.down.sql | 88 +++++++++++++++++++ .../169_drop_advisory_account_data.up.sql | 9 ++ database_admin/schema/create_schema.sql | 88 ------------------- .../schema/repair_system_advisories_0.sql | 4 - dev/test_data.sql | 1 - tasks/vmaas_sync/metrics_db_test.go | 4 +- 6 files changed, 99 insertions(+), 95 deletions(-) diff --git a/database_admin/migrations/169_drop_advisory_account_data.down.sql b/database_admin/migrations/169_drop_advisory_account_data.down.sql index 34f5fb147..fcb680722 100644 --- a/database_admin/migrations/169_drop_advisory_account_data.down.sql +++ b/database_admin/migrations/169_drop_advisory_account_data.down.sql @@ -1,3 +1,36 @@ +-- advisory_account_data +CREATE TABLE IF NOT EXISTS advisory_account_data +( + advisory_id BIGINT NOT NULL, + rh_account_id INT NOT NULL, + systems_applicable INT NOT NULL DEFAULT 0, + systems_installable INT NOT NULL DEFAULT 0, + notified TIMESTAMP WITH TIME ZONE NULL, + CONSTRAINT advisory_metadata_id + FOREIGN KEY (advisory_id) + REFERENCES advisory_metadata (id), + CONSTRAINT rh_account_id + FOREIGN KEY (rh_account_id) + REFERENCES rh_account (id), + UNIQUE (advisory_id, rh_account_id), + PRIMARY KEY (rh_account_id, advisory_id) +) WITH (fillfactor = '70', autovacuum_vacuum_scale_factor = '0.05') + TABLESPACE pg_default; + +-- manager user needs to change this table for opt-out functionality +GRANT SELECT, INSERT, UPDATE, DELETE ON advisory_account_data TO manager; +-- evaluator user needs to change this table +GRANT SELECT, INSERT, UPDATE, DELETE ON advisory_account_data TO evaluator; +-- listner user needs to change this table when deleting system +GRANT SELECT, INSERT, UPDATE, DELETE ON advisory_account_data TO listener; +-- vmaas_sync needs to update stale mark, which creates and deletes advisory_account_data +GRANT SELECT, INSERT, UPDATE, DELETE ON advisory_account_data TO vmaas_sync; + +-- indexes for filtering systems_applicable, systems_installable +CREATE INDEX ON advisory_account_data (systems_applicable); +CREATE INDEX ON advisory_account_data (systems_installable); +GRANT DELETE ON advisory_account_data TO vmaas_sync; + CREATE OR REPLACE FUNCTION backfill_account_advisory(rh_account_id_in INTEGER) RETURNS VOID AS $backfill$ @@ -86,3 +119,58 @@ BEGIN END; $refresh_all_cached_counts$ LANGUAGE 'plpgsql'; + +CREATE OR REPLACE FUNCTION on_system_update() +-- this trigger updates advisory_account_data when server changes its stale flag + RETURNS TRIGGER +AS +$system_update$ +DECLARE + was_counted BOOLEAN; + should_count BOOLEAN; + change INT; +BEGIN + -- Ignore not yet evaluated systems + IF TG_OP != 'UPDATE' OR NOT EXISTS ( + SELECT 1 + FROM system_patch + WHERE system_id = NEW.id + AND rh_account_id = NEW.rh_account_id + AND last_evaluation IS NOT NULL + ) THEN + RETURN NEW; + END IF; + + was_counted := OLD.stale = FALSE; + should_count := NEW.stale = FALSE; + + -- Determine what change we are performing + IF was_counted and NOT should_count THEN + change := -1; + ELSIF NOT was_counted AND should_count THEN + change := 1; + ELSE + -- No change + RETURN NEW; + END IF; + + -- insert/update advisories linked to the server + INSERT + INTO advisory_account_data (advisory_id, rh_account_id, systems_installable, systems_applicable) + SELECT sa.advisory_id, NEW.rh_account_id, + case when sa.status_id = 0 then change else 0 end as systems_installable, + change as systems_applicable + FROM system_advisories sa + WHERE sa.system_id = NEW.id AND sa.rh_account_id = NEW.rh_account_id + ORDER BY sa.advisory_id + ON CONFLICT (advisory_id, rh_account_id) DO UPDATE + SET systems_installable = advisory_account_data.systems_installable + EXCLUDED.systems_installable, + systems_applicable = advisory_account_data.systems_applicable + EXCLUDED.systems_applicable; + RETURN NEW; +END; +$system_update$ LANGUAGE plpgsql; + +SELECT create_table_partition_triggers('system_inventory_on_update', + $$AFTER UPDATE$$, + 'system_inventory', + $$FOR EACH ROW EXECUTE PROCEDURE on_system_update()$$); diff --git a/database_admin/migrations/169_drop_advisory_account_data.up.sql b/database_admin/migrations/169_drop_advisory_account_data.up.sql index 2459021e4..7ff449418 100644 --- a/database_admin/migrations/169_drop_advisory_account_data.up.sql +++ b/database_admin/migrations/169_drop_advisory_account_data.up.sql @@ -13,3 +13,12 @@ DROP FUNCTION IF EXISTS refresh_account_cached_counts(rh_account_in varchar); DROP FUNCTION IF EXISTS refresh_all_cached_counts(); DROP FUNCTION IF EXISTS refresh_advisory_caches_multi(advisory_ids_in INTEGER[], rh_account_id_in INTEGER); + +SELECT drop_table_partition_triggers('system_inventory_on_update', + $$AFTER UPDATE$$, + 'system_inventory', + $$FOR EACH ROW EXECUTE PROCEDURE on_system_update()$$); + +DROP FUNCTION IF EXISTS on_system_update(); + +DROP TABLE IF EXISTS advisory_account_data; diff --git a/database_admin/schema/create_schema.sql b/database_admin/schema/create_schema.sql index 5c5f8c0ff..3db46c7cc 100644 --- a/database_admin/schema/create_schema.sql +++ b/database_admin/schema/create_schema.sql @@ -65,56 +65,6 @@ END; $check_unchanged$ LANGUAGE 'plpgsql'; -CREATE OR REPLACE FUNCTION on_system_update() --- this trigger updates advisory_account_data when server changes its stale flag - RETURNS TRIGGER -AS -$system_update$ -DECLARE - was_counted BOOLEAN; - should_count BOOLEAN; - change INT; -BEGIN - -- Ignore not yet evaluated systems - IF TG_OP != 'UPDATE' OR NOT EXISTS ( - SELECT 1 - FROM system_patch - WHERE system_id = NEW.id - AND rh_account_id = NEW.rh_account_id - AND last_evaluation IS NOT NULL - ) THEN - RETURN NEW; - END IF; - - was_counted := OLD.stale = FALSE; - should_count := NEW.stale = FALSE; - - -- Determine what change we are performing - IF was_counted and NOT should_count THEN - change := -1; - ELSIF NOT was_counted AND should_count THEN - change := 1; - ELSE - -- No change - RETURN NEW; - END IF; - - -- insert/update advisories linked to the server - INSERT - INTO advisory_account_data (advisory_id, rh_account_id, systems_installable, systems_applicable) - SELECT sa.advisory_id, NEW.rh_account_id, - case when sa.status_id = 0 then change else 0 end as systems_installable, - change as systems_applicable - FROM system_advisories sa - WHERE sa.system_id = NEW.id AND sa.rh_account_id = NEW.rh_account_id - ORDER BY sa.advisory_id - ON CONFLICT (advisory_id, rh_account_id) DO UPDATE - SET systems_installable = advisory_account_data.systems_installable + EXCLUDED.systems_installable, - systems_applicable = advisory_account_data.systems_applicable + EXCLUDED.systems_applicable; - RETURN NEW; -END; -$system_update$ LANGUAGE plpgsql; - CREATE OR REPLACE FUNCTION refresh_account_advisory_caches_multi(advisory_ids_in INTEGER[] DEFAULT NULL, rh_account_id_in INTEGER DEFAULT NULL) RETURNS VOID AS @@ -579,11 +529,6 @@ SELECT create_table_partition_triggers('system_inventory_check_unchanged', 'system_inventory', $$FOR EACH ROW EXECUTE PROCEDURE check_unchanged()$$); -SELECT create_table_partition_triggers('system_inventory_on_update', - $$AFTER UPDATE$$, - 'system_inventory', - $$FOR EACH ROW EXECUTE PROCEDURE on_system_update()$$); - CREATE INDEX IF NOT EXISTS system_inventory_inventory_id_idx ON system_inventory (inventory_id); CREATE INDEX IF NOT EXISTS system_inventory_tags_index ON system_inventory USING GIN (tags JSONB_PATH_OPS); CREATE INDEX IF NOT EXISTS system_inventory_stale_timestamp_index ON system_inventory (stale_timestamp); @@ -737,38 +682,6 @@ GRANT SELECT, INSERT, UPDATE, DELETE ON system_advisories TO listener; -- vmaas_sync needs to delete culled systems, which cascades to system_advisories GRANT SELECT, DELETE ON system_advisories TO vmaas_sync; --- advisory_account_data -CREATE TABLE IF NOT EXISTS advisory_account_data -( - advisory_id BIGINT NOT NULL, - rh_account_id INT NOT NULL, - systems_applicable INT NOT NULL DEFAULT 0, - systems_installable INT NOT NULL DEFAULT 0, - notified TIMESTAMP WITH TIME ZONE NULL, - CONSTRAINT advisory_metadata_id - FOREIGN KEY (advisory_id) - REFERENCES advisory_metadata (id), - CONSTRAINT rh_account_id - FOREIGN KEY (rh_account_id) - REFERENCES rh_account (id), - UNIQUE (advisory_id, rh_account_id), - PRIMARY KEY (rh_account_id, advisory_id) -) WITH (fillfactor = '70', autovacuum_vacuum_scale_factor = '0.05') - TABLESPACE pg_default; - --- manager user needs to change this table for opt-out functionality -GRANT SELECT, INSERT, UPDATE, DELETE ON advisory_account_data TO manager; --- evaluator user needs to change this table -GRANT SELECT, INSERT, UPDATE, DELETE ON advisory_account_data TO evaluator; --- listner user needs to change this table when deleting system -GRANT SELECT, INSERT, UPDATE, DELETE ON advisory_account_data TO listener; --- vmaas_sync needs to update stale mark, which creates and deletes advisory_account_data -GRANT SELECT, INSERT, UPDATE, DELETE ON advisory_account_data TO vmaas_sync; - --- indexes for filtering systems_applicable, systems_installable -CREATE INDEX ON advisory_account_data (systems_applicable); -CREATE INDEX ON advisory_account_data (systems_installable); - -- account_advisory CREATE TABLE IF NOT EXISTS account_advisory ( @@ -952,7 +865,6 @@ GRANT SELECT, INSERT, UPDATE, DELETE ON timestamp_kv TO vmaas_sync; -- vmaas_sync needs to delete from this tables to sync CVEs correctly GRANT DELETE ON system_advisories TO vmaas_sync; -GRANT DELETE ON advisory_account_data TO vmaas_sync; -- system_patch CREATE TABLE IF NOT EXISTS system_patch diff --git a/database_admin/schema/repair_system_advisories_0.sql b/database_admin/schema/repair_system_advisories_0.sql index 4e7e7693d..907080915 100644 --- a/database_admin/schema/repair_system_advisories_0.sql +++ b/database_admin/schema/repair_system_advisories_0.sql @@ -9,10 +9,6 @@ BEGIN; TRUNCATE TABLE system_advisories_0; -- Clear denormalized counts for accounts that hash into remainder 0 (do not read old _0). -DELETE FROM advisory_account_data aad - WHERE satisfies_hash_partition( - 'system_advisories'::regclass, 32, 0, aad.rh_account_id); - DELETE FROM account_advisory aa WHERE satisfies_hash_partition( 'system_advisories'::regclass, 32, 0, aa.rh_account_id); diff --git a/dev/test_data.sql b/dev/test_data.sql index 7aa71e238..4c40f5d2c 100644 --- a/dev/test_data.sql +++ b/dev/test_data.sql @@ -6,7 +6,6 @@ DELETE FROM system_inventory; DELETE FROM deleted_system; DELETE FROM repo; DELETE FROM timestamp_kv; -DELETE FROM advisory_account_data; DELETE FROM account_advisory; DELETE FROM package_account_data; DELETE FROM package; diff --git a/tasks/vmaas_sync/metrics_db_test.go b/tasks/vmaas_sync/metrics_db_test.go index b0af71e10..bcc2077c7 100644 --- a/tasks/vmaas_sync/metrics_db_test.go +++ b/tasks/vmaas_sync/metrics_db_test.go @@ -17,8 +17,8 @@ func TestTableSizes(t *testing.T) { for _, item := range tableSizes { uniqueTables[item.Key] = true } - assert.Equal(t, 280, len(tableSizes)) - assert.Equal(t, 280, len(uniqueTables)) + assert.Equal(t, 279, len(tableSizes)) + assert.Equal(t, 279, len(uniqueTables)) assert.True(t, uniqueTables["public.system_inventory"]) // check whether table names were loaded assert.True(t, uniqueTables["public.system_patch"]) // check whether table names were loaded assert.True(t, uniqueTables["public.package"]) From 218ad0c212553dfbe92c0241caef832e2a5200cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Wed, 23 Sep 2026 18:56:39 +0200 Subject: [PATCH 15/16] RHINENG-26122: update docs references to aad --- AGENTS.md | 3 +-- docs/md/architecture.md | 9 +++++---- docs/md/database.md | 3 +-- docs/md/major-migration-runbook.md | 2 +- 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index cb5e3959a..6ba02e4a4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,14 +57,13 @@ Listener Component Evaluator-Upload Component ↓ (calls VMaaS /updates) ↓ (updates system_advisories) - ↓ (updates advisory_account_data — legacy table, to be removed) ↓ [platform.remediation-updates.patch] (optional) [platform.inventory.host-apps] (optional) [patchman.advisory.update] Kafka Topic (changed advisory IDs) ↓ Aggregator Component - ↓ (recounts from system_advisories, writes aggregates to account_advisory — new workspace-scoped table) + ↓ (recounts from system_advisories, writes aggregates to account_advisory — a workspace-scoped table) ↓ [platform.notifications.ingress] (optional) ``` diff --git a/docs/md/architecture.md b/docs/md/architecture.md index 6ed5af4b1..308e6dcd7 100644 --- a/docs/md/architecture.md +++ b/docs/md/architecture.md @@ -27,7 +27,9 @@ See [component environment variables](../../conf/listener.env) - **evaluator-upload** - connects to the Kafka service (`patchman.evaluator.upload` topic) and listens for evaluation requests from the `listener` component. For each received Kafka message it evaluates system with ID contained in the -message. It loads each system by joining **`system_inventory`** and **`system_patch`**. As an evaluation result it updates **`system_advisories`** (referencing **`system_inventory.id`**), **`system_patch`** (evaluation caches, `last_evaluation`, and related fields), and **`advisory_account_data`**. Evaluation is scaled on two levels, firstly with multiple replicas (more pods) and secondary +message. It loads each system by joining **`system_inventory`** and **`system_patch`**. As an evaluation result it updates +**`system_advisories`** (referencing **`system_inventory.id`**), and **`system_patch`** (evaluation caches, `last_evaluation`, +and related fields). Evaluation is scaled on two levels, firstly with multiple replicas (more pods) and secondary with multiple goroutines within single pod (set by `CONSUMER_COUNT` environment variable). See [component environment variables](../../conf/evaluator_upload.env) @@ -46,9 +48,8 @@ See [component environment variables](../../conf/evaluator_user_evaluation.env) - **aggregator** - maintains per-account, per-workspace advisory counts. When the evaluator processes a system upload or recalculation and updates **`system_advisories`**, it publishes an `AdvisoryUpdateEvent` to the `patchman.advisory.update` Kafka topic listing which advisory IDs changed for a given account. The aggregator consumes these events and recounts -how many systems have each advisory applicable or installable, writing the results to **`account_advisory`**. This is the -workspace-aware replacement for **`advisory_account_data`** (previously maintained by the evaluator). Incoming events are -batched before processing. When `enable_notifications` is set in `POD_CONFIG`, the aggregator also publishes +how many systems have each advisory applicable or installable, writing the results to **`account_advisory`**. Incoming events +are batched before processing. When `enable_notifications` is set in `POD_CONFIG`, the aggregator also publishes new installable advisories to `platform.notifications.ingress` and marks them as notified in **`account_advisory`**. See [component environment variables](../../conf/aggregator.env) diff --git a/docs/md/database.md b/docs/md/database.md index d34a2f6d7..6896bc2ff 100644 --- a/docs/md/database.md +++ b/docs/md/database.md @@ -6,8 +6,7 @@ Main database tables description: - **system_patch** — Partitioned evaluation output for each system, keyed by `rh_account_id` and `system_id` where `system_id` equals **system_inventory.id** on the same account. Holds advisory and package count caches, `last_evaluation`, `third_party`, `template_id`, and related aggregates. Rows are created or updated by the **listener** together with **system_inventory**; the **evaluator** persists evaluation results here (not into a single legacy table). - **advisory_metadata** - stores info about advisories (`description`, `summary`, `solution` etc.). It's synced and stored on trigger by `vmaas_sync` component. It allows to display detail information about the advisory. - **system_advisories** - stores info about advisories evaluated for particular systems (system - advisory M-N mapping table). `system_id` references **system_inventory.id**. Contains info when system advisory was firstly reported and patched (if so). Records are created and updated by `evaluator` component. It allows to display list of advisories related to a system. -- **advisory_account_data** - stores info about all advisories detected within at least one system that belongs to a given account. So it provides overall statistics about system advisories displayed by the application. -- **account_advisory** - workspace-scoped version of `advisory_account_data`. Stores per-advisory aggregate counts (`systems_applicable`, `systems_installable`) and notification state for each workspace within an account. Keyed by `(rh_account_id, workspace_id, advisory_id)`, partitioned by `rh_account_id` (32 partitions). +- **account_advisory** - stores workspace-scoped info about all advisories detected within at least one system that belongs to a given account. So it provides overall statistics about system advisories displayed by the application. Stores per-advisory aggregate counts (`systems_applicable`, `systems_installable`) and notification state for each workspace within an account. Keyed by `(rh_account_id, workspace_id, advisory_id)`, partitioned by `rh_account_id` (32 partitions). - **package_name** - names of the packages installed on systems - **package** - list of all packages versions, precisely all EVRAs (epoch-version-release-arch) - **system_package2** - list of packages installed on a system diff --git a/docs/md/major-migration-runbook.md b/docs/md/major-migration-runbook.md index 130f329bb..29f48e348 100644 --- a/docs/md/major-migration-runbook.md +++ b/docs/md/major-migration-runbook.md @@ -153,7 +153,7 @@ Config keys are defined in `database_admin/config.go`. ClowdApp comments in `dep |---|---| | **Config key** | `repair_system_advisories_0` (boolean, default `false`) | | **Where** | `DATABASE_ADMIN_CONFIG` on the **db-migration Job** only | -| **Effect** | After migrate CONTINUE/MIGRATE, runs `prepareForMigration`, then `database_admin/schema/repair_system_advisories_0.sql`: `TRUNCATE system_advisories_0`, clear bucket-0 `advisory_account_data` and `account_advisory`. **Destructive** for hash remainder 0. | +| **Effect** | After migrate CONTINUE/MIGRATE, runs `prepareForMigration`, then `database_admin/schema/repair_system_advisories_0.sql`: `TRUNCATE system_advisories_0`, clear bucket-0 `account_advisory`. **Destructive** for hash remainder 0. | **Enable when:** one-off recovery from corrupt/unreadable `system_advisories_0`. Combine with `terminate_db_sessions=true` if truncate is blocked by app sessions. From c3a20085845974d2c0899180dbcec9f654af32ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Thu, 24 Sep 2026 12:32:19 +0200 Subject: [PATCH 16/16] RHINENG-26122: create aggregator DB role Co-authored-by: Claude --- conf/aggregator_common.env | 4 +-- conf/database_admin.env | 1 + .../170_add_aggregator_role.down.sql | 11 ++++++ .../migrations/170_add_aggregator_role.up.sql | 31 ++++++++++++++++ database_admin/schema/create_schema.sql | 36 ++++++++++++++++--- database_admin/schema/create_users.sql | 2 +- database_admin/update.go | 3 +- deploy/clowdapp.yaml | 6 ++-- 8 files changed, 83 insertions(+), 11 deletions(-) create mode 100644 database_admin/migrations/170_add_aggregator_role.down.sql create mode 100644 database_admin/migrations/170_add_aggregator_role.up.sql diff --git a/conf/aggregator_common.env b/conf/aggregator_common.env index d7dd977ec..715d7389f 100644 --- a/conf/aggregator_common.env +++ b/conf/aggregator_common.env @@ -1,2 +1,2 @@ -DB_USER=evaluator -DB_PASSWD=evaluator +DB_USER=aggregator +DB_PASSWD=aggregator diff --git a/conf/database_admin.env b/conf/database_admin.env index da31cec1c..c01515709 100644 --- a/conf/database_admin.env +++ b/conf/database_admin.env @@ -5,6 +5,7 @@ MANAGER_PASSWORD=manager LISTENER_PASSWORD=listener VMAAS_SYNC_PASSWORD=vmaas_sync EVALUATOR_PASSWORD=evaluator +AGGREGATOR_PASSWORD=aggregator # Optionally set schema_migration=XXX and/or reset_schema POD_CONFIG=update_users;update_db_config;wait_for_db=empty diff --git a/database_admin/migrations/170_add_aggregator_role.down.sql b/database_admin/migrations/170_add_aggregator_role.down.sql new file mode 100644 index 000000000..e708e3e12 --- /dev/null +++ b/database_admin/migrations/170_add_aggregator_role.down.sql @@ -0,0 +1,11 @@ +SELECT revoke_table_partitions('SELECT, INSERT, UPDATE, DELETE', 'account_advisory', 'aggregator'); +REVOKE EXECUTE ON FUNCTION refresh_account_advisory_caches_multi(INTEGER[], INTEGER) FROM aggregator; +REVOKE EXECUTE ON FUNCTION refresh_account_advisory_caches(INTEGER, INTEGER) FROM aggregator; +REVOKE SELECT ON ALL TABLES IN SCHEMA public FROM aggregator; + +SELECT grant_table_partitions('SELECT, INSERT, UPDATE, DELETE', 'account_advisory', 'manager'); +SELECT grant_table_partitions('SELECT, INSERT, UPDATE, DELETE', 'account_advisory', 'evaluator'); +SELECT grant_table_partitions('SELECT, INSERT, UPDATE, DELETE', 'account_advisory', 'listener'); +SELECT grant_table_partitions('SELECT, INSERT, UPDATE, DELETE', 'account_advisory', 'vmaas_sync'); + +DROP FUNCTION IF EXISTS revoke_table_partitions(text, regclass, text); diff --git a/database_admin/migrations/170_add_aggregator_role.up.sql b/database_admin/migrations/170_add_aggregator_role.up.sql new file mode 100644 index 000000000..29306eec1 --- /dev/null +++ b/database_admin/migrations/170_add_aggregator_role.up.sql @@ -0,0 +1,31 @@ +CREATE OR REPLACE FUNCTION revoke_table_partitions(perms text, tbl regclass, grantie text) + RETURNS VOID AS +$$ +DECLARE + r record; +BEGIN + FOR r IN SELECT child.relname + FROM pg_inherits + JOIN pg_class parent + ON pg_inherits.inhparent = parent.oid + JOIN pg_class child + ON pg_inherits.inhrelid = child.oid + WHERE parent.relname = text(tbl) + LOOP + EXECUTE 'REVOKE ' || perms || ' ON TABLE ' || r.relname || ' FROM ' || grantie; + END LOOP; + EXECUTE 'REVOKE ' || perms || ' ON TABLE ' || text(tbl) || ' FROM ' || grantie; +END; +$$ LANGUAGE plpgsql; + +SELECT revoke_table_partitions('INSERT, UPDATE, DELETE', 'account_advisory', 'manager'); +SELECT revoke_table_partitions('INSERT, UPDATE, DELETE', 'account_advisory', 'evaluator'); +SELECT revoke_table_partitions('INSERT, UPDATE, DELETE', 'account_advisory', 'listener'); +SELECT revoke_table_partitions('INSERT, UPDATE', 'account_advisory', 'vmaas_sync'); + +SELECT grant_table_partitions('SELECT, INSERT, UPDATE, DELETE', 'account_advisory', 'aggregator'); + +GRANT SELECT ON ALL TABLES IN SCHEMA public TO aggregator; + +GRANT EXECUTE ON FUNCTION refresh_account_advisory_caches_multi(INTEGER[], INTEGER) TO aggregator; +GRANT EXECUTE ON FUNCTION refresh_account_advisory_caches(INTEGER, INTEGER) TO aggregator; diff --git a/database_admin/schema/create_schema.sql b/database_admin/schema/create_schema.sql index 3db46c7cc..7f63554f2 100644 --- a/database_admin/schema/create_schema.sql +++ b/database_admin/schema/create_schema.sql @@ -7,7 +7,7 @@ CREATE TABLE IF NOT EXISTS schema_migrations INSERT INTO schema_migrations -VALUES (169, false); +VALUES (170, false); -- --------------------------------------------------------------------------- -- Functions @@ -398,6 +398,26 @@ BEGIN END; $$ LANGUAGE plpgsql; +CREATE OR REPLACE FUNCTION revoke_table_partitions(perms text, tbl regclass, grantie text) + RETURNS VOID AS +$$ +DECLARE + r record; +BEGIN + FOR r IN SELECT child.relname + FROM pg_inherits + JOIN pg_class parent + ON pg_inherits.inhparent = parent.oid + JOIN pg_class child + ON pg_inherits.inhrelid = child.oid + WHERE parent.relname = text(tbl) + LOOP + EXECUTE 'REVOKE ' || perms || ' ON TABLE ' || r.relname || ' FROM ' || grantie; + END LOOP; + EXECUTE 'REVOKE ' || perms || ' ON TABLE ' || text(tbl) || ' FROM ' || grantie; +END; +$$ LANGUAGE plpgsql; + -- --------------------------------------------------------------------------- -- Tables @@ -704,10 +724,11 @@ SELECT create_table_partitions('account_advisory', 32, $$WITH (fillfactor = '70', autovacuum_vacuum_scale_factor = '0.05') TABLESPACE pg_default$$); -SELECT grant_table_partitions('SELECT, INSERT, UPDATE, DELETE', 'account_advisory', 'manager'); -SELECT grant_table_partitions('SELECT, INSERT, UPDATE, DELETE', 'account_advisory', 'evaluator'); -SELECT grant_table_partitions('SELECT, INSERT, UPDATE, DELETE', 'account_advisory', 'listener'); -SELECT grant_table_partitions('SELECT, INSERT, UPDATE, DELETE', 'account_advisory', 'vmaas_sync'); +SELECT grant_table_partitions('SELECT', 'account_advisory', 'manager'); +SELECT grant_table_partitions('SELECT', 'account_advisory', 'evaluator'); +SELECT grant_table_partitions('SELECT', 'account_advisory', 'listener'); +SELECT grant_table_partitions('SELECT, DELETE', 'account_advisory', 'vmaas_sync'); +SELECT grant_table_partitions('SELECT, INSERT, UPDATE, DELETE', 'account_advisory', 'aggregator'); SELECT create_table_partition_triggers('account_advisory_sync_notified_insert', $$BEFORE INSERT$$, @@ -934,3 +955,8 @@ BEGIN END IF; END $$; + +-- user for aggregator component +GRANT SELECT ON ALL TABLES IN SCHEMA public TO aggregator; +GRANT EXECUTE ON FUNCTION refresh_account_advisory_caches_multi(INTEGER[], INTEGER) TO aggregator; +GRANT EXECUTE ON FUNCTION refresh_account_advisory_caches(INTEGER, INTEGER) TO aggregator; diff --git a/database_admin/schema/create_users.sql b/database_admin/schema/create_users.sql index 40903f50d..e477159b4 100644 --- a/database_admin/schema/create_users.sql +++ b/database_admin/schema/create_users.sql @@ -5,7 +5,7 @@ $$ BEGIN FOR usr IN SELECT name - FROM (VALUES ('evaluator'), ('listener'), ('manager'), ('vmaas_sync')) users (name) + FROM (VALUES ('evaluator'), ('listener'), ('manager'), ('vmaas_sync'), ('aggregator')) users (name) WHERE name NOT IN (SELECT rolname FROM pg_catalog.pg_roles) LOOP execute 'CREATE USER ' || usr || ';'; diff --git a/database_admin/update.go b/database_admin/update.go index dbcccc030..b7d080624 100644 --- a/database_admin/update.go +++ b/database_admin/update.go @@ -14,7 +14,7 @@ import ( log "github.com/sirupsen/logrus" ) -var lockUsers = []string{"listener", "evaluator", "manager", "vmaas_sync"} +var lockUsers = []string{"listener", "evaluator", "manager", "vmaas_sync", "aggregator"} const activeAppSessionsWhere = `usename = ANY($1) AND pid <> pg_backend_pid()` @@ -257,6 +257,7 @@ func UpdateDB(migrationFilesURL string) { execOrPanic(db, "ALTER USER evaluator WITH PASSWORD '"+utils.GetenvOrFail("EVALUATOR_PASSWORD")+"'") execOrPanic(db, "ALTER USER manager WITH PASSWORD '"+utils.GetenvOrFail("MANAGER_PASSWORD")+"'") execOrPanic(db, "ALTER USER vmaas_sync WITH PASSWORD '"+utils.GetenvOrFail("VMAAS_SYNC_PASSWORD")+"'") + execOrPanic(db, "ALTER USER aggregator WITH PASSWORD '"+utils.GetenvOrFail("AGGREGATOR_PASSWORD")+"'") } if updateDBConfig { diff --git a/deploy/clowdapp.yaml b/deploy/clowdapp.yaml index af1bf6978..4f05608c7 100644 --- a/deploy/clowdapp.yaml +++ b/deploy/clowdapp.yaml @@ -334,9 +334,9 @@ objects: - {name: SENTRY_DSN, valueFrom: {secretKeyRef: {name: patchman-sentry, key: sentry-dsn}}} - {name: SHOW_CLOWDER_VARS, value: ''} - {name: DB_DEBUG, value: '${DB_DEBUG_AGGREGATOR}'} - - {name: DB_USER, value: evaluator} + - {name: DB_USER, value: aggregator} - {name: DB_PASSWD, valueFrom: {secretKeyRef: {name: patchman-engine-database-passwords, - key: evaluator-database-password}}} + key: aggregator-database-password}}} - {name: KAFKA_GROUP, value: patchman} - {name: KAFKA_READER_MAX_ATTEMPTS, value: '${KAFKA_READER_MAX_ATTEMPTS}'} - {name: KAFKA_WRITER_MAX_ATTEMPTS, value: '${KAFKA_WRITER_MAX_ATTEMPTS}'} @@ -376,6 +376,8 @@ objects: key: evaluator-database-password}}} - {name: VMAAS_SYNC_PASSWORD, valueFrom: {secretKeyRef: {name: patchman-engine-database-passwords, key: vmaas-sync-database-password}}} + - {name: AGGREGATOR_PASSWORD, valueFrom: {secretKeyRef: {name: patchman-engine-database-passwords, + key: aggregator-database-password}}} - {name: POD_CONFIG, value: '${DATABASE_ADMIN_CONFIG}'} resources: limits: {cpu: '${CPU_LIMIT_DATABASE_ADMIN}', memory: '${MEM_LIMIT_DATABASE_ADMIN}'}