add cluster field for PVCs #2785
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FxKu
requested review from
sdudoladov,
Jan-M,
jopadi,
idanovinda,
hughcapet and
macedigital
as code owners
hughcapet
reviewed
Oct 30, 2024
|
👍 |
1 similar comment
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PVCs are the only resource which is not covered by OwnerReferences. We have observed that on cluster deletion operator can report that all PVCs have already been deleted, although they still exist. We believe that there might be a race condition with cluster fields such as Name and Namespace getting unset after cascaded deletion via OwnerReferences resulting in an empty list when fetching PVCs to delete (labelSet function using c.Name for filtering). PVC management by StatefulSet is set to Retain in our case.
This PR present an idea which we use for other resources as well: Storing resource information in a cluster field and loop over this map on deletion. However, unlike resources like Secrets and Services, PVCs are created by K8s via StatefulSet template. So we rely on a syncVolumeClaims call to have the PVCs info stored in the cluster struct. If this sync does not happen for some reason, the PVCs will not get deleted. I don't think an extra call of listPersisntentVolumeClaims is needed as c.Name would mostly likely be empty as well in this scenario.
I found that syncVolumes is not called in Create() event. Adding it there would greatly reduce the chance that PVCs are not synced before a potential deletion.