feat: Added feast Go operator db stores support #4771

tmihalac · 2024-11-19T21:12:44Z

What this PR does / why we need it:

This PR adds db stores persistence configuration options to the FeatureStore CRD.
DB store para

Support is provided to the Online service, Offline service and Registry service.
Follows definitions in RFC #42
Configuration examples are provided in config/samples

kind: FeatureStore
metadata:
  name: sample-s3-registry
spec:
  feastProject: my_project
  services:
    onlineStore:
      persistence:
        store:
          type: postgres
          secretRef: 
            name: _SECRET_NAME_ # Secret keys should be placed as-is under the `type` (e.g postgres) key or under the secretKeyName if specified
          secretKeyName: my-postgres # The secret key the db parameters are in (optional parameter)
    offlineStore:
      persistence:
        store:
          type: postgres
          secretRef: 
            name: _SECRET_NAME_ # Secret keys should be placed as-is under the `type` (e.g postgres) key or under the secretKeyName if specified
          secretKeyName: my-postgres # The secret key the db parameters are in (optional parameter)
    registry:
      local:
        persistence:
          store:
            type: sql
            secretRef: 
              name: _SECRET_NAME_ # Secret keys should be placed as-is under the `type` (e.g sql) key or under the secretKeyName if specified
            secretKeyName: my-postgres # The secret key the db parameters are in (optional parameter)

kind: Secret
apiVersion: v1
metadata:
  name: my-secret
  namespace: test
  uid: b410b5e5-857b-4611-acc3-1302b8c6c68c
  resourceVersion: '62030'
  creationTimestamp: '2024-11-19T20:25:15Z'
  managedFields:
    - manager: kubectl-create
      operation: Update
      apiVersion: v1
      time: '2024-11-19T20:25:15Z'
      fieldsType: FieldsV1
      fieldsV1:
        'f:data':
          .: {}
          'f:mykey': {}
        'f:type': {}
data:
  postgres: |
    host: DB_HOST
    port: DB_PORT
    database: DB_NAME
    db_schema: DB_SCHEMA
    user: DB_USERNAME
    password: DB_PASSWORD
    sslmode: verify-ca
    sslkey_path: /path/to/client-key.pem
    sslcert_path: /path/to/client-cert.pem
    sslrootcert_path: /path/to/server-ca.pem
  sql: |
    path: postgresql://postgres:[email protected]:55001/feast
    cache_ttl_seconds: 60
    sqlalchemy_config_kwargs:
        echo: false
        pool_pre_ping: true
type: Opaque

Which issue(s) this PR fixes:

Relates to #4783

tchughesiv · 2024-11-20T00:11:41Z

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

+
+	if services.OfflineStore.Persistence != nil &&
+		services.OfflineStore.Persistence.FilePersistence != nil &&
+		len(services.OfflineStore.Persistence.FilePersistence.Type) > 0 {
+		persistenceType = services.OfflineStore.Persistence.FilePersistence.Type
+	} else if services.OfflineStore.Persistence != nil &&
+		services.OfflineStore.Persistence.DBPersistence != nil {
+		if len(services.OfflineStore.Persistence.DBPersistence.Type) > 0 {
+			var err error
+			persistenceType = services.OfflineStore.Persistence.DBPersistence.Type
+			secretKeyName := services.OfflineStore.Persistence.DBPersistence.SecretKeyName
+			if len(secretKeyName) == 0 {
+				secretKeyName = persistenceType
+			}
+			parametersMap, err = secretExtractionFunc(services.OfflineStore.Persistence.DBPersistence.SecretRef.Name, secretKeyName)
+			if err != nil {
+				return err
+			}
+		}
+	}


for these setRepoConfig functions, maybe we can clean it up a bit by consolidating?

Suggested change

if services.OfflineStore.Persistence != nil &&

services.OfflineStore.Persistence.FilePersistence != nil &&

len(services.OfflineStore.Persistence.FilePersistence.Type) > 0 {

persistenceType = services.OfflineStore.Persistence.FilePersistence.Type

} else if services.OfflineStore.Persistence != nil &&

