From 6c62c050851191d5171837f3b0ee4b3e88b7f57f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Tue, 19 Mar 2024 16:21:48 +0100 Subject: [PATCH 01/15] RHINENG-8141: disable refresh job and remove lock --- deploy/clowdapp.yaml | 2 +- evaluator/evaluate_advisories.go | 19 ------------------- 2 files changed, 1 insertion(+), 20 deletions(-) diff --git a/deploy/clowdapp.yaml b/deploy/clowdapp.yaml index 62d17e70e..392a4b787 100644 --- a/deploy/clowdapp.yaml +++ b/deploy/clowdapp.yaml @@ -594,7 +594,7 @@ parameters: - {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: 'false'} # Disable cronjob execution +- {name: ADVISORY_REFRESH_SUSPEND, value: 'true'} # Disable cronjob execution # Database admin - {name: IMAGE_TAG_DATABASE_ADMIN, value: v3.6.99} diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index 4549c333d..2427139ad 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -227,20 +227,6 @@ func ensureSystemAdvisories(tx *gorm.DB, rhAccountID int, systemID int64, instal return err } -func lockAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, deleteIDs, installableIDs, - applicableIDs []int64) error { - // Lock advisory-account data, so it's not changed by other concurrent queries - var aads []models.AdvisoryAccountData - err := tx.Clauses(clause.Locking{ - Strength: "UPDATE", - Table: clause.Table{Name: clause.CurrentTable}, - }).Order("advisory_id"). - Find(&aads, "rh_account_id = ? AND (advisory_id in (?) OR advisory_id in (?) OR advisory_id in (?))", - system.RhAccountID, deleteIDs, installableIDs, applicableIDs).Error - - return err -} - func calcAdvisoryChanges(system *models.SystemPlatform, deleteIDs, installableIDs, applicableIDs []int64) []models.AdvisoryAccountData { // If system is stale, we won't change any rows in advisory_account_data @@ -332,11 +318,6 @@ func updateSystemAdvisories(tx *gorm.DB, system *models.SystemPlatform, func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, deleteIDs, installableIDs, applicableIDs []int64) error { - err := lockAdvisoryAccountData(tx, system, deleteIDs, installableIDs, applicableIDs) - if err != nil { - return err - } - changes := calcAdvisoryChanges(system, deleteIDs, installableIDs, applicableIDs) if len(changes) == 0 { From c0a09e25940abd7ff1e6ea14457f969b5486b5fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Tue, 2 Apr 2024 14:14:03 +0200 Subject: [PATCH 02/15] RHINENG-8141: move transaction --- base/database/baseline.go | 6 ++-- base/database/baseline_test.go | 6 ++-- evaluator/evaluate.go | 51 +++++++++++++---------------- evaluator/evaluate_baseline.go | 8 ++--- evaluator/evaluate_baseline_test.go | 7 ++-- 5 files changed, 34 insertions(+), 44 deletions(-) diff --git a/base/database/baseline.go b/base/database/baseline.go index f4405869d..b8793f190 100644 --- a/base/database/baseline.go +++ b/base/database/baseline.go @@ -5,8 +5,6 @@ import ( "app/base/utils" "encoding/json" "time" - - "gorm.io/gorm" ) type BaselineConfig struct { @@ -14,13 +12,13 @@ type BaselineConfig struct { ToTime time.Time `json:"to_time" example:"2022-12-31T12:00:00-04:00"` } -func GetBaselineConfig(tx *gorm.DB, system *models.SystemPlatform) *BaselineConfig { +func GetBaselineConfig(system *models.SystemPlatform) *BaselineConfig { if system.BaselineID == nil { return nil } var jsonb [][]byte - err := tx.Table("baseline"). + err := DB.Table("baseline"). Where("id = ? and rh_account_id = ?", system.BaselineID, system.RhAccountID). Pluck("config", &jsonb).Error if err != nil { diff --git a/base/database/baseline_test.go b/base/database/baseline_test.go index c92c6d7a7..203b7623e 100644 --- a/base/database/baseline_test.go +++ b/base/database/baseline_test.go @@ -14,18 +14,18 @@ func TestBaselineConfig(t *testing.T) { // system without baseline system := models.SystemPlatform{ID: 8, RhAccountID: 1, BaselineID: nil} - baselineConfig := GetBaselineConfig(DB, &system) + baselineConfig := GetBaselineConfig(&system) assert.Nil(t, baselineConfig) // system with existing baseline system = models.SystemPlatform{ID: int64(1), RhAccountID: 1, BaselineID: utils.PtrInt64(1)} - baselineConfig = GetBaselineConfig(DB, &system) + baselineConfig = GetBaselineConfig(&system) assert.Equal(t, "2010-09-22 00:00:00+00", baselineConfig.ToTime.Format("2006-01-02 15:04:05-07")) baselineID := CreateBaselineWithConfig(t, "", nil, nil, nil) // baseline with empty config system = models.SystemPlatform{ID: 1, RhAccountID: 1, BaselineID: &baselineID} - baselineConfig = GetBaselineConfig(DB, &system) + baselineConfig = GetBaselineConfig(&system) assert.Nil(t, baselineConfig) DeleteBaseline(t, baselineID) } diff --git a/evaluator/evaluate.go b/evaluator/evaluate.go index be0d36aa9..c97c115dc 100644 --- a/evaluator/evaluate.go +++ b/evaluator/evaluate.go @@ -19,7 +19,6 @@ import ( "github.com/jinzhu/copier" "github.com/pkg/errors" "gorm.io/gorm" - "gorm.io/gorm/clause" log "github.com/sirupsen/logrus" ) @@ -204,11 +203,7 @@ func runEvaluate( func evaluateInDatabase(ctx context.Context, event *mqueue.PlatformEvent, inventoryID string) ( *models.SystemPlatform, *vmaas.UpdatesV3Response, error) { - tx := database.DB.WithContext(base.Context).Begin() - // Don't allow requested TX to hang around locking the rows - defer tx.Rollback() - - system, err := tryGetSystem(tx, event.AccountID, inventoryID, event.Timestamp) + system, err := tryGetSystem(event.AccountID, inventoryID, event.Timestamp) if err != nil { return nil, nil, errors.Wrap(err, "unable to get system") } @@ -217,7 +212,7 @@ func evaluateInDatabase(ctx context.Context, event *mqueue.PlatformEvent, invent return nil, nil, nil } - thirdParty, err := analyzeRepos(tx, system) + thirdParty, err := analyzeRepos(system) if err != nil { return nil, nil, errors.Wrap(err, "Repo analysis failed") } @@ -232,7 +227,7 @@ func evaluateInDatabase(ctx context.Context, event *mqueue.PlatformEvent, invent return nil, nil, nil } - vmaasData, err := evaluateWithVmaas(tx, updatesData, system, event) + vmaasData, err := evaluateWithVmaas(updatesData, system, event) if err != nil { return nil, nil, errors.Wrap(err, "evaluation with vmaas failed") } @@ -283,27 +278,21 @@ func tryGetYumUpdates(system *models.SystemPlatform) (*vmaas.UpdatesV3Response, return &resp, nil } -func evaluateWithVmaas(tx *gorm.DB, updatesData *vmaas.UpdatesV3Response, +func evaluateWithVmaas(updatesData *vmaas.UpdatesV3Response, system *models.SystemPlatform, event *mqueue.PlatformEvent) (*vmaas.UpdatesV3Response, error) { if enableBaselineEval { if !system.SatelliteManaged || (system.SatelliteManaged && !enableSatelliteFunctionality) { - err := limitVmaasToBaseline(tx, system, updatesData) + err := limitVmaasToBaseline(system, updatesData) if err != nil { return nil, errors.Wrap(err, "Failed to evaluate baseline") } } } - err := evaluateAndStore(tx, system, updatesData, event) + err := evaluateAndStore(system, updatesData, event) if err != nil { return nil, errors.Wrap(err, "Unable to evaluate and store results") } - - err = commitWithObserve(tx) - if err != nil { - evaluationCnt.WithLabelValues("error-database-commit").Inc() - return nil, errors.New("database commit failed") - } return updatesData, nil } @@ -421,9 +410,9 @@ func tryGetVmaasRequest(system *models.SystemPlatform) (*vmaas.UpdatesV3Request, return &updatesReq, nil } -func tryGetSystem(tx *gorm.DB, accountID int, inventoryID string, +func tryGetSystem(accountID int, inventoryID string, requested *types.Rfc3339Timestamp) (*models.SystemPlatform, error) { - system, err := loadSystemData(tx, accountID, inventoryID) + system, err := loadSystemData(accountID, inventoryID) if err != nil { evaluationCnt.WithLabelValues("error-db-read-inventory-data").Inc() return nil, errors.Wrap(err, "error loading system from DB") @@ -459,8 +448,12 @@ func commitWithObserve(tx *gorm.DB) error { return nil } -func evaluateAndStore(tx *gorm.DB, system *models.SystemPlatform, +func evaluateAndStore(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response, event *mqueue.PlatformEvent) error { + tx := database.DB.WithContext(base.Context).Begin() + // Don't allow requested TX to hang around locking the rows + defer tx.Rollback() + newSystemAdvisories, err := analyzeAdvisories(tx, system, vmaasData) if err != nil { return errors.Wrap(err, "Advisory analysis failed") @@ -487,11 +480,16 @@ func evaluateAndStore(tx *gorm.DB, system *models.SystemPlatform, } } + err = commitWithObserve(tx) + if err != nil { + evaluationCnt.WithLabelValues("error-database-commit").Inc() + return errors.New("database commit failed") + } + return nil } -func analyzeRepos(tx *gorm.DB, system *models.SystemPlatform) ( - thirdParty bool, err error) { +func analyzeRepos(system *models.SystemPlatform) (thirdParty bool, err error) { if !enableRepoAnalysis { utils.LogInfo("repo analysis disabled, skipping") return false, nil @@ -500,7 +498,7 @@ func analyzeRepos(tx *gorm.DB, system *models.SystemPlatform) ( // if system has associated at least one third party repo // it's marked as third party system var thirdPartyCount int64 - err = tx.Table("system_repo sr"). + err = database.DB.Table("system_repo sr"). Joins("join repo r on r.id = sr.repo_id"). Where("sr.rh_account_id = ?", system.RhAccountID). Where("sr.system_id = ?", system.ID). @@ -624,15 +622,12 @@ func callVMaas(ctx context.Context, request *vmaas.UpdatesV3Request) (*vmaas.Upd return vmaasDataPtr.(*vmaas.UpdatesV3Response), nil } -func loadSystemData(tx *gorm.DB, accountID int, inventoryID string) (*models.SystemPlatform, error) { +func loadSystemData(accountID int, inventoryID string) (*models.SystemPlatform, error) { tStart := time.Now() defer utils.ObserveSecondsSince(tStart, evaluationPartDuration.WithLabelValues("data-loading")) var system models.SystemPlatform - err := tx.Clauses(clause.Locking{ - Strength: "UPDATE", - Table: clause.Table{Name: clause.CurrentTable}, - }).Where("rh_account_id = ?", accountID). + err := database.DB.Where("rh_account_id = ?", accountID). Where("inventory_id = ?::uuid", inventoryID). Find(&system).Error return &system, err diff --git a/evaluator/evaluate_baseline.go b/evaluator/evaluate_baseline.go index 6b3990c3b..193d9cee1 100644 --- a/evaluator/evaluate_baseline.go +++ b/evaluator/evaluate_baseline.go @@ -5,12 +5,10 @@ import ( "app/base/models" "app/base/vmaas" "time" - - "gorm.io/gorm" ) -func limitVmaasToBaseline(tx *gorm.DB, system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) error { - baselineConfig := database.GetBaselineConfig(tx, system) +func limitVmaasToBaseline(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) error { + baselineConfig := database.GetBaselineConfig(system) if baselineConfig == nil { return nil // no baseline config, nothing to change } @@ -22,7 +20,7 @@ func limitVmaasToBaseline(tx *gorm.DB, system *models.SystemPlatform, vmaasData } var filterOutNames []string - err := tx.Model(&models.AdvisoryMetadata{}).Where("name IN (?)", reportedNames). + err := database.DB.Model(&models.AdvisoryMetadata{}).Where("name IN (?)", reportedNames). Where("public_date >= ?", baselineConfig.ToTime.Truncate(24*time.Hour)). Pluck("name", &filterOutNames).Error if err != nil { diff --git a/evaluator/evaluate_baseline_test.go b/evaluator/evaluate_baseline_test.go index 41366c905..82a40c975 100644 --- a/evaluator/evaluate_baseline_test.go +++ b/evaluator/evaluate_baseline_test.go @@ -2,7 +2,6 @@ package evaluator import ( "app/base/core" - "app/base/database" "app/base/models" "app/base/utils" "app/base/vmaas" @@ -22,20 +21,20 @@ func TestLimitVmaasToBaseline(t *testing.T) { system := models.SystemPlatform{ID: 5, RhAccountID: 1, BaselineID: nil} originalVmaasData := getVMaaSUpdates(t) vmaasData := getVMaaSUpdates(t) - err := limitVmaasToBaseline(database.DB, &system, &vmaasData) + err := limitVmaasToBaseline(&system, &vmaasData) assert.Nil(t, err) assert.Equal(t, originalVmaasData, vmaasData) // a system with baseline but nothing filtered out system = models.SystemPlatform{ID: 3, RhAccountID: 1, BaselineID: utils.PtrInt64(2)} - err = limitVmaasToBaseline(database.DB, &system, &vmaasData) + err = limitVmaasToBaseline(&system, &vmaasData) assert.Nil(t, err) assert.Equal(t, []string{"RH-1", "RH-100", "RH-2"}, errataInVmaasData(vmaasData, INSTALLABLE)) // a system with baseline and filtered errata system = models.SystemPlatform{ID: 1, RhAccountID: 1, BaselineID: utils.PtrInt64(1)} vmaasData = getVMaaSUpdates(t) - err = limitVmaasToBaseline(database.DB, &system, &vmaasData) + err = limitVmaasToBaseline(&system, &vmaasData) assert.Nil(t, err) assert.Equal(t, []string{"RH-100"}, errataInVmaasData(vmaasData, INSTALLABLE)) assert.Equal(t, []string{"RH-1", "RH-2"}, errataInVmaasData(vmaasData, APPLICABLE)) From 09a58bb13fa4997f67801ec2bf45fe4ed8bd98df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Tue, 2 Apr 2024 18:09:10 +0200 Subject: [PATCH 03/15] RHINENG-8141: split loading and saving, new lazy saving --- base/database/utils.go | 8 +- evaluator/evaluate.go | 31 +++- evaluator/evaluate_advisories.go | 231 ++++++++++++++++++-------- evaluator/evaluate_advisories_test.go | 6 +- evaluator/evaluate_packages.go | 59 ++++--- evaluator/evaluate_packages_test.go | 14 +- evaluator/remediations.go | 14 ++ 7 files changed, 245 insertions(+), 118 deletions(-) diff --git a/base/database/utils.go b/base/database/utils.go index 4c289ca7a..db2f0c491 100644 --- a/base/database/utils.go +++ b/base/database/utils.go @@ -69,8 +69,8 @@ func SystemAdvisoriesByInventoryID(tx *gorm.DB, accountID int, groups map[string return (joinsT)(joins).apply(tx) } -func SystemAdvisoriesBySystemID(tx *gorm.DB, accountID int, systemID int64) *gorm.DB { - query := systemAdvisoriesQuery(tx, accountID).Where("sp.id = ?", systemID) +func SystemAdvisoriesBySystemID(accountID int, systemID int64) *gorm.DB { + query := systemAdvisoriesQuery(accountID).Where("sp.id = ?", systemID) return query } @@ -79,8 +79,8 @@ func AdvisoryMetadata(tx *gorm.DB) *gorm.DB { return JoinAdvisoryType(tx) } -func systemAdvisoriesQuery(tx *gorm.DB, accountID int) *gorm.DB { - query := tx.Table("system_advisories sa").Select("sa.*"). +func systemAdvisoriesQuery(accountID int) *gorm.DB { + query := DB.Table("system_advisories sa").Select("sa.*"). Joins("join system_platform sp ON sa.rh_account_id = sp.rh_account_id AND sa.system_id = sp.id"). Where("sa.rh_account_id = ? AND sp.rh_account_id = ?", accountID, accountID) return query diff --git a/evaluator/evaluate.go b/evaluator/evaluate.go index c97c115dc..91328b35a 100644 --- a/evaluator/evaluate.go +++ b/evaluator/evaluate.go @@ -227,6 +227,10 @@ func evaluateInDatabase(ctx context.Context, event *mqueue.PlatformEvent, invent return nil, nil, nil } + // load and evaluate advisories for system + // posunut spred `updateAdvisoryAccountData` + // update in `storeAdvisoryData` + vmaasData, err := evaluateWithVmaas(updatesData, system, event) if err != nil { return nil, nil, errors.Wrap(err, "evaluation with vmaas failed") @@ -296,8 +300,7 @@ func evaluateWithVmaas(updatesData *vmaas.UpdatesV3Response, return updatesData, nil } -func getUpdatesData(ctx context.Context, system *models.SystemPlatform) ( - *vmaas.UpdatesV3Response, error) { +func getUpdatesData(ctx context.Context, system *models.SystemPlatform) (*vmaas.UpdatesV3Response, error) { var yumUpdates *vmaas.UpdatesV3Response var yumErr error if enableYumUpdatesEval { @@ -450,21 +453,33 @@ func commitWithObserve(tx *gorm.DB) error { func evaluateAndStore(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response, event *mqueue.PlatformEvent) error { + deleteIDs, installableIDs, applicableIDs, err := lazySaveAndLoadAdvisories(system, vmaasData) + if err != nil { + return errors.Wrap(err, "Advisory loading failed") + } + + pkgByName, installed, installable, applicable, err := lazySaveAndLoadPackages(system, vmaasData) + if err != nil { + return errors.Wrap(err, "Package loading failed") + } + tx := database.DB.WithContext(base.Context).Begin() // Don't allow requested TX to hang around locking the rows defer tx.Rollback() - newSystemAdvisories, err := analyzeAdvisories(tx, system, vmaasData) + systemAdvisoriesNew, err := storeAdvisoryData(tx, system, deleteIDs, installableIDs, applicableIDs) if err != nil { - return errors.Wrap(err, "Advisory analysis failed") + evaluationCnt.WithLabelValues("error-store-advisories").Inc() + return errors.Wrap(err, "Unable to store advisory data") } - installed, installable, applicable, err := analyzePackages(tx, system, vmaasData) + err = updateSystemPackages(tx, system, pkgByName) if err != nil { - return errors.Wrap(err, "Package analysis failed") + evaluationCnt.WithLabelValues("error-system-pkgs").Inc() + return errors.Wrap(err, "Unable to update system packages") } - err = updateSystemPlatform(tx, system, newSystemAdvisories, installed, installable, applicable) + err = updateSystemPlatform(tx, system, systemAdvisoriesNew, installed, installable, applicable) if err != nil { evaluationCnt.WithLabelValues("error-update-system").Inc() return errors.Wrap(err, "Unable to update system") @@ -472,7 +487,7 @@ func evaluateAndStore(system *models.SystemPlatform, // Send instant notification with new advisories if enableInstantNotifications { - err = publishNewAdvisoriesNotification(tx, system, event, system.RhAccountID, newSystemAdvisories) + err = publishNewAdvisoriesNotification(tx, system, event, system.RhAccountID, systemAdvisoriesNew) if err != nil { evaluationCnt.WithLabelValues("error-advisory-notification").Inc() utils.LogError("orgID", event.GetOrgID(), "inventoryID", system.GetInventoryID(), "err", err.Error(), diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index 2427139ad..48295643f 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -12,54 +12,143 @@ import ( "gorm.io/gorm/clause" ) -func analyzeAdvisories(tx *gorm.DB, system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( +func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( + []int64, []int64, []int64, error) { + if !enableAdvisoryAnalysis { + utils.LogInfo("advisory analysis disabled, skipping lazy saving and loading") + return nil, nil, nil, nil + } + deleteIDs, installableIDs, applicableIDs, err := processSystemAdvisories(system, vmaasData) + if err != nil { + evaluationCnt.WithLabelValues("error-process-advisories").Inc() + return nil, nil, nil, errors.Wrap(err, "unable to process system advisories") + } + + return deleteIDs, installableIDs, applicableIDs, err +} + +func lazySaveAndLoadAdvisories2(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( SystemAdvisoryMap, error) { if !enableAdvisoryAnalysis { - utils.LogInfo("advisory analysis disabled, skipping") + utils.LogInfo("advisory analysis disabled, skipping lazy saving and loading") return nil, nil } - deleteIDs, installableIDs, applicableIDs, err := processSystemAdvisories(tx, system, vmaasData) + err := lazySaveAdvisories2(vmaasData, system.InventoryID) if err != nil { - evaluationCnt.WithLabelValues("error-process-advisories").Inc() - return nil, errors.Wrap(err, "Unable to process system advisories") + return nil, errors.Wrap(err, "Unable to store unknown advisories in DB") } - newSystemAdvisories, err := storeAdvisoryData(tx, system, deleteIDs, installableIDs, applicableIDs) + // TODO: load system advisories + // rename loadSystemAdvisories -> loadAdvisories(system) + + // spracovat advisories ako packages + return FIXMEAdvisories, nil +} + +func lazySaveAdvisories2(vmaasData *vmaas.UpdatesV3Response, inventoryID string) error { + // -> load reported advisories, advisories to lazy-save can only appear in VmaasData + reportedNames := getReportedAdvisoryNames(vmaasData) + if len(reportedNames) < 1 { + return nil + } + // -> get missing from reported + missingNames, err := getMissingAdvisories(reportedNames) // namiesto getAdvisoriesFromDB if err != nil { - evaluationCnt.WithLabelValues("error-store-advisories").Inc() - return nil, errors.Wrap(err, "Unable to store advisory data") + return errors.Wrap(err, "Unable to get missing system advisories") + } + // -> log missing found + nUnknown := len(missingNames) + if nUnknown > 0 { + utils.LogInfo("inventoryID", inventoryID, "unknown", nUnknown, "unknown advisories") + updatesCnt.WithLabelValues("unknown").Add(float64(nUnknown)) + } else { + return nil + } + // -> store missing advisories + err = storeMissingAdvisories2(missingNames) + if err != nil { + return errors.Wrap(err, "failed to save missing advisory_metadata") + } + + // FIXME: treba updatnut aj models.SystemAdvisories?? + + return nil +} + +func storeMissingAdvisories2(missingNames []string) error { + toStore := make(models.AdvisoryMetadataSlice, 0, len(missingNames)) + for _, name := range missingNames { + if len(name) > 0 && len(name) < 100 { + toStore = append(toStore, models.AdvisoryMetadata{ + Name: name, + Description: "Not Available", + Synopsis: "Not Available", + Summary: "Not Available", + AdvisoryTypeID: 0, + RebootRequired: false, + Synced: false, + }) + } } - return newSystemAdvisories, nil + + var err error + if len(toStore) > 0 { + tx := database.DB.Begin() + defer tx.Commit() + err = tx.Clauses(clause.OnConflict{DoNothing: true}).Create(&toStore).Error + if err != nil { + return err + } + } + return nil +} + +// Determine if advisories from DB are propperly stored based on advisory metadata existance. +func getMissingAdvisories(advisoryNames []string) ([]string, error) { + advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(advisoryNames)) + err := database.DB.Model(&models.AdvisoryMetadata{}). + Where("name IN (?)", advisoryNames). + Select("id, name"). + Scan(&advisoryMetadata).Error + if err != nil { + return nil, err + } + + found := make(map[string]bool, len(advisoryNames)) + for _, am := range advisoryMetadata { + found[am.Name] = true + } + + missingNames := make([]string, 0, len(advisoryNames)-len(advisoryMetadata)) + for _, name := range advisoryNames { + if !found[name] { + missingNames = append(missingNames, name) + } + } + return missingNames, nil } // Changes data stored in system_advisories, in order to match newest evaluation // Before this methods stores the entries into the system_advisories table, it locks // advisory_account_data table, so other evaluations don't interfere with this one -func processSystemAdvisories(tx *gorm.DB, system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( +func processSystemAdvisories(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( []int64, []int64, []int64, error) { tStart := time.Now() defer utils.ObserveSecondsSince(tStart, evaluationPartDuration.WithLabelValues("advisories-processing")) - reported := getReportedAdvisories(vmaasData) - oldSystemAdvisories, err := getStoredAdvisoriesMap(tx, system.RhAccountID, system.ID) + deleteIDs, installableNames, applicableNames, err := getAdvisoryChanges(system, vmaasData) if err != nil { - return nil, nil, nil, errors.Wrap(err, "Unable to get system stored advisories") + return nil, nil, nil, errors.Wrap(err, "Unable to get system advisory change") } - - deleteIDs, installableNames, applicableNames := getAdvisoryChanges(reported, oldSystemAdvisories) updatesCnt.WithLabelValues("patched").Add(float64(len(deleteIDs))) utils.LogInfo("inventoryID", system.InventoryID, "fixed", len(deleteIDs), "fixed advisories") - installableIDs, missingInstallableNames, err := getAdvisoriesFromDB(tx, installableNames) + installableIDs, missingInstallableNames, err := getAdvisoriesFromDB(installableNames, system.InventoryID) if err != nil { return nil, nil, nil, errors.Wrap(err, "Unable to ensure new installable system advisories in db") } - nUnknown := len(installableNames) - len(installableIDs) - if nUnknown > 0 { - utils.LogInfo("inventoryID", system.InventoryID, "unknown", nUnknown, "unknown installable advisories") - updatesCnt.WithLabelValues("unknown").Add(float64(nUnknown)) - } + missingInstallableIDs, err := storeMissingAdvisories(missingInstallableNames) if err != nil { return nil, nil, nil, errors.Wrap(err, "unable to store unknown installable advisories in db") @@ -68,15 +157,11 @@ func processSystemAdvisories(tx *gorm.DB, system *models.SystemPlatform, vmaasDa updatesCnt.WithLabelValues("installable").Add(float64(len(installableIDs))) utils.LogInfo("inventoryID", system.InventoryID, "installable", len(installableIDs), "installable advisories") - applicableIDs, missingApplicableNames, err := getAdvisoriesFromDB(tx, applicableNames) + applicableIDs, missingApplicableNames, err := getAdvisoriesFromDB(applicableNames, system.InventoryID) if err != nil { return nil, nil, nil, errors.Wrap(err, "Unable to ensure new applicable system advisories in db") } - nUnknown = len(applicableNames) - len(applicableIDs) - if nUnknown > 0 { - utils.LogInfo("inventoryID", system.InventoryID, "unknown", nUnknown, "unknown applicable advisories") - updatesCnt.WithLabelValues("unknown").Add(float64(nUnknown)) - } + missingApplicableIDs, err := storeMissingAdvisories(missingApplicableNames) if err != nil { return nil, nil, nil, errors.Wrap(err, "unable to store unknown applicable advisories in db") @@ -90,35 +175,36 @@ func processSystemAdvisories(tx *gorm.DB, system *models.SystemPlatform, vmaasDa func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, deleteIDs, installableIDs, applicableIDs []int64) (SystemAdvisoryMap, error) { + if !enableAdvisoryAnalysis { + utils.LogInfo("advisory analysis disabled, skipping storing") + return nil, nil + } + defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("advisories-store")) - newSystemAdvisories, err := updateSystemAdvisories(tx, system, deleteIDs, installableIDs, applicableIDs) + err := updateSystemAdvisoriesTODO(tx, system, deleteIDs, installableIDs, applicableIDs) if err != nil { return nil, errors.Wrap(err, "Unable to update system advisories") } + systemAdvisoriesNew, err := loadSystemAdvisories(tx, system.RhAccountID, system.ID) // reload system advisories after update + if err != nil { + return nil, errors.Wrap(err, "Unable to load new system advisories") + } err = updateAdvisoryAccountData(tx, system, deleteIDs, installableIDs, applicableIDs) if err != nil { return nil, errors.Wrap(err, "Unable to update advisory_account_data caches") } - return newSystemAdvisories, nil + return systemAdvisoriesNew, nil } -func getStoredAdvisoriesMap(tx *gorm.DB, accountID int, systemID int64) (map[string]models.SystemAdvisories, error) { - var advisories []models.SystemAdvisories - err := database.SystemAdvisoriesBySystemID(tx, accountID, systemID).Preload("Advisory").Find(&advisories).Error +func getAdvisoryChanges(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( + []int64, []string, []string, error) { + reported := getReportedAdvisories(vmaasData) + stored, err := loadSystemAdvisories(database.DB, system.RhAccountID, system.ID) if err != nil { - return nil, err - } - - advisoriesMap := make(map[string]models.SystemAdvisories, len(advisories)) - for _, advisory := range advisories { - advisoriesMap[advisory.Advisory.Name] = advisory + return nil, nil, nil, errors.Wrap(err, "Unable to get system stored advisories") } - return advisoriesMap, nil -} -func getAdvisoryChanges(reported map[string]int, stored map[string]models.SystemAdvisories) ( - []int64, []string, []string) { installableNames := make([]string, 0, len(reported)) applicableNames := make([]string, 0, len(reported)) deleteIDs := make([]int64, 0, len(stored)) @@ -131,43 +217,48 @@ func getAdvisoryChanges(reported map[string]int, stored map[string]models.System } } } - for storedAdvisory, storedAdvisoryObj := range stored { - if _, found := reported[storedAdvisory]; !found { + for storedAdvisoryName, storedAdvisory := range stored { + if _, found := reported[storedAdvisoryName]; !found { // advisory was patched from last evaluation,let's remove it - deleteIDs = append(deleteIDs, storedAdvisoryObj.AdvisoryID) + deleteIDs = append(deleteIDs, storedAdvisory.AdvisoryID) } } - return deleteIDs, installableNames, applicableNames + return deleteIDs, installableNames, applicableNames, nil } -// Return advisory IDs, missing advisory names, error -func getAdvisoriesFromDB(tx *gorm.DB, advisories []string) ([]int64, []string, error) { - advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(advisories)) - advisoryIDs := make([]int64, 0, len(advisories)) - err := tx.Model(&models.AdvisoryMetadata{}).Where("name IN (?)", advisories). +func getAdvisoriesFromDB(advisoryNames []string, inventoryID string) ([]int64, []string, error) { + advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(advisoryNames)) + err := database.DB.Model(&models.AdvisoryMetadata{}). + Where("name IN (?)", advisoryNames). Select("id, name"). Scan(&advisoryMetadata).Error if err != nil { return nil, nil, err } - missing := []string{} - found := make(map[string]bool, len(advisories)) + found := make(map[string]bool, len(advisoryNames)) + advisoryIDs := make([]int64, 0, len(advisoryNames)) for _, am := range advisoryMetadata { found[am.Name] = true advisoryIDs = append(advisoryIDs, am.ID) } - for _, name := range advisories { + nUnknown := len(advisoryNames) - len(advisoryIDs) + if nUnknown > 0 { + utils.LogInfo("inventoryID", inventoryID, "unknown", nUnknown, "unknown advisories") + updatesCnt.WithLabelValues("unknown").Add(float64(nUnknown)) + } + missingAdvisoryNames := make([]string, 0, nUnknown) + for _, name := range advisoryNames { if !found[name] { - missing = append(missing, name) + missingAdvisoryNames = append(missingAdvisoryNames, name) } } - return advisoryIDs, missing, err + return advisoryIDs, missingAdvisoryNames, err } -func storeMissingAdvisories(missing []string) ([]int64, error) { - toStore := make(models.AdvisoryMetadataSlice, 0, len(missing)) - for _, name := range missing { +func storeMissingAdvisories(missingNames []string) ([]int64, error) { + toStore := make(models.AdvisoryMetadataSlice, 0, len(missingNames)) + for _, name := range missingNames { if len(name) > 0 && len(name) < 100 { toStore = append(toStore, models.AdvisoryMetadata{ Name: name, @@ -288,32 +379,34 @@ func deleteOldSystemAdvisories(tx *gorm.DB, accountID int, systemID int64, patch return err } -func updateSystemAdvisories(tx *gorm.DB, system *models.SystemPlatform, - deleteIDs, installableIDs, applicableIDs []int64) (SystemAdvisoryMap, error) { +func updateSystemAdvisoriesTODO(tx *gorm.DB, system *models.SystemPlatform, + deleteIDs, installableIDs, applicableIDs []int64) error { // this will remove many many old items from our "system_advisories" table err := deleteOldSystemAdvisories(tx, system.RhAccountID, system.ID, deleteIDs) if err != nil { - return nil, err + return err } err = ensureSystemAdvisories(tx, system.RhAccountID, system.ID, installableIDs, applicableIDs) if err != nil { - return nil, err + return err } - newSystemAdvisories := SystemAdvisoryMap{} - var data []models.SystemAdvisories - err = tx.Preload("Advisory"). - Find(&data, "system_id = ? AND rh_account_id = ?", system.ID, system.RhAccountID).Error + return nil +} +func loadSystemAdvisories(tx *gorm.DB, accountID int, systemID int64) (SystemAdvisoryMap, error) { + var data []models.SystemAdvisories + err := tx.Preload("Advisory").Find(&data, "system_id = ? AND rh_account_id = ?", systemID, accountID).Error if err != nil { return nil, err } + systemAdvisories := make(SystemAdvisoryMap, len(data)) for _, sa := range data { - newSystemAdvisories[sa.Advisory.Name] = sa + systemAdvisories[sa.Advisory.Name] = sa } - return newSystemAdvisories, nil + return systemAdvisories, nil } func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, deleteIDs, installableIDs, diff --git a/evaluator/evaluate_advisories_test.go b/evaluator/evaluate_advisories_test.go index 48565b119..a3bc89095 100644 --- a/evaluator/evaluate_advisories_test.go +++ b/evaluator/evaluate_advisories_test.go @@ -74,7 +74,7 @@ func TestGetStoredAdvisoriesMap(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() - systemAdvisories, err := getStoredAdvisoriesMap(database.DB, 1, 1) + systemAdvisories, err := loadSystemAdvisories(database.DB, 1, 1) assert.Nil(t, err) assert.NotNil(t, systemAdvisories) assert.Equal(t, 8, len(systemAdvisories)) @@ -124,7 +124,7 @@ func TestGetAdvisoriesFromDB(t *testing.T) { core.SetupTestEnvironment() advisories := []string{"ER-1", "RH-1", "ER-2", "RH-2"} - advisoryIDs, missingNames, err := getAdvisoriesFromDB(database.DB, advisories) + advisoryIDs, missingNames, err := getAdvisoriesFromDB(advisories) assert.Nil(t, err) assert.Equal(t, 2, len(advisoryIDs)) assert.Equal(t, 2, len(missingNames)) @@ -135,7 +135,7 @@ func TestGetAdvisoriesFromDBEmptyString(t *testing.T) { core.SetupTestEnvironment() advisories := []string{""} - advisoryIDs, missingNames, err := getAdvisoriesFromDB(database.DB, advisories) + advisoryIDs, missingNames, err := getAdvisoriesFromDB(advisories) assert.Nil(t, err) assert.Equal(t, 0, len(advisoryIDs)) assert.Equal(t, 1, len(missingNames)) diff --git a/evaluator/evaluate_packages.go b/evaluator/evaluate_packages.go index ead55865e..62ee1a94c 100644 --- a/evaluator/evaluate_packages.go +++ b/evaluator/evaluate_packages.go @@ -14,40 +14,35 @@ import ( "gorm.io/gorm/clause" ) -func analyzePackages(tx *gorm.DB, system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( - installed, installable, applicable int, err error) { +func lazySaveAndLoadPackages(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( + map[string]namedPackage, int, int, int, error) { if !enablePackageAnalysis { - return 0, 0, 0, nil + utils.LogInfo("package analysis disabled, skipping lazy saving and loading") + return nil, 0, 0, 0, nil } - err = lazySavePackages(tx, vmaasData) + err := lazySavePackages(vmaasData) if err != nil { evaluationCnt.WithLabelValues("error-lazy-pkg-save").Inc() - return 0, 0, 0, errors.Wrap(err, "lazy package save failed") + return nil, 0, 0, 0, errors.Wrap(err, "lazy package save failed") } - pkgByName, installed, installable, applicable, err := loadPackages(tx, system, vmaasData) + pkgByName, installed, installable, applicable, err := loadPackages(system, vmaasData) if err != nil { evaluationCnt.WithLabelValues("error-pkg-data").Inc() - return 0, 0, 0, errors.Wrap(err, "Unable to load package data") + return nil, 0, 0, 0, errors.Wrap(err, "Unable to load package data") } - - err = updateSystemPackages(tx, system, pkgByName) - if err != nil { - evaluationCnt.WithLabelValues("error-system-pkgs").Inc() - return 0, 0, 0, errors.Wrap(err, "Unable to update system packages") - } - return installed, installable, applicable, nil + return pkgByName, installed, installable, applicable, nil } // Add unknown EVRAs into the db if needed -func lazySavePackages(tx *gorm.DB, vmaasData *vmaas.UpdatesV3Response) error { +func lazySavePackages(vmaasData *vmaas.UpdatesV3Response) error { if !enableLazyPackageSave { return nil } defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("lazy-package-save")) - missingPackages := getMissingPackages(tx, vmaasData) + missingPackages := getMissingPackages(vmaasData) err := updatePackageDB(&missingPackages) if err != nil { return errors.Wrap(err, "packages bulk insert failed") @@ -55,7 +50,7 @@ func lazySavePackages(tx *gorm.DB, vmaasData *vmaas.UpdatesV3Response) error { return nil } -func getMissingPackage(tx *gorm.DB, nevra string) *models.Package { +func getMissingPackage(nevra string) *models.Package { _, found := memoryPackageCache.GetByNevra(nevra) if found { // package is already in db/cache, nothing needed @@ -87,7 +82,7 @@ func getMissingPackage(tx *gorm.DB, nevra string) *models.Package { if pkg.NameID == 0 { // insert conflict, it did not return ID // try to get ID from package_name table - tx.Where("name = ?", parsed.Name).First(&pkgName) + database.DB.Where("name = ?", parsed.Name).First(&pkgName) pkg.NameID = pkgName.ID } } @@ -95,17 +90,17 @@ func getMissingPackage(tx *gorm.DB, nevra string) *models.Package { } // Get packages with known name but version missing in db/cache -func getMissingPackages(tx *gorm.DB, vmaasData *vmaas.UpdatesV3Response) models.PackageSlice { +func getMissingPackages(vmaasData *vmaas.UpdatesV3Response) models.PackageSlice { updates := vmaasData.GetUpdateList() packages := make(models.PackageSlice, 0, len(updates)) for nevra, update := range updates { - if pkg := getMissingPackage(tx, nevra); pkg != nil { + if pkg := getMissingPackage(nevra); pkg != nil { packages = append(packages, *pkg) } for _, pkgUpdate := range update.GetAvailableUpdates() { // don't use pkgUpdate.Package since it might be missing epoch, construct it from name and evra updateNevra := fmt.Sprintf("%s-%s", pkgUpdate.GetPackageName(), pkgUpdate.GetEVRA()) - if pkg := getMissingPackage(tx, updateNevra); pkg != nil { + if pkg := getMissingPackage(updateNevra); pkg != nil { packages = append(packages, *pkg) } } @@ -136,12 +131,12 @@ func updatePackageNameDB(missing *models.PackageName) error { } // Find relevant package data based on vmaas results -func loadPackages(tx *gorm.DB, system *models.SystemPlatform, - vmaasData *vmaas.UpdatesV3Response) (map[string]namedPackage, int, int, int, error) { +func loadPackages(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( + map[string]namedPackage, int, int, int, error) { defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("packages-load")) - packages, installed, installable, applicable := packagesFromUpdateList(system, vmaasData) - err := loadSystemNEVRAsFromDB(tx, system, packages) + packages, installed, installable, applicable := packagesFromUpdateList(system.InventoryID, vmaasData) + err := loadSystemNEVRAsFromDB(system, packages) if err != nil { return nil, 0, 0, 0, errors.Wrap(err, "loading packages") } @@ -149,7 +144,7 @@ func loadPackages(tx *gorm.DB, system *models.SystemPlatform, return packages, installed, installable, applicable, nil } -func packagesFromUpdateList(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( +func packagesFromUpdateList(inventoryID string, vmaasData *vmaas.UpdatesV3Response) ( map[string]namedPackage, int, int, int) { installed := 0 installable := 0 @@ -184,12 +179,12 @@ func packagesFromUpdateList(system *models.SystemPlatform, vmaasData *vmaas.Upda } } } - utils.LogInfo("inventoryID", system.InventoryID, "packages", numUpdates) + utils.LogInfo("inventoryID", inventoryID, "packages", numUpdates) return packages, installed, installable, applicable } -func loadSystemNEVRAsFromDB(tx *gorm.DB, system *models.SystemPlatform, packages map[string]namedPackage) error { - rows, err := tx.Table("system_package2 sp2"). +func loadSystemNEVRAsFromDB(system *models.SystemPlatform, packages map[string]namedPackage) error { + rows, err := database.DB.Table("system_package2 sp2"). Select("sp2.name_id, sp2.package_id, sp2.installable_id, sp2.applicable_id"). Where("rh_account_id = ? AND system_id = ?", system.RhAccountID, system.ID). Rows() @@ -200,7 +195,7 @@ func loadSystemNEVRAsFromDB(tx *gorm.DB, system *models.SystemPlatform, packages numStored := 0 defer rows.Close() for rows.Next() { - err = tx.ScanRows(rows, &columns) + err = database.DB.ScanRows(rows, &columns) if err != nil { return err } @@ -269,6 +264,10 @@ func createSystemPackage(system *models.SystemPlatform, pkg namedPackage) models func updateSystemPackages(tx *gorm.DB, system *models.SystemPlatform, packagesByNEVRA map[string]namedPackage) error { + if !enablePackageAnalysis { + utils.LogInfo("package analysis disabled, skipping storing") + return nil + } defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("packages-store")) nPkgs := len(packagesByNEVRA) diff --git a/evaluator/evaluate_packages_test.go b/evaluator/evaluate_packages_test.go index f94f02beb..4c33dce2f 100644 --- a/evaluator/evaluate_packages_test.go +++ b/evaluator/evaluate_packages_test.go @@ -38,7 +38,9 @@ func TestAnalyzePackages(t *testing.T) { EVRA: utils.PtrString("0:2.2.3-1.fc33.x86_64"), }}}}} - installed, installable, applicable, err := analyzePackages(database.DB, &system, &vmaasData) + pkgByName, installed, installable, applicable, err := lazySaveAndLoadPackages(&system, &vmaasData) + assert.Nil(t, err) + err = updateSystemPackages(database.DB, &system, pkgByName) assert.Nil(t, err) assert.Equal(t, 3, installed) // kernel, firefox, custom-package assert.Equal(t, 2, installable) // firefox, custom-package have updates @@ -63,7 +65,9 @@ func TestSystemPackageRemoval(t *testing.T) { "kernel-0:5.6.14-200.fc31.x86_64": {AvailableUpdates: &[]vmaas.UpdatesV3ResponseAvailableUpdates{}}, }} - installed, installable, applicable, err := analyzePackages(database.DB, &system, &vmaasData) + pkgByName, installed, installable, applicable, err := lazySaveAndLoadPackages(&system, &vmaasData) + assert.Nil(t, err) + err = updateSystemPackages(database.DB, &system, pkgByName) assert.Nil(t, err) assert.Equal(t, 1, installed) assert.Equal(t, 0, installable) @@ -78,7 +82,9 @@ func TestSystemPackageRemoval(t *testing.T) { EVRA: utils.PtrString("0:5.6.14-200.fc31.x86_64"), }}}}} - installed, installable, applicable, err = analyzePackages(database.DB, &system, &vmaasData) + pkgByName, installed, installable, applicable, err = lazySaveAndLoadPackages(&system, &vmaasData) + assert.Nil(t, err) + err = updateSystemPackages(database.DB, &system, pkgByName) assert.Nil(t, err) // only 1 package should be analyzed assert.Equal(t, 1, installed) @@ -108,7 +114,7 @@ func TestLazySavePackages(t *testing.T) { } vmaasData := vmaas.UpdatesV3Response{UpdateList: &updateList} database.CheckEVRAsInDB(t, 0, evras...) - err := lazySavePackages(database.DB, &vmaasData) + err := lazySavePackages(&vmaasData) assert.Nil(t, err) database.CheckEVRAsInDBSynced(t, 2, false, evras[:2]...) // EVRAs were added database.CheckEVRAsInDB(t, 1, evras[2:]...) // EVRA for unknown package was added diff --git a/evaluator/remediations.go b/evaluator/remediations.go index a73f1d21a..9540ff3ad 100644 --- a/evaluator/remediations.go +++ b/evaluator/remediations.go @@ -55,6 +55,20 @@ func getReportedAdvisories(vmaasData *vmaas.UpdatesV3Response) map[string]int { return advisories } +func getReportedAdvisoryNames(vmaasData *vmaas.UpdatesV3Response) []string { + updateList := vmaasData.GetUpdateList() + reportedNames := make([]string, 0, len(updateList)) + for _, updates := range updateList { + for _, update := range updates.GetAvailableUpdates() { + advisoryName := update.GetErratum() + if len(advisoryName) > 0 { + reportedNames = append(reportedNames, advisoryName) + } + } + } + return reportedNames +} + func getReportedPackageUpdates(vmaasData *vmaas.UpdatesV3Response) map[string]bool { updateList := vmaasData.GetUpdateList() packages := make(map[string]bool, len(updateList)) From d86351aa89c73dffbd087fcd69a30b7a1d8e7a6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Wed, 26 Jun 2024 18:23:33 +0200 Subject: [PATCH 04/15] RHINENG-8141: implement new stored-reported change evaluation --- evaluator/evaluate.go | 14 +++- evaluator/evaluate_advisories.go | 127 +++++++++++++++++++++++++++++-- 2 files changed, 132 insertions(+), 9 deletions(-) diff --git a/evaluator/evaluate.go b/evaluator/evaluate.go index 91328b35a..de6380d5d 100644 --- a/evaluator/evaluate.go +++ b/evaluator/evaluate.go @@ -453,7 +453,12 @@ func commitWithObserve(tx *gorm.DB) error { func evaluateAndStore(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response, event *mqueue.PlatformEvent) error { - deleteIDs, installableIDs, applicableIDs, err := lazySaveAndLoadAdvisories(system, vmaasData) + // deleteIDs, installableIDs, applicableIDs, err := lazySaveAndLoadAdvisories(system, vmaasData) + // if err != nil { + // return errors.Wrap(err, "Advisory loading failed") + // } + + advisoriesByName, err := lazySaveAndLoadAdvisories2(system, vmaasData) if err != nil { return errors.Wrap(err, "Advisory loading failed") } @@ -467,7 +472,12 @@ func evaluateAndStore(system *models.SystemPlatform, // Don't allow requested TX to hang around locking the rows defer tx.Rollback() - systemAdvisoriesNew, err := storeAdvisoryData(tx, system, deleteIDs, installableIDs, applicableIDs) + // systemAdvisoriesNew, err := storeAdvisoryData(tx, system, deleteIDs, installableIDs, applicableIDs) + // if err != nil { + // evaluationCnt.WithLabelValues("error-store-advisories").Inc() + // return errors.Wrap(err, "Unable to store advisory data") + // } + systemAdvisoriesNew, err := storeAdvisoryData2(tx, system, advisoriesByName) // TODO: what to do with `systemAdvNew`? if err != nil { evaluationCnt.WithLabelValues("error-store-advisories").Inc() return errors.Wrap(err, "Unable to store advisory data") diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index 48295643f..28eed2b69 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -28,22 +28,101 @@ func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.U } func lazySaveAndLoadAdvisories2(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( - SystemAdvisoryMap, error) { + ExtendedAdvisoryMap, error) { if !enableAdvisoryAnalysis { utils.LogInfo("advisory analysis disabled, skipping lazy saving and loading") return nil, nil } + // TODO: should this first evaluate missing and lazy-save just the missing? err := lazySaveAdvisories2(vmaasData, system.InventoryID) if err != nil { return nil, errors.Wrap(err, "Unable to store unknown advisories in DB") } - // TODO: load system advisories - // rename loadSystemAdvisories -> loadAdvisories(system) + stored, err := loadSystemAdvisories(database.Db, system.RhAccountID, system.ID) + if err != nil { + return nil, errors.Wrap(err, "Unable to load system advisories") + } + + merged, err := evaluateChanges(vmaasData, stored, system.RhAccountID, system.ID) + if err != nil { + return nil, errors.Wrap(err, "Unable to evaluate advisory changes") + } - // spracovat advisories ako packages - return FIXMEAdvisories, nil + return merged, nil +} + +func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMap, accountID int, systemID int64) ( + ExtendedAdvisoryMap, error) { + reported := getReportedAdvisories(vmaasData) + + // TODO: check this func for errors and efficiency + + // -> map[string]int: "erratum -> helper column to diferentiate installable/applicable" + // --> moze sa porovnavat if statusID == INSTALLABLE {...} else {APPLICABLE...} + + extendedAdvisories := make(ExtendedAdvisoryMap, len(reported)+len(stored)) + missingNames := make([]string, 0, len(reported)) + for reportedName, reportedStatusID := range reported { + if storedAdvisory, found := stored[reportedName]; found { + if reportedStatusID != storedAdvisory.StatusID { + extendedAdvisories[reportedName] = ExtendedAdvisory{ + Change: Update, + SystemAdvisories: storedAdvisory, + } + } else { + extendedAdvisories[reportedName] = ExtendedAdvisory{ + Change: Keep, + SystemAdvisories: storedAdvisory, + } + } + } else { + extendedAdvisories[reportedName] = ExtendedAdvisory{ + Change: Add, + SystemAdvisories: models.SystemAdvisories{ + RhAccountID: accountID, + SystemID: systemID, + StatusID: reportedStatusID, + }, + } + missingNames = append(missingNames, reportedName) + } + } + + advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(missingNames)) + err := database.Db.Model(&models.AdvisoryMetadata{}). + Where("name IN (?)", missingNames). + Select("id, name"). + Scan(&advisoryMetadata).Error + if err != nil { + return nil, err + } + + name2AdvisoryID := make(map[string]int64, len(missingNames)) + for _, am := range advisoryMetadata { + name2AdvisoryID[am.Name] = am.ID + } + + for _, name := range missingNames { + if _, found := name2AdvisoryID[name]; !found { + return nil, errors.New("Failed to evaluate changes because an advisory was not lazy-saved") + } + extendedAdvisory := extendedAdvisories[name] + extendedAdvisory.AdvisoryID = name2AdvisoryID[name] + extendedAdvisories[name] = extendedAdvisory + } + + for storedName, storedAdvisory := range stored { + if _, found := reported[storedName]; !found { + extendedAdvisories[storedName] = ExtendedAdvisory{ + Change: Remove, + SystemAdvisories: storedAdvisory, + } + } + } + + return extendedAdvisories, nil } func lazySaveAdvisories2(vmaasData *vmaas.UpdatesV3Response, inventoryID string) error { @@ -71,8 +150,6 @@ func lazySaveAdvisories2(vmaasData *vmaas.UpdatesV3Response, inventoryID string) return errors.Wrap(err, "failed to save missing advisory_metadata") } - // FIXME: treba updatnut aj models.SystemAdvisories?? - return nil } @@ -100,6 +177,7 @@ func storeMissingAdvisories2(missingNames []string) error { if err != nil { return err } + // TODO: created toStore objects will have hadded a .ID } return nil } @@ -197,6 +275,34 @@ func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, return systemAdvisoriesNew, nil } +func storeAdvisoryData2(tx *gorm.DB, system *models.SystemPlatform, advisoriesByName ExtendedAdvisoryMap) ( + SystemAdvisoryMap, error) { + if !enableAdvisoryAnalysis { + utils.LogInfo("advisory analysis disabled, skipping storing") + return nil, nil + } + + // TODO: toto prerobit, aby sa spravit DB update a zaroven aj update v `advisoriesByName` + // => zbavime sa znovunacitania + // => co s updateAdvisoryAccountData? + + defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("advisories-store")) + err := updateSystemAdvisoriesTODO(tx, system, deleteIDs, installableIDs, applicableIDs) + if err != nil { + return nil, errors.Wrap(err, "Unable to update system advisories") + } + systemAdvisoriesNew, err := loadSystemAdvisories(tx, system.RhAccountID, system.ID) // reload system advisories after update + if err != nil { + return nil, errors.Wrap(err, "Unable to load new system advisories") + } + + err = updateAdvisoryAccountData(tx, system, deleteIDs, installableIDs, applicableIDs) + if err != nil { + return nil, errors.Wrap(err, "Unable to update advisory_account_data caches") + } + return systemAdvisoriesNew, nil +} + func getAdvisoryChanges(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( []int64, []string, []string, error) { reported := getReportedAdvisories(vmaasData) @@ -425,3 +531,10 @@ func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, delet return database.BulkInsert(txOnConflict, changes) } + +type ExtendedAdvisory struct { + Change ChangeType + models.SystemAdvisories +} + +type ExtendedAdvisoryMap map[string]ExtendedAdvisory From bc8dd509bb52448ce65102b33810d278c519feff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Thu, 27 Jun 2024 12:13:39 +0200 Subject: [PATCH 05/15] RHINENG-8141: remove unused code, propagate, and improve --- evaluator/evaluate.go | 55 ++-- evaluator/evaluate_advisories.go | 406 ++++++++------------------ evaluator/evaluate_advisories_test.go | 108 +++---- 3 files changed, 203 insertions(+), 366 deletions(-) diff --git a/evaluator/evaluate.go b/evaluator/evaluate.go index de6380d5d..ffa5d87e9 100644 --- a/evaluator/evaluate.go +++ b/evaluator/evaluate.go @@ -227,10 +227,6 @@ func evaluateInDatabase(ctx context.Context, event *mqueue.PlatformEvent, invent return nil, nil, nil } - // load and evaluate advisories for system - // posunut spred `updateAdvisoryAccountData` - // update in `storeAdvisoryData` - vmaasData, err := evaluateWithVmaas(updatesData, system, event) if err != nil { return nil, nil, errors.Wrap(err, "evaluation with vmaas failed") @@ -251,7 +247,7 @@ func tryGetYumUpdates(system *models.SystemPlatform) (*vmaas.UpdatesV3Response, } updatesMap := resp.GetUpdateList() if len(updatesMap) == 0 { - // TODO: do we need evaluationCnt.WithLabelValues("error-no-yum-packages").Inc()? + // FIXME: do we need evaluationCnt.WithLabelValues("error-no-yum-packages").Inc()? utils.LogWarn("inventoryID", system.GetInventoryID(), "No yum_updates") return nil, nil } @@ -453,12 +449,7 @@ func commitWithObserve(tx *gorm.DB) error { func evaluateAndStore(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response, event *mqueue.PlatformEvent) error { - // deleteIDs, installableIDs, applicableIDs, err := lazySaveAndLoadAdvisories(system, vmaasData) - // if err != nil { - // return errors.Wrap(err, "Advisory loading failed") - // } - - advisoriesByName, err := lazySaveAndLoadAdvisories2(system, vmaasData) + advisoriesByName, err := lazySaveAndLoadAdvisories(system, vmaasData) if err != nil { return errors.Wrap(err, "Advisory loading failed") } @@ -472,12 +463,7 @@ func evaluateAndStore(system *models.SystemPlatform, // Don't allow requested TX to hang around locking the rows defer tx.Rollback() - // systemAdvisoriesNew, err := storeAdvisoryData(tx, system, deleteIDs, installableIDs, applicableIDs) - // if err != nil { - // evaluationCnt.WithLabelValues("error-store-advisories").Inc() - // return errors.Wrap(err, "Unable to store advisory data") - // } - systemAdvisoriesNew, err := storeAdvisoryData2(tx, system, advisoriesByName) // TODO: what to do with `systemAdvNew`? + systemAdvisoriesNew, err := storeAdvisoryData(tx, system, advisoriesByName) if err != nil { evaluationCnt.WithLabelValues("error-store-advisories").Inc() return errors.Wrap(err, "Unable to store advisory data") @@ -538,9 +524,20 @@ func analyzeRepos(system *models.SystemPlatform) (thirdParty bool, err error) { return thirdParty, nil } +func incrementAdvisoryTypeCounts(advisory models.AdvisoryMetadata, enhCount, bugCount, secCount *int) { + switch advisory.AdvisoryTypeID { + case Enhancement: + *enhCount++ + case Bugfix: + *bugCount++ + case Security: + *secCount++ + } +} + // nolint: funlen func updateSystemPlatform(tx *gorm.DB, system *models.SystemPlatform, - new SystemAdvisoryMap, installed, installable, applicable int) error { + advisories SystemAdvisoryMap, installed, installable, applicable int) error { tStart := time.Now() defer utils.ObserveSecondsSince(tStart, evaluationPartDuration.WithLabelValues("system-update")) defer utils.ObserveSecondsSince(*system.LastUpload, uploadEvaluationDelay) @@ -552,7 +549,7 @@ func updateSystemPlatform(tx *gorm.DB, system *models.SystemPlatform, data["last_evaluation"] = time.Now() if enableAdvisoryAnalysis { - if new == nil { + if advisories == nil { return errors.New("Invalid args") } installableCount := 0 @@ -563,26 +560,12 @@ func updateSystemPlatform(tx *gorm.DB, system *models.SystemPlatform, applicableEnhCount := 0 applicableBugCount := 0 applicableSecCount := 0 - for _, sa := range new { + for _, sa := range advisories { if sa.StatusID == INSTALLABLE { - switch sa.Advisory.AdvisoryTypeID { - case 1: - installableEnhCount++ - case 2: - installableBugCount++ - case 3: - installableSecCount++ - } + incrementAdvisoryTypeCounts(sa.Advisory, &installableEnhCount, &installableBugCount, &installableSecCount) installableCount++ } - switch sa.Advisory.AdvisoryTypeID { - case 1: - applicableEnhCount++ - case 2: - applicableBugCount++ - case 3: - applicableSecCount++ - } + incrementAdvisoryTypeCounts(sa.Advisory, &applicableEnhCount, &applicableBugCount, &applicableSecCount) applicableCount++ } diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index 28eed2b69..d352ac4aa 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -13,39 +13,23 @@ import ( ) func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( - []int64, []int64, []int64, error) { - if !enableAdvisoryAnalysis { - utils.LogInfo("advisory analysis disabled, skipping lazy saving and loading") - return nil, nil, nil, nil - } - deleteIDs, installableIDs, applicableIDs, err := processSystemAdvisories(system, vmaasData) - if err != nil { - evaluationCnt.WithLabelValues("error-process-advisories").Inc() - return nil, nil, nil, errors.Wrap(err, "unable to process system advisories") - } - - return deleteIDs, installableIDs, applicableIDs, err -} - -func lazySaveAndLoadAdvisories2(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( ExtendedAdvisoryMap, error) { if !enableAdvisoryAnalysis { utils.LogInfo("advisory analysis disabled, skipping lazy saving and loading") return nil, nil } - // TODO: should this first evaluate missing and lazy-save just the missing? - err := lazySaveAdvisories2(vmaasData, system.InventoryID) + err := lazySaveAdvisories(vmaasData, system.InventoryID) if err != nil { return nil, errors.Wrap(err, "Unable to store unknown advisories in DB") } - stored, err := loadSystemAdvisories(database.Db, system.RhAccountID, system.ID) + stored, err := loadSystemAdvisories(database.DB, system.RhAccountID, system.ID) if err != nil { return nil, errors.Wrap(err, "Unable to load system advisories") } - merged, err := evaluateChanges(vmaasData, stored, system.RhAccountID, system.ID) + merged, err := evaluateChanges(vmaasData, stored) if err != nil { return nil, errors.Wrap(err, "Unable to evaluate advisory changes") } @@ -53,20 +37,13 @@ func lazySaveAndLoadAdvisories2(system *models.SystemPlatform, vmaasData *vmaas. return merged, nil } -func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMap, accountID int, systemID int64) ( - ExtendedAdvisoryMap, error) { - reported := getReportedAdvisories(vmaasData) - - // TODO: check this func for errors and efficiency - - // -> map[string]int: "erratum -> helper column to diferentiate installable/applicable" - // --> moze sa porovnavat if statusID == INSTALLABLE {...} else {APPLICABLE...} - +func pasrseReported(stored SystemAdvisoryMap, reported map[string]int) (ExtendedAdvisoryMap, []string) { extendedAdvisories := make(ExtendedAdvisoryMap, len(reported)+len(stored)) missingNames := make([]string, 0, len(reported)) for reportedName, reportedStatusID := range reported { if storedAdvisory, found := stored[reportedName]; found { if reportedStatusID != storedAdvisory.StatusID { + storedAdvisory.StatusID = reportedStatusID extendedAdvisories[reportedName] = ExtendedAdvisory{ Change: Update, SystemAdvisories: storedAdvisory, @@ -81,22 +58,23 @@ func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMa extendedAdvisories[reportedName] = ExtendedAdvisory{ Change: Add, SystemAdvisories: models.SystemAdvisories{ - RhAccountID: accountID, - SystemID: systemID, - StatusID: reportedStatusID, + StatusID: reportedStatusID, }, } missingNames = append(missingNames, reportedName) } } + return extendedAdvisories, missingNames +} +func loadMissingNamesIDs(missingNames []string, extendedAdvisories *ExtendedAdvisoryMap) error { advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(missingNames)) - err := database.Db.Model(&models.AdvisoryMetadata{}). + err := database.DB.Model(&models.AdvisoryMetadata{}). Where("name IN (?)", missingNames). Select("id, name"). Scan(&advisoryMetadata).Error if err != nil { - return nil, err + return err } name2AdvisoryID := make(map[string]int64, len(missingNames)) @@ -106,33 +84,54 @@ func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMa for _, name := range missingNames { if _, found := name2AdvisoryID[name]; !found { - return nil, errors.New("Failed to evaluate changes because an advisory was not lazy-saved") + return errors.New("Failed to evaluate changes because an advisory was not lazy-saved") } - extendedAdvisory := extendedAdvisories[name] + extendedAdvisory := (*extendedAdvisories)[name] extendedAdvisory.AdvisoryID = name2AdvisoryID[name] - extendedAdvisories[name] = extendedAdvisory + (*extendedAdvisories)[name] = extendedAdvisory } + return nil +} + +// Set change for advisories that are in `stored` but not in `reported` to Remove. +func parseStored(stored SystemAdvisoryMap, reported map[string]int, extendedAdvisories *ExtendedAdvisoryMap) { for storedName, storedAdvisory := range stored { if _, found := reported[storedName]; !found { - extendedAdvisories[storedName] = ExtendedAdvisory{ + (*extendedAdvisories)[storedName] = ExtendedAdvisory{ Change: Remove, SystemAdvisories: storedAdvisory, } } } +} + +// Evaluate changes to all advisories based on `stored` advisories from DB and `reported` advisories from VMaaS. +func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMap) ( + ExtendedAdvisoryMap, error) { + reported := getReportedAdvisories(vmaasData) + extendedAdvisories, missingNames := pasrseReported(stored, reported) + + err := loadMissingNamesIDs(missingNames, &extendedAdvisories) + if err != nil { + return nil, err + } + + parseStored(stored, reported, &extendedAdvisories) return extendedAdvisories, nil } -func lazySaveAdvisories2(vmaasData *vmaas.UpdatesV3Response, inventoryID string) error { +// Find missing advisories from names reported by VMaaS and lazy-save them. +func lazySaveAdvisories(vmaasData *vmaas.UpdatesV3Response, inventoryID string) error { // -> load reported advisories, advisories to lazy-save can only appear in VmaasData reportedNames := getReportedAdvisoryNames(vmaasData) if len(reportedNames) < 1 { return nil } + // -> get missing from reported - missingNames, err := getMissingAdvisories(reportedNames) // namiesto getAdvisoriesFromDB + missingNames, err := getMissingAdvisories(reportedNames) if err != nil { return errors.Wrap(err, "Unable to get missing system advisories") } @@ -145,15 +144,15 @@ func lazySaveAdvisories2(vmaasData *vmaas.UpdatesV3Response, inventoryID string) return nil } // -> store missing advisories - err = storeMissingAdvisories2(missingNames) + err = storeMissingAdvisories(missingNames) if err != nil { - return errors.Wrap(err, "failed to save missing advisory_metadata") + return errors.Wrap(err, "Failed to save missing advisory_metadata") } return nil } -func storeMissingAdvisories2(missingNames []string) error { +func storeMissingAdvisories(missingNames []string) error { toStore := make(models.AdvisoryMetadataSlice, 0, len(missingNames)) for _, name := range missingNames { if len(name) > 0 && len(name) < 100 { @@ -169,21 +168,22 @@ func storeMissingAdvisories2(missingNames []string) error { } } - var err error if len(toStore) > 0 { tx := database.DB.Begin() defer tx.Commit() - err = tx.Clauses(clause.OnConflict{DoNothing: true}).Create(&toStore).Error + err := tx.Clauses(clause.OnConflict{DoNothing: true}).Create(&toStore).Error if err != nil { return err } - // TODO: created toStore objects will have hadded a .ID } + return nil } -// Determine if advisories from DB are propperly stored based on advisory metadata existance. +// Determine if advisories from DB are properly stored based on advisory metadata existence. func getMissingAdvisories(advisoryNames []string) ([]string, error) { + utils.LogInfo("reported", len(advisoryNames), "reported advisories") + advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(advisoryNames)) err := database.DB.Model(&models.AdvisoryMetadata{}). Where("name IN (?)", advisoryNames). @@ -192,6 +192,7 @@ func getMissingAdvisories(advisoryNames []string) ([]string, error) { if err != nil { return nil, err } + utils.LogInfo("advisory_metadata", len(advisoryMetadata), "found advisory metadata") found := make(map[string]bool, len(advisoryNames)) for _, am := range advisoryMetadata { @@ -204,255 +205,61 @@ func getMissingAdvisories(advisoryNames []string) ([]string, error) { missingNames = append(missingNames, name) } } - return missingNames, nil -} - -// Changes data stored in system_advisories, in order to match newest evaluation -// Before this methods stores the entries into the system_advisories table, it locks -// advisory_account_data table, so other evaluations don't interfere with this one -func processSystemAdvisories(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( - []int64, []int64, []int64, error) { - tStart := time.Now() - defer utils.ObserveSecondsSince(tStart, evaluationPartDuration.WithLabelValues("advisories-processing")) - - deleteIDs, installableNames, applicableNames, err := getAdvisoryChanges(system, vmaasData) - if err != nil { - return nil, nil, nil, errors.Wrap(err, "Unable to get system advisory change") - } - updatesCnt.WithLabelValues("patched").Add(float64(len(deleteIDs))) - utils.LogInfo("inventoryID", system.InventoryID, "fixed", len(deleteIDs), "fixed advisories") - - installableIDs, missingInstallableNames, err := getAdvisoriesFromDB(installableNames, system.InventoryID) - if err != nil { - return nil, nil, nil, errors.Wrap(err, "Unable to ensure new installable system advisories in db") - } - missingInstallableIDs, err := storeMissingAdvisories(missingInstallableNames) - if err != nil { - return nil, nil, nil, errors.Wrap(err, "unable to store unknown installable advisories in db") - } - installableIDs = append(installableIDs, missingInstallableIDs...) - updatesCnt.WithLabelValues("installable").Add(float64(len(installableIDs))) - utils.LogInfo("inventoryID", system.InventoryID, "installable", len(installableIDs), "installable advisories") - - applicableIDs, missingApplicableNames, err := getAdvisoriesFromDB(applicableNames, system.InventoryID) - if err != nil { - return nil, nil, nil, errors.Wrap(err, "Unable to ensure new applicable system advisories in db") - } - - missingApplicableIDs, err := storeMissingAdvisories(missingApplicableNames) - if err != nil { - return nil, nil, nil, errors.Wrap(err, "unable to store unknown applicable advisories in db") - } - applicableIDs = append(applicableIDs, missingApplicableIDs...) - updatesCnt.WithLabelValues("applicable").Add(float64(len(applicableIDs))) - utils.LogInfo("inventoryID", system.InventoryID, "applicable", len(applicableIDs), "applicable advisories") - - return deleteIDs, installableIDs, applicableIDs, nil -} - -func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, - deleteIDs, installableIDs, applicableIDs []int64) (SystemAdvisoryMap, error) { - if !enableAdvisoryAnalysis { - utils.LogInfo("advisory analysis disabled, skipping storing") - return nil, nil - } - - defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("advisories-store")) - err := updateSystemAdvisoriesTODO(tx, system, deleteIDs, installableIDs, applicableIDs) - if err != nil { - return nil, errors.Wrap(err, "Unable to update system advisories") - } - systemAdvisoriesNew, err := loadSystemAdvisories(tx, system.RhAccountID, system.ID) // reload system advisories after update - if err != nil { - return nil, errors.Wrap(err, "Unable to load new system advisories") - } - - err = updateAdvisoryAccountData(tx, system, deleteIDs, installableIDs, applicableIDs) - if err != nil { - return nil, errors.Wrap(err, "Unable to update advisory_account_data caches") - } - return systemAdvisoriesNew, nil + return missingNames, nil } -func storeAdvisoryData2(tx *gorm.DB, system *models.SystemPlatform, advisoriesByName ExtendedAdvisoryMap) ( +func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, advisoriesByName ExtendedAdvisoryMap) ( SystemAdvisoryMap, error) { if !enableAdvisoryAnalysis { utils.LogInfo("advisory analysis disabled, skipping storing") return nil, nil } - // TODO: toto prerobit, aby sa spravit DB update a zaroven aj update v `advisoriesByName` - // => zbavime sa znovunacitania - // => co s updateAdvisoryAccountData? - defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("advisories-store")) - err := updateSystemAdvisoriesTODO(tx, system, deleteIDs, installableIDs, applicableIDs) + deleteIDs, systemAdvisoriesNew, err := updateSystemAdvisories(tx, system, advisoriesByName) if err != nil { return nil, errors.Wrap(err, "Unable to update system advisories") } - systemAdvisoriesNew, err := loadSystemAdvisories(tx, system.RhAccountID, system.ID) // reload system advisories after update - if err != nil { - return nil, errors.Wrap(err, "Unable to load new system advisories") - } - err = updateAdvisoryAccountData(tx, system, deleteIDs, installableIDs, applicableIDs) + err = updateAdvisoryAccountData(tx, system, deleteIDs, systemAdvisoriesNew) if err != nil { return nil, errors.Wrap(err, "Unable to update advisory_account_data caches") } return systemAdvisoriesNew, nil } -func getAdvisoryChanges(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( - []int64, []string, []string, error) { - reported := getReportedAdvisories(vmaasData) - stored, err := loadSystemAdvisories(database.DB, system.RhAccountID, system.ID) - if err != nil { - return nil, nil, nil, errors.Wrap(err, "Unable to get system stored advisories") - } - - installableNames := make([]string, 0, len(reported)) - applicableNames := make([]string, 0, len(reported)) - deleteIDs := make([]int64, 0, len(stored)) - for reportedAdvisory, statusID := range reported { - if advisory, found := stored[reportedAdvisory]; !found || advisory.StatusID != statusID { - if statusID == INSTALLABLE { - installableNames = append(installableNames, reportedAdvisory) - } else { - applicableNames = append(applicableNames, reportedAdvisory) - } - } - } - for storedAdvisoryName, storedAdvisory := range stored { - if _, found := reported[storedAdvisoryName]; !found { - // advisory was patched from last evaluation,let's remove it - deleteIDs = append(deleteIDs, storedAdvisory.AdvisoryID) - } - } - return deleteIDs, installableNames, applicableNames, nil -} - -func getAdvisoriesFromDB(advisoryNames []string, inventoryID string) ([]int64, []string, error) { - advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(advisoryNames)) - err := database.DB.Model(&models.AdvisoryMetadata{}). - Where("name IN (?)", advisoryNames). - Select("id, name"). - Scan(&advisoryMetadata).Error - if err != nil { - return nil, nil, err - } - - found := make(map[string]bool, len(advisoryNames)) - advisoryIDs := make([]int64, 0, len(advisoryNames)) - for _, am := range advisoryMetadata { - found[am.Name] = true - advisoryIDs = append(advisoryIDs, am.ID) - } - nUnknown := len(advisoryNames) - len(advisoryIDs) - if nUnknown > 0 { - utils.LogInfo("inventoryID", inventoryID, "unknown", nUnknown, "unknown advisories") - updatesCnt.WithLabelValues("unknown").Add(float64(nUnknown)) - } - missingAdvisoryNames := make([]string, 0, nUnknown) - for _, name := range advisoryNames { - if !found[name] { - missingAdvisoryNames = append(missingAdvisoryNames, name) - } - } - return advisoryIDs, missingAdvisoryNames, err -} - -func storeMissingAdvisories(missingNames []string) ([]int64, error) { - toStore := make(models.AdvisoryMetadataSlice, 0, len(missingNames)) - for _, name := range missingNames { - if len(name) > 0 && len(name) < 100 { - toStore = append(toStore, models.AdvisoryMetadata{ - Name: name, - Description: "Not Available", - Synopsis: "Not Available", - Summary: "Not Available", - AdvisoryTypeID: 0, - RebootRequired: false, - Synced: false, - }) - } - } - storedIDs, err := lazySaveAdvisories(toStore) - if err != nil { - return nil, errors.Wrap(err, "failed to save advisory_metadata") - } - return storedIDs, nil -} - -func lazySaveAdvisories(missing models.AdvisoryMetadataSlice) ([]int64, error) { - var err error - ret := make([]int64, 0, len(missing)) - if len(missing) > 0 { - tx := database.DB.Begin() - defer tx.Commit() - err = tx.Clauses(clause.OnConflict{DoNothing: true}).Create(&missing).Error - if err != nil { - return ret, err - } - for _, x := range missing { - ret = append(ret, x.ID) - } - } - return ret, nil -} - -func ensureSystemAdvisories(tx *gorm.DB, rhAccountID int, systemID int64, installableIDs, - applicableIDs []int64) error { - advisoriesObjs := make(models.SystemAdvisoriesSlice, 0, len(installableIDs)+len(applicableIDs)) - for _, advisoryID := range installableIDs { - advisoriesObjs = append(advisoriesObjs, - models.SystemAdvisories{RhAccountID: rhAccountID, - SystemID: systemID, - AdvisoryID: advisoryID, - StatusID: INSTALLABLE}) - } - for _, advisoryID := range applicableIDs { - advisoriesObjs = append(advisoriesObjs, - models.SystemAdvisories{RhAccountID: rhAccountID, - SystemID: systemID, - AdvisoryID: advisoryID, - StatusID: APPLICABLE}) - } - - tx = database.OnConflictUpdateMulti(tx, []string{"rh_account_id", "system_id", "advisory_id"}, "status_id") - err := database.BulkInsert(tx, advisoriesObjs) - return err -} - -func calcAdvisoryChanges(system *models.SystemPlatform, deleteIDs, installableIDs, - applicableIDs []int64) []models.AdvisoryAccountData { +func calcAdvisoryChanges(system *models.SystemPlatform, deleteIDs []int64, + systemAdvisories SystemAdvisoryMap) []models.AdvisoryAccountData { // If system is stale, we won't change any rows in advisory_account_data if system.Stale { return []models.AdvisoryAccountData{} } - aadMap := make(map[int64]models.AdvisoryAccountData, len(installableIDs)) - - for _, id := range installableIDs { - aadMap[id] = models.AdvisoryAccountData{ - AdvisoryID: id, - RhAccountID: system.RhAccountID, - SystemsInstallable: 1, - // every installable advisory is also applicable advisory - SystemsApplicable: 1, - } - } - - isApplicable := make(map[int64]bool, len(applicableIDs)) - for _, id := range applicableIDs { - isApplicable[id] = true - // add advisories which are only applicable and not installable to aad - if _, ok := aadMap[id]; !ok { - aadMap[id] = models.AdvisoryAccountData{ - AdvisoryID: id, - RhAccountID: system.RhAccountID, + aadMap := make(map[int64]models.AdvisoryAccountData, len(systemAdvisories)) + isApplicable := make(map[int64]bool, len(systemAdvisories)) + for _, advisory := range systemAdvisories { + isApplicable[advisory.AdvisoryID] = true + if advisory.StatusID == INSTALLABLE { + aadMap[advisory.AdvisoryID] = models.AdvisoryAccountData{ + AdvisoryID: advisory.AdvisoryID, + RhAccountID: system.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.RhAccountID, + SystemsApplicable: 1, + } + } } } @@ -470,7 +277,7 @@ func calcAdvisoryChanges(system *models.SystemPlatform, deleteIDs, installableID } } - deltas := make([]models.AdvisoryAccountData, 0, len(deleteIDs)+len(installableIDs)+len(applicableIDs)) + deltas := make([]models.AdvisoryAccountData, 0, len(deleteIDs)+len(systemAdvisories)) for _, aad := range aadMap { deltas = append(deltas, aad) } @@ -485,20 +292,56 @@ func deleteOldSystemAdvisories(tx *gorm.DB, accountID int, systemID int64, patch return err } -func updateSystemAdvisoriesTODO(tx *gorm.DB, system *models.SystemPlatform, - deleteIDs, installableIDs, applicableIDs []int64) error { +func upsertSystemAdvisories(tx *gorm.DB, advisoryObjs models.SystemAdvisoriesSlice) error { + tx = database.OnConflictUpdateMulti(tx, []string{"rh_account_id", "system_id", "advisory_id"}, "status_id") + return database.BulkInsert(tx, advisoryObjs) +} + +func processAdvisories(system *models.SystemPlatform, advisoriesByName ExtendedAdvisoryMap) ([]int64, + models.SystemAdvisoriesSlice, SystemAdvisoryMap) { + deleteIDs := make([]int64, 0, len(advisoriesByName)) + advisoryObjs := make(models.SystemAdvisoriesSlice, 0, len(advisoriesByName)) + updatedAdvisories := make(SystemAdvisoryMap, len(advisoriesByName)) + for name, advisory := range advisoriesByName { + switch advisory.Change { + case Remove: + deleteIDs = append(deleteIDs, advisory.AdvisoryID) + case Update: + // StatusID already changed in `evaluateChanges` to reportedStatusID + fallthrough + case Add: + adv := models.SystemAdvisories{ + RhAccountID: system.RhAccountID, + SystemID: system.ID, + AdvisoryID: advisory.AdvisoryID, + Advisory: advisory.Advisory, + StatusID: advisory.StatusID, + } + advisoryObjs = append(advisoryObjs, adv) + updatedAdvisories[name] = adv + case Keep: + updatedAdvisories[name] = advisory.SystemAdvisories + } + } + return deleteIDs, advisoryObjs, updatedAdvisories +} + +func updateSystemAdvisories(tx *gorm.DB, system *models.SystemPlatform, + advisoriesByName ExtendedAdvisoryMap) ([]int64, SystemAdvisoryMap, error) { + deleteIDs, advisoryObjs, updatedAdvisories := processAdvisories(system, advisoriesByName) + // this will remove many many old items from our "system_advisories" table err := deleteOldSystemAdvisories(tx, system.RhAccountID, system.ID, deleteIDs) if err != nil { - return err + return nil, nil, err } - err = ensureSystemAdvisories(tx, system.RhAccountID, system.ID, installableIDs, applicableIDs) + err = upsertSystemAdvisories(tx, advisoryObjs) if err != nil { - return err + return nil, nil, err } - return nil + return deleteIDs, updatedAdvisories, nil } func loadSystemAdvisories(tx *gorm.DB, accountID int, systemID int64) (SystemAdvisoryMap, error) { @@ -515,9 +358,9 @@ func loadSystemAdvisories(tx *gorm.DB, accountID int, systemID int64) (SystemAdv return systemAdvisories, nil } -func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, deleteIDs, installableIDs, - applicableIDs []int64) error { - changes := calcAdvisoryChanges(system, deleteIDs, installableIDs, applicableIDs) +func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, + deleteIDs []int64, systemAdvisories SystemAdvisoryMap) error { + changes := calcAdvisoryChanges(system, deleteIDs, systemAdvisories) if len(changes) == 0 { return nil @@ -538,3 +381,10 @@ type ExtendedAdvisory struct { } type ExtendedAdvisoryMap map[string]ExtendedAdvisory + +const ( + UnknownFixme int = iota + Enhancement + Bugfix + Security +) diff --git a/evaluator/evaluate_advisories_test.go b/evaluator/evaluate_advisories_test.go index a3bc89095..5179310d0 100644 --- a/evaluator/evaluate_advisories_test.go +++ b/evaluator/evaluate_advisories_test.go @@ -3,7 +3,6 @@ package evaluator import ( "app/base/core" "app/base/database" - "app/base/models" "app/base/utils" "app/base/vmaas" "context" @@ -83,50 +82,53 @@ func TestGetStoredAdvisoriesMap(t *testing.T) { assert.Equal(t, "2016-09-22 16:00:00 +0000 UTC", (systemAdvisories)["RH-1"].Advisory.PublicDate.String()) } -func TestAdvisoryChanges(t *testing.T) { - stored := database.CreateStoredAdvisories([]int64{1, 2, 3}) - reported := database.CreateReportedAdvisories([]string{"ER-1", "ER-3", "ER-4"}, - []int{INSTALLABLE, INSTALLABLE, INSTALLABLE}) - patchedAIDs, installableNames, applicableNames := getAdvisoryChanges(reported, stored) - assert.Equal(t, 1, len(installableNames)) - assert.Equal(t, "ER-4", installableNames[0]) - assert.Equal(t, 0, len(applicableNames)) - assert.Equal(t, 1, len(patchedAIDs)) - assert.Equal(t, int64(2), patchedAIDs[0]) -} - -func TestUpdatePatchedSystemAdvisories(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - - system := models.SystemPlatform{ID: 12, RhAccountID: 3} - advisoryIDs := []int64{2, 3, 4} - database.CreateSystemAdvisories(t, system.RhAccountID, system.ID, advisoryIDs) - database.CreateAdvisoryAccountData(t, system.RhAccountID, advisoryIDs, 1) - // Update as-if the advisories had become patched - err := updateAdvisoryAccountData(database.DB, &system, advisoryIDs, []int64{}, []int64{}) - assert.NoError(t, err) - - database.CheckSystemAdvisories(t, system.ID, advisoryIDs) - database.CheckAdvisoriesAccountData(t, system.RhAccountID, advisoryIDs, 0) - - // Update as-if the advisories had become unpatched - err = updateAdvisoryAccountData(database.DB, &system, []int64{}, advisoryIDs, []int64{}) - assert.NoError(t, err) - - database.CheckAdvisoriesAccountData(t, system.RhAccountID, advisoryIDs, 1) - database.DeleteSystemAdvisories(t, system.ID, advisoryIDs) - database.DeleteAdvisoryAccountData(t, system.RhAccountID, advisoryIDs) -} +// FIXME +// func TestAdvisoryChanges(t *testing.T) { +// stored := database.CreateStoredAdvisories([]int64{1, 2, 3}) +// reported := database.CreateReportedAdvisories([]string{"ER-1", "ER-3", "ER-4"}, +// []int{INSTALLABLE, INSTALLABLE, INSTALLABLE}) +// patchedAIDs, installableNames, applicableNames := getAdvisoryChanges(reported, stored) +// assert.Equal(t, 1, len(installableNames)) +// assert.Equal(t, "ER-4", installableNames[0]) +// assert.Equal(t, 0, len(applicableNames)) +// assert.Equal(t, 1, len(patchedAIDs)) +// assert.Equal(t, int64(2), patchedAIDs[0]) +// } + +// FIXME +// func TestUpdatePatchedSystemAdvisories(t *testing.T) { +// utils.SkipWithoutDB(t) +// core.SetupTestEnvironment() + +// system := models.SystemPlatform{ID: 12, RhAccountID: 3} +// advisoryIDs := []int64{2, 3, 4} +// database.CreateSystemAdvisories(t, system.RhAccountID, system.ID, advisoryIDs) +// database.CreateAdvisoryAccountData(t, system.RhAccountID, advisoryIDs, 1) +// // Update as-if the advisories had become patched +// err := updateAdvisoryAccountData(database.DB, &system, advisoryIDs, []int64{}, []int64{}) +// assert.NoError(t, err) + +// database.CheckSystemAdvisories(t, system.ID, advisoryIDs) +// database.CheckAdvisoriesAccountData(t, system.RhAccountID, advisoryIDs, 0) + +// // Update as-if the advisories had become unpatched +// err = updateAdvisoryAccountData(database.DB, &system, []int64{}, advisoryIDs, []int64{}) +// assert.NoError(t, err) + +// database.CheckAdvisoriesAccountData(t, system.RhAccountID, advisoryIDs, 1) +// database.DeleteSystemAdvisories(t, system.ID, advisoryIDs) +// database.DeleteAdvisoryAccountData(t, system.RhAccountID, advisoryIDs) +// } func TestGetAdvisoriesFromDB(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() advisories := []string{"ER-1", "RH-1", "ER-2", "RH-2"} - advisoryIDs, missingNames, err := getAdvisoriesFromDB(advisories) + // advisoryIDs, missingNames, err := getAdvisoriesFromDB(advisories) + missingNames, err := getMissingAdvisories(advisories) assert.Nil(t, err) - assert.Equal(t, 2, len(advisoryIDs)) + // assert.Equal(t, 2, len(advisoryIDs)) // FIXME assert.Equal(t, 2, len(missingNames)) } @@ -135,24 +137,26 @@ func TestGetAdvisoriesFromDBEmptyString(t *testing.T) { core.SetupTestEnvironment() advisories := []string{""} - advisoryIDs, missingNames, err := getAdvisoriesFromDB(advisories) + // advisoryIDs, missingNames, err := getAdvisoriesFromDB(advisories) + missingNames, err := getMissingAdvisories(advisories) assert.Nil(t, err) - assert.Equal(t, 0, len(advisoryIDs)) + // assert.Equal(t, 0, len(advisoryIDs)) // FIXME assert.Equal(t, 1, len(missingNames)) } -func TestEnsureSystemAdvisories(t *testing.T) { - utils.SkipWithoutDB(t) - core.SetupTestEnvironment() - - rhAccountID := 1 - systemID := int64(2) - advisoryIDs := []int64{2, 3, 4} - err := ensureSystemAdvisories(database.DB, rhAccountID, systemID, advisoryIDs, []int64{}) - assert.Nil(t, err) - database.CheckSystemAdvisories(t, systemID, advisoryIDs) - database.DeleteSystemAdvisories(t, systemID, advisoryIDs) -} +// FIXME +// func TestEnsureSystemAdvisories(t *testing.T) { +// utils.SkipWithoutDB(t) +// core.SetupTestEnvironment() + +// rhAccountID := 1 +// systemID := int64(2) +// advisoryIDs := []int64{2, 3, 4} +// err := ensureSystemAdvisories(database.DB, rhAccountID, systemID, advisoryIDs, []int64{}) +// assert.Nil(t, err) +// database.CheckSystemAdvisories(t, systemID, advisoryIDs) +// database.DeleteSystemAdvisories(t, systemID, advisoryIDs) +// } func getVMaaSUpdates(t *testing.T) vmaas.UpdatesV3Response { ctx := context.Background() From 70745cd2f776aa1c1ec649fcdfb629808a0dc292 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Thu, 4 Jul 2024 11:04:23 +0200 Subject: [PATCH 06/15] RHINENG-8141: fix and add tests --- evaluator/evaluate_advisories.go | 10 +- evaluator/evaluate_advisories_test.go | 169 +++++++++++++++++--------- 2 files changed, 116 insertions(+), 63 deletions(-) diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index d352ac4aa..f2a027c96 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -24,7 +24,7 @@ func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.U return nil, errors.Wrap(err, "Unable to store unknown advisories in DB") } - stored, err := loadSystemAdvisories(database.DB, system.RhAccountID, system.ID) + stored, err := loadSystemAdvisories(system.RhAccountID, system.ID) if err != nil { return nil, errors.Wrap(err, "Unable to load system advisories") } @@ -175,6 +175,7 @@ func storeMissingAdvisories(missingNames []string) error { if err != nil { return err } + // after creation, toStore will include newly added .ID attributes } return nil @@ -182,8 +183,6 @@ func storeMissingAdvisories(missingNames []string) error { // Determine if advisories from DB are properly stored based on advisory metadata existence. func getMissingAdvisories(advisoryNames []string) ([]string, error) { - utils.LogInfo("reported", len(advisoryNames), "reported advisories") - advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(advisoryNames)) err := database.DB.Model(&models.AdvisoryMetadata{}). Where("name IN (?)", advisoryNames). @@ -192,7 +191,6 @@ func getMissingAdvisories(advisoryNames []string) ([]string, error) { if err != nil { return nil, err } - utils.LogInfo("advisory_metadata", len(advisoryMetadata), "found advisory metadata") found := make(map[string]bool, len(advisoryNames)) for _, am := range advisoryMetadata { @@ -344,9 +342,9 @@ func updateSystemAdvisories(tx *gorm.DB, system *models.SystemPlatform, return deleteIDs, updatedAdvisories, nil } -func loadSystemAdvisories(tx *gorm.DB, accountID int, systemID int64) (SystemAdvisoryMap, error) { +func loadSystemAdvisories(accountID int, systemID int64) (SystemAdvisoryMap, error) { var data []models.SystemAdvisories - err := tx.Preload("Advisory").Find(&data, "system_id = ? AND rh_account_id = ?", systemID, accountID).Error + err := database.DB.Preload("Advisory").Find(&data, "system_id = ? AND rh_account_id = ?", systemID, accountID).Error if err != nil { return nil, err } diff --git a/evaluator/evaluate_advisories_test.go b/evaluator/evaluate_advisories_test.go index 5179310d0..c66e48634 100644 --- a/evaluator/evaluate_advisories_test.go +++ b/evaluator/evaluate_advisories_test.go @@ -3,9 +3,11 @@ package evaluator import ( "app/base/core" "app/base/database" + "app/base/models" "app/base/utils" "app/base/vmaas" "context" + "fmt" "net/http" "strings" "testing" @@ -73,7 +75,7 @@ func TestGetStoredAdvisoriesMap(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() - systemAdvisories, err := loadSystemAdvisories(database.DB, 1, 1) + systemAdvisories, err := loadSystemAdvisories(1, 1) assert.Nil(t, err) assert.NotNil(t, systemAdvisories) assert.Equal(t, 8, len(systemAdvisories)) @@ -82,81 +84,117 @@ func TestGetStoredAdvisoriesMap(t *testing.T) { assert.Equal(t, "2016-09-22 16:00:00 +0000 UTC", (systemAdvisories)["RH-1"].Advisory.PublicDate.String()) } -// FIXME -// func TestAdvisoryChanges(t *testing.T) { -// stored := database.CreateStoredAdvisories([]int64{1, 2, 3}) -// reported := database.CreateReportedAdvisories([]string{"ER-1", "ER-3", "ER-4"}, -// []int{INSTALLABLE, INSTALLABLE, INSTALLABLE}) -// patchedAIDs, installableNames, applicableNames := getAdvisoryChanges(reported, stored) -// assert.Equal(t, 1, len(installableNames)) -// assert.Equal(t, "ER-4", installableNames[0]) -// assert.Equal(t, 0, len(applicableNames)) -// assert.Equal(t, 1, len(patchedAIDs)) -// assert.Equal(t, int64(2), patchedAIDs[0]) -// } - -// FIXME -// func TestUpdatePatchedSystemAdvisories(t *testing.T) { -// utils.SkipWithoutDB(t) -// core.SetupTestEnvironment() - -// system := models.SystemPlatform{ID: 12, RhAccountID: 3} -// advisoryIDs := []int64{2, 3, 4} -// database.CreateSystemAdvisories(t, system.RhAccountID, system.ID, advisoryIDs) -// database.CreateAdvisoryAccountData(t, system.RhAccountID, advisoryIDs, 1) -// // Update as-if the advisories had become patched -// err := updateAdvisoryAccountData(database.DB, &system, advisoryIDs, []int64{}, []int64{}) -// assert.NoError(t, err) - -// database.CheckSystemAdvisories(t, system.ID, advisoryIDs) -// database.CheckAdvisoriesAccountData(t, system.RhAccountID, advisoryIDs, 0) - -// // Update as-if the advisories had become unpatched -// err = updateAdvisoryAccountData(database.DB, &system, []int64{}, advisoryIDs, []int64{}) -// assert.NoError(t, err) - -// database.CheckAdvisoriesAccountData(t, system.RhAccountID, advisoryIDs, 1) -// database.DeleteSystemAdvisories(t, system.ID, advisoryIDs) -// database.DeleteAdvisoryAccountData(t, system.RhAccountID, advisoryIDs) -// } - -func TestGetAdvisoriesFromDB(t *testing.T) { +func TestAdvisoryChanges(t *testing.T) { + utils.SkipWithoutDB(t) + core.SetupTestEnvironment() + + stored := database.CreateStoredAdvisories([]int64{1, 2, 3}) + // create vmaasData with reported names: "ER-1", "ER-3", and "ER-4" + updates := []vmaas.UpdatesV3ResponseAvailableUpdates{ + {Erratum: utils.PtrString("ER-1"), StatusID: INSTALLABLE}, + {Erratum: utils.PtrString("ER-3"), StatusID: APPLICABLE}, + {Erratum: utils.PtrString("ER-4"), StatusID: INSTALLABLE}, + } + updateList := map[string]*vmaas.UpdatesV3ResponseUpdateList{ + "pkg-a": {AvailableUpdates: &updates}, + } + vmaasData := vmaas.UpdatesV3Response{UpdateList: &updateList} + + // advisories must be lazy saved before evaluating changes + err := lazySaveAdvisories(&vmaasData, inventoryID) + defer database.DeleteNewlyAddedAdvisories(t) + assert.Nil(t, err) + + extendedAdvisories, err := evaluateChanges(&vmaasData, stored) + assert.Nil(t, err) + assert.Equal(t, 4, len(extendedAdvisories)) + + assert.Equal(t, Keep, extendedAdvisories["ER-1"].Change) + assert.Equal(t, Remove, extendedAdvisories["ER-2"].Change) + assert.Equal(t, Update, extendedAdvisories["ER-3"].Change) + assert.Equal(t, Add, extendedAdvisories["ER-4"].Change) +} + +func TestUpdatePatchedSystemAdvisories(t *testing.T) { + utils.SkipWithoutDB(t) + core.SetupTestEnvironment() + + system := models.SystemPlatform{ID: 12, RhAccountID: 3} + advisoryIDs := []int64{2, 3, 4} + database.CreateSystemAdvisories(t, system.RhAccountID, system.ID, advisoryIDs) + database.CreateAdvisoryAccountData(t, system.RhAccountID, advisoryIDs, 1) + + // Update as if the advisories became patched + err := updateAdvisoryAccountData(database.DB, &system, advisoryIDs, SystemAdvisoryMap{}) + assert.NoError(t, err) + database.CheckSystemAdvisories(t, system.ID, advisoryIDs) + database.CheckAdvisoriesAccountData(t, system.RhAccountID, advisoryIDs, 0) + + // Update as if the advisories became unpatched + systemAdvisories := make(SystemAdvisoryMap, len(advisoryIDs)) + for _, id := range advisoryIDs { + systemAdvisories[fmt.Sprintf("ER-%v", id)] = models.SystemAdvisories{AdvisoryID: id} + } + err = updateAdvisoryAccountData(database.DB, &system, []int64{}, systemAdvisories) + assert.NoError(t, err) + database.CheckAdvisoriesAccountData(t, system.RhAccountID, advisoryIDs, 1) + + database.DeleteSystemAdvisories(t, system.ID, advisoryIDs) + database.DeleteAdvisoryAccountData(t, system.RhAccountID, advisoryIDs) +} + +func TestGetMissingAdvisories(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() advisories := []string{"ER-1", "RH-1", "ER-2", "RH-2"} - // advisoryIDs, missingNames, err := getAdvisoriesFromDB(advisories) + advisoryIDs := getAdvisoriesFromDB(advisories) missingNames, err := getMissingAdvisories(advisories) assert.Nil(t, err) - // assert.Equal(t, 2, len(advisoryIDs)) // FIXME + assert.Equal(t, 2, len(advisoryIDs)) assert.Equal(t, 2, len(missingNames)) } -func TestGetAdvisoriesFromDBEmptyString(t *testing.T) { +func TestGetMissingAdvisoriesEmptyString(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() advisories := []string{""} - // advisoryIDs, missingNames, err := getAdvisoriesFromDB(advisories) + advisoryIDs := getAdvisoriesFromDB(advisories) missingNames, err := getMissingAdvisories(advisories) assert.Nil(t, err) - // assert.Equal(t, 0, len(advisoryIDs)) // FIXME + assert.Equal(t, 0, len(advisoryIDs)) assert.Equal(t, 1, len(missingNames)) } -// FIXME -// func TestEnsureSystemAdvisories(t *testing.T) { -// utils.SkipWithoutDB(t) -// core.SetupTestEnvironment() +func TestProcessAndUpsertSystemAdvisories(t *testing.T) { + utils.SkipWithoutDB(t) + core.SetupTestEnvironment() + + systemID := int64(2) + system := models.SystemPlatform{RhAccountID: 1, ID: systemID} + extendedAdvisories := ExtendedAdvisoryMap{ + "ER-2": ExtendedAdvisory{Change: Keep, SystemAdvisories: models.SystemAdvisories{ + AdvisoryID: int64(2), + }}, + "ER-3": ExtendedAdvisory{Change: Add, SystemAdvisories: models.SystemAdvisories{ + AdvisoryID: int64(3), + }}, + "ER-4": ExtendedAdvisory{Change: Update, SystemAdvisories: models.SystemAdvisories{ + AdvisoryID: int64(4), + }}, + } + + deleteIDs, advisoryObjs, updatedAdvisories := processAdvisories(&system, extendedAdvisories) + assert.Equal(t, 0, len(deleteIDs)) + assert.Equal(t, 2, len(advisoryObjs)) + assert.Equal(t, len(updatedAdvisories), len(extendedAdvisories)-len(deleteIDs)) -// rhAccountID := 1 -// systemID := int64(2) -// advisoryIDs := []int64{2, 3, 4} -// err := ensureSystemAdvisories(database.DB, rhAccountID, systemID, advisoryIDs, []int64{}) -// assert.Nil(t, err) -// database.CheckSystemAdvisories(t, systemID, advisoryIDs) -// database.DeleteSystemAdvisories(t, systemID, advisoryIDs) -// } + err := upsertSystemAdvisories(database.DB, advisoryObjs) + assert.Nil(t, err) + database.CheckSystemAdvisories(t, systemID, []int64{3, 4}) + database.DeleteSystemAdvisories(t, systemID, []int64{3, 4}) +} func getVMaaSUpdates(t *testing.T) vmaas.UpdatesV3Response { ctx := context.Background() @@ -167,3 +205,20 @@ func getVMaaSUpdates(t *testing.T) vmaas.UpdatesV3Response { assert.Nil(t, resp.Body.Close()) return vmaasData } + +func getAdvisoriesFromDB(advisories []string) []int64 { + advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(advisories)) + err := database.Db.Model(&models.AdvisoryMetadata{}). + Where("name IN (?)", advisories). + Select("id, name"). + Scan(&advisoryMetadata).Error + if err != nil { + return nil + } + + advisoryIDs := make([]int64, 0, len(advisories)) + for _, am := range advisoryMetadata { + advisoryIDs = append(advisoryIDs, am.ID) + } + return advisoryIDs +} From d6eb093648d49ab74c975fcffee97134e04f9115 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Fri, 12 Jul 2024 17:49:53 +0200 Subject: [PATCH 07/15] RHINENG-8141: extract advisory metadata query into a function, renaming, fix and add tests --- evaluator/evaluate.go | 2 +- evaluator/evaluate_advisories.go | 38 +++---- evaluator/evaluate_advisories_test.go | 156 +++++++++++++++++++++----- evaluator/remediations.go | 4 +- evaluator/remediations_test.go | 6 + 5 files changed, 154 insertions(+), 52 deletions(-) diff --git a/evaluator/evaluate.go b/evaluator/evaluate.go index ffa5d87e9..261ff8672 100644 --- a/evaluator/evaluate.go +++ b/evaluator/evaluate.go @@ -247,7 +247,7 @@ func tryGetYumUpdates(system *models.SystemPlatform) (*vmaas.UpdatesV3Response, } updatesMap := resp.GetUpdateList() if len(updatesMap) == 0 { - // FIXME: do we need evaluationCnt.WithLabelValues("error-no-yum-packages").Inc()? + // TODO: do we need evaluationCnt.WithLabelValues("error-no-yum-packages").Inc()? utils.LogWarn("inventoryID", system.GetInventoryID(), "No yum_updates") return nil, nil } diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index f2a027c96..c0df21d15 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -68,11 +68,7 @@ func pasrseReported(stored SystemAdvisoryMap, reported map[string]int) (Extended } func loadMissingNamesIDs(missingNames []string, extendedAdvisories *ExtendedAdvisoryMap) error { - advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(missingNames)) - err := database.DB.Model(&models.AdvisoryMetadata{}). - Where("name IN (?)", missingNames). - Select("id, name"). - Scan(&advisoryMetadata).Error + advisoryMetadata, err := getAdvisoryMetadataByNames(missingNames) if err != nil { return err } @@ -122,20 +118,18 @@ func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMa return extendedAdvisories, nil } -// Find missing advisories from names reported by VMaaS and lazy-save them. +// From names reported by VMaaS, find advisories missing in the DB and lazy-save them. func lazySaveAdvisories(vmaasData *vmaas.UpdatesV3Response, inventoryID string) error { - // -> load reported advisories, advisories to lazy-save can only appear in VmaasData reportedNames := getReportedAdvisoryNames(vmaasData) if len(reportedNames) < 1 { return nil } - // -> get missing from reported missingNames, err := getMissingAdvisories(reportedNames) if err != nil { return errors.Wrap(err, "Unable to get missing system advisories") } - // -> log missing found + nUnknown := len(missingNames) if nUnknown > 0 { utils.LogInfo("inventoryID", inventoryID, "unknown", nUnknown, "unknown advisories") @@ -143,7 +137,7 @@ func lazySaveAdvisories(vmaasData *vmaas.UpdatesV3Response, inventoryID string) } else { return nil } - // -> store missing advisories + err = storeMissingAdvisories(missingNames) if err != nil { return errors.Wrap(err, "Failed to save missing advisory_metadata") @@ -175,19 +169,24 @@ func storeMissingAdvisories(missingNames []string) error { if err != nil { return err } - // after creation, toStore will include newly added .ID attributes + // FIXME: after creation, toStore will include newly added .ID attributes } return nil } -// Determine if advisories from DB are properly stored based on advisory metadata existence. -func getMissingAdvisories(advisoryNames []string) ([]string, error) { - advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(advisoryNames)) +func getAdvisoryMetadataByNames(names []string) (models.AdvisoryMetadataSlice, error) { + metadata := make(models.AdvisoryMetadataSlice, 0, len(names)) err := database.DB.Model(&models.AdvisoryMetadata{}). - Where("name IN (?)", advisoryNames). + Where("name IN (?)", names). Select("id, name"). - Scan(&advisoryMetadata).Error + Scan(&metadata).Error + return metadata, err +} + +// Determine if advisories from DB are properly stored based on advisory metadata existence. +func getMissingAdvisories(advisoryNames []string) ([]string, error) { + advisoryMetadata, err := getAdvisoryMetadataByNames(advisoryNames) if err != nil { return nil, err } @@ -220,6 +219,7 @@ func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, advisoriesByN return nil, errors.Wrap(err, "Unable to update system advisories") } + // FIXME: Advisory account data should calculate changes from the before-update data err = updateAdvisoryAccountData(tx, system, deleteIDs, systemAdvisoriesNew) if err != nil { return nil, errors.Wrap(err, "Unable to update advisory_account_data caches") @@ -235,9 +235,8 @@ func calcAdvisoryChanges(system *models.SystemPlatform, deleteIDs []int64, } aadMap := make(map[int64]models.AdvisoryAccountData, len(systemAdvisories)) - isApplicable := make(map[int64]bool, len(systemAdvisories)) + isApplicableOnly := make(map[int64]bool, len(systemAdvisories)) for _, advisory := range systemAdvisories { - isApplicable[advisory.AdvisoryID] = true if advisory.StatusID == INSTALLABLE { aadMap[advisory.AdvisoryID] = models.AdvisoryAccountData{ AdvisoryID: advisory.AdvisoryID, @@ -247,6 +246,7 @@ func calcAdvisoryChanges(system *models.SystemPlatform, deleteIDs []int64, SystemsApplicable: 1, } } else { // APPLICABLE + isApplicableOnly[advisory.AdvisoryID] = true // 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. @@ -267,7 +267,7 @@ func calcAdvisoryChanges(system *models.SystemPlatform, deleteIDs []int64, RhAccountID: system.RhAccountID, SystemsInstallable: -1, } - if !isApplicable[id] { + if !isApplicableOnly[id] { // advisory is no longer applicable aad := aadMap[id] aad.SystemsApplicable = -1 diff --git a/evaluator/evaluate_advisories_test.go b/evaluator/evaluate_advisories_test.go index c66e48634..a8579a568 100644 --- a/evaluator/evaluate_advisories_test.go +++ b/evaluator/evaluate_advisories_test.go @@ -39,18 +39,7 @@ func TestGetReportedAdvisories1(t *testing.T) { } func TestGetReportedAdvisories2(t *testing.T) { - aUpdates := []vmaas.UpdatesV3ResponseAvailableUpdates{ - {Erratum: utils.PtrString("ER1")}, {Erratum: utils.PtrString("ER2")}} - bUpdates := []vmaas.UpdatesV3ResponseAvailableUpdates{ - {Erratum: utils.PtrString("ER2")}, {Erratum: utils.PtrString("ER3")}} - cUpdates := []vmaas.UpdatesV3ResponseAvailableUpdates{ - {Erratum: utils.PtrString("ER3")}, {Erratum: utils.PtrString("ER4")}} - updateList := map[string]*vmaas.UpdatesV3ResponseUpdateList{ - "pkg-a": {AvailableUpdates: &aUpdates}, - "pkg-b": {AvailableUpdates: &bUpdates}, - "pkg-c": {AvailableUpdates: &cUpdates}, - } - vmaasData := vmaas.UpdatesV3Response{UpdateList: &updateList} + vmaasData := mockVMaaSResponse() advisories := getReportedAdvisories(&vmaasData) assert.Equal(t, 4, len(advisories)) } @@ -71,7 +60,7 @@ func TestGetReportedAdvisoriesEmpty(t *testing.T) { assert.Equal(t, 0, len(advisories)) } -func TestGetStoredAdvisoriesMap(t *testing.T) { +func TestLoadSystemAdvisories(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() @@ -84,7 +73,7 @@ func TestGetStoredAdvisoriesMap(t *testing.T) { assert.Equal(t, "2016-09-22 16:00:00 +0000 UTC", (systemAdvisories)["RH-1"].Advisory.PublicDate.String()) } -func TestAdvisoryChanges(t *testing.T) { +func TestEvaluateChanges(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() @@ -115,7 +104,48 @@ func TestAdvisoryChanges(t *testing.T) { assert.Equal(t, Add, extendedAdvisories["ER-4"].Change) } -func TestUpdatePatchedSystemAdvisories(t *testing.T) { +func TestLoadMissingNamesIDs(t *testing.T) { + utils.SkipWithoutDB(t) + core.SetupTestEnvironment() + + vmaasData := mockVMaaSResponse() + missingNames := []string{"ER1", "ER2", "ER3", "ER4"} + extendedAdvisories := ExtendedAdvisoryMap{"ER1": {}, "ER2": {}, "ER3": {}, "ER4": {}} + + // test error if not lazy saved + err := loadMissingNamesIDs(missingNames, &extendedAdvisories) + assert.Error(t, err) + + // test OK if lazy saved + err = lazySaveAdvisories(&vmaasData, inventoryID) + defer database.DeleteNewlyAddedAdvisories(t) + assert.NoError(t, err) + err = loadMissingNamesIDs(missingNames, &extendedAdvisories) + assert.NoError(t, err) + assert.NotEqual(t, int64(0), extendedAdvisories["ER1"].AdvisoryID) + assert.NotEqual(t, int64(0), extendedAdvisories["ER2"].AdvisoryID) + assert.NotEqual(t, int64(0), extendedAdvisories["ER3"].AdvisoryID) + assert.NotEqual(t, int64(0), extendedAdvisories["ER4"].AdvisoryID) +} + +func TestParseStored(t *testing.T) { + stored := SystemAdvisoryMap{ + "ER-42": models.SystemAdvisories{}, + "ER-43": models.SystemAdvisories{}, + } + reported := map[string]int{ + "ER-43": 43, + } + extendedAdvisories := ExtendedAdvisoryMap{ + "ER-43": ExtendedAdvisory{Change: Keep, SystemAdvisories: stored["ER-43"]}, + } + parseStored(stored, reported, &extendedAdvisories) + assert.Equal(t, 2, len(extendedAdvisories)) + assert.Equal(t, Remove, extendedAdvisories["ER-42"].Change) + assert.Equal(t, Keep, extendedAdvisories["ER-43"].Change) +} + +func TestUpdateAdvisoryAccountData(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() @@ -148,7 +178,7 @@ func TestGetMissingAdvisories(t *testing.T) { core.SetupTestEnvironment() advisories := []string{"ER-1", "RH-1", "ER-2", "RH-2"} - advisoryIDs := getAdvisoriesFromDB(advisories) + advisoryIDs := getAdvisoryIDsByNames(t, advisories) missingNames, err := getMissingAdvisories(advisories) assert.Nil(t, err) assert.Equal(t, 2, len(advisoryIDs)) @@ -160,14 +190,14 @@ func TestGetMissingAdvisoriesEmptyString(t *testing.T) { core.SetupTestEnvironment() advisories := []string{""} - advisoryIDs := getAdvisoriesFromDB(advisories) + advisoryIDs := getAdvisoryIDsByNames(t, advisories) missingNames, err := getMissingAdvisories(advisories) assert.Nil(t, err) assert.Equal(t, 0, len(advisoryIDs)) assert.Equal(t, 1, len(missingNames)) } -func TestProcessAndUpsertSystemAdvisories(t *testing.T) { +func TestProcessAdvisories(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() @@ -189,13 +219,70 @@ func TestProcessAndUpsertSystemAdvisories(t *testing.T) { assert.Equal(t, 0, len(deleteIDs)) assert.Equal(t, 2, len(advisoryObjs)) assert.Equal(t, len(updatedAdvisories), len(extendedAdvisories)-len(deleteIDs)) +} + +func TestUpsertSystemAdvisories(t *testing.T) { + utils.SkipWithoutDB(t) + core.SetupTestEnvironment() + + // ensure consistent environment + database.DeleteSystemAdvisories(t, systemID, []int64{3, 4}) + database.CreateSystemAdvisories(t, rhAccountID, systemID, []int64{3}) + + // mock data: the system advisory with ID=3 exists and will be updated, + // the system advisory with ID=4 will be created + advisoryObjs := models.SystemAdvisoriesSlice{ + models.SystemAdvisories{SystemID: systemID, RhAccountID: rhAccountID, + AdvisoryID: int64(3), + StatusID: APPLICABLE, + }, + models.SystemAdvisories{SystemID: systemID, RhAccountID: rhAccountID, + AdvisoryID: int64(4), + }, + } + // check insert err := upsertSystemAdvisories(database.DB, advisoryObjs) assert.Nil(t, err) database.CheckSystemAdvisories(t, systemID, []int64{3, 4}) + + // check update + var updatedAdvisory models.SystemAdvisories + err = database.DB.Model(models.SystemAdvisories{}).Find(&updatedAdvisory, []int64{3}).Error + assert.Nil(t, err) + assert.Equal(t, APPLICABLE, updatedAdvisory.StatusID) + + // cleanup database.DeleteSystemAdvisories(t, systemID, []int64{3, 4}) } +func TestCalcAdvisoryChanges(t *testing.T) { + // TODO: finish creating the data and add asserts + // mock data + deleteIDs := []int64{103, 104} + system := models.SystemPlatform{ID: systemID, RhAccountID: rhAccountID} + systemAdvisories := SystemAdvisoryMap{ + "ER-102": models.SystemAdvisories{AdvisoryID: int64(102), StatusID: INSTALLABLE}, + "ER-103": models.SystemAdvisories{AdvisoryID: int64(103), StatusID: INSTALLABLE}, + "ER-104": models.SystemAdvisories{AdvisoryID: int64(104), StatusID: APPLICABLE}, + "ER-105": models.SystemAdvisories{AdvisoryID: int64(105), StatusID: APPLICABLE}, + } + + changes := calcAdvisoryChanges(&system, deleteIDs, systemAdvisories) + 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 getVMaaSUpdates(t *testing.T) vmaas.UpdatesV3Response { ctx := context.Background() vmaasData := vmaas.UpdatesV3Response{} @@ -206,19 +293,26 @@ func getVMaaSUpdates(t *testing.T) vmaas.UpdatesV3Response { return vmaasData } -func getAdvisoriesFromDB(advisories []string) []int64 { - advisoryMetadata := make(models.AdvisoryMetadataSlice, 0, len(advisories)) - err := database.Db.Model(&models.AdvisoryMetadata{}). - Where("name IN (?)", advisories). - Select("id, name"). - Scan(&advisoryMetadata).Error - if err != nil { - return nil +func mockVMaaSResponse() vmaas.UpdatesV3Response { + aUpdates := []vmaas.UpdatesV3ResponseAvailableUpdates{ + {Erratum: utils.PtrString("ER1")}, {Erratum: utils.PtrString("ER2")}} + bUpdates := []vmaas.UpdatesV3ResponseAvailableUpdates{ + {Erratum: utils.PtrString("ER2")}, {Erratum: utils.PtrString("ER3")}} + cUpdates := []vmaas.UpdatesV3ResponseAvailableUpdates{ + {Erratum: utils.PtrString("ER3")}, {Erratum: utils.PtrString("ER4")}} + updateList := map[string]*vmaas.UpdatesV3ResponseUpdateList{ + "pkg-a": {AvailableUpdates: &aUpdates}, + "pkg-b": {AvailableUpdates: &bUpdates}, + "pkg-c": {AvailableUpdates: &cUpdates}, } + return vmaas.UpdatesV3Response{UpdateList: &updateList} +} - advisoryIDs := make([]int64, 0, len(advisories)) - for _, am := range advisoryMetadata { - advisoryIDs = append(advisoryIDs, am.ID) - } - return advisoryIDs +func getAdvisoryIDsByNames(t *testing.T, names []string) []int64 { + ids := make([]int64, 0, len(names)) + err := database.DB.Model(&models.AdvisoryMetadata{}). + Where("name IN (?)", names). + Pluck("id", &ids).Error + assert.NoError(t, err) + return ids } diff --git a/evaluator/remediations.go b/evaluator/remediations.go index 9540ff3ad..37c333aad 100644 --- a/evaluator/remediations.go +++ b/evaluator/remediations.go @@ -58,11 +58,13 @@ func getReportedAdvisories(vmaasData *vmaas.UpdatesV3Response) map[string]int { func getReportedAdvisoryNames(vmaasData *vmaas.UpdatesV3Response) []string { updateList := vmaasData.GetUpdateList() reportedNames := make([]string, 0, len(updateList)) + present := make(map[string]bool, len(updateList)) for _, updates := range updateList { for _, update := range updates.GetAvailableUpdates() { advisoryName := update.GetErratum() - if len(advisoryName) > 0 { + if len(advisoryName) > 0 && !present[advisoryName] { reportedNames = append(reportedNames, advisoryName) + present[advisoryName] = true } } } diff --git a/evaluator/remediations_test.go b/evaluator/remediations_test.go index 86e5803cc..85471561c 100644 --- a/evaluator/remediations_test.go +++ b/evaluator/remediations_test.go @@ -44,3 +44,9 @@ func TestCreateRemediationsState(t *testing.T) { "patch:firefox-0:77.0.1-1.fc31.x86_64", "patch:firefox-1:76.0.1-1.fc31.x86_64", "patch:kernel-5.10.13-200.fc31.x86_64"}) } + +func TestGetReportedAdvisoryNames(t *testing.T) { + vmaasData := mockVMaaSResponse() + names := getReportedAdvisoryNames(&vmaasData) + assert.Equal(t, 4, len(names)) +} From 9764363f54c4fed0b5fd19d69bfd0e31642db7d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Wed, 24 Jul 2024 19:16:47 +0200 Subject: [PATCH 08/15] RHINENG-8141: make entities unexported, fix and add tests --- evaluator/evaluate.go | 8 ++-- evaluator/evaluate_advisories.go | 60 +++++++++++------------ evaluator/evaluate_advisories_test.go | 68 +++++++++++++++++++++------ 3 files changed, 89 insertions(+), 47 deletions(-) diff --git a/evaluator/evaluate.go b/evaluator/evaluate.go index 261ff8672..26a1fd845 100644 --- a/evaluator/evaluate.go +++ b/evaluator/evaluate.go @@ -447,6 +447,8 @@ func commitWithObserve(tx *gorm.DB) error { return nil } +// First, load advisories and packages (including change evaluation). +// Then, in a single transaction, do the deletions, updates, and insertions. func evaluateAndStore(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response, event *mqueue.PlatformEvent) error { advisoriesByName, err := lazySaveAndLoadAdvisories(system, vmaasData) @@ -526,11 +528,11 @@ func analyzeRepos(system *models.SystemPlatform) (thirdParty bool, err error) { func incrementAdvisoryTypeCounts(advisory models.AdvisoryMetadata, enhCount, bugCount, secCount *int) { switch advisory.AdvisoryTypeID { - case Enhancement: + case enhancement: *enhCount++ - case Bugfix: + case bugfix: *bugCount++ - case Security: + case security: *secCount++ } } diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index c0df21d15..2c9a54c90 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -12,8 +12,9 @@ import ( "gorm.io/gorm/clause" ) +// Lazy save missing advisories from reported, load stored ones, and evaluate changes between the two. func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( - ExtendedAdvisoryMap, error) { + extendedAdvisoryMap, error) { if !enableAdvisoryAnalysis { utils.LogInfo("advisory analysis disabled, skipping lazy saving and loading") return nil, nil @@ -37,26 +38,27 @@ func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.U return merged, nil } -func pasrseReported(stored SystemAdvisoryMap, reported map[string]int) (ExtendedAdvisoryMap, []string) { - extendedAdvisories := make(ExtendedAdvisoryMap, len(reported)+len(stored)) +// Create extendedAdvisoryMap from `stored` and `reported` advisories. It tracks changes between the two. +func pasrseReported(stored SystemAdvisoryMap, reported map[string]int) (extendedAdvisoryMap, []string) { + extendedAdvisories := make(extendedAdvisoryMap, len(reported)+len(stored)) missingNames := make([]string, 0, len(reported)) for reportedName, reportedStatusID := range reported { if storedAdvisory, found := stored[reportedName]; found { if reportedStatusID != storedAdvisory.StatusID { storedAdvisory.StatusID = reportedStatusID - extendedAdvisories[reportedName] = ExtendedAdvisory{ - Change: Update, + extendedAdvisories[reportedName] = extendedAdvisory{ + change: Update, SystemAdvisories: storedAdvisory, } } else { - extendedAdvisories[reportedName] = ExtendedAdvisory{ - Change: Keep, + extendedAdvisories[reportedName] = extendedAdvisory{ + change: Keep, SystemAdvisories: storedAdvisory, } } } else { - extendedAdvisories[reportedName] = ExtendedAdvisory{ - Change: Add, + extendedAdvisories[reportedName] = extendedAdvisory{ + change: Add, SystemAdvisories: models.SystemAdvisories{ StatusID: reportedStatusID, }, @@ -67,7 +69,7 @@ func pasrseReported(stored SystemAdvisoryMap, reported map[string]int) (Extended return extendedAdvisories, missingNames } -func loadMissingNamesIDs(missingNames []string, extendedAdvisories *ExtendedAdvisoryMap) error { +func loadMissingNamesIDs(missingNames []string, extendedAdvisories *extendedAdvisoryMap) error { advisoryMetadata, err := getAdvisoryMetadataByNames(missingNames) if err != nil { return err @@ -90,12 +92,12 @@ func loadMissingNamesIDs(missingNames []string, extendedAdvisories *ExtendedAdvi return nil } -// Set change for advisories that are in `stored` but not in `reported` to Remove. -func parseStored(stored SystemAdvisoryMap, reported map[string]int, extendedAdvisories *ExtendedAdvisoryMap) { +// ParseStored sets Change for advisories that are in stored but not in reported to Remove. +func parseStored(stored SystemAdvisoryMap, reported map[string]int, extendedAdvisories *extendedAdvisoryMap) { for storedName, storedAdvisory := range stored { if _, found := reported[storedName]; !found { - (*extendedAdvisories)[storedName] = ExtendedAdvisory{ - Change: Remove, + (*extendedAdvisories)[storedName] = extendedAdvisory{ + change: Remove, SystemAdvisories: storedAdvisory, } } @@ -104,7 +106,7 @@ func parseStored(stored SystemAdvisoryMap, reported map[string]int, extendedAdvi // Evaluate changes to all advisories based on `stored` advisories from DB and `reported` advisories from VMaaS. func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMap) ( - ExtendedAdvisoryMap, error) { + extendedAdvisoryMap, error) { reported := getReportedAdvisories(vmaasData) extendedAdvisories, missingNames := pasrseReported(stored, reported) @@ -206,7 +208,7 @@ func getMissingAdvisories(advisoryNames []string) ([]string, error) { return missingNames, nil } -func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, advisoriesByName ExtendedAdvisoryMap) ( +func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, advisoriesByName extendedAdvisoryMap) ( SystemAdvisoryMap, error) { if !enableAdvisoryAnalysis { utils.LogInfo("advisory analysis disabled, skipping storing") @@ -229,7 +231,7 @@ func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, advisoriesByN func calcAdvisoryChanges(system *models.SystemPlatform, deleteIDs []int64, systemAdvisories SystemAdvisoryMap) []models.AdvisoryAccountData { - // If system is stale, we won't change any rows in advisory_account_data + // If system is stale, we won't change any rows in advisory_account_data if system.Stale { return []models.AdvisoryAccountData{} } @@ -295,13 +297,13 @@ func upsertSystemAdvisories(tx *gorm.DB, advisoryObjs models.SystemAdvisoriesSli return database.BulkInsert(tx, advisoryObjs) } -func processAdvisories(system *models.SystemPlatform, advisoriesByName ExtendedAdvisoryMap) ([]int64, +func processAdvisories(system *models.SystemPlatform, advisoriesByName extendedAdvisoryMap) ([]int64, models.SystemAdvisoriesSlice, SystemAdvisoryMap) { deleteIDs := make([]int64, 0, len(advisoriesByName)) advisoryObjs := make(models.SystemAdvisoriesSlice, 0, len(advisoriesByName)) updatedAdvisories := make(SystemAdvisoryMap, len(advisoriesByName)) for name, advisory := range advisoriesByName { - switch advisory.Change { + switch advisory.change { case Remove: deleteIDs = append(deleteIDs, advisory.AdvisoryID) case Update: @@ -325,7 +327,7 @@ func processAdvisories(system *models.SystemPlatform, advisoriesByName ExtendedA } func updateSystemAdvisories(tx *gorm.DB, system *models.SystemPlatform, - advisoriesByName ExtendedAdvisoryMap) ([]int64, SystemAdvisoryMap, error) { + advisoriesByName extendedAdvisoryMap) ([]int64, SystemAdvisoryMap, error) { deleteIDs, advisoryObjs, updatedAdvisories := processAdvisories(system, advisoriesByName) // this will remove many many old items from our "system_advisories" table @@ -356,8 +358,8 @@ func loadSystemAdvisories(accountID int, systemID int64) (SystemAdvisoryMap, err return systemAdvisories, nil } -func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, - deleteIDs []int64, systemAdvisories SystemAdvisoryMap) error { +func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, deleteIDs []int64, + systemAdvisories SystemAdvisoryMap) error { changes := calcAdvisoryChanges(system, deleteIDs, systemAdvisories) if len(changes) == 0 { @@ -373,16 +375,16 @@ func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, return database.BulkInsert(txOnConflict, changes) } -type ExtendedAdvisory struct { - Change ChangeType +type extendedAdvisory struct { + change ChangeType models.SystemAdvisories } -type ExtendedAdvisoryMap map[string]ExtendedAdvisory +type extendedAdvisoryMap map[string]extendedAdvisory const ( - UnknownFixme int = iota - Enhancement - Bugfix - Security + undefinedChangeType int = iota + enhancement + bugfix + security ) diff --git a/evaluator/evaluate_advisories_test.go b/evaluator/evaluate_advisories_test.go index a8579a568..814434a11 100644 --- a/evaluator/evaluate_advisories_test.go +++ b/evaluator/evaluate_advisories_test.go @@ -98,10 +98,10 @@ func TestEvaluateChanges(t *testing.T) { assert.Nil(t, err) assert.Equal(t, 4, len(extendedAdvisories)) - assert.Equal(t, Keep, extendedAdvisories["ER-1"].Change) - assert.Equal(t, Remove, extendedAdvisories["ER-2"].Change) - assert.Equal(t, Update, extendedAdvisories["ER-3"].Change) - assert.Equal(t, Add, extendedAdvisories["ER-4"].Change) + assert.Equal(t, Keep, extendedAdvisories["ER-1"].change) + assert.Equal(t, Remove, extendedAdvisories["ER-2"].change) + assert.Equal(t, Update, extendedAdvisories["ER-3"].change) + assert.Equal(t, Add, extendedAdvisories["ER-4"].change) } func TestLoadMissingNamesIDs(t *testing.T) { @@ -110,7 +110,7 @@ func TestLoadMissingNamesIDs(t *testing.T) { vmaasData := mockVMaaSResponse() missingNames := []string{"ER1", "ER2", "ER3", "ER4"} - extendedAdvisories := ExtendedAdvisoryMap{"ER1": {}, "ER2": {}, "ER3": {}, "ER4": {}} + extendedAdvisories := extendedAdvisoryMap{"ER1": {}, "ER2": {}, "ER3": {}, "ER4": {}} // test error if not lazy saved err := loadMissingNamesIDs(missingNames, &extendedAdvisories) @@ -128,21 +128,59 @@ func TestLoadMissingNamesIDs(t *testing.T) { assert.NotEqual(t, int64(0), extendedAdvisories["ER4"].AdvisoryID) } +func TestParseReported(t *testing.T) { + stored := SystemAdvisoryMap{ + "ER-42": models.SystemAdvisories{StatusID: INSTALLABLE}, + "ER-43": models.SystemAdvisories{StatusID: INSTALLABLE}, + } + reported := map[string]int{ + "ER-42": INSTALLABLE, + "ER-43": APPLICABLE, + "ER-44": INSTALLABLE, + } + extendedAdvisories, missingNames := pasrseReported(stored, reported) + assert.Equal(t, 3, len(extendedAdvisories)) + assert.Equal(t, Keep, extendedAdvisories["ER-42"].change) + assert.Equal(t, Update, extendedAdvisories["ER-43"].change) + assert.Equal(t, Add, extendedAdvisories["ER-44"].change) + assert.Equal(t, []string{"ER-44"}, missingNames) +} + func TestParseStored(t *testing.T) { stored := SystemAdvisoryMap{ "ER-42": models.SystemAdvisories{}, - "ER-43": models.SystemAdvisories{}, + "ER-43": models.SystemAdvisories{StatusID: INSTALLABLE}, } reported := map[string]int{ - "ER-43": 43, + "ER-43": INSTALLABLE, } - extendedAdvisories := ExtendedAdvisoryMap{ - "ER-43": ExtendedAdvisory{Change: Keep, SystemAdvisories: stored["ER-43"]}, + extendedAdvisories := extendedAdvisoryMap{ + "ER-43": extendedAdvisory{change: Keep, SystemAdvisories: stored["ER-43"]}, } parseStored(stored, reported, &extendedAdvisories) assert.Equal(t, 2, len(extendedAdvisories)) - assert.Equal(t, Remove, extendedAdvisories["ER-42"].Change) - assert.Equal(t, Keep, extendedAdvisories["ER-43"].Change) + assert.Equal(t, Remove, extendedAdvisories["ER-42"].change) + assert.Equal(t, Keep, extendedAdvisories["ER-43"].change) +} + +func TestIncrementAdvisoryTypeCounts(t *testing.T) { + var ( + enhCount int + bugCount int + secCount int + ) + advisories := []models.AdvisoryMetadata{ + {AdvisoryTypeID: enhancement}, + {AdvisoryTypeID: bugfix}, + {AdvisoryTypeID: security}, + } + + for _, advisory := range advisories { + incrementAdvisoryTypeCounts(advisory, &enhCount, &bugCount, &secCount) + } + assert.Equal(t, 1, enhCount) + assert.Equal(t, 1, bugCount) + assert.Equal(t, 1, secCount) } func TestUpdateAdvisoryAccountData(t *testing.T) { @@ -203,14 +241,14 @@ func TestProcessAdvisories(t *testing.T) { systemID := int64(2) system := models.SystemPlatform{RhAccountID: 1, ID: systemID} - extendedAdvisories := ExtendedAdvisoryMap{ - "ER-2": ExtendedAdvisory{Change: Keep, SystemAdvisories: models.SystemAdvisories{ + extendedAdvisories := extendedAdvisoryMap{ + "ER-2": extendedAdvisory{change: Keep, SystemAdvisories: models.SystemAdvisories{ AdvisoryID: int64(2), }}, - "ER-3": ExtendedAdvisory{Change: Add, SystemAdvisories: models.SystemAdvisories{ + "ER-3": extendedAdvisory{change: Add, SystemAdvisories: models.SystemAdvisories{ AdvisoryID: int64(3), }}, - "ER-4": ExtendedAdvisory{Change: Update, SystemAdvisories: models.SystemAdvisories{ + "ER-4": extendedAdvisory{change: Update, SystemAdvisories: models.SystemAdvisories{ AdvisoryID: int64(4), }}, } From 759e89fd8f756cbeef9c7a279bb5c59bde5a5a6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Wed, 24 Jul 2024 19:42:05 +0200 Subject: [PATCH 09/15] RHINENG-8141: update function doc comments to adhere to go conventions --- evaluator/evaluate.go | 6 +++--- evaluator/evaluate_advisories.go | 12 +++++++----- evaluator/evaluate_packages.go | 6 +++--- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/evaluator/evaluate.go b/evaluator/evaluate.go index 26a1fd845..bcd0079b5 100644 --- a/evaluator/evaluate.go +++ b/evaluator/evaluate.go @@ -165,7 +165,7 @@ func Evaluate(ctx context.Context, event *mqueue.PlatformEvent, inventoryID, eva return nil } -// Runs Evaluate method in Goroutines +// RunEvaluate runs Evaluate method in Goroutines func runEvaluate( ctx context.Context, event mqueue.PlatformEvent, // makes a copy to avoid races @@ -447,8 +447,8 @@ func commitWithObserve(tx *gorm.DB) error { return nil } -// First, load advisories and packages (including change evaluation). -// Then, in a single transaction, do the deletions, updates, and insertions. +// EvaluateAndStore first loads advisories and packages (including change evaluation) +// and then executes all deletions, updates, and insertions in a single transaction. func evaluateAndStore(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response, event *mqueue.PlatformEvent) error { advisoriesByName, err := lazySaveAndLoadAdvisories(system, vmaasData) diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index 2c9a54c90..d2451d20e 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -12,7 +12,8 @@ import ( "gorm.io/gorm/clause" ) -// Lazy save missing advisories from reported, load stored ones, and evaluate changes between the two. +// LazySaveAndLoadAdvisories lazy saves missing advisories from reported, loads stored ones from DB, +// and evaluates changes between the two. func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( extendedAdvisoryMap, error) { if !enableAdvisoryAnalysis { @@ -38,7 +39,7 @@ func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.U return merged, nil } -// Create extendedAdvisoryMap from `stored` and `reported` advisories. It tracks changes between the two. +// PasrseReported evaluates changes of type Add/Update/Keep and tracks them in extendedAdvisoryMap. func pasrseReported(stored SystemAdvisoryMap, reported map[string]int) (extendedAdvisoryMap, []string) { extendedAdvisories := make(extendedAdvisoryMap, len(reported)+len(stored)) missingNames := make([]string, 0, len(reported)) @@ -104,7 +105,8 @@ func parseStored(stored SystemAdvisoryMap, reported map[string]int, extendedAdvi } } -// Evaluate changes to all advisories based on `stored` advisories from DB and `reported` advisories from VMaaS. +// EvaluateChanges calls functions that evaluate all types of changes between stored advisories from DB +// and reported advisories from VMaaS. func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMap) ( extendedAdvisoryMap, error) { reported := getReportedAdvisories(vmaasData) @@ -120,7 +122,7 @@ func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMa return extendedAdvisories, nil } -// From names reported by VMaaS, find advisories missing in the DB and lazy-save them. +// LazySaveAdvisories finds advisories reported by VMaaS and missing in the DB and lazy saves them. func lazySaveAdvisories(vmaasData *vmaas.UpdatesV3Response, inventoryID string) error { reportedNames := getReportedAdvisoryNames(vmaasData) if len(reportedNames) < 1 { @@ -186,7 +188,7 @@ func getAdvisoryMetadataByNames(names []string) (models.AdvisoryMetadataSlice, e return metadata, err } -// Determine if advisories from DB are properly stored based on advisory metadata existence. +// GetMissingAdvisories determines if advisories from DB are properly stored based on advisory metadata existence. func getMissingAdvisories(advisoryNames []string) ([]string, error) { advisoryMetadata, err := getAdvisoryMetadataByNames(advisoryNames) if err != nil { diff --git a/evaluator/evaluate_packages.go b/evaluator/evaluate_packages.go index 62ee1a94c..e4ea33411 100644 --- a/evaluator/evaluate_packages.go +++ b/evaluator/evaluate_packages.go @@ -35,7 +35,7 @@ func lazySaveAndLoadPackages(system *models.SystemPlatform, vmaasData *vmaas.Upd return pkgByName, installed, installable, applicable, nil } -// Add unknown EVRAs into the db if needed +// LazySavePackages adds unknown EVRAs into the db if needed. func lazySavePackages(vmaasData *vmaas.UpdatesV3Response) error { if !enableLazyPackageSave { return nil @@ -89,7 +89,7 @@ func getMissingPackage(nevra string) *models.Package { return &pkg } -// Get packages with known name but version missing in db/cache +// GetMissingPackages gets packages with a known name but a version missing in db/cache. func getMissingPackages(vmaasData *vmaas.UpdatesV3Response) models.PackageSlice { updates := vmaasData.GetUpdateList() packages := make(models.PackageSlice, 0, len(updates)) @@ -130,7 +130,7 @@ func updatePackageNameDB(missing *models.PackageName) error { return nil } -// Find relevant package data based on vmaas results +// LoadPackages finds relevant package data based on vmaas results. func loadPackages(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( map[string]namedPackage, int, int, int, error) { defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("packages-load")) From c75a7c69ce2796763e5284fc01db4b1533c79bcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Thu, 25 Jul 2024 20:47:43 +0200 Subject: [PATCH 10/15] RHINENG-8141: minor improvements and fixes --- evaluator/evaluate_advisories.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index d2451d20e..127a037dc 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -76,14 +76,14 @@ func loadMissingNamesIDs(missingNames []string, extendedAdvisories *extendedAdvi return err } - name2AdvisoryID := make(map[string]int64, len(missingNames)) + name2AdvisoryID := make(map[string]int64, len(advisoryMetadata)) for _, am := range advisoryMetadata { name2AdvisoryID[am.Name] = am.ID } for _, name := range missingNames { if _, found := name2AdvisoryID[name]; !found { - return errors.New("Failed to evaluate changes because an advisory was not lazy-saved") + return errors.New("Failed to evaluate changes because an advisory was not lazy saved") } extendedAdvisory := (*extendedAdvisories)[name] extendedAdvisory.AdvisoryID = name2AdvisoryID[name] @@ -135,12 +135,11 @@ func lazySaveAdvisories(vmaasData *vmaas.UpdatesV3Response, inventoryID string) } nUnknown := len(missingNames) - if nUnknown > 0 { - utils.LogInfo("inventoryID", inventoryID, "unknown", nUnknown, "unknown advisories") - updatesCnt.WithLabelValues("unknown").Add(float64(nUnknown)) - } else { + if nUnknown <= 0 { return nil } + utils.LogInfo("inventoryID", inventoryID, "unknown", nUnknown, "unknown advisories") + updatesCnt.WithLabelValues("unknown").Add(float64(nUnknown)) err = storeMissingAdvisories(missingNames) if err != nil { @@ -223,7 +222,6 @@ func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, advisoriesByN return nil, errors.Wrap(err, "Unable to update system advisories") } - // FIXME: Advisory account data should calculate changes from the before-update data err = updateAdvisoryAccountData(tx, system, deleteIDs, systemAdvisoriesNew) if err != nil { return nil, errors.Wrap(err, "Unable to update advisory_account_data caches") From 1547de3d508a0c824bac97bd1e6f3b0ff5127b65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Tue, 30 Jul 2024 15:50:26 +0200 Subject: [PATCH 11/15] RHINENG-8141: fix data flow and tests --- evaluator/evaluate_advisories.go | 134 ++++++++++++-------------- evaluator/evaluate_advisories_test.go | 57 +++++++---- 2 files changed, 103 insertions(+), 88 deletions(-) diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index 127a037dc..259d80c3d 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -26,7 +26,7 @@ func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.U return nil, errors.Wrap(err, "Unable to store unknown advisories in DB") } - stored, err := loadSystemAdvisories(system.RhAccountID, system.ID) + stored, err := loadSystemAdvisories(database.DB, system.RhAccountID, system.ID) if err != nil { return nil, errors.Wrap(err, "Unable to load system advisories") } @@ -135,7 +135,7 @@ func lazySaveAdvisories(vmaasData *vmaas.UpdatesV3Response, inventoryID string) } nUnknown := len(missingNames) - if nUnknown <= 0 { + if nUnknown == 0 { return nil } utils.LogInfo("inventoryID", inventoryID, "unknown", nUnknown, "unknown advisories") @@ -165,17 +165,13 @@ func storeMissingAdvisories(missingNames []string) error { } } - if len(toStore) > 0 { - tx := database.DB.Begin() - defer tx.Commit() - err := tx.Clauses(clause.OnConflict{DoNothing: true}).Create(&toStore).Error - if err != nil { - return err - } - // FIXME: after creation, toStore will include newly added .ID attributes + if len(toStore) == 0 { + return nil } - return nil + tx := database.DB.Begin() + defer tx.Commit() + return tx.Clauses(clause.OnConflict{DoNothing: true}).Create(&toStore).Error } func getAdvisoryMetadataByNames(names []string) (models.AdvisoryMetadataSlice, error) { @@ -217,71 +213,80 @@ func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, advisoriesByN } defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("advisories-store")) - deleteIDs, systemAdvisoriesNew, err := updateSystemAdvisories(tx, system, advisoriesByName) + err := updateSystemAdvisories(tx, system, advisoriesByName) if err != nil { return nil, errors.Wrap(err, "Unable to update system advisories") } - err = updateAdvisoryAccountData(tx, system, deleteIDs, systemAdvisoriesNew) + // reload all after updates + systemAdvisoriesNew, err := loadSystemAdvisories(tx, system.RhAccountID, system.ID) + if err != nil { + 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.SystemPlatform, deleteIDs []int64, - systemAdvisories SystemAdvisoryMap) []models.AdvisoryAccountData { +func calcAdvisoryChanges(system *models.SystemPlatform, + advisoriesByName extendedAdvisoryMap) []models.AdvisoryAccountData { // If system is stale, we won't change any rows in advisory_account_data if system.Stale { return []models.AdvisoryAccountData{} } - aadMap := make(map[int64]models.AdvisoryAccountData, len(systemAdvisories)) - isApplicableOnly := make(map[int64]bool, len(systemAdvisories)) - for _, advisory := range systemAdvisories { - if advisory.StatusID == INSTALLABLE { + 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.RhAccountID, - SystemsInstallable: 1, - // every installable advisory is also applicable advisory - SystemsApplicable: 1, + SystemsInstallable: -1, } - } else { // APPLICABLE - isApplicableOnly[advisory.AdvisoryID] = true - // 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? + 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.RhAccountID, + AdvisoryID: advisory.AdvisoryID, + RhAccountID: system.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.RhAccountID, + SystemsApplicable: 1, + } + } } } } - for _, id := range deleteIDs { - aadMap[id] = models.AdvisoryAccountData{ - AdvisoryID: id, - RhAccountID: system.RhAccountID, - SystemsInstallable: -1, - } - if !isApplicableOnly[id] { - // advisory is no longer applicable - aad := aadMap[id] - aad.SystemsApplicable = -1 - aadMap[id] = aad - } - } - - deltas := make([]models.AdvisoryAccountData, 0, len(deleteIDs)+len(systemAdvisories)) + // aadMap into aadSlice + aadSlice := make([]models.AdvisoryAccountData, 0, len(advisoriesByName)) for _, aad := range aadMap { - deltas = append(deltas, aad) + aadSlice = append(aadSlice, aad) } - return deltas + return aadSlice } func deleteOldSystemAdvisories(tx *gorm.DB, accountID int, systemID int64, patched []int64) error { @@ -298,11 +303,10 @@ func upsertSystemAdvisories(tx *gorm.DB, advisoryObjs models.SystemAdvisoriesSli } func processAdvisories(system *models.SystemPlatform, advisoriesByName extendedAdvisoryMap) ([]int64, - models.SystemAdvisoriesSlice, SystemAdvisoryMap) { + models.SystemAdvisoriesSlice) { deleteIDs := make([]int64, 0, len(advisoriesByName)) advisoryObjs := make(models.SystemAdvisoriesSlice, 0, len(advisoriesByName)) - updatedAdvisories := make(SystemAdvisoryMap, len(advisoriesByName)) - for name, advisory := range advisoriesByName { + for _, advisory := range advisoriesByName { switch advisory.change { case Remove: deleteIDs = append(deleteIDs, advisory.AdvisoryID) @@ -318,35 +322,26 @@ func processAdvisories(system *models.SystemPlatform, advisoriesByName extendedA StatusID: advisory.StatusID, } advisoryObjs = append(advisoryObjs, adv) - updatedAdvisories[name] = adv - case Keep: - updatedAdvisories[name] = advisory.SystemAdvisories } } - return deleteIDs, advisoryObjs, updatedAdvisories + return deleteIDs, advisoryObjs } func updateSystemAdvisories(tx *gorm.DB, system *models.SystemPlatform, - advisoriesByName extendedAdvisoryMap) ([]int64, SystemAdvisoryMap, error) { - deleteIDs, advisoryObjs, updatedAdvisories := processAdvisories(system, advisoriesByName) + advisoriesByName extendedAdvisoryMap) error { + deleteIDs, advisoryObjs := processAdvisories(system, advisoriesByName) - // this will remove many many old items from our "system_advisories" table err := deleteOldSystemAdvisories(tx, system.RhAccountID, system.ID, deleteIDs) if err != nil { - return nil, nil, err - } - - err = upsertSystemAdvisories(tx, advisoryObjs) - if err != nil { - return nil, nil, err + return err } - return deleteIDs, updatedAdvisories, nil + return upsertSystemAdvisories(tx, advisoryObjs) } -func loadSystemAdvisories(accountID int, systemID int64) (SystemAdvisoryMap, error) { +func loadSystemAdvisories(tx *gorm.DB, accountID int, systemID int64) (SystemAdvisoryMap, error) { var data []models.SystemAdvisories - err := database.DB.Preload("Advisory").Find(&data, "system_id = ? AND rh_account_id = ?", systemID, accountID).Error + err := tx.Preload("Advisory").Find(&data, "system_id = ? AND rh_account_id = ?", systemID, accountID).Error if err != nil { return nil, err } @@ -358,9 +353,8 @@ func loadSystemAdvisories(accountID int, systemID int64) (SystemAdvisoryMap, err return systemAdvisories, nil } -func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, deleteIDs []int64, - systemAdvisories SystemAdvisoryMap) error { - changes := calcAdvisoryChanges(system, deleteIDs, systemAdvisories) +func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, advisoriesByName extendedAdvisoryMap) error { + changes := calcAdvisoryChanges(system, advisoriesByName) if len(changes) == 0 { return nil diff --git a/evaluator/evaluate_advisories_test.go b/evaluator/evaluate_advisories_test.go index 814434a11..99f46cbfc 100644 --- a/evaluator/evaluate_advisories_test.go +++ b/evaluator/evaluate_advisories_test.go @@ -7,7 +7,6 @@ import ( "app/base/utils" "app/base/vmaas" "context" - "fmt" "net/http" "strings" "testing" @@ -64,7 +63,7 @@ func TestLoadSystemAdvisories(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() - systemAdvisories, err := loadSystemAdvisories(1, 1) + systemAdvisories, err := loadSystemAdvisories(database.DB, 1, 1) assert.Nil(t, err) assert.NotNil(t, systemAdvisories) assert.Equal(t, 8, len(systemAdvisories)) @@ -191,19 +190,33 @@ func TestUpdateAdvisoryAccountData(t *testing.T) { advisoryIDs := []int64{2, 3, 4} database.CreateSystemAdvisories(t, system.RhAccountID, system.ID, advisoryIDs) database.CreateAdvisoryAccountData(t, system.RhAccountID, advisoryIDs, 1) + advisoriesByName := extendedAdvisoryMap{ + "ER-2": { + change: Remove, + SystemAdvisories: models.SystemAdvisories{AdvisoryID: 2, SystemID: system.ID, RhAccountID: system.RhAccountID}, + }, + "ER-3": { + change: Remove, + SystemAdvisories: models.SystemAdvisories{AdvisoryID: 3, SystemID: system.ID, RhAccountID: system.RhAccountID}, + }, + "ER-4": { + change: Remove, + SystemAdvisories: models.SystemAdvisories{AdvisoryID: 4, SystemID: system.ID, RhAccountID: system.RhAccountID}, + }, + } // Update as if the advisories became patched - err := updateAdvisoryAccountData(database.DB, &system, advisoryIDs, SystemAdvisoryMap{}) + err := updateAdvisoryAccountData(database.DB, &system, advisoriesByName) assert.NoError(t, err) database.CheckSystemAdvisories(t, system.ID, advisoryIDs) database.CheckAdvisoriesAccountData(t, system.RhAccountID, advisoryIDs, 0) // Update as if the advisories became unpatched - systemAdvisories := make(SystemAdvisoryMap, len(advisoryIDs)) - for _, id := range advisoryIDs { - systemAdvisories[fmt.Sprintf("ER-%v", id)] = models.SystemAdvisories{AdvisoryID: id} + for name, ea := range advisoriesByName { + ea.change = Add + advisoriesByName[name] = ea } - err = updateAdvisoryAccountData(database.DB, &system, []int64{}, systemAdvisories) + err = updateAdvisoryAccountData(database.DB, &system, advisoriesByName) assert.NoError(t, err) database.CheckAdvisoriesAccountData(t, system.RhAccountID, advisoryIDs, 1) @@ -253,10 +266,9 @@ func TestProcessAdvisories(t *testing.T) { }}, } - deleteIDs, advisoryObjs, updatedAdvisories := processAdvisories(&system, extendedAdvisories) + deleteIDs, advisoryObjs := processAdvisories(&system, extendedAdvisories) assert.Equal(t, 0, len(deleteIDs)) assert.Equal(t, 2, len(advisoryObjs)) - assert.Equal(t, len(updatedAdvisories), len(extendedAdvisories)-len(deleteIDs)) } func TestUpsertSystemAdvisories(t *testing.T) { @@ -295,18 +307,27 @@ func TestUpsertSystemAdvisories(t *testing.T) { } func TestCalcAdvisoryChanges(t *testing.T) { - // TODO: finish creating the data and add asserts - // mock data - deleteIDs := []int64{103, 104} system := models.SystemPlatform{ID: systemID, RhAccountID: rhAccountID} - systemAdvisories := SystemAdvisoryMap{ - "ER-102": models.SystemAdvisories{AdvisoryID: int64(102), StatusID: INSTALLABLE}, - "ER-103": models.SystemAdvisories{AdvisoryID: int64(103), StatusID: INSTALLABLE}, - "ER-104": models.SystemAdvisories{AdvisoryID: int64(104), StatusID: APPLICABLE}, - "ER-105": models.SystemAdvisories{AdvisoryID: int64(105), StatusID: APPLICABLE}, + 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, deleteIDs, systemAdvisories) + changes := calcAdvisoryChanges(&system, advisoriesByName) expected := map[int64]models.AdvisoryAccountData{ 102: {SystemsApplicable: 1, SystemsInstallable: 1}, 103: {SystemsApplicable: -1, SystemsInstallable: -1}, From 861e0a1ec6519bba066c4b9a30f8f62921c6b60b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Tue, 13 Aug 2024 12:51:50 +0200 Subject: [PATCH 12/15] RHINENG-8141: resolve evaluateChanges code review comments - put `parseStored` and `parseReported` code into `evaluateChanges` -- separate functions are not needed - remove pointer to extendedAdvisories because it's unnecessary - edit function doc comment after changes --- evaluator/evaluate_advisories.go | 74 ++++++++++++--------------- evaluator/evaluate_advisories_test.go | 39 +------------- 2 files changed, 34 insertions(+), 79 deletions(-) diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index 259d80c3d..428741843 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -39,8 +39,35 @@ func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.U return merged, nil } -// PasrseReported evaluates changes of type Add/Update/Keep and tracks them in extendedAdvisoryMap. -func pasrseReported(stored SystemAdvisoryMap, reported map[string]int) (extendedAdvisoryMap, []string) { +func loadMissingNamesIDs(missingNames []string, extendedAdvisories extendedAdvisoryMap) error { + advisoryMetadata, err := getAdvisoryMetadataByNames(missingNames) + if err != nil { + return err + } + + name2AdvisoryID := make(map[string]int64, len(advisoryMetadata)) + for _, am := range advisoryMetadata { + name2AdvisoryID[am.Name] = am.ID + } + + for _, name := range missingNames { + if _, found := name2AdvisoryID[name]; !found { + return errors.New("Failed to evaluate changes because an advisory was not lazy saved") + } + extendedAdvisory := extendedAdvisories[name] + extendedAdvisory.AdvisoryID = name2AdvisoryID[name] + extendedAdvisories[name] = extendedAdvisory + } + + return nil +} + +// EvaluateChanges evaluates all types of changes (Keep, Add, Update, Remove) +// between the stored advisories from DB and the reported advisories from VMaaS, +// and tracks them in extendedAdvisoryMap. +func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMap) ( + extendedAdvisoryMap, error) { + reported := getReportedAdvisories(vmaasData) extendedAdvisories := make(extendedAdvisoryMap, len(reported)+len(stored)) missingNames := make([]string, 0, len(reported)) for reportedName, reportedStatusID := range reported { @@ -67,57 +94,20 @@ func pasrseReported(stored SystemAdvisoryMap, reported map[string]int) (extended missingNames = append(missingNames, reportedName) } } - return extendedAdvisories, missingNames -} -func loadMissingNamesIDs(missingNames []string, extendedAdvisories *extendedAdvisoryMap) error { - advisoryMetadata, err := getAdvisoryMetadataByNames(missingNames) + err := loadMissingNamesIDs(missingNames, extendedAdvisories) if err != nil { - return err - } - - name2AdvisoryID := make(map[string]int64, len(advisoryMetadata)) - for _, am := range advisoryMetadata { - name2AdvisoryID[am.Name] = am.ID - } - - for _, name := range missingNames { - if _, found := name2AdvisoryID[name]; !found { - return errors.New("Failed to evaluate changes because an advisory was not lazy saved") - } - extendedAdvisory := (*extendedAdvisories)[name] - extendedAdvisory.AdvisoryID = name2AdvisoryID[name] - (*extendedAdvisories)[name] = extendedAdvisory + return nil, err } - return nil -} - -// ParseStored sets Change for advisories that are in stored but not in reported to Remove. -func parseStored(stored SystemAdvisoryMap, reported map[string]int, extendedAdvisories *extendedAdvisoryMap) { for storedName, storedAdvisory := range stored { if _, found := reported[storedName]; !found { - (*extendedAdvisories)[storedName] = extendedAdvisory{ + extendedAdvisories[storedName] = extendedAdvisory{ change: Remove, SystemAdvisories: storedAdvisory, } } } -} - -// EvaluateChanges calls functions that evaluate all types of changes between stored advisories from DB -// and reported advisories from VMaaS. -func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMap) ( - extendedAdvisoryMap, error) { - reported := getReportedAdvisories(vmaasData) - extendedAdvisories, missingNames := pasrseReported(stored, reported) - - err := loadMissingNamesIDs(missingNames, &extendedAdvisories) - if err != nil { - return nil, err - } - - parseStored(stored, reported, &extendedAdvisories) return extendedAdvisories, nil } diff --git a/evaluator/evaluate_advisories_test.go b/evaluator/evaluate_advisories_test.go index 99f46cbfc..6e0f8f92c 100644 --- a/evaluator/evaluate_advisories_test.go +++ b/evaluator/evaluate_advisories_test.go @@ -112,14 +112,14 @@ func TestLoadMissingNamesIDs(t *testing.T) { extendedAdvisories := extendedAdvisoryMap{"ER1": {}, "ER2": {}, "ER3": {}, "ER4": {}} // test error if not lazy saved - err := loadMissingNamesIDs(missingNames, &extendedAdvisories) + err := loadMissingNamesIDs(missingNames, extendedAdvisories) assert.Error(t, err) // test OK if lazy saved err = lazySaveAdvisories(&vmaasData, inventoryID) defer database.DeleteNewlyAddedAdvisories(t) assert.NoError(t, err) - err = loadMissingNamesIDs(missingNames, &extendedAdvisories) + err = loadMissingNamesIDs(missingNames, extendedAdvisories) assert.NoError(t, err) assert.NotEqual(t, int64(0), extendedAdvisories["ER1"].AdvisoryID) assert.NotEqual(t, int64(0), extendedAdvisories["ER2"].AdvisoryID) @@ -127,41 +127,6 @@ func TestLoadMissingNamesIDs(t *testing.T) { assert.NotEqual(t, int64(0), extendedAdvisories["ER4"].AdvisoryID) } -func TestParseReported(t *testing.T) { - stored := SystemAdvisoryMap{ - "ER-42": models.SystemAdvisories{StatusID: INSTALLABLE}, - "ER-43": models.SystemAdvisories{StatusID: INSTALLABLE}, - } - reported := map[string]int{ - "ER-42": INSTALLABLE, - "ER-43": APPLICABLE, - "ER-44": INSTALLABLE, - } - extendedAdvisories, missingNames := pasrseReported(stored, reported) - assert.Equal(t, 3, len(extendedAdvisories)) - assert.Equal(t, Keep, extendedAdvisories["ER-42"].change) - assert.Equal(t, Update, extendedAdvisories["ER-43"].change) - assert.Equal(t, Add, extendedAdvisories["ER-44"].change) - assert.Equal(t, []string{"ER-44"}, missingNames) -} - -func TestParseStored(t *testing.T) { - stored := SystemAdvisoryMap{ - "ER-42": models.SystemAdvisories{}, - "ER-43": models.SystemAdvisories{StatusID: INSTALLABLE}, - } - reported := map[string]int{ - "ER-43": INSTALLABLE, - } - extendedAdvisories := extendedAdvisoryMap{ - "ER-43": extendedAdvisory{change: Keep, SystemAdvisories: stored["ER-43"]}, - } - parseStored(stored, reported, &extendedAdvisories) - assert.Equal(t, 2, len(extendedAdvisories)) - assert.Equal(t, Remove, extendedAdvisories["ER-42"].change) - assert.Equal(t, Keep, extendedAdvisories["ER-43"].change) -} - func TestIncrementAdvisoryTypeCounts(t *testing.T) { var ( enhCount int From 18cb5cae19a5026293b547b24c8cec9309a1e2ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Tue, 13 Aug 2024 13:06:09 +0200 Subject: [PATCH 13/15] RHINENG-8141: resolve getAdvisoryMetadataByName code review comments - move slice-to-map logic into `getAdvisoryMetadataByNames` since all uses converted the returned slice into a map - rename the function to `getAdvisoryMetadataIDs` to reflect changes - propagate changes into functions that used `getAdvisoryMetadataByNames` --- evaluator/evaluate_advisories.go | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index 428741843..1ddec9d88 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -40,16 +40,11 @@ func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.U } func loadMissingNamesIDs(missingNames []string, extendedAdvisories extendedAdvisoryMap) error { - advisoryMetadata, err := getAdvisoryMetadataByNames(missingNames) + name2AdvisoryID, err := getAdvisoryMetadataIDs(missingNames) if err != nil { return err } - name2AdvisoryID := make(map[string]int64, len(advisoryMetadata)) - for _, am := range advisoryMetadata { - name2AdvisoryID[am.Name] = am.ID - } - for _, name := range missingNames { if _, found := name2AdvisoryID[name]; !found { return errors.New("Failed to evaluate changes because an advisory was not lazy saved") @@ -164,30 +159,33 @@ func storeMissingAdvisories(missingNames []string) error { return tx.Clauses(clause.OnConflict{DoNothing: true}).Create(&toStore).Error } -func getAdvisoryMetadataByNames(names []string) (models.AdvisoryMetadataSlice, error) { +func getAdvisoryMetadataIDs(names []string) (map[string]int64, error) { metadata := make(models.AdvisoryMetadataSlice, 0, len(names)) err := database.DB.Model(&models.AdvisoryMetadata{}). Where("name IN (?)", names). Select("id, name"). Scan(&metadata).Error - return metadata, err + if err != nil { + return nil, err + } + + name2AdvisoryID := make(map[string]int64, len(metadata)) + for _, am := range metadata { + name2AdvisoryID[am.Name] = am.ID + } + return name2AdvisoryID, err } // GetMissingAdvisories determines if advisories from DB are properly stored based on advisory metadata existence. func getMissingAdvisories(advisoryNames []string) ([]string, error) { - advisoryMetadata, err := getAdvisoryMetadataByNames(advisoryNames) + name2AdvisoryID, err := getAdvisoryMetadataIDs(advisoryNames) if err != nil { return nil, err } - found := make(map[string]bool, len(advisoryNames)) - for _, am := range advisoryMetadata { - found[am.Name] = true - } - - missingNames := make([]string, 0, len(advisoryNames)-len(advisoryMetadata)) + missingNames := make([]string, 0, len(advisoryNames)-len(name2AdvisoryID)) for _, name := range advisoryNames { - if !found[name] { + if _, found := name2AdvisoryID[name]; !found { missingNames = append(missingNames, name) } } From 5724476b13149a7d65cae7eddaf38a344ec66c6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Tue, 13 Aug 2024 13:19:02 +0200 Subject: [PATCH 14/15] RHINENG-8141: resolve moving const/struct definitions to the top of the file --- evaluator/evaluate_advisories.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index 1ddec9d88..98100ad89 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -12,6 +12,20 @@ import ( "gorm.io/gorm/clause" ) +const ( + undefinedChangeType int = iota + enhancement + bugfix + security +) + +type extendedAdvisory struct { + change ChangeType + models.SystemAdvisories +} + +type extendedAdvisoryMap map[string]extendedAdvisory + // LazySaveAndLoadAdvisories lazy saves missing advisories from reported, loads stored ones from DB, // and evaluates changes between the two. func lazySaveAndLoadAdvisories(system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( @@ -356,17 +370,3 @@ func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, advis return database.BulkInsert(txOnConflict, changes) } - -type extendedAdvisory struct { - change ChangeType - models.SystemAdvisories -} - -type extendedAdvisoryMap map[string]extendedAdvisory - -const ( - undefinedChangeType int = iota - enhancement - bugfix - security -) From e5890ece41c30cbfb85a9908e928acbdd8cfab6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Dugovi=C4=8D?= Date: Tue, 13 Aug 2024 14:41:57 +0200 Subject: [PATCH 15/15] RHINENG-8141: resolve using getReportedAdvisories instead of getReportedAdvisoryNames --- evaluator/evaluate_advisories.go | 21 +++++++++++++++++---- evaluator/evaluate_advisories_test.go | 9 +++++---- evaluator/remediations.go | 16 ---------------- evaluator/remediations_test.go | 6 ------ 4 files changed, 22 insertions(+), 30 deletions(-) diff --git a/evaluator/evaluate_advisories.go b/evaluator/evaluate_advisories.go index 98100ad89..294368537 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -123,7 +123,7 @@ func evaluateChanges(vmaasData *vmaas.UpdatesV3Response, stored SystemAdvisoryMa // LazySaveAdvisories finds advisories reported by VMaaS and missing in the DB and lazy saves them. func lazySaveAdvisories(vmaasData *vmaas.UpdatesV3Response, inventoryID string) error { - reportedNames := getReportedAdvisoryNames(vmaasData) + reportedNames := getReportedAdvisories(vmaasData) if len(reportedNames) < 1 { return nil } @@ -191,14 +191,27 @@ func getAdvisoryMetadataIDs(names []string) (map[string]int64, error) { } // GetMissingAdvisories determines if advisories from DB are properly stored based on advisory metadata existence. -func getMissingAdvisories(advisoryNames []string) ([]string, error) { +func getMissingAdvisories(advisoriesMap map[string]int) ([]string, error) { + advisoryNames := make([]string, 0, len(advisoriesMap)) + present := make(map[string]bool, len(advisoriesMap)) + for name := range advisoriesMap { + if len(name) > 0 && !present[name] { + advisoryNames = append(advisoryNames, name) + present[name] = true + } + } + name2AdvisoryID, err := getAdvisoryMetadataIDs(advisoryNames) if err != nil { return nil, err } - missingNames := make([]string, 0, len(advisoryNames)-len(name2AdvisoryID)) - for _, name := range advisoryNames { + if len(advisoriesMap) == len(name2AdvisoryID) { + return []string{}, nil + } + + missingNames := make([]string, 0, len(advisoriesMap)-len(name2AdvisoryID)) + for name := range advisoriesMap { if _, found := name2AdvisoryID[name]; !found { missingNames = append(missingNames, name) } diff --git a/evaluator/evaluate_advisories_test.go b/evaluator/evaluate_advisories_test.go index 6e0f8f92c..6b24d6fcb 100644 --- a/evaluator/evaluate_advisories_test.go +++ b/evaluator/evaluate_advisories_test.go @@ -193,9 +193,10 @@ func TestGetMissingAdvisories(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() - advisories := []string{"ER-1", "RH-1", "ER-2", "RH-2"} - advisoryIDs := getAdvisoryIDsByNames(t, advisories) - missingNames, err := getMissingAdvisories(advisories) + advisoryNames := []string{"ER-1", "RH-1", "ER-2", "RH-2"} + advisoryMap := map[string]int{"ER-1": 0, "RH-1": 0, "ER-2": 0, "RH-2": 0} + advisoryIDs := getAdvisoryIDsByNames(t, advisoryNames) + missingNames, err := getMissingAdvisories(advisoryMap) assert.Nil(t, err) assert.Equal(t, 2, len(advisoryIDs)) assert.Equal(t, 2, len(missingNames)) @@ -207,7 +208,7 @@ func TestGetMissingAdvisoriesEmptyString(t *testing.T) { advisories := []string{""} advisoryIDs := getAdvisoryIDsByNames(t, advisories) - missingNames, err := getMissingAdvisories(advisories) + missingNames, err := getMissingAdvisories(map[string]int{"": 0}) assert.Nil(t, err) assert.Equal(t, 0, len(advisoryIDs)) assert.Equal(t, 1, len(missingNames)) diff --git a/evaluator/remediations.go b/evaluator/remediations.go index 37c333aad..a73f1d21a 100644 --- a/evaluator/remediations.go +++ b/evaluator/remediations.go @@ -55,22 +55,6 @@ func getReportedAdvisories(vmaasData *vmaas.UpdatesV3Response) map[string]int { return advisories } -func getReportedAdvisoryNames(vmaasData *vmaas.UpdatesV3Response) []string { - updateList := vmaasData.GetUpdateList() - reportedNames := make([]string, 0, len(updateList)) - present := make(map[string]bool, len(updateList)) - for _, updates := range updateList { - for _, update := range updates.GetAvailableUpdates() { - advisoryName := update.GetErratum() - if len(advisoryName) > 0 && !present[advisoryName] { - reportedNames = append(reportedNames, advisoryName) - present[advisoryName] = true - } - } - } - return reportedNames -} - func getReportedPackageUpdates(vmaasData *vmaas.UpdatesV3Response) map[string]bool { updateList := vmaasData.GetUpdateList() packages := make(map[string]bool, len(updateList)) diff --git a/evaluator/remediations_test.go b/evaluator/remediations_test.go index 85471561c..86e5803cc 100644 --- a/evaluator/remediations_test.go +++ b/evaluator/remediations_test.go @@ -44,9 +44,3 @@ func TestCreateRemediationsState(t *testing.T) { "patch:firefox-0:77.0.1-1.fc31.x86_64", "patch:firefox-1:76.0.1-1.fc31.x86_64", "patch:kernel-5.10.13-200.fc31.x86_64"}) } - -func TestGetReportedAdvisoryNames(t *testing.T) { - vmaasData := mockVMaaSResponse() - names := getReportedAdvisoryNames(&vmaasData) - assert.Equal(t, 4, len(names)) -}