Skip to content

[COST-8115] Fix partition-drop deadlock in OCP retention purge - #6270

Open
jordigilh wants to merge 3 commits into
project-koku:mainfrom
jordigilh:fix/partition-drop-lock-gap
Open

[COST-8115] Fix partition-drop deadlock in OCP retention purge#6270
jordigilh wants to merge 3 commits into
project-koku:mainfrom
jordigilh:fix/partition-drop-lock-gap

Conversation

@jordigilh

Copy link
Copy Markdown
Contributor

Summary

COST-7249 deadlock preflight, expanded audit, Finding H (see also companion Findings E/F/G in #6268, #6269).

OCPReportDBCleaner.purge_expired_report_data_by_date (invoked by the remove_expired_data Celery beat task) drops expired partitions for rates_to_usage and every UI_SUMMARY_TABLES entry via one combined execute_delete_sql(PartitionedTable.objects.filter(partition_of_table_name__in=table_names, ...)) call.

Deleting a PartitionedTable row fires trfn_partition_manager()'s DELETE branch: ALTER TABLE ... DETACH PARTITION (ACCESS EXCLUSIVE on the parent table, not just the partition being dropped) + TRUNCATE + DROP TABLE. Since table_names spans multiple parent tables and the whole thing is one SQL statement / one transaction, a single purge run can hold ACCESS EXCLUSIVE locks on several different parent tables simultaneously whenever it has expired partitions on more than one of them -- which is the normal case, not an edge case.

This reproduces Postgres's documented ATTACH/DETACH-partition "queue-jump" deadlock: if a concurrent writer touches two of those same tables in the opposite order within one transaction, Postgres's lock-queue fairness rule turns ordinary contention into a genuine wait-for cycle, and the deadlock detector aborts one side.

This is a different statement from the one Finding F (#6269) fixed in the same method -- that fix covers the later cascade_delete(all_usage_periods...) call. This partition-drop step, earlier in the same method, had no lock of any kind, and a provider-scoped advisory lock (as used elsewhere in this audit) wouldn't help here anyway, since DETACH/CREATE PARTITION DDL locks the whole parent table regardless of which provider's data triggered it.

Fix

Split the combined multi-table execute_delete_sql call into one call per table name, so the drop never holds ACCESS EXCLUSIVE on more than one parent table at a time. A wait on a single resource cannot form a cycle, eliminating this deadlock class without adding a new locking primitive.

Testing

New spike/regression tests in masu/test/database/test_partition_drop_write_deadlock.py, run against a real Postgres test database:

  • test_unrelated_write_blocks_behind_unprotected_partition_drop -- confirms an unrelated write to a different, active partition of the same parent table stalls behind the unprotected DROP (contention, not yet a deadlock).
  • test_cross_table_partition_drop_deadlocks_against_cross_table_writer -- characterization test: two sessions touching the same two parent tables in opposite order reproduce a genuine Postgres deadlock detected error. Confirmed reproducible across 5 consecutive runs. Kept permanently to document the underlying mechanism.
  • test_fixed_per_table_partition_drop_does_not_deadlock_against_cross_table_writer -- regression test: the same adversarial writer completes cleanly once the drop is split per-table (the fix).

Also ran the full existing suites for masu.test.processor.ocp.test_ocp_report_db_cleaner, koku.test_pg_partition, and the AWS/Azure/GCP report DB cleaner suites -- all pass unmodified (37 + 11 + 3 = 51 tests, no regressions).

Related

The sibling AWS/Azure/GCP report DB cleaners use the same combined multi-table execute_delete_sql pattern for their own partition-drop steps and are very likely exposed to the same mechanism (observed while running their test suites during this spike). Tracked separately so this fix can land narrowly scoped to OCP.

Fixes COST-8115.

Test plan

  • New deterministic spike/regression tests added and passing (5/5 stable runs)
  • Existing test_ocp_report_db_cleaner suite passes unmodified
  • black/flake8 clean
  • Maintainer review

purge_expired_report_data_by_date dropped expired partitions for
rates_to_usage and every UI_SUMMARY_TABLES entry via one combined
execute_delete_sql call spanning all of them. Deleting a
PartitionedTable row fires an ACCESS EXCLUSIVE DETACH/TRUNCATE/DROP on
its parent table, so one statement/transaction could hold that lock on
several different parent tables at once -- the precondition for
Postgres's documented ATTACH/DETACH-partition queue-jump deadlock
against any concurrent writer touching the same tables in the opposite
order.

Split the drop into one execute_delete_sql call per table so no single
transaction ever holds ACCESS EXCLUSIVE on more than one parent table,
eliminating the cross-table deadlock cycle.

Spiked and confirmed with a real Postgres test database (deadlock
reproduced deterministically across 5 runs); see
masu/test/database/test_partition_drop_write_deadlock.py.
@jordigilh jordigilh added smokes-required Label to show that smokes tests should be run against these changes. ocp-smoke-tests pr_check will run ocp + ocp on cloud smoke tests, used when changes affect ocp. labels Aug 19, 2026
@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@jordigilh, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 59 minutes

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 03e44c73-860b-4a49-bf14-df37b453884c

📥 Commits

Reviewing files that changed from the base of the PR and between 768be82 and e3b0019.

📒 Files selected for processing (3)
  • koku/masu/processor/ocp/ocp_report_db_cleaner.py
  • koku/masu/test/database/test_partition_drop_write_deadlock.py
  • koku/masu/test/processor/ocp/test_phase2_rates_to_usage.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Finding H's fix replaced a single combined multi-table
PartitionedTable.objects.filter(...) call with one call per table
name, so the existing test's assertion against the last call's args
no longer saw the full table list. Updated to aggregate across all
calls instead.

@koku-ci-triager-bot koku-ci-triager-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 CI Triager — Suggestion

Check: Sanity
Root cause: reorder-python-imports flagged the import order in the new test file. reporting.provider.models must come before reporting.provider.ocp.models alphabetically.

Accept the suggestion below with one click.

Comment on lines +76 to +77
from reporting.provider.ocp.models import OCPCostSummaryP
from reporting.provider.models import TenantAPIProvider

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Swap these two imports so provider.models comes before provider.ocp.models.

Suggested change
from reporting.provider.ocp.models import OCPCostSummaryP
from reporting.provider.models import TenantAPIProvider
from reporting.provider.models import TenantAPIProvider
from reporting.provider.ocp.models import OCPCostSummaryP

@koku-ci-triager-bot

Copy link
Copy Markdown
Collaborator

🤖 CI Triager — Diagnosis

Check: Units - 3.11
Root cause: The check_migrations step failed because GitHub Actions could not find Django in the virtual environment — a stale pip cache, not a code problem. The script itself detects this and emits: Django is not installed or the virtual environment isn't activated. Maybe clear github-action cache?

Evidence:

ModuleNotFoundError: No module named 'django'
...
Django is not installed or the virtual environment isn't activated. Maybe clear github-action cache?

Action: Re-run the failing Units - 3.11 check. If it fails again with the same error, ask a maintainer to clear the GitHub Actions cache for this repository.

Generated automatically. Review before applying.

Pure cleanup post-TDD-green: test_partition_drop_write_deadlock.py
imported 'time' locally in two separate methods instead of once at
module scope. No behavior change; reorder-python-imports also fixed
a pre-existing import-ordering nit in the same file.
@koku-ci-triager-bot

Copy link
Copy Markdown
Collaborator

🤖 CI Triager — Diagnosis

Check: Units - 3.11
Run: https://github.com/project-koku/koku/actions/runs/32308499887


Root cause: The check_migrations step failed with a GitHub Actions pip cache staleness issue — the runner found Python 3.12 instead of 3.11, so the cached virtualenv had no Django installed. This is a transient CI infrastructure problem unrelated to this PR's code changes.

Evidence:

Warning: Your Pipfile requires "python_version" 3.11, but you are using 3.12.3
from /usr/bin/python3.
$ pipenv --rm and rebuilding the virtual environment may resolve the issue.
ModuleNotFoundError: No module named 'django'
Django is not installed or the virtual environment isn't activated. Maybe clear github-action cache?

Action: Re-run the failing Units - 3.11 check. If it fails again with the same error, ask a maintainer to clear the GitHub Actions cache for this repository.

Generated automatically. Review before applying.

@koku-ci-triager-bot

Copy link
Copy Markdown
Collaborator

🤖 CI Triager — Diagnosis

Check: Red Hat Konflux / koku-ci / koku
PipelineRun: koku-ci-77g65 (SHA e3b00199, completed 2026-08-20T04:19)


Root cause: IQE infra/environment issue — an OCP-on-AWS multi-source ingest timed out after 1600 seconds (26 min) waiting for the July ocp_on_cloud_updated_datetime field to be populated. The August manifest completed successfully (process_complete_date: 2026-08-20 03:37:02), but the July OCP-on-cloud correlation stalled. This pattern (ingest completes but OCP-on-cloud correlation never finishes) is not caused by the partition-drop loop serialization change in this PR — that change affects the retention purge path (OCPReportDBCleaner), not the ingest/cost-update pipeline.

Evidence:

2026-08-20 04:02:54 ERROR Couldn't complete 'function check_ingest_complete()'
                           in time, took 1600.00, 159 tries
{'provider_linked': True,
 'data_updated_date': '2026-08-20 03:38:36',
 '2026-07-01': {
   'ocp_on_cloud': [{'ocp_source_uuid': '24349769-...',
                     'ocp_on_cloud_updated_datetime': ''}]  # never set
 }}
XFAIL (Source fixture failed)  [44%]
Some jobs failed: {'koku-iqe-vlwa1qk': 'Failed'}

Action: Re-trigger the smoke tests. The OCP-on-cloud correlation stall is an ephemeral environment / pipeline scheduling issue unrelated to the partition DROP serialization change introduced by this PR.

Generated automatically. Review before applying.

@jordigilh jordigilh added the flightpath-pr Issues being worked on by the flight path team label Aug 20, 2026
@jordigilh

Copy link
Copy Markdown
Contributor Author

/retest

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.4%. Comparing base (ddbcac3) to head (e3b0019).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #6270     +/-   ##
=======================================
- Coverage   94.4%   94.4%   -0.0%     
=======================================
  Files        369     369             
  Lines      33578   33580      +2     
  Branches    3755    3756      +1     
=======================================
+ Hits       31708   31709      +1     
- Misses      1210    1211      +1     
  Partials     660     660             
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jordigilh
jordigilh marked this pull request as ready for review August 20, 2026 16:26
@jordigilh
jordigilh requested review from a team as code owners August 20, 2026 16:26
@koku-ci-triager-bot

Copy link
Copy Markdown
Collaborator

🤖 CI Triager — Diagnosis

Check: Red Hat Konflux / koku-ci / koku
PipelineRun: koku-ci-fc7dq (SHA e3b00199, completed 2026-08-20T19:28:31Z)


Root cause: IQE infra/environment issue — the ephemeral koku API pod became unavailable late in the test run. All 108 failures are iqe_cost_management_api.exceptions.ApiException: (502) Bad Gateway from http://koku-clowder-api.ephemeral-xx5yow.svc:8000, all timestamped 2026-08-20 19:18:53 (a single simultaneous crash). Tests in test__ocp_on_aws_costs.py and test__ocp_on_aws_instance.py that ran before 19:18 passed normally; only those starting after the pod crash failed.

Evidence:

E  iqe_cost_management_api.exceptions.ApiException: (502)
E  Reason: Bad Gateway
E  HTTP response headers: {'Date': 'Thu, 20 Aug 2026 19:18:53 GMT', ...}
E  HTTP response body: <html><head><title>502 Bad Gateway</title></head>...

= 108 failed, 4097 passed, 15 skipped in 9979.78s =

Action: This is a transient ephemeral environment failure unrelated to the PR's changes (the PR only touches ocp_report_db_cleaner.py and test files). Re-trigger the koku-ci check. If it fails again with a different error, investigate further.

Generated automatically. Review before acting.

@jordigilh

Copy link
Copy Markdown
Contributor Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flightpath-pr Issues being worked on by the flight path team ocp-smoke-tests pr_check will run ocp + ocp on cloud smoke tests, used when changes affect ocp. smokes-required Label to show that smokes tests should be run against these changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants