Skip to content

Commit

Permalink
RHINENG-8141: fix data flow and tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@Dugowitch
Dugowitch committed Aug 7, 2024
1 parent 8120c22 commit 39dffbe
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 88 deletions.
134 changes: 64 additions & 70 deletions evaluator/evaluate_advisories.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
Expand All @@ -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
}
Expand All @@ -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
Expand Down
57 changes: 39 additions & 18 deletions evaluator/evaluate_advisories_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"app/base/utils"
"app/base/vmaas"
"context"
"fmt"
"net/http"
"strings"
"testing"
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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},
Expand Down

0 comments on commit 39dffbe

Please sign in to comment.