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

Feature/279 add snapshot modules #283

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JaymitP
Copy link

@JaymitP JaymitP commented Jul 15, 2024

SUMMARY

Hi, thought I would implement the snapshot functionality since it was something I was looking for.
I saw in #279 that it was a module of interest as well and I am learning to create ansible modules.
I have of course included integration tests 😄
Do let me know if anything should be changed/improved.

Some caveats:

  • A check mode could be added easily using listsnapshot (I could definitely do so)
  • Merging both snapshot and clearsnapshot into a single module is possible by making use of a 'present/state' variable, but is messy.
  • Nodetool requests for a snapshot to be created/removed, so although the command is run successfully, changes may not be made...
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

cassandra_snapshot
cassandra_clearsnapshot

@rhysmeister
Copy link
Collaborator

rhysmeister commented Jul 15, 2024

First two points...

  • A check mode could be added easily using listsnapshot (I could definitely do so)
  • Merging both snapshot and clearsnapshot into a single module is possible by making use of a 'present/state' variable, but is messy.

yes, definitely both of these. snapshot and clearsnapshot should be merged into a single module. It's not messy, I do a similar thing for elastic here...

https://github.com/ansible-collections/community.elastic/blob/master/plugins/modules/elastic_snapshot.py

  • Nodetool requests for a snapshot to be created/removed, so although the command is run successfully, changes may not be made...

The alternative here is to request the action, and then continually poll for success, before confirming the action. That's probably a bit much. I would just assume it works, if rc == 0, and see how it goes. Perhaps make this clear in module documentation.

@JaymitP
Copy link
Author

JaymitP commented Jul 16, 2024

yes, definitely both of these. snapshot and clearsnapshot should be merged into a single module. It's not messy, I do a similar thing for elastic here...

https://github.com/ansible-collections/community.elastic/blob/master/plugins/modules/elastic_snapshot.py

Rather than messy, I mean more so confusing. Implementation wise it's fine, but it would make the module difficult to use.
snapshot can take tables, keyspace.tables and keyspaces as arguments. Whilst clearsnapshot can only take keyspaces...
Alebit it's still usable, just a tad annoying.

PS: Accidental git push, no need to approve workflow for now

- Removed failing snapshot test that was specific to cassandra version 4.x
- Changed ansible documentation to include no_log for some parameters and fix incorrect parameter types
@rhysmeister
Copy link
Collaborator

@JaymitP It's up to you what you want implement but you don't have to support everything the underlying cmds do. Perhaps instance & keyspace snapshots are enough for 99% of people? Even if not... you could push to v2 and keep things simpler for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants