From d937dcb7fc1ea081f9b682f37d99310691094e38 Mon Sep 17 00:00:00 2001 From: Tommy Hughes IV Date: Mon, 10 Feb 2025 16:27:32 -0600 Subject: [PATCH] chore: Operator / Feast should have matching Data Source types (#5041) data store types Signed-off-by: Tommy Hughes --- .github/workflows/operator_pr.yml | 4 +- .../workflows/pr_local_integration_tests.yml | 6 ++ infra/feast-operator/.golangci.yml | 3 + infra/feast-operator/Makefile | 8 +- .../api/v1alpha1/featurestore_types.go | 11 ++- .../crd/bases/feast.dev_featurestores.yaml | 2 - infra/feast-operator/dist/install.yaml | 2 - infra/feast-operator/docs/api/markdown/ref.md | 4 +- .../test/api/featurestore_types_test.go | 18 +++- .../data-source-types/data-source-types.py | 18 ++++ .../data_source_types_test.go | 88 +++++++++++++++++++ 11 files changed, 145 insertions(+), 19 deletions(-) create mode 100644 infra/feast-operator/test/data-source-types/data-source-types.py create mode 100644 infra/feast-operator/test/data-source-types/data_source_types_test.go diff --git a/.github/workflows/operator_pr.yml b/.github/workflows/operator_pr.yml index 2feed8dbf3..aefdcbdcbb 100644 --- a/.github/workflows/operator_pr.yml +++ b/.github/workflows/operator_pr.yml @@ -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 diff --git a/.github/workflows/pr_local_integration_tests.yml b/.github/workflows/pr_local_integration_tests.yml index d3488cd08c..7fa900b94e 100644 --- a/.github/workflows/pr_local_integration_tests.yml +++ b/.github/workflows/pr_local_integration_tests.yml @@ -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 diff --git a/infra/feast-operator/.golangci.yml b/infra/feast-operator/.golangci.yml index bf54dc4602..020e576865 100644 --- a/infra/feast-operator/.golangci.yml +++ b/infra/feast-operator/.golangci.yml @@ -16,6 +16,9 @@ issues: linters: - dupl - lll + - path: "test/*" + linters: + - lll linters: disable-all: true enable: diff --git a/infra/feast-operator/Makefile b/infra/feast-operator/Makefile index 445b98fd1a..9dc38a2b04 100644 --- a/infra/feast-operator/Makefile +++ b/infra/feast-operator/Makefile @@ -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 diff --git a/infra/feast-operator/api/v1alpha1/featurestore_types.go b/infra/feast-operator/api/v1alpha1/featurestore_types.go index c73806df3b..9c25d2c1d4 100644 --- a/infra/feast-operator/api/v1alpha1/featurestore_types.go +++ b/infra/feast-operator/api/v1alpha1/featurestore_types.go @@ -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"` @@ -130,7 +130,6 @@ var ValidOfflineStoreDBStorePersistenceTypes = []string{ "spark", "postgres", "trino", - "redis", "athena", "mssql", } @@ -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. @@ -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. @@ -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. diff --git a/infra/feast-operator/config/crd/bases/feast.dev_featurestores.yaml b/infra/feast-operator/config/crd/bases/feast.dev_featurestores.yaml index 7d9385df43..dbe08d4cd5 100644 --- a/infra/feast-operator/config/crd/bases/feast.dev_featurestores.yaml +++ b/infra/feast-operator/config/crd/bases/feast.dev_featurestores.yaml @@ -170,7 +170,6 @@ spec: - spark - postgres - trino - - redis - athena - mssql type: string @@ -2083,7 +2082,6 @@ spec: - spark - postgres - trino - - redis - athena - mssql type: string diff --git a/infra/feast-operator/dist/install.yaml b/infra/feast-operator/dist/install.yaml index a57848769c..7010ae1cae 100644 --- a/infra/feast-operator/dist/install.yaml +++ b/infra/feast-operator/dist/install.yaml @@ -178,7 +178,6 @@ spec: - spark - postgres - trino - - redis - athena - mssql type: string @@ -2091,7 +2090,6 @@ spec: - spark - postgres - trino - - redis - athena - mssql type: string diff --git a/infra/feast-operator/docs/api/markdown/ref.md b/infra/feast-operator/docs/api/markdown/ref.md index 820d9b9053..a1eaa753d2 100644 --- a/infra/feast-operator/docs/api/markdown/ref.md +++ b/infra/feast-operator/docs/api/markdown/ref.md @@ -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 | @@ -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 | diff --git a/infra/feast-operator/test/api/featurestore_types_test.go b/infra/feast-operator/test/api/featurestore_types_test.go index 1049436fec..12a7406e80 100644 --- a/infra/feast-operator/test/api/featurestore_types_test.go +++ b/infra/feast-operator/test/api/featurestore_types_test.go @@ -2,6 +2,8 @@ package api import ( "context" + "fmt" + "strings" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -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" @@ -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)) }) }) @@ -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)) }) }) @@ -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)) }) }) diff --git a/infra/feast-operator/test/data-source-types/data-source-types.py b/infra/feast-operator/test/data-source-types/data-source-types.py new file mode 100644 index 0000000000..be7d70e5ed --- /dev/null +++ b/infra/feast-operator/test/data-source-types/data-source-types.py @@ -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) diff --git a/infra/feast-operator/test/data-source-types/data_source_types_test.go b/infra/feast-operator/test/data-source-types/data_source_types_test.go new file mode 100644 index 0000000000..8448b2c421 --- /dev/null +++ b/infra/feast-operator/test/data-source-types/data_source_types_test.go @@ -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()) +}