-
Notifications
You must be signed in to change notification settings - Fork 835
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
feat: stateful set persistent volume claim retention policy #5946
feat: stateful set persistent volume claim retention policy #5946
Conversation
@@ -41,14 +41,15 @@ func NewServerStatefulSetReconciler( | |||
podSpec *v1.PodSpec, | |||
volumeClaimTemplates []mlopsv1alpha1.PersistentVolumeClaim, | |||
scaling *mlopsv1alpha1.ScalingSpec, | |||
policy *appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy, |
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.
A suggestion to rename this to volumeClaimRetentionPolicy. Mostly because in the future we might end up with more policies for different things, and to eliminate any confusion.
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.
Sure, this is update and renamed to volumeClaimRetentionPolicy
test.podSpec, | ||
test.volumeClaimTemplates, | ||
test.scaling, | ||
test.statefulSetPersistentVolumeClaimRetentionPolicy, |
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.
While you're referencing test.statefulSetPersistentVolumeClaimRetentionPolicy
here, this remains null in all the test cases created above. I would add a test case that has an actual retention policy added to the Server and then expect that it appears with the same values in the StatefulSet.
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.
I have updated a current test, to have PersistentVolumeClaimRetentionPolicy in it :)
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.
A request for some improvements to the tests, plus some other minor nits. Really good contribution!
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.
lgtm! Thanks for working on this, it will allow us to apply a PVC delete policy by default in some of our test clusters as well
What this PR does / why we need it:
This PR is to enhance this default behaviour from Core v2, such that a PVC should be released/deleted when the associated STS pod is scaled down or deleted.
Screenshot from unit test
Quick info
From k8s v1.27 we have a beta feature called PersistentVolumeClaim retention StatefulSets which can be used to manage the pvc’s which are created by STS. In Core v2 we are creating the PVC for a model as a part of serverconfig's volumeClaimTemplates spec which will be present in the statefulset which gets created as soon as a server is created.
Which issue(s) this PR fixes:
Fixes #INFRA-851