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

Introducing a ReclaimPolicy for ephemerally created Volume resource #1153

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

Conversation

ushabelgur
Copy link
Contributor

@ushabelgur ushabelgur commented Nov 14, 2024

Proposed Changes

  • Add ReclaimPolicy for ephemerally created Volume resource with 2 supported modes:
    • Retain: the resource is not deleted after the managing resource has been deleted
    • Delete: the current behavior, the resource is garbage-collected when the managing resource has been deleted
  • By default ReclaimPolicy would be assumed to be Delete to not to not break current behavior.
  • Add wrapper type for VolumeTemplateSpec to support ReclaimPolicy type along with existing VolumeSpec
  • OwnerReference is added to ephemerally created volume in both the case of ReclaimPolicy is set to Delete to avoid deletion of ephemeral volumes accidently.
  • Machine controller will take care of removing ownerRef from ephemerally created volumes if ReclaimPolicy is set to Retain.
  • Add Test cases
    [Note: volume_release_controller is already taking care of releasing volumes whose claimer doesn't existing, by setting .spec.claimRef to nil when claimer Machine object is deleted. So not adding any extra logic for this]

Fixes #1114

@ushabelgur ushabelgur self-assigned this Nov 14, 2024
@ushabelgur ushabelgur requested a review from balpert89 November 14, 2024 09:46
@ushabelgur ushabelgur marked this pull request as ready for review November 18, 2024 05:08
@ushabelgur ushabelgur requested a review from a team as a code owner November 18, 2024 05:08
@afritzler afritzler added the enhancement New feature or request label Nov 18, 2024
@@ -36,6 +36,10 @@ func IsDefaultEphemeralControlledBy(o metav1.Object, owner metav1.Object) bool {
return metav1.IsControlledBy(o, owner) && IsEphemeralManagedBy(o, commonv1alpha1.DefaultEphemeralManager)
}

func IsDefaultEphemeralOrControlledBy(o metav1.Object, owner metav1.Object) bool {
return metav1.IsControlledBy(o, owner) || IsEphemeralManagedBy(o, commonv1alpha1.DefaultEphemeralManager)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I added new method with OR condition here, as above original method IsDefaultEphemeralControlledBy() is used by all other ephemeral controllers and it will break behavior of those controllers.

@lukas016 lukas016 force-pushed the osc/enh-volume-reclaim branch 2 times, most recently from c7fbc7e to 3eb6dca Compare November 25, 2024 06:36
}
annotations.SetDefaultEphemeralManagedBy(volume)
_ = ctrl.SetControllerReference(machine, volume, r.Scheme())
if machineVolume.Ephemeral.VolumeTemplate.Spec.ReclaimPolicy != storagev1alpha1.Retain {
Copy link
Member

Choose a reason for hiding this comment

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

In case we use Retain policy for ephemeral Volumes and we don't set the controller reference, there will be no finalizer preventing the deletion of a Volume. We should prohibit the deletion of an ephemeral volume at all times as long as the Machine object exists. One idea would be to always set the controller reference and remove it in the case of a Machine deletion.

Copy link
Member

Choose a reason for hiding this comment

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

We might can utilize RESTDeleteStrategy in /k8s.io/[email protected]/pkg/registry/rest/delete.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, If we want to handle removal of OwnerRef in registry using RESTDeleteStrategy, we are not adding ReclaimPolicy directly under VolumeSpec we have created wrapper around VolumeTemplateSpec as we didn't want to set this field for volumes other than ephemeral. so will not have reference to ReclaimPolicy in volme strategy.
So handling removal of controller reference in machine controller before deleting machine.

@github-actions github-actions bot added size/L and removed size/XL labels Dec 20, 2024
@lukas016 lukas016 force-pushed the osc/enh-volume-reclaim branch from aba6b94 to c9a8408 Compare December 20, 2024 05:57
@github-actions github-actions bot added size/XL and removed size/L labels Dec 20, 2024
@ushabelgur ushabelgur requested a review from afritzler December 20, 2024 07:13
@lukas016 lukas016 force-pushed the osc/enh-volume-reclaim branch from d5f6a51 to c7fbc7e Compare January 16, 2025 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce a reclaim policy for ephemerally created Volume resources
4 participants