Skip to content

Commit c39cb0e

Browse files
committed
a4
1 parent a82d7e1 commit c39cb0e

5 files changed

Lines changed: 189 additions & 4 deletions

File tree

‎.github/workflows/backend-tiered-poc.yml‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,12 +104,27 @@ jobs:
104104
/tmp/tier2-tests.txt
105105
retention-days: 1
106106

107-
# Tier 1: Postgres + Redis only (fast setup)
107+
# Tier 1: Postgres + Redis + Redis-cluster (fast setup, no Snuba)
108108
tier1-test:
109109
needs: split-tiers
110110
name: "tier1 (${{ matrix.instance }})"
111111
runs-on: ubuntu-24.04
112112
timeout-minutes: 30
113+
114+
# Redis-cluster via services block (temporary; long-term: add to devservices mode)
115+
services:
116+
redis-cluster:
117+
image: ghcr.io/getsentry/docker-redis-cluster:7.0.10
118+
ports:
119+
- 7000:7000
120+
- 7001:7001
121+
- 7002:7002
122+
- 7003:7003
123+
- 7004:7004
124+
- 7005:7005
125+
env:
126+
IP: 0.0.0.0
127+
113128
strategy:
114129
fail-fast: false
115130
matrix:
@@ -118,6 +133,10 @@ jobs:
118133
env:
119134
MATRIX_INSTANCE_TOTAL: ${{ needs.split-tiers.outputs.tier1-shard-count }}
120135
TEST_GROUP_STRATEGY: roundrobin
136+
# Mock Kafka producers - Tier 1 tests don't need Kafka but application
137+
# code triggers produces as side effects of normal model operations.
138+
# See docs/service-classification-investigation.md for tradeoff analysis.
139+
SENTRY_MOCK_KAFKA_PRODUCERS: "1"
121140

122141
steps:
123142
- uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
@@ -138,8 +157,9 @@ jobs:
138157
env:
139158
SELECTED_TESTS_FILE: /tmp/tier1-tests.txt
140159
run: |
141-
echo "Running tier 1 (Postgres + Redis) tests..."
160+
echo "Running tier 1 (Postgres + Redis, no Snuba) tests..."
142161
echo "Test files: $(wc -l < /tmp/tier1-tests.txt)"
162+
echo "Kafka producers mocked: SENTRY_MOCK_KAFKA_PRODUCERS=$SENTRY_MOCK_KAFKA_PRODUCERS"
143163
make test-python-ci
144164
145165
- name: Inspect failure

‎docs/service-classification-investigation.md‎

Lines changed: 99 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -396,10 +396,107 @@ Each tier is independently sharded:
396396

397397
Both tiers run in parallel. Wall-clock time is dominated by Tier 2, but Tier 1 finishes much faster, freeing runners sooner.
398398

