scylla-cluster-tests icon indicating copy to clipboard operation
scylla-cluster-tests copied to clipboard

improvement(nemesis.py): Change into module

Open pehala opened this issue 1 year ago • 4 comments

What?

  • Change nemesis.py into a module
    • Backwards compatible change
      • Might affect other unmerged fixes to Nemesis, but that should be easy to fix

Why?

Currently, the nemesis.py is an incredibly large file (6K+ lines), which makes it hard to orient in, not to mention having wrong object structure with all code in the base class. This PR paves the way for future PRs that can split individual nemesis into their own files/module (like nemesis/topology_change.py), without ever breaking imports and thus be backwards compatible. This would not be possible if the nemesis.py was still a file not a module. This change is not necessary and can be rejected.

Testing

  • [x] 3h Longevity (https://argus.scylladb.com/workspace?state=WyJiNmRhNTNhNC02YWFhLTQ5NTUtODZiNy0xYWNjMTA4NTQzNDgiXQ)
    • Test failed, but nemesis were executed, proving this change did not break any imports

PR pre-checks (self review)

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

pehala avatar Sep 20 '24 15:09 pehala

The direction is something we sure want, but this change alone isn't really getting us any closer to that target.

We do have examples of nemesis that were written outside of this module in siren-tests.

I think we should pick one nemesis to start moving it to its own module, and start what's the needed changes for that.

fruch avatar Sep 22 '24 06:09 fruch

The direction is something we sure want, but this change alone isn't really getting us any closer to that target.

I can use this PR to also migrate one of the nemesis, or create followup PR that will utilize this one, if you want.

We do have examples of nemesis that were written outside of this module in siren-tests.

I would not like to scatter nemesis across codebase, I would still like them to see inside nemesis module, but to do that we first need to have the module itself.

pehala avatar Sep 22 '24 07:09 pehala

The direction is something we sure want, but this change alone isn't really getting us any closer to that target.

I can use this PR to also migrate one of the nemesis, or create followup PR that will utilize this one, if you want.

I think it should happen together, this change on it's own doesn't really adds anything.

We do have examples of nemesis that were written outside of this module in siren-tests.

I would not like to scatter nemesis across codebase, I would still like them to see inside nemesis module, but to do that we first need to have the module itself.

regardless where the parts would go, we first need to be able to see how to break it ot smaller parts.

fruch avatar Sep 22 '24 07:09 fruch

I wouldn't even proceed with this PR. Unless you want it to become a discussion of weeks instead of a code being merged. I'm also not in favor of "migrating" one nemesis and be in a limbo state for years till one will decide to convert all the nemesis or some will write new nemesis in different ways.

There is a long standing task to refactor nemesis class in qa-task (I think it should also have context and history). If you want to make it right, pick that task, start a design document, arrange a discussion and get a review and agreement and then we can look at PRs and plans how to change the current situation.

roydahan avatar Sep 22 '24 22:09 roydahan

Closing this for now

pehala avatar Oct 23 '24 08:10 pehala