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

Verify PVCs are not used by any Pods before deleting them #738

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

Conversation

burmanm
Copy link
Contributor

@burmanm burmanm commented Dec 12, 2024

…a PVC, verify that no Pods match this index search

What this PR does:
Verify that PersistentVolumeClaim is not used by any Pod before deleting it, in case there were some ghost pods still existing with the PVC mounted.

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

Checklist

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

@burmanm burmanm requested a review from a team as a code owner December 12, 2024 16:01
@burmanm burmanm changed the title Add a new indexer for matching Pod ClaimNames to PVCs. When deleting … Verify PVCs are not used by any Pods before deleting them Dec 12, 2024
Copy link

@rzvoncek rzvoncek left a comment

Choose a reason for hiding this comment

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

I cound not easily reproduce a failure scenario (bad migration or restore), so I just manually tested deleting clusters. I have a small comment on the secerity of the log lines, otherwise lgtm.

logger.Error(err, "Failed to check if PVC is being used")
return err
} else if isBeingUsed {
return fmt.Errorf("PersistentVolumeClaim %s is still being used by a pod", pvc.Name)

Choose a reason for hiding this comment

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

Question: Upon ordinarily deleting a k8ssandra cluster, I've seen a bunch of ERROR lines caused by this in the operator logs. I understand it's a normal pattern to error out until a condition (in this case pod existence) changes. But the "ERROR Reconciler error" seems a bit harsh. Perhaps we can do something to soften this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That logging comes from the controller-runtime, so there's not much we can do about it. A warn in the case of reconciler error which retry might fix makes sense, but I'm not aware of having that ability.

In the end, it's still an error. It's not like we would log this at "FATAL".

…a PVC, verify that no Pods match this index search
@burmanm burmanm force-pushed the prevent_pvc_deletion-mc-1398 branch from 7983384 to 1082eee Compare January 24, 2025 11:58
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.

Add additional checks before PVC deletion
2 participants