Skip to content

Add ASM (Atomic Slot Migration) cluster maintenance-notification scenario tests - #4158

Open
kiryazovi-redis wants to merge 5 commits into
redis:masterfrom
kiryazovi-redis:feat/asm-cluster-scenario-tests
Open

Add ASM (Atomic Slot Migration) cluster maintenance-notification scenario tests#4158
kiryazovi-redis wants to merge 5 commits into
redis:masterfrom
kiryazovi-redis:feat/asm-cluster-scenario-tests

Conversation

@kiryazovi-redis

@kiryazovi-redis kiryazovi-redis commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Description of change

Cluster client maintenance-notification scenario tests for the ASM (Atomic Slot Migration) path. All ASM logic lives in the fault injector behind its SLOT_MIGRATE_ASM toggle, so the client stays generic (no ASM-specific code): when the FI runs in ASM mode, the existing effect-trigger cluster tests exercise the FI's reshard trigger and assert SMIGRATINGMaintenanceState.MAINTENANCESMIGRATED with node-cache reconciliation.

This PR is therefore small and test-only.

Changes

  • Add generic test_notification_handling_with_node_add (+1 node), the mirror of test_notification_handling_with_node_remove.
  • Add _wait_for_node_cache_delta settle-poll on the cluster test base: ASM scale (add/remove) is a cascade of SMIGRATING/SMIGRATED (the decision engine moves slots across shards), so the client reconciles its node cache over several notifications after the FI op completes — drain + poll until the ±1 delta before asserting. node_remove now joins the trigger thread + waits for −1 first; reconnect count relaxed to >= 1 (a cascade can mark more than one).
  • Skip test_new_connections_receive_last_smigrating_smigrated_notification for all reshard effects (slot-shuffle, add, remove): "a new connection catches the in-flight SMIGRATING" needs a maintenance window that stays open long enough to open a fresh connection mid-flight. ASM add/remove is a multi-window cascade, and a single ASM slot-shuffle over a small slot range closes (SMIGRATED) ~1s after SMIGRATING — the window shuts before a new connection can be opened. Same "ends too fast" class already skipped for slot_shuffle+failover. Existing-connection handling for slot-shuffle stays covered by the handling and cmd-execution tests.
  • Size the reshard test timeouts to the FI's real reshard budget so a slow-but-successful reshard is not counted as a failure (raising a timeout only affects the failure path, so it is free on green runs). The FI blocks on a reshard up to 300s (slot-shuffle) / 600s (ASM scale); the client was smaller (op-wait 120s, SMIGRATING waits 60s, SMIGRATED 40s, per-test cap 300s), so a slow reshard flaked and the trigger thread could give up early and leave the command-execution workers hanging. Now: trigger get_operation_result waits RESHARD_OP_TIMEOUT (600s), SLOT_SHUFFLE_TIMEOUT 120→300, SMIGRATED_TIMEOUT 40→120, and the OSS reshard tests use a RESHARD_TEST_TIMEOUT (660s) cap (standalone/non-reshard tests keep 300s).

Requirements

Needs the fault injector with the reshard trigger + SLOT_MIGRATE_ASM toggle. CI runs the generic tests against an ASM-mode fault injector (asm=true).

Pull Request check-list

  • Do tests and lints pass with this change? Full single-version (100.0.20) ASM run green — all 4 matrix variants (plain/hiredis × handling/cmd_execution), 0 failures; new_connections[slot-shuffle-reshard] skipped as designed.
  • Do the CI tests pass with this change? Validated against an ASM-mode fault injector (asm=true); green end-to-end.
  • Is the new or changed code fully tested?

History (condensed)

Earlier revisions added an AsmTriggers enum, include_asm threading, and a dedicated TestClusterClientPushNotificationsWithAsmEffectTrigger with a node-cache "drain until settled" loop. Per review, all ASM logic moved into the FI behind the toggle, so those client-side constructs were removed and the change collapsed to the small, generic diff above. The branch was rebased onto the current feat_add_sch_async base (the earlier huge diff was stale-base divergence, not real change). Follow-up commits then made the generic tests robust to real ASM reshard latency (new-connection skip + FI-budget timeouts) after live ASM CI runs.


Note

Low Risk
Test-only changes in scenario integration tests; no production client or library behavior is modified.

Overview
Extends cluster maintenance-notification scenario tests for ASM (reshard) fault-injector runs without changing redis-py client code.

Timeouts are aligned with the FI reshard budget (RESHARD_OP_TIMEOUT 600s, longer SMIGRATED / slot-shuffle waits, RESHARD_TEST_TIMEOUT on OSS reshard tests) so slow-but-successful reshards are not treated as failures and trigger threads do not exit early and leave workers hanging. Non-reshard triggers keep a shorter default op-wait via DEFAULT_TRIGGER_OP_TIMEOUT.

Adds _wait_for_node_cache_delta to drain push notifications and poll until the cluster client’s node cache reaches the expected ±1 change after the FI op completes—needed because ASM scale add/remove is a cascade of SMIGRATING/SMIGRATED events. test_notification_handling_with_node_add mirrors node-remove for a +1 cache delta; node-remove joins the trigger first, waits for −1, and relaxes “marked for reconnect” to >= 1.

Skips test_new_connections_receive_last_smigrating_smigrated_notification when trigger == "reshard" for add/remove/slot-shuffle (maintenance windows too short to open a fresh connection mid-flight). Reshard effect-trigger tests pass RESHARD_OP_TIMEOUT into _trigger_effect.