services.OfflineStore.Persistence.DBPersistence != nil {

if len(services.OfflineStore.Persistence.DBPersistence.Type) > 0 {

var err error

persistenceType = services.OfflineStore.Persistence.DBPersistence.Type

secretKeyName := services.OfflineStore.Persistence.DBPersistence.SecretKeyName

if len(secretKeyName) == 0 {

secretKeyName = persistenceType

}

parametersMap, err = secretExtractionFunc(services.OfflineStore.Persistence.DBPersistence.SecretRef.Name, secretKeyName)

if err != nil {

return err

}

}

}

offlinePersistence := services.OfflineStore.Persistence

if offlinePersistence != nil &&

offlinePersistence.FilePersistence != nil &&

len(offlinePersistence.FilePersistence.Type) > 0 {

persistenceType = offlinePersistence.FilePersistence.Type

} else if offlinePersistence != nil &&

offlinePersistence.DBPersistence != nil {

if len(offlinePersistence.DBPersistence.Type) > 0 {

var err error

persistenceType = offlinePersistence.DBPersistence.Type

secretKeyName := offlinePersistence.DBPersistence.SecretKeyName

if len(secretKeyName) == 0 {

secretKeyName = persistenceType

}

parametersMap, err = secretExtractionFunc(offlinePersistence.DBPersistence.SecretRef.Name, secretKeyName)

if err != nil {

return err

}

}

}

tchughesiv · 2024-11-20T00:16:54Z

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

+			var err error
+			filePath = ""
+			persistenceType = services.Registry.Local.Persistence.DBPersistence.Type
+			secretKeyName := services.Registry.Local.Persistence.DBPersistence.SecretKeyName
+			if len(secretKeyName) == 0 {
+				secretKeyName = persistenceType
+			}
+			parametersMap, err = secretExtractionFunc(services.Registry.Local.Persistence.DBPersistence.SecretRef.Name, secretKeyName)


no need to declare an error var... instead use a short assignment statement as we do elsewhere in the code.

Suggested change

var err error

filePath = ""

persistenceType = services.Registry.Local.Persistence.DBPersistence.Type

secretKeyName := services.Registry.Local.Persistence.DBPersistence.SecretKeyName

if len(secretKeyName) == 0 {

secretKeyName = persistenceType

}

parametersMap, err = secretExtractionFunc(services.Registry.Local.Persistence.DBPersistence.SecretRef.Name, secretKeyName)

filePath = ""

persistenceType = services.Registry.Local.Persistence.DBPersistence.Type

secretKeyName := services.Registry.Local.Persistence.DBPersistence.SecretKeyName

if len(secretKeyName) == 0 {

secretKeyName = persistenceType

}

parametersMap, err := secretExtractionFunc(services.Registry.Local.Persistence.DBPersistence.SecretRef.Name, secretKeyName)

I can't since parametersMap is defined outside the if and is used outside the if and if I use := it shadows that value

repoConfig.Registry = RegistryConfig{ RegistryType: RegistryConfigType(persistenceType), Path: filePath, S3AdditionalKwargs: s3AdditionalKwargs, DBParameters: parametersMap, }

if I use := it shadows that value

what does this mean?
fwiw, go will detect that parametersMap has already been declared and reassign it accordingly.

e.g. this will print 3 4 5 6

package main import "fmt" func main() { var i, j int = 1, 2 k := 3 i, j, k, l := 3, 4, 5, 6 fmt.Println(i, j, k, l) }

When I change that to := I am getting an error unused variable parametersMap
see https://go.dev/play/p/WDtgq1kGlFc

ok... because its not being used until after the if statement... i see now.

tchughesiv · 2024-11-20T00:17:10Z

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

