Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(nemesis-target-pool): set method attribute for target pool #9517

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

aleksbykov
Copy link
Contributor

@aleksbykov aleksbykov commented Dec 9, 2024

Decorator disrupt_method_wrapper rewrites target node pool and always set Nemesis.target_node_pool as data_nodes. This cause that target node is chosen from cluster.data_nodes always. This happemed because we have decorators chain:
disrupt_method_wrapper -> target
(data/zero/all)nodes -> disrupt*

Target node is chosen in disrupt_method_wrapper, and any changes in target_*_pool does not affect of target node if it is run in complex nemesis as SisyphusMonkey and also has affect in parallel nemesis run

Set method attribute disrupt_*.target_pool for later use in decorator disrupt_method_wrapper, allows correctly set target pool for disrupt nemesis

Fixes: #9448

Used solution from pr: #9502

Testing

  • Test run . Job marked as failed because nemesis was terminated

PR pre-checks (self review)

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

Reminders

  • Add New configuration option and document them (in sdcm/sct_config.py)
  • Add unit tests to cover my changes (under unit-test/ folder)
  • Update the Readme/doc folder relevant to this change (if needed)

@aleksbykov aleksbykov force-pushed the fix-target-pool-for-nemesis branch from ca7682d to f0917ff Compare December 9, 2024 19:09
@fruch
Copy link
Contributor

fruch commented Dec 9, 2024

@aleksbykov

see #9502

you still have this in you PR inside set_targer_node(), and it won't work cause _target_node_pool is not a list of nodes anymore:

nodes = [node for node in self._target_node_pool if not node.running_nemesis]

Copy link
Contributor

@fruch fruch left a comment

Choose a reason for hiding this comment

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

it's not enough just to move the assigning to _target_node_pool form the decorators
_target_node_pool shouldn't hold a list of nodes ontop of the nemesis class.

set_target node should alway look at cluster current nodes lists, and not cache anything in that regards

@aleksbykov
Copy link
Contributor Author

@aleksbykov

see #9502

you still have this in you PR inside set_targer_node(), and it won't work cause _target_node_pool is not a list of nodes anymore:

nodes = [node for node in self._target_node_pool if not node.running_nemesis]

@fruch , i took a look into your PR. It also will have same problem even with node type, because the disrupt_wrapper will always use the data_nodes, and target node has been chosen before, the node_pool_type would changed by decorator

Decorator `disrupt_method_wrapper` rewrites target node pool and
always set Nemesis._target_node_pool as data_nodes. This cause
that target node is chosen from cluster.data_nodes always.
This happemed because we have decorators chain:
disrupt_method_wrapper -> target_(data/zero/all)_nodes -> disrupt_*

Target node is chosen in `disrupt_method_wrapper`, and any changes in
target_*_pool does not affect of target node if it is run in complex
nemesis as SisyphusMonkey and also has affect in parallel nemesis run

Set method attribute disrupt_*.target_pool for later use in
decorator `disrupt_method_wrapper`, allows correctly set
target pool for disrupt nemesis

Using solution from PR: scylladb#9502 to store pool type instead of
cache of filtered nodes
@aleksbykov aleksbykov force-pushed the fix-target-pool-for-nemesis branch from f0917ff to 7c286fb Compare December 10, 2024 09:09
@aleksbykov
Copy link
Contributor Author

@fruch , can you take a look. I use your solution related to storing pool type

@aleksbykov aleksbykov requested a review from fruch December 10, 2024 09:10
@aleksbykov
Copy link
Contributor Author

@aleksbykov aleksbykov marked this pull request as ready for review December 10, 2024 11:44
@aleksbykov aleksbykov requested review from temichus, vponomaryov, soyacz and a team December 10, 2024 11:44
Copy link
Contributor

@fruch fruch left a comment

Choose a reason for hiding this comment

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

LGTM

@fruch fruch merged commit 69ad0dd into scylladb:master Dec 10, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants