Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions deploy/clowdapp.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -566,10 +566,10 @@ objects:
- {name: DB_DEBUG, value: '${DB_DEBUG_JOBS}'}
- {name: POD_CONFIG, value: '${JOBS_CONFIG}'}

- name: clean-advisory-account-data
- name: clean-account-advisory
activeDeadlineSeconds: ${{JOBS_TIMEOUT}}
schedule: ${CLEAN_AAD_SCHEDULE}
suspend: ${{CLEAN_AAD_SUSPEND}}
schedule: ${CLEAN_AA_SCHEDULE}
suspend: ${{CLEAN_AA_SUSPEND}}
concurrencyPolicy: Forbid
podSpec:
image: ${IMAGE}:${IMAGE_TAG}
Expand All @@ -583,7 +583,7 @@ objects:
command:
- ./scripts/entrypoint.sh
- job
- clean_advisory_account_data
- clean_account_advisory
env:
- {name: LOG_LEVEL, value: '${LOG_LEVEL_JOBS}'}
- {name: GIN_MODE, value: '${GIN_MODE}'}
Expand Down Expand Up @@ -931,9 +931,9 @@ parameters:
# Repack
- {name: REPACK_SCHEDULE, value: '0 11 * * 5'} # Cronjob schedule definition
- {name: REPACK_SUSPEND, value: 'false'} # Disable cronjob execution
# Clean advisory_account_data
- {name: CLEAN_AAD_SCHEDULE, value: '0 12 * * *'} # Cronjob schedule definition
- {name: CLEAN_AAD_SUSPEND, value: 'false'} # Disable cronjob execution
# Clean account_advisory
- {name: CLEAN_AA_SCHEDULE, value: '0 12 * * *'} # Cronjob schedule definition
- {name: CLEAN_AA_SUSPEND, value: 'false'} # Disable cronjob execution

# Database admin
- {name: MIGRATION_TIMEOUT, value: '7200'} # 2h timeout for db-migration job
Expand Down
4 changes: 2 additions & 2 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ func runJob(name string) {
caches.RunPackageRefresh()
case "repack":
repack.RunRepack()
case "clean_advisory_account_data":
cleaning.RunCleanAdvisoryAccountData()
case "clean_account_advisory":
cleaning.RunCleanAccountAdvisory()
case "system_advisories_0_recovery":
system_advisories_0_recovery.Run()
}
Expand Down
72 changes: 72 additions & 0 deletions tasks/cleaning/clean_account_advisory.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package cleaning

import (
"app/base/core"
"app/base/models"
"app/base/utils"
"app/tasks"
"sync"
)

func RunCleanAccountAdvisory() {
tasks.HandleContextCancel(tasks.WaitAndExit)
core.ConfigureApp()
defer utils.LogPanics(true)

var wg sync.WaitGroup
var aadErr, aaErr error
wg.Add(2)

go func() {
defer wg.Done()
Comment thread
sourcery-ai[bot] marked this conversation as resolved.
utils.LogInfo("Deleting advisory rows with 0 applicable/installable systems from advisory_account_data")
aadErr = CleanAdvisoryAccountData()
if aadErr != nil {
utils.LogError("err", aadErr, "Cleaning advisory_account_data")
}
}()

go func() {
defer wg.Done()
utils.LogInfo("Deleting advisory rows with 0 applicable/installable systems from account_advisory")
aaErr = CleanAccountAdvisory()
if aaErr != nil {
utils.LogError("err", aaErr, "Cleaning account_advisory")
}
}()

wg.Wait()
if aadErr != nil || aaErr != nil {
utils.LogWarn("RunCleanAccountAdvisory task completed with errors")
} else {
utils.LogInfo("RunCleanAccountAdvisory task performed successfully")
}
}

func CleanAdvisoryAccountData() error {
tx := tasks.CancelableDB().Begin()
defer tx.Rollback()

result := tx.Delete(&models.AdvisoryAccountData{}, "systems_installable <= 0 AND systems_applicable <= 0")
if result.Error != nil {
return result.Error
}

tx.Commit()
Comment on lines +46 to +55

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Avoid deferring Rollback after a successful Commit and check Commit errors

With Begin + defer tx.Rollback() + tx.Commit(), Rollback still runs after Commit, which can generate spurious errors/logs depending on the driver, and Commit errors are currently ignored.

Prefer an explicit pattern that:

  • Checks Begin error
  • Uses defer only for panic recovery
  • Rolls back on intermediate errors
  • Checks and returns the Commit error

For example:

func CleanAdvisoryAccountData() error {
    tx := tasks.CancelableDB().Begin()
    if tx.Error != nil {
        return tx.Error
    }

    defer func() {
        if r := recover(); r != nil {
            tx.Rollback()
            panic(r)
        }
    }()

    result := tx.Delete(&models.AdvisoryAccountData{}, "systems_installable <= 0 AND systems_applicable <= 0")
    if result.Error != nil {
        tx.Rollback()
        return result.Error
    }

    if err := tx.Commit().Error; err != nil {
        return err
    }

    utils.LogInfo("nDeleted", result.RowsAffected, "advisory_account_data cleaned successfully")
    return nil
}

Same fix applies to CleanAccountAdvisory().

utils.LogInfo("nDeleted", result.RowsAffected, "advisory_account_data cleaned successfully")
return nil
}

func CleanAccountAdvisory() error {
tx := tasks.CancelableDB().Begin()
defer tx.Rollback()

result := tx.Delete(&models.AccountAdvisory{}, "systems_installable <= 0 AND systems_applicable <= 0")
if result.Error != nil {
return result.Error
}

tx.Commit()
utils.LogInfo("nDeleted", result.RowsAffected, "account_advisory cleaned successfully")
return nil
}
34 changes: 0 additions & 34 deletions tasks/cleaning/clean_advisory_account_data.go

This file was deleted.

2 changes: 2 additions & 0 deletions tasks/cleaning/clean_unused_data.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,14 @@ func deleteUnusedAdvisories() {
// remove unused advisories not synced from vmaas
// before changing the query below test its performance on big data otherwise it can lock database
// Time: 18988.223 ms (00:18.988) for 50k advisories, 75M system_advisories, 1.6M package and 50k rh_account
// both advisory_account_data and account_advisory are checked temporarily until the legacy table is dropped
subq := tx.Select("id").Table("advisory_metadata am").
Where("am.synced = ?", false).
Where("NOT EXISTS (SELECT 1 FROM system_advisories sa WHERE am.id = sa.advisory_id)").
Where("NOT EXISTS (SELECT 1 FROM template_advisory ta WHERE am.id = ta.advisory_id)").
Where("NOT EXISTS (SELECT 1 FROM package p WHERE am.id = p.advisory_id)").
Where("NOT EXISTS (SELECT 1 FROM advisory_account_data aad WHERE am.id = aad.advisory_id)").
Where("NOT EXISTS (SELECT 1 FROM account_advisory aa WHERE am.id = aa.advisory_id)").
Limit(tasks.DeleteUnusedDataLimit)

err := tx.Delete(&models.AdvisoryMetadata{}, "id IN (?)", subq).Error
Expand Down
41 changes: 41 additions & 0 deletions tasks/cleaning/clean_unused_data_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"app/base/utils"
"testing"

"github.com/google/uuid"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -95,3 +96,43 @@ func TestCleanUnusedAdvisories(t *testing.T) {
assert.Nil(t, err)
assert.Equal(t, beforeAdvCount-rh100count, afterAdvCount)
}

func TestDeleteUnusedAdvisoriesKeptByAccountAdvisory(t *testing.T) {
utils.SkipWithoutDB(t)
core.SetupTestEnvironment()

advisory := "CUSTOM-5678"
customAdv := models.AdvisoryMetadata{
Name: advisory,
Description: "Custom desc",
Synopsis: "Custom syn",
Summary: "Custom sum",
Solution: utils.PtrString("Custom sol"),
AdvisoryTypeID: 1,
RebootRequired: false,
Synced: false,
}
err := database.DB.Create(&customAdv).Error
assert.Nil(t, err)

aa := models.AccountAdvisory{
AdvisoryID: customAdv.ID,
RhAccountID: 1,
WorkspaceID: uuid.MustParse("00000000-0000-0000-0000-000000000001"),
SystemsApplicable: 1,
SystemsInstallable: 0,
}
err = database.DB.Create(&aa).Error
assert.Nil(t, err)

deleteUnusedAdvisories()

var count int64
err = database.DB.Model(models.AdvisoryMetadata{}).Where("name = ?", advisory).Count(&count).Error
assert.Nil(t, err)
assert.Equal(t, int64(1), count, "advisory with account_advisory row should not be deleted")

// cleanup
database.DB.Delete(&aa)
database.DB.Delete(&customAdv)
}
Loading