+	} else if services.OnlineStore.Persistence != nil &&
+		services.OnlineStore.Persistence.DBPersistence != nil {
+		if len(services.OnlineStore.Persistence.DBPersistence.Type) > 0 {
+			var err error


See my pervious comment

tchughesiv · 2024-11-20T00:17:16Z

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

+	} else if services.OfflineStore.Persistence != nil &&
+		services.OfflineStore.Persistence.DBPersistence != nil {
+		if len(services.OfflineStore.Persistence.DBPersistence.Type) > 0 {
+			var err error


See my pervious comment

tchughesiv · 2024-11-20T00:21:47Z

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

+	objectKey := client.ObjectKeyFromObject(secret)
+	if err := feast.Client.Get(feast.Context, objectKey, secret); err != nil {
+		if apierrors.IsNotFound(err) || err != nil {
+			return nil, fmt.Errorf("invalid secret %s for offline store", secretRef)


maybe just return the error here but log an Error prior to the return

Suggested change

return nil, fmt.Errorf("invalid secret %s for offline store", secretRef)

logger := log.FromContext(feast.Context)

logger.Error("invalid secret %s for offline store", secretRef)

return nil, err

dmartinol · 2024-11-20T07:33:54Z

@tmihalac the issue you are referring to is already closed. @tchughesiv should we create a new one or reopen it?

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

dmartinol · 2024-11-20T07:45:02Z

In the description, the postgres type is not supported by registry, pls change it to sql (and also add a sql key to the secret name)

    registry:
      local:
        persistence:
          store:
          type: postgres

dmartinol · 2024-11-20T07:49:08Z

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

can you also add a sample Secret (with postgres properties) to the same file?

Also, an example of the sql registry type could be helpful, since it has some specific management of the db type in the path property:

registry: registry_type: sql path: postgresql://postgres:[email protected]:55001/feast cache_ttl_seconds: 60 sqlalchemy_config_kwargs: echo: false pool_pre_ping: true

did you commit the change? II can't see it

Not yet still working on it, trying to find a fix for db parameters the same as the config object parameters

dmartinol · 2024-11-20T08:02:05Z

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

+
+			Expect(err.Error()).To(Equal("invalid secret invalid_secret for offline store"))
+
+			By("Referring to a secret that doesn't exist")


maybe change of of the two By("Referring to a secret that doesn't exist")?

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

dmartinol · 2024-11-20T08:31:09Z

infra/feast-operator/test/api/featurestore_types_test.go

can you add simple tests for invalid store type for each of the feast service?

tchughesiv · 2024-11-20T12:28:59Z

@tmihalac @dmartinol we should be opening new issues for operator PRs

tchughesiv · 2024-11-20T13:40:39Z

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

+	repoConfig.Registry = RegistryConfig{
+		RegistryType:       RegistryConfigType(persistenceType),
+		Path:               filePath,
+		S3AdditionalKwargs: s3AdditionalKwargs,
+		DBParameters:       parametersMap,
+	}


have you tested what happens if a user specificies a conflicting registry_type in their secret? we need to be sure the operator has final say on setting that field in the feature_store.yaml. Same goes for type in online/offlineStores. If you find it can be overwritten by the parametersMap, maybe we set RegistryType last? As a secondary line of code after this RegistryConfig declaration?

maybe the same needs to be done for filePath and s3AdditionalKwargs ?

and some unit tests around this phenomenon? :)

i'm realizing this is more important at the time of yaml marshaling... and i'm expecting that the defined fields should take precedence when that happens. it's possible all we really need here are good unit tests around the marshaled yaml to ensure this is the case.

maybe test how setting a different registry_type in DBParameters map will effect the resulting yaml

tchughesiv

@tmihalac please rebase on master branch

Signed-off-by: Theodor Mihalache <[email protected]>

tchughesiv · 2024-11-21T20:00:52Z

@tmihalac please open a new gh issue for this PR

tchughesiv · 2024-11-21T20:03:46Z

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

-	S3AdditionalKwargs *map[string]string `json:"s3_additional_kwargs,omitempty"`
+	Path               string                 `yaml:"path,omitempty"`
+	RegistryType       RegistryConfigType     `yaml:"registry_type,omitempty"`
+	S3AdditionalKwargs *map[string]string     `json:"s3_additional_kwargs,omitempty"`


can you fix this? its not related to your change but needs to be fixed.

Suggested change

S3AdditionalKwargs *map[string]string `json:"s3_additional_kwargs,omitempty"`

S3AdditionalKwargs *map[string]string `yaml:"s3_additional_kwargs,omitempty"`

tchughesiv · 2024-11-21T20:05:08Z

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

+			//val := reflect.ValueOf(s).Field(i)
+
+			//// Check that the object is a pointer so we can modify it
+			//if val.Kind() != reflect.Ptr || val.IsNil() {
+			//	return false, fmt.Errorf("expected a pointer to struct, got %v", val.Kind())
+			//}


should this be cleaned up?

Suggested change

//val := reflect.ValueOf(s).Field(i)

//// Check that the object is a pointer so we can modify it

//if val.Kind() != reflect.Ptr || val.IsNil() {

// return false, fmt.Errorf("expected a pointer to struct, got %v", val.Kind())

//}

tchughesiv · 2024-11-21T20:05:22Z

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

+	// Check if the value is a pointer and dereference it if necessary
+	//if t.Kind() == reflect.Ptr {
+	//	t = t.Elem()
+	//}


tchughesiv · 2024-11-21T20:10:43Z

@tmihalac should your descrip example be fixed? i'd imagine it will look like this? with the secretKeyName pulled back to under store?

kind: FeatureStore
metadata:
  name: sample-s3-registry
spec:
  feastProject: my_project
  services:
    onlineStore:
      persistence:
        store:
          type: postgres
          secretRef: 
            name: _SECRET_NAME_ # Secret keys should be placed as-is under the `type` (e.g postgres) key or under the secretKeyName if specified
          secretKeyName: my-postgres # The secret key the db parameters are in (optional parameter)

tmihalac · 2024-11-21T20:27:22Z

@tmihalac should your descrip example be fixed? i'd imagine it will look like this? with the secretKeyName pulled back to under store?

kind: FeatureStore
metadata:
  name: sample-s3-registry
spec:
  feastProject: my_project
  services:
    onlineStore:
      persistence:
        store:
          type: postgres
          secretRef: 
            name: _SECRET_NAME_ # Secret keys should be placed as-is under the `type` (e.g postgres) key or under the secretKeyName if specified
          secretKeyName: my-postgres # The secret key the db parameters are in (optional parameter)

You are right fixing it now

tmihalac · 2024-11-21T20:28:12Z

Done

tchughesiv · 2024-11-21T20:58:18Z

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

+	Type          string                       `json:"type,omitempty"`
+	SecretRef     *corev1.LocalObjectReference `json:"secretRef,omitempty"`


should these be required fields?

Suggested change

Type string `json:"type,omitempty"`

SecretRef *corev1.LocalObjectReference `json:"secretRef,omitempty"`

Type string `json:"type"`

SecretRef *corev1.LocalObjectReference `json:"secretRef"`

@dmartinol @lokeshrangineni @redhatHameed
I am not sure, what do you think ? anyone ?

@dmartinol gave a thumbs up. @tmihalac from a code & usability standpoint... aren't these 2 fields required if the user decides to configure a db store? (i believe they are) if so, we need to make them required in the CRD.

tchughesiv · 2024-11-21T20:58:32Z

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

+	Type          string                       `json:"type,omitempty"`
+	SecretRef     *corev1.LocalObjectReference `json:"secretRef,omitempty"`
+	SecretKeyName string                       `json:"secretKeyName,omitempty"`


tchughesiv · 2024-11-21T20:58:39Z

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

+	Type          string                       `json:"type,omitempty"`
+	SecretRef     *corev1.LocalObjectReference `json:"secretRef,omitempty"`
+	SecretKeyName string                       `json:"secretKeyName,omitempty"`


tchughesiv · 2024-11-21T21:00:10Z

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

+	// +kubebuilder:validation:Enum=snowflake.offline;bigquery;redshift;spark;postgres;feast_trino.trino.TrinoOfflineStore;redis
+	Type          string                       `json:"type,omitempty"`
+	SecretRef     *corev1.LocalObjectReference `json:"secretRef,omitempty"`
+	SecretKeyName string                       `json:"secretKeyName,omitempty"`


maybe add a comment to SecretKeyName explaining what the default will be?

@tchughesiv
What do you mean by default ? there is no default , do you mean when that field is not specified in the CR ?

do you mean when that field is not specified in the CR ?

yes ... isn't the default here the db store type? i'm saying just add a comment above the field (which will add documentation to the CRD) so the user understands this

Signed-off-by: Theodor Mihalache <[email protected]>

tmihalac requested a review from a team as a code owner November 19, 2024 21:12

tchughesiv suggested changes Nov 20, 2024

View reviewed changes