Skip to content

feat: Operator - make server container creation explicit in the CR#5024

Merged
tchughesiv merged 1 commit intofeast-dev:masterfrom
tchughesiv:serverOpt
Feb 7, 2025
Merged

feat: Operator - make server container creation explicit in the CR#5024
tchughesiv merged 1 commit intofeast-dev:masterfrom
tchughesiv:serverOpt

Conversation

@tchughesiv
Copy link
Copy Markdown
Contributor

@tchughesiv tchughesiv commented Feb 6, 2025 •
edited
Loading

What this PR does / why we need it:

Currently, the operator creates a running server/container for each feast service by default. This is unnecessary and should only be done on an as-needed basis. With this change, the user will have to explicitly configure a new server subsection for a given service type (online/offline/etc). Otherwise, the operator will not run a separate container w/ remote server for that service. By default, the registry container will continue created.

Which issue(s) this PR fixes:

Fixes #5023

Misc

In the following example the online store, but NOT the offline, would have a separate container running a remote online feature server. The offline persistence configs, however, would still be reflected in the shared feature_store.yaml.

apiVersion: feast.dev/v1alpha1
kind: FeatureStore
metadata:
  name: example
spec:
  feastProject: my_project
  services:
    offlineStore:
      persistence:
        file:
          type: duckdb
    onlineStore:
      server: {}

an example of some of the optional fields within this server object -

      server:
        image: 'feastdev/feature-server:0.45.0'
        logLevel: debug
        tls:
          secretKeyNames:
            tlsCrt: tls.crt
            tlsKey: tls.key
          secretRef:
            name: feast-sample-all-servers-online-tls
        envFrom:
          - secretRef:
              name: postgres-secret
        resources:
          requests:
            cpu: 150m
            memory: 128Mi

@tchughesiv tchughesiv requested a review from a team as a code owner February 6, 2025 21:54
@tchughesiv tchughesiv marked this pull request as draft February 6, 2025 21:54
@tchughesiv tchughesiv changed the title feat: Operator- Make server container creation explicit in the CR feat: Operator - make server container creation explicit in the CR Feb 6, 2025
@tchughesiv
add server option
eb5c7e8 
Signed-off-by: Tommy Hughes <[email protected]>
@tchughesiv tchughesiv marked this pull request as ready for review February 6, 2025 22:43
@redhatHameed
Copy link
Copy Markdown
Contributor

redhatHameed commented Feb 7, 2025 •
edited
Loading

@tchughesiv WDYT
if we can just flag remote it will be be false for offline server, for online always true

example

apiVersion: feast.dev/v1alpha1
kind: FeatureStore
metadata:
  name: sample-all-services
spec:
  feastProject: my_project
  services:
    onlineStore:
      remote: true
    offlineStore:
      remote: false
    registry:
      remote:?

if this is just for offline server setting we just go with this settings

apiVersion: feast.dev/v1alpha1
kind: FeatureStore
metadata:
  name: sample-all-services
spec:
  feastProject: my_project
  services:
    onlineStore {}
    offlineStore:
      remote: false
    registry:
      local: {}

tmihalac
tmihalac reviewed Feb 7, 2025
View reviewed changes
infra/feast-operator/internal/controller/featurestore_controller_loglevel_test.go
})
})

func strPtr(str string) *string {
Copy link
Copy Markdown
Contributor

@tmihalac tmihalac Feb 7, 2025

Choose a reason for hiding this comment

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

Are you sure a one line function like this is needed?

Copy link
Copy Markdown
Contributor Author

@tchughesiv tchughesiv Feb 7, 2025

Choose a reason for hiding this comment

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

unfortunately yes... the alternative would be separate vars for each.

@redhatHameed
Copy link
Copy Markdown
Contributor

redhatHameed commented Feb 7, 2025

@tchughesiv WDYT if we can just flag remote it will be be false for offline server, for online always true

example

apiVersion: feast.dev/v1alpha1
kind: FeatureStore
metadata:
  name: sample-all-services
spec:
  feastProject: my_project
  services:
    onlineStore:
      remote: true
    offlineStore:
      remote: false
    registry:
      remote:?

if this is just for offline server setting we just go with this settings

apiVersion: feast.dev/v1alpha1
kind: FeatureStore
metadata:
  name: sample-all-services
spec:
  feastProject: my_project
  services:
    onlineStore {}
    offlineStore:
      remote: false
    registry:
      local: {}

Thanks, @tchughesiv for explaining on this on meet/call, I am good to keep current approach using sever section

redhatHameed
redhatHameed approved these changes Feb 7, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@redhatHameed redhatHameed left a comment

Choose a reason for hiding this comment

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

lgtm

@tchughesiv tchughesiv merged commit b16fb40 into feast-dev:master Feb 7, 2025
franciscojavierarceo pushed a commit that referenced this pull request Feb 17, 2025
@feast-ci-bot
chore(release): release 0.46.0
6372f1c 
# [0.46.0](v0.45.0...v0.46.0) (2025-02-17)

### Bug Fixes

* Add scylladb to online stores list in docs ([#5061](#5061)) ([08183ed](08183ed))
* Changed feast operator to set status of featurestore cr to ready based on deployment.status = available ([#5020](#5020)) ([fce0d35](fce0d35))
* Ensure Postgres queries are committed or autocommit is used ([#5039](#5039)) ([46f8d7a](46f8d7a))
* Fixing the release workflow to refresh the stable branch when the release is not running in the dry run mode. ([#5057](#5057)) ([a13fa9b](a13fa9b))
* Operator - make onlineStore the default service ([#5044](#5044)) ([6c92447](6c92447))
* Operator - resolve infinite reconciler loop in authz controller ([#5056](#5056)) ([11e4548](11e4548))
* Resolve module on windows ([#4827](#4827)) ([efbffa4](efbffa4))
* Setting the github_token explicitly to see if that solves the problem. ([#5012](#5012)) ([3834ffa](3834ffa))
* Validate entities when running get_online_features ([#5031](#5031)) ([3bb0dca](3bb0dca))

### Features

* Add SQLite retrieve_online_documents_v2 ([#5032](#5032)) ([0fffe21](0fffe21))
* Adding Click command to display configuration details ([#5036](#5036)) ([ae68e4d](ae68e4d))
* Adding volumes and volumeMounts support to Feature Store CR. ([#4983](#4983)) ([ec6f1b7](ec6f1b7))
* Moving the job to seperate action so that we can test it easily. ([#5013](#5013)) ([b9325b7](b9325b7))
* Operator - make server container creation explicit in the CR ([#5024](#5024)) ([b16fb40](b16fb40))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@redhatHameed redhatHameed redhatHameed approved these changes

+1 more reviewer

@tmihalac tmihalac tmihalac left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Operator should only create online/offline remote server containers when necessary

3 participants

@tchughesiv @redhatHameed @tmihalac