-
Notifications
You must be signed in to change notification settings - Fork 6
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
base: main
Are you sure you want to change the base?
Introducing a ReclaimPolicy
for ephemerally created Volume
resource
#1153
Conversation
@@ -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) |
There was a problem hiding this comment.
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.
c7fbc7e
to
3eb6dca
Compare
} | ||
annotations.SetDefaultEphemeralManagedBy(volume) | ||
_ = ctrl.SetControllerReference(machine, volume, r.Scheme()) | ||
if machineVolume.Ephemeral.VolumeTemplate.Spec.ReclaimPolicy != storagev1alpha1.Retain { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
aba6b94
to
c9a8408
Compare
d5f6a51
to
c7fbc7e
Compare
Proposed Changes
ReclaimPolicy
for ephemerally createdVolume
resource with 2 supported modes:Retain
: the resource is not deleted after the managing resource has been deletedDelete
: the current behavior, the resource is garbage-collected when the managing resource has been deletedReclaimPolicy
would be assumed to beDelete
to not to not break current behavior.VolumeTemplateSpec
to supportReclaimPolicy
type along with existing VolumeSpecOwnerReference
is added to ephemerally created volume in both the case ofReclaimPolicy
is set toDelete
to avoid deletion of ephemeral volumes accidently.ReclaimPolicy
is set toRetain
.[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