399+
## First Tiered CI Run
400+
401+
### Results
402+
403+
Tier 2 (Full Snuba stack): **All 9 shards passed.** No issues.
404+
405+
Tier 1 (Postgres + Redis only): **~97% pass rate.** Three categories of failures:
406+
407+
#### Category 1: `requires_snuba` misclassification (setup errors)
408+
409+
Tests using `pytestmark = [requires_snuba]` were not detected by our classifier because `_requires_snuba` was missing from the static detection fixture map. The fixture is session-scoped (runs once), so runtime socket detection didn't catch it either.
410+
411+
**Fix:** Added `_requires_snuba` to `FIXTURE_SERVICE_MAP` in the classifier plugin. These tests now correctly go to Tier 2.
412+
413+
#### Category 2: Redis-cluster tests (setup errors)
414+
415+
Tests parametrized with `[cluster]` (e.g., `TestRedisHashSortedSetBuffer[cluster]`) need Redis-cluster (ports 7000-7005). The `migrations` devservices mode only starts `[postgres, redis]`, not `redis-cluster`.
416+
417+
**Fix:** Start redis-cluster via GitHub Actions `services:` block in Tier 1 (temporary; long-term solution is a new devservices mode `backend-ci-light`).
418+
419+
#### Category 3: Kafka timeouts (test failures)
420+
421+
3-6 tests per shard fail with `KafkaError{code=_MSG_TIMED_OUT}`. These are NOT tests that explicitly need Kafka. They trigger Kafka produces as a side effect of normal application code.
422+
423+
### Deep Dive: Kafka as an Implicit Dependency
424+
425+
#### What is Kafka in this context?
426+
427+
Kafka is a message queue. Sentry's taskworker system uses it to dispatch asynchronous tasks. When application code calls `task.apply_async()`, it produces a message to a Kafka topic. In production, a separate worker (taskbroker) consumes and executes the task. In CI tests, **no consumer runs** - even in the current `backend-ci` mode. The messages go to Kafka and are never read.
428+
429+
#### Why do "simple" tests trigger Kafka?
430+
431+
Django model managers have `post_save`/`post_delete` hooks that call `schedule_invalidate_project_config()`, which wraps `invalidate_project_config.apply_async()` in `transaction.on_commit()`. This fires whenever these common models are saved:
432+
433+
- `ProjectKey` (created in almost every test via fixtures)
434+
- `ProjectOption`
435+
- `OrganizationOption`
436+
- `ReleaseProject`
437+
438+
So any test that creates a project, sets an option, or creates a release triggers a Kafka produce on transaction commit.
439+
440+
#### Why does it timeout instead of failing fast?
441+
442+
The `SingletonProducer` (in `src/sentry/utils/arroyo_producer.py`) tracks up to 1,000 futures. When Kafka is down, all futures fail. Once 1,000 accumulate, the producer blocks on `future.result()` of the oldest future, which times out with `MSG_TIMED_OUT`. Most tests don't hit this limit (< 1,000 Kafka produces per test), but a few long-running tests do.
443+
444+
#### Why not set `TASKWORKER_ALWAYS_EAGER=True`?
445+
446+
Test settings explicitly set `TASKWORKER_ALWAYS_EAGER = False` (in `src/sentry/testutils/pytest/sentry.py`). This is deliberate because:
447+
448+
- Tests use `BurstTaskRunner` to control when tasks execute. `TASKWORKER_ALWAYS_EAGER=True` would bypass this, executing tasks immediately and changing execution order.
449+
- Some tests assert tasks were queued but NOT executed.
450+
- `TaskRunner()` context manager sets `ALWAYS_EAGER=True` when tests explicitly want synchronous execution.
451+
452+
Setting it to `True` globally would change test behavior and could introduce false passes/failures.
453+
454+
### Kafka Resolution: Mock Approach (Chosen for PoC)
455+
456+
For Tier 1 tests, Kafka produces are purely side effects - the tests don't check whether the messages were delivered, and no consumer processes them even in the current `backend-ci` setup. The mock approach patches `SingletonProducer.produce` to return an immediately-resolved future, skipping the actual Kafka produce.
457+
458+
This is safe for Tier 1 because:
459+
- Tasks already don't execute (no consumer in either tier)
460+
- The produce is fire-and-forget (`wait_for_delivery=False` by default)
461+
- Tests that explicitly need Kafka use `requires_kafka` and are in Tier 2
462+
463+
**Risk:** If a Tier 1 test is added that subtly depends on Kafka produce succeeding (without using `requires_kafka`), the mock would silently swallow it. The test would pass but behavior would be wrong. Over time this could mask regressions.
464+
465+
### Alternative: Kafka via GitHub Actions `services:` block
466+
467+
A more robust production alternative: start a real Kafka container alongside Tier 1 using GitHub Actions `services:` block. The Relay repo already uses this pattern:
468+
469+
```yaml
470+
services:
471+
zookeeper:
472+
image: ghcr.io/getsentry/image-mirror-confluentinc-cp-zookeeper:6.2.0
473+
env:
474+
ZOOKEEPER_CLIENT_PORT: 2181
475+
kafka:
476+
image: ghcr.io/getsentry/image-mirror-confluentinc-cp-kafka:6.2.0
477+
env:
478+
KAFKA_ZOOKEEPER_CONNECT: zookeeper:2181
479+
KAFKA_ADVERTISED_LISTENERS: PLAINTEXT://127.0.0.1:9092
480+
KAFKA_OFFSETS_TOPIC_REPLICATION_FACTOR: 1
481+
KAFKA_OFFSETS_TOPIC_NUM_PARTITIONS: 1
482+
ports:
483+
- 9092:9092
484+
```
485+
486+
**Tradeoffs:**
487+
488+
| Approach | Startup Cost | Risk | Production-ready? |
489+
|----------|-------------|------|-------------------|
490+
| Mock `SingletonProducer.produce` | 0s | Silent behavioral drift | Acceptable for PoC |
491+
| Kafka via `services:` block | ~10-15s | None (real infrastructure) | Yes |
492+
| `TASKWORKER_ALWAYS_EAGER=True` | 0s | Changes task execution behavior | No |
493+
494+
If the mock approach proves problematic, switching to the `services:` block approach requires only a workflow change - no code changes needed.
495+
399496
## Validation
400497

