Repository navigation
fix: Allow scaling a FeatureStore whose online store is disabled - #7070
Open
NopeCharon wants to merge 2 commits into
Open
NopeCharon wants to merge 2 commits into
NopeCharon wants to merge 2 commits into
Conversation
The CEL rule that guards horizontal scaling required services.onlineStore.persistence.store unconditionally. That made it impossible to set spec.replicas > 1 or services.scaling.autoscaling on a FeatureStore that sets services.onlineStore.disabled, such as a registry-only or offline-only FeatureStore, even though no online store pod or persistence is deployed for it. The scaling docs already say the DB-backed requirement applies to enabled services only. Let the online store rule also pass when the online store is explicitly disabled. An omitted onlineStore block is still rejected, because the operator deploys the online store with file-based defaults in that case. The offline store and registry rules are unchanged, so a disabled online store still needs a DB-backed, object-store or remote registry to scale. Regenerate the CRD and dist/install.yaml, add envtest cases for static replicas and autoscaling with a disabled online store, and document the behaviour in the scaling guide. Fixes feast-dev#6865 Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7070 +/- ##
==========================================
+ Coverage 50.97% 50.99% +0.01%
==========================================
Files 455 455
Lines 56994 56994
Branches 8321 8321
==========================================
+ Hits 29055 29063 +8
+ Misses 25954 25946 -8
Partials 1985 1985
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Regenerate bundle/manifests/feast.dev_featurestores.yaml so it matches config/crd/bases after the scaling rule change, fixing the bundle sync check in operator-test. Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
The CEL rule that guards horizontal scaling (
spec.replicas > 1orservices.scaling.autoscaling) always requiredservices.onlineStore.persistence.store. A FeatureStore that setsservices.onlineStore.disabled: truedeploys no online store pod or persistence, but it still failed admission with "Scaling requires DB-backed persistence for the online store". So a registry-only, offline-only or UI-only FeatureStore could not be scaled or used as an HPA target. The scaling guide already says the DB-backed requirement covers only enabled services.This PR lets the online store rule pass when the online store is explicitly disabled:
The issue also suggests allowing an omitted
onlineStoreblock and an omittedregistry. I left those out on purpose. WhenonlineStoreis omitted,ApplyDefaultsToStatusstill deploys the online store with file-based (SQLite) defaults. When there is no registry, each pod falls back to its own file registry. Both setups break with several replicas, and the existing testsshould reject scaling when online store is missing (implicit file default)andshould reject scaling when no registry is configured (implicit file default)cover that intent. The offline store and registry rules are unchanged, so a FeatureStore with a disabled online store still needs a DB-backed, S3/GCS or remote registry to scale.Changes:
api/v1/featurestore_types.go: online store scaling rule and message.config/crd/bases/feast.dev_featurestores.yaml(make manifests) anddist/install.yaml(make build-installer). The rule appears twice in each file, underspecandstatus.applied.scaling_test.go: envtest cases that accept static replicas and autoscaling with a disabled online store, plus one that still rejects a disabled online store without a registry.docs/how-to-guides/scaling-feast.md: explains that an omitted online store counts as enabled, and how to scale a FeatureStore that runs without one.Which issue(s) this PR fixes:
Fixes #6865
Checks
git commit -s)Testing Strategy
Misc
I ran the operator test packages against envtest (Kubernetes 1.31.0):
cmd,internal/controller,internal/controller/servicesandtest/apiall pass. With the rule change reverted, the two new "accept" cases fail with the online store CEL error.