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 MedusaTask to delete only its own backups #1025

Merged
merged 1 commit into from Aug 8, 2023

Conversation

dnugmanov
Copy link
Contributor

@dnugmanov dnugmanov commented Jul 24, 2023

What this PR does: This commit addresses the issue where MedusaTask was erroneously deleting medusabackups belonging to other cassandraDatacenters in the same namespace. The bug occurred due to the lack of a condition to check if the backup.Spec.CassandraDatacenter matched the task.Spec.CassandraDatacenter. This fix adds the necessary condition to ensure that MedusaTask only deletes its own Cassandra backups, resolving the issue reported in the PR.

Which issue(s) this PR fixes:
Fixes #1017

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CHANGELOG.md updated (not required for documentation PRs)
  • CLA Signed: DataStax CLA

@dnugmanov dnugmanov requested a review from a team as a code owner July 24, 2023 10:05
@dnugmanov dnugmanov force-pushed the fix-medusa-task-cleanup branch 2 times, most recently from 211e408 to b38f32d Compare July 24, 2023 10:14
@burmanm burmanm self-assigned this Jul 24, 2023
@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #1025 (5ad0143) into main (4e2cd71) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 5ad0143 differs from pull request most recent head 70821b1. Consider uploading reports for the commit 70821b1 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1025      +/-   ##
==========================================
- Coverage   57.49%   57.48%   -0.01%     
==========================================
  Files          99       99              
  Lines       10011    10011              
==========================================
- Hits         5756     5755       -1     
  Misses       3764     3764              
- Partials      491      492       +1     
Files Changed Coverage Δ
controllers/medusa/medusatask_controller.go 48.12% <0.00%> (+0.34%) ⬆️

... and 3 files with indirect coverage changes

@burmanm
Copy link
Contributor

burmanm commented Jul 24, 2023

Hey, thanks for the PR. What I'd really like to see on this one is a test, to ensure that the functionality actually does fix the issue (so a test which would fail without the fix and passes with the fix in place). e2e test isn't necessary, unit / integration test (envtest) is perfectly fine.

The issue in reality comes from the fact that the Pod's fetched backups do not match the localBackup search. Even with just comparing Datacenters we're really comparing the Task's Datacenter vs. whatever the Pod believes is the correct Datacenter. This is not necessarily the same thing, due to the ability to override the Datacenter. In that case, the task will have a Datacenter name that's not recognized by the Pod's Datacenter.

@adejanovski I think the above issue plagues any user who uses DatacenterName overrides in cass-operator. Doesn't the Pod backup store the Cassandra's real Datacenter name instead of the CassandraDatacenter.Name? If so, we need to fetch the original CassandraDatacenter and use its DatacenterName() for comparison.

@burmanm
Copy link
Contributor

burmanm commented Jul 25, 2023

Alright, the Datacenter name is fine. So the only thing to add would be the tests that verify the correct behavior.

@dnugmanov
Copy link
Contributor Author

Hello, @burmanm! I've implemented an integration test for the following scenarios:

  1. Create a multi-datacenter K8ssandra cluster.
  2. Generate backups for each datacenter (dc).
  3. Purge backups for dc1.
  4. Verify that backups for dc2 still exist.

The final condition will pass the test only if we verify that backup.Spec.CassandraDatacenter is identical to task.Spec.CassandraDatacenter. Otherwise, the sync task may unintentionally delete backup4 for dc2.

If you have the time, please take a look at the PR and let me know your thoughts. Your feedback is much appreciated, and if there's anything that needs further rephrasing or improvement, feel free to point it out. Thank you for your help!

Copy link
Contributor

@burmanm burmanm left a comment

Choose a reason for hiding this comment

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

This will need a rebase for the changelog file.

@sonarcloud
Copy link

sonarcloud bot commented Aug 8, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
17.2% 17.2% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@dnugmanov
Copy link
Contributor Author

This will need a rebase for the changelog file.

Done

@burmanm burmanm merged commit 285790b into k8ssandra:main Aug 8, 2023
57 of 60 checks passed
@burmanm
Copy link
Contributor

burmanm commented Aug 8, 2023

Thank you for the PR.

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.

MedusaTask deletes medusaBackups of another clusters in the same namespace
2 participants