fix(operator): Only deploy an online store when it is declared#6591
fix(operator): Only deploy an online store when it is declared#6591nikolauspschuetz wants to merge 1 commit into
Conversation
|
The red `unit-test-python (3.11, macos-14)` leg is an unrelated CI flake, not caused by this change (which is Go-only, scoped to `infra/feast-operator/`): ``` A `feast apply` subprocess timed out after 120s on the macOS-14 runner. The macOS-3.12 leg and all Ubuntu legs (3.10/3.11/3.12) passed on this same commit, and `operator-test` is green. I do not have permission to re-run the job from the fork — a maintainer re-run of that single leg should clear it. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Feast Kubernetes operator’s defaulting logic so an online store service is only deployed when spec.services.onlineStore is explicitly declared, matching the existing behavior for registry and offlineStore. This enables FeatureStore deployments that intentionally omit the online store (e.g., registry-only / offline-only patterns).
Changes:
- Gate online-store defaulting in
ApplyDefaultsToStatusbehindservices.OnlineStore != nilto avoid implicitly creating/deploying an online store. - Add unit tests covering both “onlineStore omitted” and “onlineStore declared” behaviors.
- Update existing controller/service tests to explicitly declare
onlineStorewhere those tests still require an online store to exist.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| infra/feast-operator/internal/controller/services/util.go | Stop implicitly creating online-store defaults unless the online store is declared. |
| infra/feast-operator/internal/controller/services/util_test.go | Add regression/unit tests for online-store defaulting behavior. |
| infra/feast-operator/internal/controller/services/tls_test.go | Explicitly declare onlineStore so TLS defaulting assertions remain valid. |
| infra/feast-operator/internal/controller/services/repo_config_test.go | Adjust expectations/helpers to reflect that online store is no longer implicit. |
| infra/feast-operator/internal/controller/featurestore_controller_test.go | Ensure test FeatureStores declare onlineStore where required for validity. |
| infra/feast-operator/internal/controller/featurestore_controller_loglevel_test.go | Explicitly include onlineStore in the test services spec. |
| infra/feast-operator/internal/controller/featurestore_controller_cronjob_test.go | Ensure test FeatureStores declare onlineStore where required for validity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Only apply online store defaults when the online store is declared, so a | ||
| // FeatureStore can omit spec.services.onlineStore (mirroring registry and | ||
| // offlineStore) and not have an online store deployed. (#6586) | ||
| if services.OnlineStore != nil { | ||
| if services.OnlineStore.Persistence == nil { |
|
@nikolauspschuetz instead I think fix should be to allow online server deployment to opt-out. |
|
Thanks @ntkathole — that makes sense for backward compatibility. Omitting To make it opt-out instead, I'd keep the current default-on behavior and add an explicit disable switch on the online store spec, mirroring the existing type OnlineStore struct {
// Disabled skips deploying the online store service.
// +optional
Disabled bool `json:"disabled,omitempty"`
Server *ServerConfigs `json:"server,omitempty"`
Persistence *OnlineStorePersistence `json:"persistence,omitempty"`
// ...
}
One clarification so I build the right thing: do you want the opt-out at the whole online-store level (skip both persistence and the serving pod), or only the online server (serving pod), keeping the online store config? Happy to rework the PR either way once you confirm. |
opt-out at the whole onlineStore level (disabled: true), not server-only. |
4a735a8 to
1008ca2
Compare
|
Thanks — reworked it to opt-out. The default is back to deploying the online store, so nothing changes on upgrade for existing setups. Setting |
…he online store The operator always deploys an online store, forcing a serving pod and persistence onto registry-only or offline-only FeatureStores (feast-dev#6586). Add a `disabled` field on the online store spec to opt those deployments out explicitly. Deployment is gated through isOnlineStore(), so disabling also skips the serving pod, metrics, and service monitor. The default is unchanged -- omitting the online store block still deploys it with defaults -- so this is not a breaking change on upgrade. Signed-off-by: Nikolaus Schuetz <nikolauspschuetz@gmail.com>
1008ca2 to
6a27e6b
Compare
Closes #6586.
Problem
The Feast operator always deployed an online store service (including a serving pod), even when the
FeatureStoreCR did not declarespec.services.onlineStore. This makes it impossible to run a standalone registry or offline store (e.g. the documented federated / shared-registry pattern), and is inconsistent with howregistryandofflineStorebehave — both are only defaulted when declared.Cause
In
ApplyDefaultsToStatus(infra/feast-operator/internal/controller/services/util.go), the online-store block force-created the service when nil:whereas
registryandofflineStoreare gated behindif services.X != nil.Fix
Gate the online-store defaulting behind
services.OnlineStore != nil, mirroring the two sibling services. A declared online store still receives all of its defaults (persistence, path, PVC, server, container configs).Tests
Added
util_test.gowith two specs forApplyDefaultsToStatus:Status.Applied.Services.OnlineStorestaysnil(fails onmaster, passes with this change),Verified with
KUBEBUILDER_ASSETS=... go test ./internal/controller/services/(envtest 1.31.0);go build ./...,go vet, andgofmtare clean. The existing controller tests are unaffected because they declareonlineStoreexplicitly.