401-
The classification can be validated empirically: run Tier 1 tests with only Postgres + Redis running (no Snuba). Any test that fails reveals a misclassification. The `requires_*` fixtures provide built-in fail-fast guards for their respective services.
498+
The classification can be validated empirically: run Tier 1 tests with only the Tier 1 services. Any test that fails reveals a misclassification. The `requires_*` fixtures provide built-in fail-fast guards for their respective services.
402499

403500
## Expected Impact
404501

405-
~71% of backend tests can run with just Postgres + Redis (setup time: ~10s) instead of the full Snuba stack (setup time: 4-5min). Combined with intelligent sharding, this could significantly reduce CI wall-clock time and runner costs.
502+
~71% of backend tests can run with just Postgres + Redis + Redis-cluster (setup time: ~15s) instead of the full Snuba stack (setup time: 4-5min). Combined with intelligent sharding, this could significantly reduce CI wall-clock time and runner costs.

‎src/sentry/testutils/pytest/__init__.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,4 +12,5 @@
1212
"sentry.testutils.pytest.show_flaky_failures",
1313
"sentry.testutils.thread_leaks.pytest",
1414
"sentry.testutils.pytest.service_classifier",
15+
"sentry.testutils.pytest.kafka_mock",
1516
]
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
"""
2+
Pytest plugin that mocks Kafka producers when Kafka is unavailable.
3+
4+
When SENTRY_MOCK_KAFKA_PRODUCERS=1 is set, patches SingletonProducer.produce
5+
to return immediately-resolved futures instead of actually producing to Kafka.
6+
7+
This is used in Tier 1 CI (Postgres + Redis only) where Kafka is not running.
8+
Tier 1 tests don't explicitly need Kafka, but application code (e.g.,
9+
invalidate_project_config via on_commit hooks) triggers Kafka produces as a
10+
side effect of normal Django model operations.
11+
12+
Without this mock, those produces accumulate failed futures in SingletonProducer
13+
and eventually block with MSG_TIMED_OUT when the future queue fills up.
14+
15+
If this approach proves problematic, an alternative is to start a real Kafka
16+
container via GitHub Actions services: block (~10-15s startup). See
17+
docs/service-classification-investigation.md for full tradeoff analysis.
18+
"""
19+
20+
from __future__ import annotations
21+
22+
import os
23+
from concurrent.futures import Future
24+
from typing import Any
25+
from unittest import mock
26+
27+
_mock_active = False
28+
_patches: list[Any] = []
29+
30+
31+
def _make_resolved_future(value: Any = None) -> Future:
32+
"""Create a Future that is already resolved."""
33+
f: Future = Future()
34+
f.set_result(value)
35+
return f
36+
37+
38+
def pytest_configure(config: object) -> None:
39+
global _mock_active
40+
if os.environ.get("SENTRY_MOCK_KAFKA_PRODUCERS") == "1":
41+
_mock_active = True
42+
_install_kafka_mock()
43+
44+
45+
def pytest_unconfigure(config: object) -> None:
46+
if _mock_active:
47+
_uninstall_kafka_mock()
48+
49+
50+
def _install_kafka_mock() -> None:
51+
"""Mock SingletonProducer.produce to return resolved futures."""
52+
from sentry.utils.arroyo_producer import SingletonProducer
53+
54+
patch = mock.patch.object(
55+
SingletonProducer,
56+
"produce",
57+
side_effect=lambda *args, **kwargs: _make_resolved_future(),
58+
)
59+
_patches.append(patch)
60+
patch.start()
61+
62+
63+
def _uninstall_kafka_mock() -> None:
64+
for patch in _patches:
65+
patch.stop()
66+
_patches.clear()

‎src/sentry/testutils/pytest/service_classifier.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040

4141
# --- Static detection: known fixture names that indicate service requirements ---
4242
FIXTURE_SERVICE_MAP: dict[str, str] = {
43+
"_requires_snuba": "snuba",
4344
"_requires_kafka": "kafka",
4445
"_requires_symbolicator": "symbolicator",
4546
"_requires_objectstore": "objectstore",

0 commit comments

Comments
 (0)