From 3f49517dfeabea5ffbd3f6b589cc0f2280ee4018 Mon Sep 17 00:00:00 2001 From: Tommy Hughes IV Date: Mon, 23 Dec 2024 11:35:02 -0600 Subject: [PATCH] fix: Improve status.applied updates & add offline pvc unit test (#4871) improve status.applied update & add offline pvc unit test Signed-off-by: Tommy Hughes --- .../controller/services/repo_config.go | 4 +- .../controller/services/repo_config_test.go | 30 ++++++++- .../internal/controller/services/util.go | 64 +++++++++---------- 3 files changed, 60 insertions(+), 38 deletions(-) diff --git a/infra/feast-operator/internal/controller/services/repo_config.go b/infra/feast-operator/internal/controller/services/repo_config.go index 675fbd047f..0a5dd11544 100644 --- a/infra/feast-operator/internal/controller/services/repo_config.go +++ b/infra/feast-operator/internal/controller/services/repo_config.go @@ -85,17 +85,17 @@ func getBaseServiceRepoConfig( featureStore *feastdevv1alpha1.FeatureStore, secretExtractionFunc func(storeType string, secretRef string, secretKeyName string) (map[string]interface{}, error)) (RepoConfig, error) { - appliedSpec := featureStore.Status.Applied repoConfig := defaultRepoConfig(featureStore) clientRepoConfig, err := getClientRepoConfig(featureStore, secretExtractionFunc) if err != nil { return repoConfig, err } - repoConfig.AuthzConfig = clientRepoConfig.AuthzConfig if isRemoteRegistry(featureStore) { repoConfig.Registry = clientRepoConfig.Registry } + repoConfig.AuthzConfig = clientRepoConfig.AuthzConfig + appliedSpec := featureStore.Status.Applied if appliedSpec.AuthzConfig != nil && appliedSpec.AuthzConfig.OidcAuthz != nil { propertiesMap, authSecretErr := secretExtractionFunc("", appliedSpec.AuthzConfig.OidcAuthz.SecretRef.Name, "") if authSecretErr != nil { diff --git a/infra/feast-operator/internal/controller/services/repo_config_test.go b/infra/feast-operator/internal/controller/services/repo_config_test.go index 42525700a4..9138f00f2f 100644 --- a/infra/feast-operator/internal/controller/services/repo_config_test.go +++ b/infra/feast-operator/internal/controller/services/repo_config_test.go @@ -46,7 +46,6 @@ var _ = Describe("Repo Config", func() { Path: EphemeralPath + "/" + DefaultOnlineStorePath, } - var repoConfig RepoConfig repoConfig, err := getServiceRepoConfig(featureStore, emptyMockExtractConfigFromSecret) Expect(err).NotTo(HaveOccurred()) Expect(repoConfig.AuthzConfig.Type).To(Equal(NoAuthAuthType)) @@ -100,6 +99,35 @@ var _ = Describe("Repo Config", func() { Expect(repoConfig.OnlineStore).To(Equal(expectedOnlineConfig)) Expect(repoConfig.Registry).To(Equal(emptyRegistryConfig)) + By("Having an offlineStore with PVC") + mountPath := "/testing" + expectedOnlineConfig.Path = mountPath + "/" + DefaultOnlineStorePath + expectedRegistryConfig.Path = mountPath + "/" + DefaultRegistryPath + + featureStore = minimalFeatureStore() + featureStore.Spec.Services = &feastdevv1alpha1.FeatureStoreServices{ + OfflineStore: &feastdevv1alpha1.OfflineStore{ + Persistence: &feastdevv1alpha1.OfflineStorePersistence{ + FilePersistence: &feastdevv1alpha1.OfflineStoreFilePersistence{ + PvcConfig: &feastdevv1alpha1.PvcConfig{ + MountPath: mountPath, + }, + }, + }, + }, + } + ApplyDefaultsToStatus(featureStore) + appliedServices := featureStore.Status.Applied.Services + Expect(appliedServices.OnlineStore).To(BeNil()) + Expect(appliedServices.Registry.Local.Persistence.FilePersistence.Path).To(Equal(expectedRegistryConfig.Path)) + + repoConfig, err = getServiceRepoConfig(featureStore, emptyMockExtractConfigFromSecret) + Expect(err).NotTo(HaveOccurred()) + Expect(repoConfig.OfflineStore).To(Equal(defaultOfflineStoreConfig)) + Expect(repoConfig.AuthzConfig.Type).To(Equal(NoAuthAuthType)) + Expect(repoConfig.Registry).To(Equal(expectedRegistryConfig)) + Expect(repoConfig.OnlineStore).To(Equal(expectedOnlineConfig)) + By("Having the all the file services") featureStore = minimalFeatureStore() featureStore.Spec.Services = &feastdevv1alpha1.FeatureStoreServices{ diff --git a/infra/feast-operator/internal/controller/services/util.go b/infra/feast-operator/internal/controller/services/util.go index d0ca94ff86..7b9c177c89 100644 --- a/infra/feast-operator/internal/controller/services/util.go +++ b/infra/feast-operator/internal/controller/services/util.go @@ -37,15 +37,18 @@ func hasPvcConfig(featureStore *feastdevv1alpha1.FeatureStore, feastType FeastSe if services != nil { switch feastType { case OnlineFeastType: - if services.OnlineStore != nil && services.OnlineStore.Persistence.FilePersistence != nil { + if services.OnlineStore != nil && services.OnlineStore.Persistence != nil && + services.OnlineStore.Persistence.FilePersistence != nil { pvcConfig = services.OnlineStore.Persistence.FilePersistence.PvcConfig } case OfflineFeastType: - if services.OfflineStore != nil && services.OfflineStore.Persistence.FilePersistence != nil { + if services.OfflineStore != nil && services.OfflineStore.Persistence != nil && + services.OfflineStore.Persistence.FilePersistence != nil { pvcConfig = services.OfflineStore.Persistence.FilePersistence.PvcConfig } case RegistryFeastType: - if IsLocalRegistry(featureStore) && services.Registry.Local.Persistence.FilePersistence != nil { + if IsLocalRegistry(featureStore) && services.Registry.Local.Persistence != nil && + services.Registry.Local.Persistence.FilePersistence != nil { pvcConfig = services.Registry.Local.Persistence.FilePersistence.PvcConfig } } @@ -66,18 +69,18 @@ func shouldMountEmptyDir(featureStore *feastdevv1alpha1.FeatureStore) bool { } func getOfflineMountPath(featureStore *feastdevv1alpha1.FeatureStore) string { - if featureStore.Status.Applied.Services != nil { - if pvcConfig, ok := hasPvcConfig(featureStore, OfflineFeastType); ok { - return pvcConfig.MountPath - } + if pvcConfig, ok := hasPvcConfig(featureStore, OfflineFeastType); ok { + return pvcConfig.MountPath } return EphemeralPath } func ApplyDefaultsToStatus(cr *feastdevv1alpha1.FeatureStore) { + // overwrite status.applied with every reconcile + cr.Spec.DeepCopyInto(&cr.Status.Applied) cr.Status.FeastVersion = feastversion.FeastVersion - applied := cr.Spec.DeepCopy() + applied := &cr.Status.Applied if applied.Services == nil { applied.Services = &feastdevv1alpha1.FeatureStoreServices{} } @@ -106,10 +109,7 @@ func ApplyDefaultsToStatus(cr *feastdevv1alpha1.FeatureStore) { services.Registry.Local.Persistence.FilePersistence.Path = defaultRegistryPath(cr) } - if services.Registry.Local.Persistence.FilePersistence.PvcConfig != nil { - pvc := services.Registry.Local.Persistence.FilePersistence.PvcConfig - ensurePVCDefaults(pvc, RegistryFeastType) - } + ensurePVCDefaults(services.Registry.Local.Persistence.FilePersistence.PvcConfig, RegistryFeastType) } setServiceDefaultConfigs(&services.Registry.Local.ServiceConfigs.DefaultConfigs) @@ -131,10 +131,7 @@ func ApplyDefaultsToStatus(cr *feastdevv1alpha1.FeatureStore) { services.OfflineStore.Persistence.FilePersistence.Type = string(OfflineFilePersistenceDaskConfigType) } - if services.OfflineStore.Persistence.FilePersistence.PvcConfig != nil { - pvc := services.OfflineStore.Persistence.FilePersistence.PvcConfig - ensurePVCDefaults(pvc, OfflineFeastType) - } + ensurePVCDefaults(services.OfflineStore.Persistence.FilePersistence.PvcConfig, OfflineFeastType) } setServiceDefaultConfigs(&services.OfflineStore.ServiceConfigs.DefaultConfigs) @@ -154,16 +151,11 @@ func ApplyDefaultsToStatus(cr *feastdevv1alpha1.FeatureStore) { services.OnlineStore.Persistence.FilePersistence.Path = defaultOnlineStorePath(cr) } - if services.OnlineStore.Persistence.FilePersistence.PvcConfig != nil { - pvc := services.OnlineStore.Persistence.FilePersistence.PvcConfig - ensurePVCDefaults(pvc, OnlineFeastType) - } + ensurePVCDefaults(services.OnlineStore.Persistence.FilePersistence.PvcConfig, OnlineFeastType) } setServiceDefaultConfigs(&services.OnlineStore.ServiceConfigs.DefaultConfigs) } - // overwrite status.applied with every reconcile - applied.DeepCopyInto(&cr.Status.Applied) } func setServiceDefaultConfigs(defaultConfigs *feastdevv1alpha1.DefaultConfigs) { @@ -189,19 +181,21 @@ func ensureRequestedStorage(resources *v1.VolumeResourceRequirements, requestedS } func ensurePVCDefaults(pvc *feastdevv1alpha1.PvcConfig, feastType FeastServiceType) { - var storageRequest string - switch feastType { - case OnlineFeastType: - storageRequest = DefaultOnlineStorageRequest - case OfflineFeastType: - storageRequest = DefaultOfflineStorageRequest - case RegistryFeastType: - storageRequest = DefaultRegistryStorageRequest - } - if pvc.Create != nil { - ensureRequestedStorage(&pvc.Create.Resources, storageRequest) - if pvc.Create.AccessModes == nil { - pvc.Create.AccessModes = DefaultPVCAccessModes + if pvc != nil { + var storageRequest string + switch feastType { + case OnlineFeastType: + storageRequest = DefaultOnlineStorageRequest + case OfflineFeastType: + storageRequest = DefaultOfflineStorageRequest + case RegistryFeastType: + storageRequest = DefaultRegistryStorageRequest + } + if pvc.Create != nil { + ensureRequestedStorage(&pvc.Create.Resources, storageRequest) + if pvc.Create.AccessModes == nil { + pvc.Create.AccessModes = DefaultPVCAccessModes + } } } }