From 692814fffd1494ca3f7ca75dcb8437a611cb8c23 Mon Sep 17 00:00:00 2001 From: semantic-release <> Date: Fri, 12 Jul 2024 13:22:32 +0000 Subject: [PATCH] RHINENG-8141: improve evaluation performance --- base/database/baseline.go | 6 +- base/database/baseline_test.go | 6 +- base/database/utils.go | 8 +- evaluator/evaluate.go | 118 ++++--- evaluator/evaluate_advisories.go | 481 ++++++++++++++------------ evaluator/evaluate_advisories_test.go | 292 +++++++++++++--- evaluator/evaluate_baseline.go | 8 +- evaluator/evaluate_baseline_test.go | 7 +- evaluator/evaluate_packages.go | 65 ++-- evaluator/evaluate_packages_test.go | 14 +- evaluator/remediations.go | 16 + evaluator/remediations_test.go | 6 + 12 files changed, 649 insertions(+), 378 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/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 be0d36aa9..9db00a538 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" ) @@ -166,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 @@ -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") } @@ -227,12 +222,13 @@ func evaluateInDatabase(ctx context.Context, event *mqueue.PlatformEvent, invent if err != nil { return nil, nil, errors.Wrap(err, "unable to get updates data") } + if updatesData == nil { utils.LogWarn("inventoryID", inventoryID, "No vmaas updates") 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,32 +279,25 @@ 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 } -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 { @@ -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,19 +448,37 @@ func commitWithObserve(tx *gorm.DB) error { return nil } -func evaluateAndStore(tx *gorm.DB, system *models.SystemPlatform, +// 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 { - newSystemAdvisories, err := analyzeAdvisories(tx, system, vmaasData) + advisoriesByName, err := lazySaveAndLoadAdvisories(system, vmaasData) if err != nil { - return errors.Wrap(err, "Advisory analysis failed") + return errors.Wrap(err, "Advisory loading failed") } - installed, installable, applicable, err := analyzePackages(tx, system, vmaasData) + pkgByName, installed, installable, applicable, err := lazySaveAndLoadPackages(system, vmaasData) if err != nil { - return errors.Wrap(err, "Package analysis failed") + return errors.Wrap(err, "Package loading failed") } - err = updateSystemPlatform(tx, system, newSystemAdvisories, installed, installable, applicable) + tx := database.DB.WithContext(base.Context).Begin() + // Don't allow requested TX to hang around locking the rows + defer tx.Rollback() + + systemAdvisoriesNew, err := storeAdvisoryData(tx, system, advisoriesByName) + if err != nil { + evaluationCnt.WithLabelValues("error-store-advisories").Inc() + return errors.Wrap(err, "Unable to store advisory data") + } + + err = updateSystemPackages(tx, system, pkgByName) + if err != nil { + evaluationCnt.WithLabelValues("error-system-pkgs").Inc() + return errors.Wrap(err, "Unable to update system packages") + } + + 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") @@ -479,7 +486,7 @@ func evaluateAndStore(tx *gorm.DB, 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(), @@ -487,11 +494,27 @@ 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 incrementAdvisoryTypeCounts(advisory models.AdvisoryMetadata, enhCount, bugCount, secCount *int) { + switch advisory.AdvisoryTypeID { + case enhancement: + *enhCount++ + case bugfix: + *bugCount++ + case security: + *secCount++ + } +} + +func analyzeRepos(system *models.SystemPlatform) (thirdParty bool, err error) { if !enableRepoAnalysis { utils.LogInfo("repo analysis disabled, skipping") return false, nil @@ -500,7 +523,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). @@ -517,7 +540,7 @@ func analyzeRepos(tx *gorm.DB, system *models.SystemPlatform) ( // 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) @@ -529,7 +552,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 @@ -540,26 +563,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++ } @@ -624,15 +633,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_advisories.go b/evaluator/evaluate_advisories.go index 4549c333d..259d80c3d 100644 --- a/evaluator/evaluate_advisories.go +++ b/evaluator/evaluate_advisories.go @@ -12,162 +12,146 @@ import ( "gorm.io/gorm/clause" ) -func analyzeAdvisories(tx *gorm.DB, system *models.SystemPlatform, vmaasData *vmaas.UpdatesV3Response) ( - SystemAdvisoryMap, error) { +// 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 { - 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 := lazySaveAdvisories(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) + stored, err := loadSystemAdvisories(database.DB, system.RhAccountID, system.ID) if err != nil { - evaluationCnt.WithLabelValues("error-store-advisories").Inc() - return nil, errors.Wrap(err, "Unable to store advisory data") + return nil, errors.Wrap(err, "Unable to load system advisories") } - return newSystemAdvisories, 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) ( - []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) + merged, err := evaluateChanges(vmaasData, stored) if err != nil { - return nil, nil, nil, errors.Wrap(err, "Unable to get system stored advisories") + return nil, errors.Wrap(err, "Unable to evaluate advisory changes") } - deleteIDs, installableNames, applicableNames := getAdvisoryChanges(reported, oldSystemAdvisories) - updatesCnt.WithLabelValues("patched").Add(float64(len(deleteIDs))) - utils.LogInfo("inventoryID", system.InventoryID, "fixed", len(deleteIDs), "fixed advisories") + return merged, nil +} - installableIDs, missingInstallableNames, err := getAdvisoriesFromDB(tx, installableNames) - 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") +// 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)) + 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, + } + } else { + extendedAdvisories[reportedName] = extendedAdvisory{ + change: Keep, + SystemAdvisories: storedAdvisory, + } + } + } else { + extendedAdvisories[reportedName] = extendedAdvisory{ + change: Add, + SystemAdvisories: models.SystemAdvisories{ + StatusID: reportedStatusID, + }, + } + missingNames = append(missingNames, reportedName) + } } - installableIDs = append(installableIDs, missingInstallableIDs...) - updatesCnt.WithLabelValues("installable").Add(float64(len(installableIDs))) - utils.LogInfo("inventoryID", system.InventoryID, "installable", len(installableIDs), "installable advisories") + return extendedAdvisories, missingNames +} - applicableIDs, missingApplicableNames, err := getAdvisoriesFromDB(tx, applicableNames) +func loadMissingNamesIDs(missingNames []string, extendedAdvisories *extendedAdvisoryMap) error { + advisoryMetadata, err := getAdvisoryMetadataByNames(missingNames) if err != nil { - return nil, nil, nil, errors.Wrap(err, "Unable to ensure new applicable system advisories in db") + return err } - nUnknown = len(applicableNames) - len(applicableIDs) - if nUnknown > 0 { - utils.LogInfo("inventoryID", system.InventoryID, "unknown", nUnknown, "unknown applicable advisories") - updatesCnt.WithLabelValues("unknown").Add(float64(nUnknown)) + + name2AdvisoryID := make(map[string]int64, len(advisoryMetadata)) + for _, am := range advisoryMetadata { + name2AdvisoryID[am.Name] = am.ID } - missingApplicableIDs, err := storeMissingAdvisories(missingApplicableNames) - if err != nil { - return nil, nil, nil, errors.Wrap(err, "unable to store unknown applicable advisories in db") + + 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 } - 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 + return nil } -func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, - deleteIDs, installableIDs, applicableIDs []int64) (SystemAdvisoryMap, error) { - defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("advisories-store")) - newSystemAdvisories, err := updateSystemAdvisories(tx, system, deleteIDs, installableIDs, applicableIDs) - if err != nil { - return nil, errors.Wrap(err, "Unable to update system advisories") - } - - err = updateAdvisoryAccountData(tx, system, deleteIDs, installableIDs, applicableIDs) - if err != nil { - return nil, errors.Wrap(err, "Unable to update advisory_account_data caches") +// 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, + SystemAdvisories: storedAdvisory, + } + } } - return newSystemAdvisories, 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 +// 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 } - advisoriesMap := make(map[string]models.SystemAdvisories, len(advisories)) - for _, advisory := range advisories { - advisoriesMap[advisory.Advisory.Name] = advisory - } - return advisoriesMap, nil + parseStored(stored, reported, &extendedAdvisories) + + return extendedAdvisories, 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)) - 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 storedAdvisory, storedAdvisoryObj := range stored { - if _, found := reported[storedAdvisory]; !found { - // advisory was patched from last evaluation,let's remove it - deleteIDs = append(deleteIDs, storedAdvisoryObj.AdvisoryID) - } +// 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 { + return nil } - return deleteIDs, installableNames, applicableNames -} -// 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). - Select("id, name"). - Scan(&advisoryMetadata).Error + missingNames, err := getMissingAdvisories(reportedNames) if err != nil { - return nil, nil, err + return errors.Wrap(err, "Unable to get missing system advisories") } - missing := []string{} - found := make(map[string]bool, len(advisories)) - for _, am := range advisoryMetadata { - found[am.Name] = true - advisoryIDs = append(advisoryIDs, am.ID) + nUnknown := len(missingNames) + if nUnknown == 0 { + return nil } - for _, name := range advisories { - if !found[name] { - missing = append(missing, name) - } + utils.LogInfo("inventoryID", inventoryID, "unknown", nUnknown, "unknown advisories") + updatesCnt.WithLabelValues("unknown").Add(float64(nUnknown)) + + err = storeMissingAdvisories(missingNames) + if err != nil { + return errors.Wrap(err, "Failed to save missing advisory_metadata") } - return advisoryIDs, missing, err + + return nil } -func storeMissingAdvisories(missing []string) ([]int64, error) { - toStore := make(models.AdvisoryMetadataSlice, 0, len(missing)) - for _, name := range missing { +func storeMissingAdvisories(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, @@ -180,118 +164,129 @@ func storeMissingAdvisories(missing []string) ([]int64, error) { }) } } - storedIDs, err := lazySaveAdvisories(toStore) - if err != nil { - return nil, errors.Wrap(err, "failed to save advisory_metadata") + + if len(toStore) == 0 { + return nil } - return storedIDs, nil + + tx := database.DB.Begin() + defer tx.Commit() + return tx.Clauses(clause.OnConflict{DoNothing: true}).Create(&toStore).Error } -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 getAdvisoryMetadataByNames(names []string) (models.AdvisoryMetadataSlice, 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 } -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}) +// 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 { + return nil, err } - tx = database.OnConflictUpdateMulti(tx, []string{"rh_account_id", "system_id", "advisory_id"}, "status_id") - err := database.BulkInsert(tx, advisoriesObjs) - return err -} + found := make(map[string]bool, len(advisoryNames)) + for _, am := range advisoryMetadata { + found[am.Name] = true + } -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 + missingNames := make([]string, 0, len(advisoryNames)-len(advisoryMetadata)) + for _, name := range advisoryNames { + if !found[name] { + missingNames = append(missingNames, name) + } + } - return err + return missingNames, nil } -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 - if system.Stale { - return []models.AdvisoryAccountData{} +func storeAdvisoryData(tx *gorm.DB, system *models.SystemPlatform, advisoriesByName extendedAdvisoryMap) ( + SystemAdvisoryMap, error) { + if !enableAdvisoryAnalysis { + utils.LogInfo("advisory analysis disabled, skipping storing") + return nil, nil } - aadMap := make(map[int64]models.AdvisoryAccountData, len(installableIDs)) + defer utils.ObserveSecondsSince(time.Now(), evaluationPartDuration.WithLabelValues("advisories-store")) + err := updateSystemAdvisories(tx, system, advisoriesByName) + if err != nil { + return nil, errors.Wrap(err, "Unable to update system advisories") + } - for _, id := range installableIDs { - aadMap[id] = models.AdvisoryAccountData{ - AdvisoryID: id, - RhAccountID: system.RhAccountID, - SystemsInstallable: 1, - // every installable advisory is also applicable advisory - SystemsApplicable: 1, - } + // reload all after updates + systemAdvisoriesNew, err := loadSystemAdvisories(tx, system.RhAccountID, system.ID) + if err != nil { + return nil, err } - 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, - SystemsApplicable: 1, - } - } + err = updateAdvisoryAccountData(tx, system, advisoriesByName) + if err != nil { + return nil, errors.Wrap(err, "Unable to update advisory_account_data caches") } + return systemAdvisoriesNew, nil +} - for _, id := range deleteIDs { - aadMap[id] = models.AdvisoryAccountData{ - AdvisoryID: id, - RhAccountID: system.RhAccountID, - SystemsInstallable: -1, - } - if !isApplicable[id] { - // advisory is no longer applicable - aad := aadMap[id] - aad.SystemsApplicable = -1 - aadMap[id] = aad +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(advisoriesByName)) + for _, advisory := range advisoriesByName { + switch advisory.change { + case Remove: + aadMap[advisory.AdvisoryID] = models.AdvisoryAccountData{ + AdvisoryID: advisory.AdvisoryID, + RhAccountID: system.RhAccountID, + SystemsInstallable: -1, + } + if advisory.StatusID != APPLICABLE { // advisory is no longer applicable + aad := aadMap[advisory.AdvisoryID] + aad.SystemsApplicable = -1 + aadMap[advisory.AdvisoryID] = aad + } + case Keep: + continue + case Add: + fallthrough + case Update: + if advisory.StatusID == INSTALLABLE { + aadMap[advisory.AdvisoryID] = models.AdvisoryAccountData{ + AdvisoryID: advisory.AdvisoryID, + RhAccountID: system.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, + } + } + } } } - deltas := make([]models.AdvisoryAccountData, 0, len(deleteIDs)+len(installableIDs)+len(applicableIDs)) + // 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 { @@ -302,42 +297,64 @@ func deleteOldSystemAdvisories(tx *gorm.DB, accountID int, systemID int64, patch return err } +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) { + deleteIDs := make([]int64, 0, len(advisoriesByName)) + advisoryObjs := make(models.SystemAdvisoriesSlice, 0, len(advisoriesByName)) + for _, 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) + } + } + return deleteIDs, advisoryObjs +} + func updateSystemAdvisories(tx *gorm.DB, system *models.SystemPlatform, - deleteIDs, installableIDs, applicableIDs []int64) (SystemAdvisoryMap, error) { - // this will remove many many old items from our "system_advisories" table + advisoriesByName extendedAdvisoryMap) error { + deleteIDs, advisoryObjs := processAdvisories(system, advisoriesByName) + 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 upsertSystemAdvisories(tx, advisoryObjs) +} - newSystemAdvisories := SystemAdvisoryMap{} +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 = ?", system.ID, system.RhAccountID).Error - + 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, - applicableIDs []int64) error { - err := lockAdvisoryAccountData(tx, system, deleteIDs, installableIDs, applicableIDs) - if err != nil { - return err - } - - changes := calcAdvisoryChanges(system, deleteIDs, installableIDs, applicableIDs) +func updateAdvisoryAccountData(tx *gorm.DB, system *models.SystemPlatform, advisoriesByName extendedAdvisoryMap) error { + changes := calcAdvisoryChanges(system, advisoriesByName) if len(changes) == 0 { return nil @@ -351,3 +368,17 @@ 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 + +const ( + undefinedChangeType int = iota + enhancement + bugfix + security +) diff --git a/evaluator/evaluate_advisories_test.go b/evaluator/evaluate_advisories_test.go index 48565b119..99f46cbfc 100644 --- a/evaluator/evaluate_advisories_test.go +++ b/evaluator/evaluate_advisories_test.go @@ -38,18 +38,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)) } @@ -70,11 +59,11 @@ 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() - 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)) @@ -83,19 +72,117 @@ 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() + 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]) + // 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) { +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 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 + 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) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() @@ -103,55 +190,156 @@ func TestUpdatePatchedSystemAdvisories(t *testing.T) { 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) + 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, advisoriesByName) + 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{}) + // Update as if the advisories became unpatched + for name, ea := range advisoriesByName { + ea.change = Add + advisoriesByName[name] = ea + } + err = updateAdvisoryAccountData(database.DB, &system, advisoriesByName) assert.NoError(t, err) - database.CheckAdvisoriesAccountData(t, system.RhAccountID, advisoryIDs, 1) + database.DeleteSystemAdvisories(t, system.ID, advisoryIDs) database.DeleteAdvisoryAccountData(t, system.RhAccountID, advisoryIDs) } -func TestGetAdvisoriesFromDB(t *testing.T) { +func TestGetMissingAdvisories(t *testing.T) { utils.SkipWithoutDB(t) core.SetupTestEnvironment() advisories := []string{"ER-1", "RH-1", "ER-2", "RH-2"} - advisoryIDs, missingNames, err := getAdvisoriesFromDB(database.DB, advisories) + advisoryIDs := getAdvisoryIDsByNames(t, advisories) + missingNames, err := getMissingAdvisories(advisories) assert.Nil(t, err) 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(database.DB, 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 TestEnsureSystemAdvisories(t *testing.T) { +func TestProcessAdvisories(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{}) + 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 := processAdvisories(&system, extendedAdvisories) + assert.Equal(t, 0, len(deleteIDs)) + assert.Equal(t, 2, len(advisoryObjs)) +} + +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, advisoryIDs) - database.DeleteSystemAdvisories(t, systemID, advisoryIDs) + 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) { + system := models.SystemPlatform{ID: systemID, RhAccountID: rhAccountID} + advisoriesByName := extendedAdvisoryMap{ + "ER-102": { + change: Update, + SystemAdvisories: models.SystemAdvisories{AdvisoryID: int64(102), StatusID: INSTALLABLE}, + }, + "ER-103": { + change: Remove, + SystemAdvisories: models.SystemAdvisories{AdvisoryID: int64(103), StatusID: INSTALLABLE}, + }, + "ER-104": { + change: Remove, + SystemAdvisories: models.SystemAdvisories{AdvisoryID: int64(104), StatusID: APPLICABLE}, + }, + "ER-105": { + change: Add, + SystemAdvisories: models.SystemAdvisories{AdvisoryID: int64(105), StatusID: APPLICABLE}, + }, + } + + changes := calcAdvisoryChanges(&system, advisoriesByName) + expected := map[int64]models.AdvisoryAccountData{ + 102: {SystemsApplicable: 1, SystemsInstallable: 1}, + 103: {SystemsApplicable: -1, SystemsInstallable: -1}, + 104: {SystemsInstallable: -1}, + 105: {SystemsApplicable: 1}, + } + assert.Equal(t, len(expected), len(changes)) + for _, change := range changes { + advisoryID := change.AdvisoryID + assert.Equal(t, change.SystemsApplicable, expected[advisoryID].SystemsApplicable) + assert.Equal(t, change.SystemsInstallable, expected[advisoryID].SystemsInstallable) + } } func getVMaaSUpdates(t *testing.T) vmaas.UpdatesV3Response { @@ -163,3 +351,27 @@ func getVMaaSUpdates(t *testing.T) vmaas.UpdatesV3Response { assert.Nil(t, resp.Body.Close()) return vmaasData } + +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} +} + +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/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)) diff --git a/evaluator/evaluate_packages.go b/evaluator/evaluate_packages.go index ead55865e..e4ea33411 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 { +// LazySavePackages adds unknown EVRAs into the db if needed. +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,25 +82,25 @@ 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 } } return &pkg } -// Get packages with known name but version missing in db/cache -func getMissingPackages(tx *gorm.DB, vmaasData *vmaas.UpdatesV3Response) models.PackageSlice { +// 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)) 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) } } @@ -135,13 +130,13 @@ func updatePackageNameDB(missing *models.PackageName) error { return nil } -// 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) { +// 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")) - 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..37c333aad 100644 --- a/evaluator/remediations.go +++ b/evaluator/remediations.go @@ -55,6 +55,22 @@ 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 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)) +}