feat: PVC configuration and impl #4750

dmartinol · 2024-11-08T17:26:20Z

What this PR does / why we need it:

Adds durable persistence configuration to Feast services deployed with the operator.

PVC configuration allows to either create a new PVC or use an existing one:

apiVersion: feast.dev/v1alpha1
kind: FeatureStore
metadata:
  name: sample-pvc-persistence
spec:
  feastProject: my_project
  services:
    onlineStore:
      persistence:
        file:
          path: online_store.db
          pvc:
            ref:
              name: online-pvc
            mountPath: /data/online
    offlineStore:
      persistence:
        file:
          type: duckdb
          pvc:
            create:
              storageClassName: standard
              resources:
                requests:
                  storage: 5Gi
            mountPath: /data/offline
    registry:
      local:
        persistence:
          file:
            path: registry.db
            pvc:
              create: {}
              mountPath: /data/registry

Which issue(s) this PR fixes:

Relates to #4561

Signed-off-by: Daniele Martinoli <[email protected]>

dmartinol · 2024-11-08T17:27:23Z

@tchughesiv @tmihalac I also splitted the controller tests in multiple files to simplify them

infra/feast-operator/config/samples/v1alpha1_featurestore_ephemeral_persistence.yaml

tchughesiv · 2024-11-08T17:40:41Z

infra/feast-operator/internal/controller/services/services.go

+	err := feast.setPVC(pvc, pvcCreate, feastType)
+	if err != nil {
+		return err
+	}
+	if op, err := controllerutil.CreateOrUpdate(feast.Context, feast.Client, pvc, controllerutil.MutateFn(func() error {
+		return nil


Suggested change

err := feast.setPVC(pvc, pvcCreate, feastType)

if err != nil {

return err

}

if op, err := controllerutil.CreateOrUpdate(feast.Context, feast.Client, pvc, controllerutil.MutateFn(func() error {

return nil

if op, err := controllerutil.CreateOrUpdate(feast.Context, feast.Client, pvc, controllerutil.MutateFn(func() error {

return feast.setPVC(pvc, pvcCreate, feastType)

the PVC object mods need to happen within the CreateOrUpdate func ()

was this moved because of the immutable nature of the PVC?

if so, we could be explicit with PVC creation and use feast.Client.Create() here instead? it could be used alongside if !apierrors.IsAlreadyExists(err) {}

Tried the Create option but creation was not an issue. Deletion seems to fail, at least in the tests (just restored them to see the failure)

Signed-off-by: Daniele Martinoli <[email protected]>

tchughesiv · 2024-11-11T17:37:34Z

infra/feast-operator/api/v1alpha1/featurestore_types.go

 type RegistryFilePersistence struct {
-	Path string `json:"path,omitempty"`
+	Path      string     `json:"path,omitempty"`
+	PvcConfig *PvcConfig `json:"pvc,omitempty"`
+}


OnlineStoreFilePersistence and RegistryFilePersistence are identical. Any reason we shouldn't make a single FilePersistence struct instead?

Registry also supports the s3_additional_kwargs options (which will go under the Registry.Persistence.File section). Will we add it to a new PR dedicated to object store settings.

ok, so this is in prep for future work... got it, thanks

infra/feast-operator/internal/controller/services/services.go

Signed-off-by: Daniele Martinoli <[email protected]>

infra/feast-operator/internal/controller/services/services.go

Signed-off-by: Daniele Martinoli <[email protected]>

tchughesiv

lgtm

dmartinol · 2024-11-12T06:56:19Z

@lokeshrangineni @redhatHameed can you take a look?

redhatHameed

Thanks lgtm some minor comments.

redhatHameed · 2024-11-12T15:34:43Z

infra/feast-operator/internal/controller/services/repo_config.go

 		// Offline server has an `offline_store` section and a remote `registry`
-		if feastType == OfflineFeastType && appliedSpec.Services.OfflineStore != nil {
+		if feastType == OfflineFeastType && services.OfflineStore != nil {


Not an issue, how about using switch statement here

switch feastType { case OfflineFeastType: if services.OfflineStore != nil {

redhatHameed · 2024-11-12T15:46:40Z

infra/feast-operator/config/samples/v1alpha1_featurestore_objectstore_persistence.yaml

+            s3_additional_kwargs:
+              ServerSideEncryption: AES256
+              ACL: bucket-owner-full-control
+              CacheControl: max-age=3600


add new line

this file should not be in the PR, will remove it

redhatHameed · 2024-11-12T16:01:47Z

infra/feast-operator/config/samples/v1alpha1_featurestore_pvc_persistence.yaml

+            path: registry.db
+            pvc:
+              create: {}
+              mountPath: /data/registry


why create section empty here, is there related docs added for this configuration. and looks the settings not consistent with each type

For example registry

file: path: registry.db pvc: create: {} mountPath: /data/registry

online

path: online_store.db pvc: ref: name: online-pvc mountPath: /data/online

i think he's demonstrating 3 different types of pvc configurations. all 3 feast service types can be configured similarly. an empty create: tells the operator that the user wants a pvc, but doesn't want to define it. the operator will apply defaults during the creation.

There is no doc apart from the CRD definition, which has some comments on it.
wrt your questions:

pvc can have either create or ref, and create has default fields in case they are omitted
Anyway, to avoid misunderstandings, I will add the other settings too and avoid defaults

imo, CRD should be the doc of choice for API. @dmartinol i like the file in its current state, shows the user all their options.

agree, reverted previous change

redhatHameed · 2024-11-12T16:08:10Z

infra/feast-operator/internal/controller/services/util.go

+func hasPvcConfig(featureStore *feastdevv1alpha1.FeatureStore, feastType FeastServiceType) (*feastdevv1alpha1.PvcConfig, bool) {
+	services := featureStore.Status.Applied.Services
+	var pvcConfig *feastdevv1alpha1.PvcConfig = nil
+	if feastType == OnlineFeastType && services.OnlineStore != nil && services.OnlineStore.Persistence.FilePersistence != nil {


not an issue, how about considering switch statment here

switch feastType { case OnlineFeastType: if services.OnlineStore != nil {

@tchughesiv ? I also thought of using the switch but I wanted to be consistent with the existing code

i think that would be a better approach, so def go for it

tchughesiv · 2024-11-12T16:29:37Z

infra/feast-operator/internal/controller/services/services.go

+func (feast *FeastServices) createNewPVC(pvcCreate *feastdevv1alpha1.PvcCreate, feastType FeastServiceType) (*corev1.PersistentVolumeClaim, error) {
+	pvc := feast.initPVC(feastType)
+
+	pvc.Spec = corev1.PersistentVolumeClaimSpec{
+		AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteMany},
+		Resources:   pvcCreate.Resources,
+	}
+	return pvc, controllerutil.SetControllerReference(feast.FeatureStore, pvc, feast.Scheme)
+}
+


is StorageClassName set elsewhere? i can't seem to find where its being set.

good catch. will add a test for it

Signed-off-by: Daniele Martinoli <[email protected]>

tchughesiv · 2024-11-12T21:52:52Z

infra/feast-operator/internal/controller/featurestore_controller_pvc_test.go

@@ -67,6 +67,8 @@ var _ = Describe("FeatureStore Controller-Ephemeral services", func() {
 		onlineStoreMountPath := "/online"
 		registryMountPath := "/registry"

+		storageClassName := "default"


it's a bit confusing to use "default" as the storageClassName here, since you're testing a scenario where a user actually defines the storage class name... as opposed to leaving it blank and using the default. maybe use a different name? "test" ?

dmartinol added 2 commits November 8, 2024 18:21

PVC configuration and impl

ec7c829

Signed-off-by: Daniele Martinoli <[email protected]>

typo in sample manifest

5a4f954

Signed-off-by: Daniele Martinoli <[email protected]>

dmartinol requested a review from a team as a code owner November 8, 2024 17:26

tchughesiv suggested changes Nov 8, 2024

View reviewed changes

dmartinol added 5 commits November 8, 2024 20:45

removing private images

4ea2284

Signed-off-by: Daniele Martinoli <[email protected]>

More logs for PVC deletion and changing create strategy

62a6669

Signed-off-by: Daniele Martinoli <[email protected]>

using createNewPVC

8426e7b

Signed-off-by: Daniele Martinoli <[email protected]>

fixed broken test

cf3027a

Signed-off-by: Daniele Martinoli <[email protected]>

validating PVC config in applied status

32d47ad

Signed-off-by: Daniele Martinoli <[email protected]>

tchughesiv reviewed Nov 11, 2024

View reviewed changes

tchughesiv suggested changes Nov 11, 2024

View reviewed changes

infra/feast-operator/internal/controller/services/services.go Outdated Show resolved Hide resolved

tchughesiv reviewed Nov 11, 2024

View reviewed changes

infra/feast-operator/internal/controller/services/services.go Outdated Show resolved Hide resolved

infra/feast-operator/internal/controller/services/services.go Outdated Show resolved Hide resolved

tchughesiv suggested changes Nov 11, 2024

View reviewed changes

infra/feast-operator/internal/controller/services/services.go Outdated Show resolved Hide resolved

simplified PVC deletion logic

398e51d

Signed-off-by: Daniele Martinoli <[email protected]>

tchughesiv suggested changes Nov 11, 2024

View reviewed changes

infra/feast-operator/internal/controller/services/services.go Outdated Show resolved Hide resolved

ignoring err when PVC is deleted

6644853

Signed-off-by: Daniele Martinoli <[email protected]>

tchughesiv approved these changes Nov 11, 2024

View reviewed changes

lokeshrangineni approved these changes Nov 12, 2024

View reviewed changes

lokeshrangineni enabled auto-merge (squash) November 12, 2024 15:20

redhatHameed reviewed Nov 12, 2024

View reviewed changes

tchughesiv reviewed Nov 12, 2024

View reviewed changes

dmartinol force-pushed the persistence_pvc branch from 206f3a7 to 6644853 Compare November 12, 2024 20:32

dmartinol added 2 commits November 12, 2024 22:26

integrating latest comments

ab395da

Signed-off-by: Daniele Martinoli <[email protected]>

reverting sample

2ae0161

Signed-off-by: Daniele Martinoli <[email protected]>

tchughesiv suggested changes Nov 12, 2024

View reviewed changes

redhatHameed approved these changes Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: PVC configuration and impl #4750

feat: PVC configuration and impl #4750

dmartinol commented Nov 8, 2024 •

edited

Loading

dmartinol commented Nov 8, 2024

tchughesiv Nov 8, 2024

tchughesiv Nov 8, 2024

tchughesiv Nov 8, 2024

tchughesiv Nov 8, 2024 •

edited

Loading

dmartinol Nov 8, 2024

tchughesiv Nov 11, 2024

dmartinol Nov 11, 2024

tchughesiv Nov 11, 2024

tchughesiv left a comment

dmartinol commented Nov 12, 2024

redhatHameed left a comment •

edited

Loading

redhatHameed Nov 12, 2024

redhatHameed Nov 12, 2024

dmartinol Nov 12, 2024

redhatHameed Nov 12, 2024

tchughesiv Nov 12, 2024

dmartinol Nov 12, 2024

tchughesiv Nov 12, 2024

dmartinol Nov 12, 2024

redhatHameed Nov 12, 2024

dmartinol Nov 12, 2024

tchughesiv Nov 12, 2024

tchughesiv Nov 12, 2024

dmartinol Nov 12, 2024

tchughesiv Nov 12, 2024

feat: PVC configuration and impl #4750

Are you sure you want to change the base?

feat: PVC configuration and impl #4750

Conversation

dmartinol commented Nov 8, 2024 • edited Loading

What this PR does / why we need it:

Which issue(s) this PR fixes:

dmartinol commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchughesiv Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchughesiv left a comment

Choose a reason for hiding this comment

dmartinol commented Nov 12, 2024

redhatHameed left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmartinol commented Nov 8, 2024 •

edited

Loading

tchughesiv Nov 8, 2024 •

edited

Loading

redhatHameed left a comment •

edited

Loading