Reviewed by Cursor Bugbot for commit 851a112. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread tests/test_scenario/maint_notifications_helpers.py Outdated
Comment thread tests/test_scenario/test_maint_notifications.py Outdated
Comment thread tests/test_scenario/maint_notifications_helpers.py Outdated
Comment thread tests/test_scenario/test_maint_notifications.py Outdated
Comment thread tests/test_scenario/test_maint_notifications.py Outdated
@petyaslavova
petyaslavova force-pushed the feat_add_sch_async branch 2 times, most recently from 4b18f44 to 5602af3 Compare July 9, 2026 11:32
@kiryazovi-redis
kiryazovi-redis force-pushed the feat/asm-cluster-scenario-tests branch from 4b85297 to aa38fc4 Compare July 10, 2026 19:32
Comment thread tests/test_scenario/test_maint_notifications.py Outdated
@kiryazovi-redis
kiryazovi-redis force-pushed the feat/asm-cluster-scenario-tests branch from aa38fc4 to e7f6396 Compare July 10, 2026 19:37
Comment thread tests/test_scenario/test_maint_notifications.py

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

There are 3 total unresolved issues (including 1 from previous review).

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 51a8fd6. Configure here.

Comment thread tests/test_scenario/test_maint_notifications.py Outdated
Comment thread tests/test_scenario/test_maint_notifications.py Outdated
@petyaslavova petyaslavova added the maintenance Maintenance (CI, Releases, etc) label Jul 13, 2026
@petyaslavova
petyaslavova changed the base branch from feat_add_sch_async to master July 13, 2026 14:41
…ove) + generic node_add

The FI's SLOT_MIGRATE_ASM toggle routes the generic cluster tests through the ASM `reshard`
trigger. ASM scale (add/remove) is a cascade of SMIGRATING/SMIGRATED, so the client reconciles
its node cache over several notifications after the op completes:

- Add a `_wait_for_node_cache_delta` settle-poll helper; use it in node_remove and the new
  node_add test before the node-delta assertion (single-step migrate/failover return
  immediately). Relax node_remove reconnect count to >= 1 (a cascade may touch more than one).
- Add generic `test_notification_handling_with_node_add` (+1) mirroring node_remove.
- Skip `test_new_connections_receive_last_smigrating_smigrated_notification` for reshard
  add/remove (catching the in-flight notification on a new connection is a single-window
  property; ASM scale is a multi-window cascade). slot-shuffle keeps a single window.
…on catch

The new-connection-catches-SMIGRATING assertion needs a maintenance window that
stays open long enough to open a fresh connection mid-flight. ASM `reshard`
slot-shuffle (single migrate_slots over a small slot range) closes ~1s after
SMIGRATING (SMIGRATING t+0 -> SMIGRATED t+1.2s in CI), so a connection opened
after the first wait/assert loop legitimately observes MaintenanceState.NONE.
This is the same "ends too fast" class already skipped for slot_shuffle+failover.

Existing-connection SMIGRATING/SMIGRATED handling for slot-shuffle+reshard stays
covered by the handling and cmd-execution tests; new-connection catch is still
exercised via slot_shuffle+migrate and remove_add+reshard (longer windows).
Guard against env0 reshard-latency flakiness. The client's patience was smaller
than what the fault injector allows a reshard to take:
  FI: slot-shuffle migrate_slots up to 300s, ASM scale up to 600s (blocking).
  client (before): trigger op-wait 120s, SMIGRATING waits 60s, SMIGRATED 40s,
  per-test cap 300s.
So a slow-but-successful reshard was counted as a failure - and when the trigger
thread gave up at 120s the command-execution workers were orphaned and their
join() hung to the 300s pytest cap.

Raise the ceilings to cover the FI's real budget (raising a timeout only affects
the failure path, so it is free on green runs):
  - SLOT_SHUFFLE_TIMEOUT 120 -> 300, SMIGRATED_TIMEOUT 40 -> 120
  - trigger get_operation_result wait -> RESHARD_OP_TIMEOUT (600), so the trigger
    thread never gives up before the FI finishes
  - per-test cap -> RESHARD_TEST_TIMEOUT (660) on the OSS reshard tests only
    (standalone/non-reshard tests keep 300s)
…ut guard

- Pytest cap vs drain (Medium): RESHARD_TEST_TIMEOUT is now
  RESHARD_OP_TIMEOUT + SMIGRATED_TIMEOUT + margin (900s), so a slow ASM scale
  that uses the full op-wait and then drains the node cache post-join is not
  killed mid-reconciliation.
- Shared trigger timeout leaking to standalone (Low): revert _trigger_effect's
  default op-wait to a modest DEFAULT_TRIGGER_OP_TIMEOUT (180s, under the
  standalone tests' 300s cap); the 6 OSS reshard tests now pass
  RESHARD_OP_TIMEOUT (600s) explicitly.
- Drain helper silent timeout (Medium): _wait_for_node_cache_delta now
  pytest.fail()s on timeout instead of returning silently, so a failed
  reconciliation surfaces clearly instead of as a misleading node-count assert.
- node_add missing invariants (Low): assert cache size unchanged + all initial
  node keys present during SMIGRATING, mirroring test_notification_handling_with_node_remove.
@kiryazovi-redis
kiryazovi-redis force-pushed the feat/asm-cluster-scenario-tests branch from 96889a7 to d1fd9b3 Compare July 13, 2026 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Maintenance (CI, Releases, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants