Skip to content

Commit

Permalink
chore: Operator / Feast should have matching Data Source types (#5041)
Browse files Browse the repository at this point in the history
data store types

Signed-off-by: Tommy Hughes <tohughes@redhat.com>
  • Loading branch information
tchughesiv authored Feb 10, 2025
1 parent b24d531 commit d937dcb
Show file tree
Hide file tree
Showing 11 changed files with 145 additions and 19 deletions.
4 changes: 1 addition & 3 deletions .github/workflows/operator_pr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ jobs:
with:
go-version: 1.22.9
- name: Operator tests
run: |
cd infra/feast-operator/
make test
run: make -C infra/feast-operator test
- name: After code formatting, check for uncommitted differences
run: git diff --exit-code infra/feast-operator
6 changes: 6 additions & 0 deletions .github/workflows/pr_local_integration_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,9 @@ jobs:
- name: Test local integration tests
if: ${{ always() }} # this will guarantee that step won't be canceled and resources won't leak
run: make test-python-integration-local
- name: Install Go
uses: actions/setup-go@v5
with:
go-version: 1.22.9
- name: Operator Data Source types test
run: make -C infra/feast-operator test-datasources
3 changes: 3 additions & 0 deletions infra/feast-operator/.golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ issues:
linters:
- dupl
- lll
- path: "test/*"
linters:
- lll
linters:
disable-all: true
enable:
Expand Down
8 changes: 7 additions & 1 deletion infra/feast-operator/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,19 @@ vet: ## Run go vet against code.

.PHONY: test
test: build-installer vet lint envtest ## Run tests.
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test $$(go list ./... | grep -v /e2e) -coverprofile cover.out
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test $$(go list ./... | grep -v test/e2e | grep -v test/data-source-types) -coverprofile cover.out

# Utilize Kind or modify the e2e tests to load the image locally, enabling compatibility with other vendors.
.PHONY: test-e2e # Run the e2e tests against a Kind k8s instance that is spun up.
test-e2e:
go test -timeout 30m ./test/e2e/ -v -ginkgo.v

# Requires python3
.PHONY: test-datasources
test-datasources:
python3 test/data-source-types/data-source-types.py
go test ./test/data-source-types/

.PHONY: lint
lint: golangci-lint ## Run golangci-lint linter & yamllint
$(GOLANGCI_LINT) run
Expand Down
11 changes: 5 additions & 6 deletions infra/feast-operator/api/v1alpha1/featurestore_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ var ValidOfflineStoreFilePersistenceTypes = []string{

// OfflineStoreDBStorePersistence configures the DB store persistence for the offline store service
type OfflineStoreDBStorePersistence struct {
// Type of the persistence type you want to use. Allowed values are: snowflake.offline, bigquery, redshift, spark, postgres, trino, redis, athena, mssql
// +kubebuilder:validation:Enum=snowflake.offline;bigquery;redshift;spark;postgres;trino;redis;athena;mssql
// Type of the persistence type you want to use.
// +kubebuilder:validation:Enum=snowflake.offline;bigquery;redshift;spark;postgres;trino;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 @@ -130,7 +130,6 @@ var ValidOfflineStoreDBStorePersistenceTypes = []string{
"spark",
"postgres",
"trino",
"redis",
"athena",
"mssql",
}
Expand Down Expand Up @@ -160,7 +159,7 @@ type OnlineStoreFilePersistence struct {

// OnlineStoreDBStorePersistence configures the DB store persistence for the online store service
type OnlineStoreDBStorePersistence struct {
// Type of the persistence type you want to use. Allowed values are: snowflake.online, redis, ikv, datastore, dynamodb, bigtable, postgres, cassandra, mysql, hazelcast, singlestore, hbase, elasticsearch, qdrant, couchbase, milvus
// Type of the persistence type you want to use.
// +kubebuilder:validation:Enum=snowflake.online;redis;ikv;datastore;dynamodb;bigtable;postgres;cassandra;mysql;hazelcast;singlestore;hbase;elasticsearch;qdrant;couchbase;milvus
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.
Expand Down Expand Up @@ -215,7 +214,7 @@ type RegistryFilePersistence struct {

// RegistryDBStorePersistence configures the DB store persistence for the registry service
type RegistryDBStorePersistence struct {
// Type of the persistence type you want to use. Allowed values are: sql, snowflake.registry
// Type of the persistence type you want to use.
// +kubebuilder:validation:Enum=sql;snowflake.registry
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.
Expand All @@ -225,8 +224,8 @@ type RegistryDBStorePersistence struct {
}

var ValidRegistryDBStorePersistenceTypes = []string{
"snowflake.registry",
"sql",
"snowflake.registry",
}

// PvcConfig defines the settings for a persistent file store based on PVCs.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ spec:
- spark
- postgres
- trino
- redis
- athena
- mssql
type: string
Expand Down Expand Up @@ -2083,7 +2082,6 @@ spec:
- spark
- postgres
- trino
- redis
- athena
- mssql
type: string
Expand Down
2 changes: 0 additions & 2 deletions infra/feast-operator/dist/install.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,6 @@ spec:
- spark
- postgres
- trino
- redis
- athena
- mssql
type: string
Expand Down Expand Up @@ -2091,7 +2090,6 @@ spec:
- spark
- postgres
- trino
- redis
- athena
- mssql
type: string
Expand Down
4 changes: 2 additions & 2 deletions infra/feast-operator/docs/api/markdown/ref.md
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ _Appears in:_

| Field | Description |
| --- | --- |
| `type` _string_ | Type of the persistence type you want to use. Allowed values are: snowflake.offline, bigquery, redshift, spark, postgres, trino, redis, athena, mssql |
| `type` _string_ | Type of the persistence type you want to use. |
| `secretRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#localobjectreference-v1-core)_ | Data store parameters should be placed as-is from the "feature_store.yaml" under the secret key. "registry_type" & "type" fields should be removed. |
| `secretKeyName` _string_ | By default, the selected store "type" is used as the SecretKeyName |

Expand Down Expand Up @@ -286,7 +286,7 @@ _Appears in:_

| Field | Description |
| --- | --- |
| `type` _string_ | Type of the persistence type you want to use. Allowed values are: snowflake.online, redis, ikv, datastore, dynamodb, bigtable, postgres, cassandra, mysql, hazelcast, singlestore, hbase, elasticsearch, qdrant, couchbase, milvus |
| `type` _string_ | Type of the persistence type you want to use. |
| `secretRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#localobjectreference-v1-core)_ | Data store parameters should be placed as-is from the "feature_store.yaml" under the secret key. "registry_type" & "type" fields should be removed. |
| `secretKeyName` _string_ | By default, the selected store "type" is used as the SecretKeyName |

Expand Down
18 changes: 15 additions & 3 deletions infra/feast-operator/test/api/featurestore_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package api

import (
"context"
"fmt"
"strings"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
Expand Down Expand Up @@ -334,6 +336,16 @@ func registryStoreWithDBPersistenceType(dbPersistenceType string, featureStore *
return fsCopy
}

func quotedSlice(stringSlice []string) string {
quotedSlice := make([]string, len(stringSlice))

for i, str := range stringSlice {
quotedSlice[i] = fmt.Sprintf("\"%s\"", str)
}

return strings.Join(quotedSlice, ", ")
}

const resourceName = "test-resource"
const namespaceName = "default"

Expand Down Expand Up @@ -377,7 +389,7 @@ var _ = Describe("FeatureStore API", func() {
})

It("should fail when db persistence type is invalid", func() {
attemptInvalidCreationAndAsserts(ctx, onlineStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: \"snowflake.online\", \"redis\", \"ikv\", \"datastore\", \"dynamodb\", \"bigtable\", \"postgres\", \"cassandra\", \"mysql\", \"hazelcast\", \"singlestore\", \"hbase\", \"elasticsearch\", \"qdrant\", \"couchbase\"")
attemptInvalidCreationAndAsserts(ctx, onlineStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: "+quotedSlice(feastdevv1alpha1.ValidOnlineStoreDBStorePersistenceTypes))
})
})

Expand All @@ -388,7 +400,7 @@ var _ = Describe("FeatureStore API", func() {
attemptInvalidCreationAndAsserts(ctx, offlineStoreWithUnmanagedFileType(featurestore), "Unsupported value")
})
It("should fail when db persistence type is invalid", func() {
attemptInvalidCreationAndAsserts(ctx, offlineStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: \"snowflake.offline\", \"bigquery\", \"redshift\", \"spark\", \"postgres\", \"trino\", \"redis\", \"athena\", \"mssql\"")
attemptInvalidCreationAndAsserts(ctx, offlineStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: "+quotedSlice(feastdevv1alpha1.ValidOfflineStoreDBStorePersistenceTypes))
})
})

Expand All @@ -410,7 +422,7 @@ var _ = Describe("FeatureStore API", func() {
attemptInvalidCreationAndAsserts(ctx, registryWithS3AdditionalKeywordsForGsBucket(featurestore), "Additional S3 settings are available only for S3 object store URIs")
})
It("should fail when db persistence type is invalid", func() {
attemptInvalidCreationAndAsserts(ctx, registryStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: \"sql\", \"snowflake.registry\"")
attemptInvalidCreationAndAsserts(ctx, registryStoreWithDBPersistenceType("invalid", featurestore), "Unsupported value: \"invalid\": supported values: "+quotedSlice(feastdevv1alpha1.ValidRegistryDBStorePersistenceTypes))
})
})

Expand Down
18 changes: 18 additions & 0 deletions infra/feast-operator/test/data-source-types/data-source-types.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import os
from feast.repo_config import REGISTRY_CLASS_FOR_TYPE, OFFLINE_STORE_CLASS_FOR_TYPE, ONLINE_STORE_CLASS_FOR_TYPE, LEGACY_ONLINE_STORE_CLASS_FOR_TYPE

def save_in_script_directory(filename: str, typedict: dict[str, str]):
script_dir = os.path.dirname(os.path.abspath(__file__))
file_path = os.path.join(script_dir, filename)

with open(file_path, 'w') as file:
for k in typedict.keys():
file.write(k+"\n")

for legacyType in LEGACY_ONLINE_STORE_CLASS_FOR_TYPE.keys():
if legacyType in ONLINE_STORE_CLASS_FOR_TYPE:
del ONLINE_STORE_CLASS_FOR_TYPE[legacyType]

save_in_script_directory("registry.out", REGISTRY_CLASS_FOR_TYPE)
save_in_script_directory("online-store.out", ONLINE_STORE_CLASS_FOR_TYPE)
save_in_script_directory("offline-store.out", OFFLINE_STORE_CLASS_FOR_TYPE)
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package datasources

import (
"bufio"
"os"
"slices"
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

feastdevv1alpha1 "github.com/feast-dev/feast/infra/feast-operator/api/v1alpha1"
"github.com/feast-dev/feast/infra/feast-operator/internal/controller/services"
)

func TestDataSourceTypes(t *testing.T) {
RegisterFailHandler(Fail)

RunSpecs(t, "Data Source Suite")
}

var _ = Describe("FeatureStore Data Source Types", func() {
Context("When checking against the python code in feast.repo_config", func() {
It("should match defined registry persistence types in the operator", func() {
registryFilePersistenceTypes := []string{string(services.RegistryFileConfigType)}
registryPersistenceTypes := append(feastdevv1alpha1.ValidRegistryDBStorePersistenceTypes, registryFilePersistenceTypes...)
checkPythonPersistenceTypes("registry.out", registryPersistenceTypes)
})
It("should match defined onlineStore persistence types in the operator", func() {
onlineFilePersistenceTypes := []string{string(services.OnlineSqliteConfigType)}
onlinePersistenceTypes := append(feastdevv1alpha1.ValidOnlineStoreDBStorePersistenceTypes, onlineFilePersistenceTypes...)
checkPythonPersistenceTypes("online-store.out", onlinePersistenceTypes)
})
It("should match defined offlineStore persistence types in the operator", func() {
offlinePersistenceTypes := append(feastdevv1alpha1.ValidOfflineStoreDBStorePersistenceTypes, feastdevv1alpha1.ValidOfflineStoreFilePersistenceTypes...)
checkPythonPersistenceTypes("offline-store.out", offlinePersistenceTypes)
})
})
})

func checkPythonPersistenceTypes(fileName string, operatorDsTypes []string) {
feastDsTypes, err := readFileLines(fileName)
Expect(err).NotTo(HaveOccurred())

// Add remote type to slice, as its not a file or db type and we want to limit its use to registry service when deploying with the operator
operatorDsTypes = append(operatorDsTypes, "remote")
missingFeastTypes := []string{}
for _, ods := range operatorDsTypes {
if len(ods) > 0 {
if !slices.Contains(feastDsTypes, ods) {
missingFeastTypes = append(missingFeastTypes, ods)
}
}
}
Expect(missingFeastTypes).To(BeEmpty())

missingOperatorTypes := []string{}
for _, fds := range feastDsTypes {
if len(fds) > 0 {
if !slices.Contains(operatorDsTypes, fds) {
missingOperatorTypes = append(missingOperatorTypes, fds)
}
}
}
Expect(missingOperatorTypes).To(BeEmpty())
}

func readFileLines(filePath string) ([]string, error) {
file, err := os.Open(filePath)
Expect(err).NotTo(HaveOccurred())
defer closeFile(file)

var lines []string
scanner := bufio.NewScanner(file)
for scanner.Scan() {
lines = append(lines, scanner.Text())
}

err = scanner.Err()
Expect(err).NotTo(HaveOccurred())

return lines, nil
}

func closeFile(file *os.File) {
err := file.Close()
Expect(err).NotTo(HaveOccurred())
}

0 comments on commit d937dcb

Please sign in to comment.