Skip to content

feature(nemesis): Migrate extracted nemesis precheck guards - #15723

Open
pehala wants to merge 4 commits into
scylladb:masterfrom
pehala:nemesis-precheck-extracted
Open

feature(nemesis): Migrate extracted nemesis precheck guards#15723
pehala wants to merge 4 commits into
scylladb:masterfrom
pehala:nemesis-precheck-extracted

Conversation

@pehala

@pehala pehala commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Migrate extracted nemesis into using precheck.

Testing

  • unittests

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

@scylladb-promoter

scylladb-promoter commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

✅ Test Summary: PASSED

✅ Precommit: PASSED

Total Passed Failed Skipped
67 19 0 48

✅ Tests: PASSED

Total Passed Failed Errors Skipped
4523 4492 0 0 31

Full build log

@pehala

pehala commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude finished @pehala's task in 2m 48s —— View job


Code Review

  • Gather context: read diff and changed files
  • Review nemesis modules (add_remove_dc, network, sla)
  • Review unit tests
  • Post findings

The PR is a clean, well-scoped migration. The core logic is correct across all three modules. A few observations below — two worth addressing, one nit.

sdcm/nemesis/monkey/sla.py

Behavior change in enterprise check (sla.py:32): The old validate_sla_preconditions() always used self.runner.cluster.nodes[0].is_enterprise. The new precheck(node) uses the node parameter passed by the framework. This is the right change — the representative node API is the correct abstraction — but worth noting as a deliberate semantic shift, since the old hard-code to nodes[0] is gone.

requires_prefilled_cs_data class attribute (sla.py:19,94): Clean design. RemoveServiceLevelMonkey sets it to False with a clear comment. The inheritance chain is easy to follow.

get_stress_params() safety-net raise (sla.py:68-80): The comment and the preserved raise are good — they document that the precheck() prune is the primary gate but the in-method guard still defends against any bypass path.

sdcm/nemesis/monkey/network.py

extra_network_interface_precheck() helper (network.py:220-224): Good extraction. Three consumers, no duplication.

Kubernetes bypass (network.py:220,380): RandomInterruptionNetworkMonkey and BlockNetworkMonkey both have kubernetes = True and their own _disrupt_k8s() paths, so skipping the interface check on k8s is correct. StopStartInterfacesNetworkMonkey lacks kubernetes = True and has no k8s code path in disrupt(), so its precheck() correctly always enforces the interface check.

RejectInterNodeNetworkMonkey.precheck() (network.py:280-285): Minor — there is no unit test for the "issue is active → return URL" path. The fixture patches SkipPerIssues.get_issue_details to None globally, so every test in the module exercises only the "issue closed → return None" branch. A test flipping that mock to a real-looking value would close the gap. Fix this →

sdcm/nemesis/monkey/add_remove_dc.py

precheck() (add_remove_dc.py:192-195): Straightforward. The MULTI_REGION check is a config-time constant, correctly classified as a static guard. The prepare_phase_active check stays in disrupt() because it reads event state at execution time — this split is correct.

unit_tests/unit/nemesis/monkey/test_add_remove_dc.py

Stale assertions in test_precheck_keeps_for_single_region (test_add_remove_dc.py:123-125): The test asserts runner.run_repair.assert_not_called(), runner.decommission_nodes.assert_not_called(), and not runner.executed. Since disrupt() is never called, these are vacuously true — they pass unconditionally and give no signal. They're carryover from the replaced test_disrupt_raises_unsupported_for_multi_region test. They don't hurt correctness but make the test misleading. Fix this →

unit_tests/unit/nemesis/monkey/test_sla.py

Parametrized coverage over all 7 SLA monkeys: Good approach — the _ALL_SLA_MONKEYS list covers every concrete subclass and the RemoveServiceLevelMonkey-specific exclusion in test_precheck_skips_when_no_prepare_write_cmd is explicit and clearly explained.

Docs

The plan updates accurately reflect what was done vs. what remains. Phase 3 DoD checkboxes are all ticked with matching deliverables. The phase description change (category-based → location-based split) is a cleaner framing of the remaining work.


Summary: Two items worth addressing — a missing negative-path test for RejectInterNodeNetworkMonkey.precheck() and stale assertions in one test function. Production code has no bugs. CI is green.

pehala and others added 4 commits August 6, 2026 15:04
The SLA preconditions (sla param, Scylla Enterprise, authenticator,
prepare_write_cmd) are all static config/edition checks, so they were
re-evaluated on every nemesis cycle behind the expensive cluster health
check and target-node selection.

Move them into SlaMonkeyBase.precheck(), which the runner evaluates once
before the run loop. The prepare_write_cmd gate is guarded by the new
requires_prefilled_cs_data class attribute, set to False on
RemoveServiceLevelMonkey which operates on pre-defined roles instead.
The raise inside get_stress_params() is kept as an in-method safety net,
since skip_optional_stage("prepare_write") can skip the prepare load even
when the param is set.

Dynamic checks (tester.roles, remaining service-level slots) stay in
disrupt().

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
extra_network_interface is a test-config flag and the scylladb#6522 skip
is a static issue gate, yet both were re-checked on every cycle after the
cluster health check and node selection had already run.

Move them into precheck(): RandomInterruptionNetworkMonkey and
BlockNetworkMonkey return no reason on Kubernetes, where the Chaos Mesh
path needs no secondary interface; StopStartInterfacesNetworkMonkey always
requires it. RejectInterNodeNetworkMonkey reports the SkipPerIssues gate.

install_traffic_control() stays in disrupt() — it installs a package on
the target node, which is a mutating action rather than a static probe.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_config.MULTI_REGION is fixed for the whole run, so the multi-dc skip
belongs in precheck() where it is evaluated once and prunes the nemesis
from the rotation.

The prepare_phase_active check stays in disrupt(): it reflects live test
state that changes as the prepare phase starts and ends.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dary

Phases 3 and 4 split the migration by skip category, but a single nemesis
module holds guards of both categories, so no PR could ever complete one
phase on its own. Split them by where the guard lives instead: Phase 3
covers the classes under sdcm/nemesis/monkey/ that already own their
disruption logic, Phase 4 the guards still sitting in
NemesisRunner.disrupt_* behind the thin wrappers. The Category 1/2/3
taxonomy keeps deciding which guards may move, it just no longer defines
the phase boundary.

Also correct the Phase 3 adaptation note: it classified
get_cassandra_stress_write_cmds() as a Category 3 data-presence gate, read
from the skip message rather than the code. The helper only reads the
prepare_write_cmd config param and never touches the cluster, so it is
Category 1.

Phase 3 is complete with this PR, so mark it done in the plan, the PR
history and progress.json (4 of 5 phases).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pehala
pehala force-pushed the nemesis-precheck-extracted branch from fc20e1f to 201bfde Compare August 6, 2026 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants