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

fix: Made fixes to Go Operator DB persistence #4830

Merged
merged 2 commits into from
Dec 11, 2024
+128 −94
Merged

fix: Made fixes to Go Operator DB persistence #4830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cab352
Made fixes to Go Operator DB persistence
tmihalac Dec 10, 2024
feedb20
Fixes following review
tmihalac Dec 11, 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
15 changes: 11 additions & 4 deletions infra/feast-operator/api/v1alpha1/featurestore_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,19 +99,20 @@ type OfflineStorePersistence struct {

// OfflineStoreFilePersistence configures the file-based persistence for the offline store service
type OfflineStoreFilePersistence struct {
// +kubebuilder:validation:Enum=dask;duckdb
// +kubebuilder:validation:Enum=file;dask;duckdb
Type string `json:"type,omitempty"`
PvcConfig *PvcConfig `json:"pvc,omitempty"`
}

var ValidOfflineStoreFilePersistenceTypes = []string{
"dask",
"duckdb",
"file",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it appears this is the same as dask... so maybe we don't include it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is supported and not deprecated yet, so I think we should include it as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was removed from the doc since v0.40
https://docs.feast.dev/v0.40-branch/reference/offline-stores/overview

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was removed from the doc since v0.40 https://docs.feast.dev/v0.40-branch/reference/offline-stores/overview

But not the code and is not deprecated in the code

}

// OfflineStoreDBStorePersistence configures the DB store persistence for the offline store service
type OfflineStoreDBStorePersistence struct {
// +kubebuilder:validation:Enum=snowflake.offline;bigquery;redshift;spark;postgres;feast_trino.trino.TrinoOfflineStore;redis
// +kubebuilder:validation:Enum=snowflake.offline;bigquery;redshift;spark;postgres;trino;redis;athena;mssql
Type string `json:"type"`
// Data store parameters should be placed as-is from the "feature_store.yaml" under the secret key. "registry_type" & "type" fields should be removed.
SecretRef corev1.LocalObjectReference `json:"secretRef"`
Expand All @@ -125,8 +126,10 @@ var ValidOfflineStoreDBStorePersistenceTypes = []string{
"redshift",
"spark",
"postgres",
"feast_trino.trino.TrinoOfflineStore",
"trino",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the docs are using "feast_trino.trino.TrinoOfflineStore" in the example configs. Is there an issue tracking the defect? otherwise, you can simply fix it here 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmartinol it appears the docs are out of date ... here's the code where these classes/types are declared -

feast/sdk/python/feast/repo_config.py

Lines 87 to 100 in a36f7e5

OFFLINE_STORE_CLASS_FOR_TYPE = {
"file": "feast.infra.offline_stores.dask.DaskOfflineStore",
"dask": "feast.infra.offline_stores.dask.DaskOfflineStore",
"bigquery": "feast.infra.offline_stores.bigquery.BigQueryOfflineStore",
"redshift": "feast.infra.offline_stores.redshift.RedshiftOfflineStore",
"snowflake.offline": "feast.infra.offline_stores.snowflake.SnowflakeOfflineStore",
"spark": "feast.infra.offline_stores.contrib.spark_offline_store.spark.SparkOfflineStore",
"trino": "feast.infra.offline_stores.contrib.trino_offline_store.trino.TrinoOfflineStore",
"postgres": "feast.infra.offline_stores.contrib.postgres_offline_store.postgres.PostgreSQLOfflineStore",
"athena": "feast.infra.offline_stores.contrib.athena_offline_store.athena.AthenaOfflineStore",
"mssql": "feast.infra.offline_stores.contrib.mssql_offline_store.mssql.MsSqlServerOfflineStore",
"duckdb": "feast.infra.offline_stores.duckdb.DuckDBOfflineStore",
"remote": "feast.infra.offline_stores.remote.RemoteOfflineStore",
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I was trying to suggest: raise an issue (optional) and fix the docs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ... agree @dmartinol

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue #4837

"redis",
"athena",
"mssql",
}

// OnlineStore configures the deployed online store service
Expand Down Expand Up @@ -158,7 +161,7 @@ type OnlineStoreFilePersistence struct {

// OnlineStoreDBStorePersistence configures the DB store persistence for the offline store service
type OnlineStoreDBStorePersistence struct {
// +kubebuilder:validation:Enum=snowflake.online;redis;ikv;datastore;dynamodb;bigtable;postgres;cassandra;mysql;hazelcast;singlestore
// +kubebuilder:validation:Enum=snowflake.online;redis;ikv;datastore;dynamodb;bigtable;postgres;cassandra;mysql;hazelcast;singlestore;hbase;elasticsearch;qdrant;couchbase
Type string `json:"type"`
// Data store parameters should be placed as-is from the "feature_store.yaml" under the secret key. "registry_type" & "type" fields should be removed.
SecretRef corev1.LocalObjectReference `json:"secretRef"`
Expand All @@ -178,6 +181,10 @@ var ValidOnlineStoreDBStorePersistenceTypes = []string{
"mysql",
"hazelcast",
"singlestore",
"hbase",
"elasticsearch",
"qdrant",
"couchbase",
}

// LocalRegistryConfig configures the deployed registry service
Expand Down
18 changes: 16 additions & 2 deletions infra/feast-operator/config/crd/bases/feast.dev_featurestores.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ spec:
rule: self.mountPath.matches('^/[^:]*$')
type:
enum:
- file
- dask
- duckdb
type: string
Expand Down Expand Up @@ -355,8 +356,10 @@ spec:
- redshift
- spark
- postgres
- feast_trino.trino.TrinoOfflineStore
- trino
- redis
- athena
- mssql
type: string
required:
- secretRef
Expand Down Expand Up @@ -729,6 +732,10 @@ spec:
- mysql
- hazelcast
- singlestore
- hbase
- elasticsearch
- qdrant
- couchbase
type: string
required:
- secretRef
Expand Down Expand Up @@ -1559,6 +1566,7 @@ spec:
rule: self.mountPath.matches('^/[^:]*$')
type:
enum:
- file
- dask
- duckdb
type: string
Expand Down Expand Up @@ -1592,8 +1600,10 @@ spec:
- redshift
- spark
- postgres
- feast_trino.trino.TrinoOfflineStore
- trino
- redis
- athena
- mssql
type: string
required:
- secretRef
Expand Down Expand Up @@ -1973,6 +1983,10 @@ spec:
- mysql
- hazelcast
- singlestore
- hbase
- elasticsearch
- qdrant
- couchbase
type: string
required:
- secretRef
Expand Down
18 changes: 16 additions & 2 deletions infra/feast-operator/dist/install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ spec:
rule: self.mountPath.matches('^/[^:]*$')
type:
enum:
- file
- dask
- duckdb
type: string
Expand Down Expand Up @@ -363,8 +364,10 @@ spec:
- redshift
- spark
- postgres
- feast_trino.trino.TrinoOfflineStore
- trino
- redis
- athena
- mssql
type: string
required:
- secretRef
Expand Down Expand Up @@ -737,6 +740,10 @@ spec:
- mysql
- hazelcast
- singlestore
- hbase
- elasticsearch
- qdrant
- couchbase
type: string
required:
- secretRef
Expand Down Expand Up @@ -1567,6 +1574,7 @@ spec:
rule: self.mountPath.matches('^/[^:]*$')
type:
enum:
- file
- dask
- duckdb
type: string
Expand Down Expand Up @@ -1600,8 +1608,10 @@ spec:
- redshift
- spark
- postgres
- feast_trino.trino.TrinoOfflineStore
- trino
- redis
- athena
- mssql
type: string
required:
- secretRef
Expand Down Expand Up @@ -1981,6 +1991,10 @@ spec:
- mysql
- hazelcast
- singlestore
- hbase
- elasticsearch
- qdrant
- couchbase
type: string
required:
- secretRef
Expand Down
117 changes: 57 additions & 60 deletions infra/feast-operator/internal/controller/featurestore_controller_db_store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ sqlalchemy_config_kwargs:
pool_pre_ping: true
`

var invalidSecretContainingTypeYamlString = `
var secretContainingValidTypeYamlString = `
type: cassandra
hosts:
- 192.168.1.1
Expand Down Expand Up @@ -305,37 +305,12 @@ var _ = Describe("FeatureStore Controller - db storage services", func() {

Expect(err.Error()).To(Equal("secret key invalid.secret.key doesn't exist in secret online-store-secret"))

By("Referring to a secret that contains parameter named type")
resource = &feastdevv1alpha1.FeatureStore{}
err = k8sClient.Get(ctx, typeNamespacedName, resource)
Expect(err).NotTo(HaveOccurred())

secret := &corev1.Secret{}
err = k8sClient.Get(ctx, onlineSecretNamespacedName, secret)
Expect(err).NotTo(HaveOccurred())
secret.Data[string(services.OnlineDBPersistenceCassandraConfigType)] = []byte(invalidSecretContainingTypeYamlString)
Expect(k8sClient.Update(ctx, secret)).To(Succeed())

resource.Spec.Services.OnlineStore.Persistence.DBPersistence.SecretRef = corev1.LocalObjectReference{Name: "online-store-secret"}
resource.Spec.Services.OnlineStore.Persistence.DBPersistence.SecretKeyName = ""
Expect(k8sClient.Update(ctx, resource)).To(Succeed())
resource = &feastdevv1alpha1.FeatureStore{}
err = k8sClient.Get(ctx, typeNamespacedName, resource)
Expect(err).NotTo(HaveOccurred())

_, err = controllerReconciler.Reconcile(ctx, reconcile.Request{
NamespacedName: typeNamespacedName,
})
Expect(err).To(HaveOccurred())

Expect(err.Error()).To(Equal("secret key cassandra in secret online-store-secret contains invalid tag named type"))

By("Referring to a secret that contains parameter named type with invalid value")
resource = &feastdevv1alpha1.FeatureStore{}
err = k8sClient.Get(ctx, typeNamespacedName, resource)
Expect(err).NotTo(HaveOccurred())

secret = &corev1.Secret{}
secret := &corev1.Secret{}
err = k8sClient.Get(ctx, onlineSecretNamespacedName, secret)
Expect(err).NotTo(HaveOccurred())
secret.Data[string(services.OnlineDBPersistenceCassandraConfigType)] = []byte(invalidSecretTypeYamlString)
Expand All @@ -353,39 +328,7 @@ var _ = Describe("FeatureStore Controller - db storage services", func() {
})
Expect(err).To(HaveOccurred())

Expect(err.Error()).To(Equal("secret key cassandra in secret online-store-secret contains invalid tag named type"))

By("Referring to a secret that contains parameter named registry_type")
resource = &feastdevv1alpha1.FeatureStore{}
err = k8sClient.Get(ctx, typeNamespacedName, resource)
Expect(err).NotTo(HaveOccurred())

secret = &corev1.Secret{}
err = k8sClient.Get(ctx, onlineSecretNamespacedName, secret)
Expect(err).NotTo(HaveOccurred())
secret.Data[string(services.OnlineDBPersistenceCassandraConfigType)] = []byte(cassandraYamlString)
Expect(k8sClient.Update(ctx, secret)).To(Succeed())

secret = &corev1.Secret{}
err = k8sClient.Get(ctx, registrySecretNamespacedName, secret)
Expect(err).NotTo(HaveOccurred())
secret.Data["sql_custom_registry_key"] = nil
secret.Data[string(services.RegistryDBPersistenceSQLConfigType)] = []byte(invalidSecretRegistryTypeYamlString)
Expect(k8sClient.Update(ctx, secret)).To(Succeed())

resource.Spec.Services.Registry.Local.Persistence.DBPersistence.SecretRef = corev1.LocalObjectReference{Name: "registry-store-secret"}
resource.Spec.Services.Registry.Local.Persistence.DBPersistence.SecretKeyName = ""
Expect(k8sClient.Update(ctx, resource)).To(Succeed())
resource = &feastdevv1alpha1.FeatureStore{}
err = k8sClient.Get(ctx, typeNamespacedName, resource)
Expect(err).NotTo(HaveOccurred())

_, err = controllerReconciler.Reconcile(ctx, reconcile.Request{
NamespacedName: typeNamespacedName,
})
Expect(err).To(HaveOccurred())

Expect(err.Error()).To(Equal("secret key sql in secret registry-store-secret contains invalid tag named registry_type"))
Expect(err.Error()).To(Equal("secret key cassandra in secret online-store-secret contains tag named type with value wrong"))
tchughesiv marked this conversation as resolved.
Show resolved Hide resolved
})

It("should successfully reconcile the resource", func() {
Expand Down Expand Up @@ -506,6 +449,60 @@ var _ = Describe("FeatureStore Controller - db storage services", func() {
Expect(err).NotTo(HaveOccurred())
Expect(controllerutil.HasControllerReference(svc)).To(BeTrue())
Expect(svc.Spec.Ports[0].TargetPort).To(Equal(intstr.FromInt(int(services.FeastServiceConstants[services.RegistryFeastType].TargetHttpPort))))

By("Referring to a secret that contains parameter named type")
resource = &feastdevv1alpha1.FeatureStore{}
err = k8sClient.Get(ctx, typeNamespacedName, resource)
Expect(err).NotTo(HaveOccurred())

secret := &corev1.Secret{}
err = k8sClient.Get(ctx, onlineSecretNamespacedName, secret)
Expect(err).NotTo(HaveOccurred())
secret.Data[string(services.OnlineDBPersistenceCassandraConfigType)] = []byte(secretContainingValidTypeYamlString)
Expect(k8sClient.Update(ctx, secret)).To(Succeed())

resource.Spec.Services.OnlineStore.Persistence.DBPersistence.SecretRef = corev1.LocalObjectReference{Name: "online-store-secret"}
resource.Spec.Services.OnlineStore.Persistence.DBPersistence.SecretKeyName = ""
Expect(k8sClient.Update(ctx, resource)).To(Succeed())
resource = &feastdevv1alpha1.FeatureStore{}
err = k8sClient.Get(ctx, typeNamespacedName, resource)
Expect(err).NotTo(HaveOccurred())

_, err = controllerReconciler.Reconcile(ctx, reconcile.Request{
NamespacedName: typeNamespacedName,
})

Expect(err).To(Not(HaveOccurred()))

By("Referring to a secret that contains parameter named registry_type")
resource = &feastdevv1alpha1.FeatureStore{}
err = k8sClient.Get(ctx, typeNamespacedName, resource)
Expect(err).NotTo(HaveOccurred())

secret = &corev1.Secret{}
err = k8sClient.Get(ctx, onlineSecretNamespacedName, secret)
Expect(err).NotTo(HaveOccurred())
secret.Data[string(services.OnlineDBPersistenceCassandraConfigType)] = []byte(cassandraYamlString)
Expect(k8sClient.Update(ctx, secret)).To(Succeed())

secret = &corev1.Secret{}
err = k8sClient.Get(ctx, registrySecretNamespacedName, secret)
Expect(err).NotTo(HaveOccurred())
secret.Data["sql_custom_registry_key"] = nil
secret.Data[string(services.RegistryDBPersistenceSQLConfigType)] = []byte(invalidSecretRegistryTypeYamlString)
Expect(k8sClient.Update(ctx, secret)).To(Succeed())

resource.Spec.Services.Registry.Local.Persistence.DBPersistence.SecretRef = corev1.LocalObjectReference{Name: "registry-store-secret"}
resource.Spec.Services.Registry.Local.Persistence.DBPersistence.SecretKeyName = ""
Expect(k8sClient.Update(ctx, resource)).To(Succeed())
resource = &feastdevv1alpha1.FeatureStore{}
err = k8sClient.Get(ctx, typeNamespacedName, resource)
Expect(err).NotTo(HaveOccurred())

_, err = controllerReconciler.Reconcile(ctx, reconcile.Request{
NamespacedName: typeNamespacedName,
})
Expect(err).To(Not(HaveOccurred()))
})

It("should properly encode a feature_store.yaml config", func() {
Expand Down
Loading
Loading