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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 15, 2024
Merged

feat: stateful set persistent volume claim retention policy #5946

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7619210
feat: stateful set persistent volume claim retention policy
abhimanyu003 Oct 1, 2024
2f4fadd
refactor: naming convention and test case update
abhimanyu003 Oct 4, 2024
32ad67a
feat: adding statefulSetPersistentVolumeClaimRetentionPolicy crd yaml
abhimanyu003 Oct 4, 2024
352b027
Merge branch 'v2' into volumeClaimRetentionPolicy
abhimanyu003 Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions operator/apis/mlops/v1alpha1/server_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ the Change License after the Change Date as each is defined in accordance with t
package v1alpha1

import (
appsv1 "k8s.io/api/apps/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
Expand Down Expand Up @@ -37,6 +38,8 @@ type ServerSpec struct {
// PodSpec overrides
// Slices such as containers would be appended not overridden
PodSpec *PodSpec `json:"podSpec,omitempty"`
// StatefulSetPersistentVolumeClaimRetentionPolicy policy for stateful set pvc
StatefulSetPersistentVolumeClaimRetentionPolicy *appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy `json:"statefulSetPersistentVolumeClaimRetentionPolicy,omitempty"`
// Scaling spec
ScalingSpec `json:",inline"`
// +Optional
Expand Down
1 change: 1 addition & 0 deletions operator/controllers/reconcilers/server/server_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ func (s *ServerReconciler) createStatefulSetReconciler(server *mlopsv1alpha1.Ser
podSpec,
serverConfig.Spec.VolumeClaimTemplates,
&server.Spec.ScalingSpec,
server.Spec.StatefulSetPersistentVolumeClaimRetentionPolicy,
serverConfig.ObjectMeta,
annotator)
return statefulSetReconciler, nil
Expand Down
34 changes: 34 additions & 0 deletions operator/controllers/reconcilers/server/server_reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,40 @@ func TestServerReconcile(t *testing.T) {
expectedSvcNames: []string{"myserver-0"},
expectedStatefulSetName: "myserver",
},
{
name: "Test StatefulSetPersistentVolumeClaimRetentionPolicy",
serverConfig: &mlopsv1alpha1.ServerConfig{
ObjectMeta: metav1.ObjectMeta{
Name: mlserverConfigName,
Namespace: constants.SeldonNamespace,
},
Spec: mlopsv1alpha1.ServerConfigSpec{
PodSpec: v1.PodSpec{
Containers: []v1.Container{
{
Name: "mlserver",
Image: "seldonio/mlserver:0.5",
},
},
},
},
},
server: &mlopsv1alpha1.Server{
ObjectMeta: metav1.ObjectMeta{
Name: "myserver",
Namespace: constants.SeldonNamespace,
},
Spec: mlopsv1alpha1.ServerSpec{
ServerConfig: mlserverConfigName,
StatefulSetPersistentVolumeClaimRetentionPolicy: &appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy{
WhenDeleted: appsv1.RetainPersistentVolumeClaimRetentionPolicyType,
WhenScaled: appsv1.RetainPersistentVolumeClaimRetentionPolicyType,
},
},
},
expectedSvcNames: []string{"myserver-0"},
expectedStatefulSetName: "myserver",
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down
7 changes: 5 additions & 2 deletions operator/controllers/reconcilers/server/statefulset_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,15 @@ func NewServerStatefulSetReconciler(
podSpec *v1.PodSpec,
volumeClaimTemplates []mlopsv1alpha1.PersistentVolumeClaim,
scaling *mlopsv1alpha1.ScalingSpec,
policy *appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy,
Copy link
Member

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.

Copy link
Member Author

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

serverConfigMeta metav1.ObjectMeta,
annotator *patch.Annotator,
) *ServerStatefulSetReconciler {
labels := utils.MergeMaps(meta.Labels, serverConfigMeta.Labels)
annotations := utils.MergeMaps(meta.Annotations, serverConfigMeta.Annotations)
return &ServerStatefulSetReconciler{
ReconcilerConfig: common,
StatefulSet: toStatefulSet(meta, podSpec, volumeClaimTemplates, scaling, labels, annotations),
StatefulSet: toStatefulSet(meta, podSpec, volumeClaimTemplates, scaling, policy, labels, annotations),
Annotator: annotator,
}
}
Expand All @@ -61,6 +62,7 @@ func toStatefulSet(meta metav1.ObjectMeta,
podSpec *v1.PodSpec,
volumeClaimTemplates []mlopsv1alpha1.PersistentVolumeClaim,
scaling *mlopsv1alpha1.ScalingSpec,
policy *appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy,
labels map[string]string,
annotations map[string]string) *appsv1.StatefulSet {
labels[constants.KubernetesNameLabelKey] = constants.ServerLabelValue
Expand Down Expand Up @@ -88,7 +90,8 @@ func toStatefulSet(meta metav1.ObjectMeta,
},
Spec: *podSpec,
},
PodManagementPolicy: appsv1.ParallelPodManagement,
PodManagementPolicy: appsv1.ParallelPodManagement,
PersistentVolumeClaimRetentionPolicy: policy,
},
}

Expand Down
44 changes: 27 additions & 17 deletions operator/controllers/reconcilers/server/statefulset_reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,15 @@ func TestStatefulSetReconcile(t *testing.T) {
g.Expect(err).To(BeNil())

type test struct {
name string
metaServer metav1.ObjectMeta
metaServerConfig metav1.ObjectMeta
podSpec *v1.PodSpec
volumeClaimTemplates []mlopsv1alpha1.PersistentVolumeClaim
scaling *mlopsv1alpha1.ScalingSpec
existing *appsv1.StatefulSet
expectedReconcileOp constants.ReconcileOperation
name string
metaServer metav1.ObjectMeta
metaServerConfig metav1.ObjectMeta
podSpec *v1.PodSpec
volumeClaimTemplates []mlopsv1alpha1.PersistentVolumeClaim
scaling *mlopsv1alpha1.ScalingSpec
statefulSetPersistentVolumeClaimRetentionPolicy *appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy
existing *appsv1.StatefulSet
expectedReconcileOp constants.ReconcileOperation
}

getIntPtr := func(i int32) *int32 {
Expand Down Expand Up @@ -334,6 +335,7 @@ func TestStatefulSetReconcile(t *testing.T) {
test.podSpec,
test.volumeClaimTemplates,
test.scaling,
test.statefulSetPersistentVolumeClaimRetentionPolicy,
test.metaServerConfig,
annotator)
rop, err := r.getReconcileOperation()
Expand All @@ -355,14 +357,15 @@ func TestToStatefulSet(t *testing.T) {
g := NewGomegaWithT(t)

type test struct {
name string
meta metav1.ObjectMeta
podSpec *v1.PodSpec
labels map[string]string
annotations map[string]string
volumeClaimTemplates []mlopsv1alpha1.PersistentVolumeClaim
scaling *mlopsv1alpha1.ScalingSpec
statefulSet *appsv1.StatefulSet
name string
meta metav1.ObjectMeta
podSpec *v1.PodSpec
labels map[string]string
annotations map[string]string
volumeClaimTemplates []mlopsv1alpha1.PersistentVolumeClaim
statefulSetPersistentVolumeClaimRetentionPolicy *appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy
scaling *mlopsv1alpha1.ScalingSpec
statefulSet *appsv1.StatefulSet
}

getIntPtr := func(i int32) *int32 {
Expand Down Expand Up @@ -463,7 +466,13 @@ func TestToStatefulSet(t *testing.T) {

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
statefulSet := toStatefulSet(test.meta, test.podSpec, test.volumeClaimTemplates, test.scaling, test.labels, test.annotations)
statefulSet := toStatefulSet(test.meta,
test.podSpec,
test.volumeClaimTemplates,
test.scaling,
test.statefulSetPersistentVolumeClaimRetentionPolicy,
Copy link
Member

@lc525 lc525 Oct 2, 2024 •
edited
Loading

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.

Copy link
Member Author

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 :)

test.labels,
test.annotations)
g.Expect(equality.Semantic.DeepEqual(statefulSet, test.statefulSet)).To(BeTrue())
})
}
Expand Down Expand Up @@ -523,6 +532,7 @@ func TestLabelsAnnotations(t *testing.T) {
&v1.PodSpec{},
[]mlopsv1alpha1.PersistentVolumeClaim{},
&mlopsv1alpha1.ScalingSpec{},
&appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy{},
test.metaServerConfig,
annotator)
for k, v := range test.expectedLabels {
Expand Down
Loading