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.

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

feat: Added feast Go operator db stores support #4771

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tmihalac
Copy link
Contributor

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: 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)
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:
  cassandra: |
    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
    vector_enabled: false
    vector_len: 512
type: Opaque

Which issue(s) this PR fixes:

Relates to #4561

@tmihalac tmihalac requested a review from a team as a code owner November 19, 2024 21:12
Comment on lines +166 to +185

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
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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
}
}
}

Comment on lines +104 to +111
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

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

Choose a reason for hiding this comment

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

same

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

Choose a reason for hiding this comment

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

same

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants