Repository navigation
chore: Manually Sync upstream master to midstream as auto-sync failed - #166
Merged
Merged
Conversation
Signed-off-by: ntkathole <[email protected]> Co-authored-by: Cursor <[email protected]>
Signed-off-by: ntkathole <[email protected]>
…adlock (feast-dev#6648) feast_metrics.start_metrics_server() starts a feature-freshness thread in the Gunicorn master process, before Gunicorn forks its worker(s). That thread's first action, with no delay, is update_feature_freshness() -> store.list_feature_views(), which lazily builds the registry for the first time. For registry backends needing a lazy DBAPI import (e.g. the SQL registry importing pymysql via SQLAlchemy's create_engine()), this means a thread in the master can be mid-import, holding CPython's per-module import lock, at the exact moment Gunicorn forks a worker. POSIX fork() only duplicates the calling thread into the child process; every other thread in the parent, including this one, simply ceases to exist in the worker. If the fork lands while that thread holds a module's import lock, the lock stays permanently held in the new worker, since there is no longer any thread that can finish the import and release it. The worker's own later attempt to build its registry then deadlocks forever with no error - the process just hangs at "Waiting for application startup." This is intermittent by nature: it only manifests if the fork lands inside that narrow timing window. Resource monitoring already avoids this correctly (start_resource_monitoring= not uses_gunicorn plus the post_worker_init hook calling init_worker_monitoring()), but the freshness thread was not given the same treatment. This applies the identical pattern: start_metrics_server gains a start_freshness_monitoring flag (deferred exactly like resource monitoring's), and FeastServeApplication's post_worker_init hook now also calls the new init_worker_freshness_monitoring(store) after the fork, instead of feast_metrics.py starting it unconditionally beforehand. Fixes feast-dev#6647 Signed-off-by: Carlos Sánchez <[email protected]> Co-authored-by: Carlos Sánchez <[email protected]>
📝 WalkthroughWalkthroughThe change adds protected-project tagging across the operator, registry servers, and FeatureStore lifecycle operations. It adds per-worker freshness monitoring for Gunicorn deployments and corresponding unit tests. Docker build workflows and the Makefile now support multi-platform Feast Operator image publishing through buildx and imagetools. The secrets baseline timestamp is refreshed. Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
sdk/python/tests/unit/test_feature_server.py (1)
755-766: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the configured Gunicorn callback, not only the helper.
This test bypasses
FeastServeApplication.load_config, so it would still pass if the newfunctools.partial(..., self._store)wiring were missing or bound to the wrong store. Capture thepost_worker_initcallback installed byload_config, invoke it, and assert the exactFeatureStoreis forwarded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/python/tests/unit/test_feature_server.py` around lines 755 - 766, Update test_gunicorn_post_worker_init_starts_resource_and_freshness_monitoring to exercise FeastServeApplication.load_config’s configured post_worker_init callback instead of calling _gunicorn_post_worker_init directly. Capture the callback installed by load_config, invoke it with a worker, and assert init_worker_freshness_monitoring receives the exact FeatureStore instance while retaining the monitoring assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 733-736: Update the Feast Operator publish command in the
DOCKER_PLATFORMS block to run only when explicit Docker push approval is
enabled, and validate $(REGISTRY) against the project’s approved registry
allowlist before invoking docker buildx build --push. Preserve the existing
platform and image-tag behavior, but fail or skip publishing when approval is
absent or the registry is not allowlisted.
- Around line 733-742: Update the Makefile target’s shell commands to quote all
caller-controlled variables: quote the DOCKER_PLATFORMS and REGISTRY/VERSION
values used in the docker buildx arguments, and quote the REGISTRY and VERSION
values in the fallback IMAGE_TAG_BASE and VERSION environment assignments.
Preserve the existing build and fallback behavior.
In `@sdk/python/feast/feature_store.py`:
- Around line 1889-1900: Update the protected-project lookup guards in
sdk/python/feast/feature_store.py at lines 1889-1900 and 4797-4807 to catch only
the known project-not-found exception; allow all other registry errors to
propagate before teardown or deletion. Preserve the existing protected-project
rejection behavior and apply the same fail-closed handling at both sites.
In `@sdk/python/feast/registry_server.py`:
- Around line 1503-1509: Apply the same PROTECTED_PROJECT_TAG exclusion used by
ListProjects within RegistryServer.Proto before it serializes RBAC-visible
projects and their objects. Filter the permitted project collection before the
Proto serialization loop so projects tagged "true" cannot be returned through
that RPC.
- Around line 1748-1780: Update the protection logic around should_protect and
the existing-project lookup: treat an unset FEAST_PROTECTED_PROJECT_ENV as a
no-op, only remove PROTECTED_PROJECT_TAG when the environment explicitly
specifies false, and catch the registry-specific project-not-found exception
rather than all exceptions before creating a project. Preserve protection
tagging for explicit true and avoid mutating the registry on registry/auth
failures.
In `@sdk/python/tests/unit/test_metrics.py`:
- Around line 486-499: Update
test_freshness_monitoring_deferred_same_as_resource_monitoring to assert
kwargs["start_resource_monitoring"] and kwargs["start_freshness_monitoring"]
individually against whether sys.platform equals "win32", rather than asserting
only their equality. Preserve the existing mock setup and call flow.
- Around line 347-391: Restore feast.metrics._config after each test in
TestInitWorkerFreshnessMonitoring by using pytest monkeypatch.setattr or an
equivalent fixture that automatically reverts the module-global value during
teardown. Update both tests’ direct _config assignments while preserving their
existing flag values and assertions.
---
Nitpick comments:
In `@sdk/python/tests/unit/test_feature_server.py`:
- Around line 755-766: Update
test_gunicorn_post_worker_init_starts_resource_and_freshness_monitoring to
exercise FeastServeApplication.load_config’s configured post_worker_init
callback instead of calling _gunicorn_post_worker_init directly. Capture the
callback installed by load_config, invoke it with a worker, and assert
init_worker_freshness_monitoring receives the exact FeatureStore instance while
retaining the monitoring assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 29584724-ba0b-4952-8252-87b02eaeabe4
📒 Files selected for processing (15)
.github/workflows/master_only.yml.github/workflows/publish_images.yml.secrets.baselineMakefileinfra/feast-operator/internal/controller/services/namespace_registry.goinfra/feast-operator/internal/controller/services/services.goinfra/feast-operator/internal/controller/services/services_types.gosdk/python/feast/api/registry/rest/rest_registry_server.pysdk/python/feast/constants.pysdk/python/feast/feature_server.pysdk/python/feast/feature_store.pysdk/python/feast/metrics.pysdk/python/feast/registry_server.pysdk/python/tests/unit/test_feature_server.pysdk/python/tests/unit/test_metrics.py
Comment on lines
+733
to
+736
| @if [ -n "$(DOCKER_PLATFORMS)" ]; then \ | ||
| cd infra/feast-operator && \ | ||
| docker buildx build --push --platform=$(DOCKER_PLATFORMS) \ | ||
| --tag $(REGISTRY)/feast-operator:$(VERSION) -f Dockerfile .; \ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Makefile context around feast-operator build =="
sed -n '700,755p' Makefile | cat -n
echo
echo "== Search for DOCKER_PLATFORMS and DOCKER_PUSH usages in Makefile =="
rg -n 'DOCKER_PLATFORMS|DOCKER_PUSH|REGISTRY|feast-operator|docker buildx build --push' Makefile .github 2>/dev/null || true
echo
echo "== Find registry approval references =="
rg -n 'approved registry|approval|REGISTRY|DOCKER_PUSH|DOCKER_PLATFORMS' Makefile README.md .github Makefile* 2>/dev/null || trueRepository: opendatahub-io/feast
Length of output: 16352
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Makefile Make variables =="
sed -n '1,100p' Makefile | cat -n
echo
echo "== Infra feast-operator docker targets =="
sed -n '1,220p' infra/feast-operator/Makefile | cat -n || (test -f infra/feast-operator/Makefile && echo "infra/feast-operator/Makefile not found")
echo
echo "== Behavioral probe for Makefile condition in Make syntax approximation =="
python3 - <<'PY'
from datetime import datetime
now = datetime.utcnow()
print(now.isoformat() + " probe not needed for this review; Make string truthiness is context-dependent, but file evidence already shows lines 733-738 unconditional publish branch when $(DOCKER_PLATFORMS) non-empty.")
PYRepository: opendatahub-io/feast
Length of output: 17024
Gate Feast Operator image publish on explicit approval and an approved registry (CWE-15).
Makefile:733-738 runs docker buildx build --push ... $(REGISTRY)/feast-operator:$(VERSION) whenever DOCKER_PLATFORMS is non-empty; DOCKER_PUSH is not consulted here. Add an explicit approval/push gate and reject registries outside the approved allowlist before publishing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 733 - 736, Update the Feast Operator publish command
in the DOCKER_PLATFORMS block to run only when explicit Docker push approval is
enabled, and validate $(REGISTRY) against the project’s approved registry
allowlist before invoking docker buildx build --push. Preserve the existing
platform and image-tag behavior, but fail or skip publishing when approval is
absent or the registry is not allowlisted.
Source: Path instructions
Comment on lines
+733
to
+742
| @if [ -n "$(DOCKER_PLATFORMS)" ]; then \ | ||
| cd infra/feast-operator && \ | ||
| docker buildx build --push --platform=$(DOCKER_PLATFORMS) \ | ||
| --tag $(REGISTRY)/feast-operator:$(VERSION) -f Dockerfile .; \ | ||
| else \ | ||
| cd infra/feast-operator && \ | ||
| IMAGE_TAG_BASE=$(REGISTRY)/feast-operator \ | ||
| VERSION=$(VERSION) \ | ||
| $(MAKE) docker-build; \ | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Quote all Makefile variables passed to the shell (CWE-78).
DOCKER_PLATFORMS, REGISTRY, and VERSION are unquoted. A caller-controlled value containing shell metacharacters can terminate the Docker command and execute arbitrary commands. Quote both the buildx arguments and the fallback environment assignments.
Proposed fix
- docker buildx build --push --platform=$(DOCKER_PLATFORMS) \
- --tag $(REGISTRY)/feast-operator:$(VERSION) -f Dockerfile .; \
+ docker buildx build --push --platform="$(DOCKER_PLATFORMS)" \
+ --tag "$(REGISTRY)/feast-operator:$(VERSION)" -f Dockerfile .; \
...
- IMAGE_TAG_BASE=$(REGISTRY) \
- VERSION=$(VERSION) \
+ IMAGE_TAG_BASE="$(REGISTRY)" \
+ VERSION="$(VERSION)" \As per path instructions: “Quote shell variables in targets.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 733 - 742, Update the Makefile target’s shell commands
to quote all caller-controlled variables: quote the DOCKER_PLATFORMS and
REGISTRY/VERSION values used in the docker buildx arguments, and quote the
REGISTRY and VERSION values in the fallback IMAGE_TAG_BASE and VERSION
environment assignments. Preserve the existing build and fallback behavior.
Source: Path instructions
Comment on lines
+1889
to
+1900
| # Prevent teardown of protected projects | ||
| try: | ||
| current = self.registry.get_project(name=self.project, allow_cache=False) | ||
| if current and current.tags.get(PROTECTED_PROJECT_TAG) == "true": | ||
| raise ValueError( | ||
| f'Teardown is not allowed on protected project "{self.project}". ' | ||
| "Protected projects are managed externally and cannot be torn down via Feast." | ||
| ) | ||
| except ValueError: | ||
| raise | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail closed when protected-project lookup fails.
Both guards swallow every registry error. A timeout or authorization failure makes a protected project appear unprotected; teardown can then destroy its infrastructure.
sdk/python/feast/feature_store.py#L1889-L1900: catch only the known project-not-found exception; propagate all other lookup failures before infrastructure teardown.sdk/python/feast/feature_store.py#L4797-L4807: catch only the known project-not-found exception; propagate all other lookup failures before deletion.
CWE-693, CWE-703.
📍 Affects 1 file
sdk/python/feast/feature_store.py#L1889-L1900(this comment)sdk/python/feast/feature_store.py#L4797-L4807
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/python/feast/feature_store.py` around lines 1889 - 1900, Update the
protected-project lookup guards in sdk/python/feast/feature_store.py at lines
1889-1900 and 4797-4807 to catch only the known project-not-found exception;
allow all other registry errors to propagate before teardown or deletion.
Preserve the existing protected-project rejection behavior and apply the same
fail-closed handling at both sites.
Comment on lines
+1503
to
+1509
| # Exclude protected projects after RBAC check | ||
| visible_projects = [ | ||
| p for p in permitted_projects if p.tags.get(PROTECTED_PROJECT_TAG) != "true" | ||
| ] | ||
|
|
||
| paginated_projects, pagination_metadata = apply_pagination_and_sorting( | ||
| visible_projects, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Filter protected projects in Proto as well.
Line 1503 only protects ListProjects. RegistryServer.Proto at Lines 207-265 independently serializes every RBAC-visible project and its objects, including protected projects. A client can bypass this filter through the Proto RPC. Apply the same protected-tag exclusion before that loop. CWE-200.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/python/feast/registry_server.py` around lines 1503 - 1509, Apply the same
PROTECTED_PROJECT_TAG exclusion used by ListProjects within RegistryServer.Proto
before it serializes RBAC-visible projects and their objects. Filter the
permitted project collection before the Proto serialization loop so projects
tagged "true" cannot be returned through that RPC.
Comment on lines
+1748
to
+1780
| should_protect = os.environ.get(FEAST_PROTECTED_PROJECT_ENV, "").lower() == "true" | ||
|
|
||
| try: | ||
| existing = store.registry.get_project(name=store.project, allow_cache=False) | ||
| except Exception: | ||
| if should_protect: | ||
| from feast.project import Project | ||
|
|
||
| project = Project( | ||
| name=store.project, | ||
| tags={PROTECTED_PROJECT_TAG: "true"}, | ||
| ) | ||
| store.registry.apply_project(project, commit=True) | ||
| logger.info( | ||
| "Tagged project '%s' as protected (%s=true)", | ||
| store.project, | ||
| PROTECTED_PROJECT_TAG, | ||
| ) | ||
| return | ||
|
|
||
| is_protected = existing.tags.get(PROTECTED_PROJECT_TAG) == "true" | ||
|
|
||
| if should_protect and not is_protected: | ||
| existing.tags[PROTECTED_PROJECT_TAG] = "true" | ||
| store.registry.apply_project(existing, commit=True) | ||
| logger.info( | ||
| "Tagged project '%s' as protected (%s=true)", | ||
| store.project, | ||
| PROTECTED_PROJECT_TAG, | ||
| ) | ||
| elif not should_protect and is_protected: | ||
| del existing.tags[PROTECTED_PROJECT_TAG] | ||
| store.registry.apply_project(existing, commit=True) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not infer an unprotect request from a missing environment variable.
Line 1748 treats an unset variable as false, so any registry server started outside the operator clears an existing protection tag at Lines 1778-1780. Line 1752 also treats registry/auth failures as a missing project. Make an unset value a no-op, require an explicit managed false value to remove the tag, and catch only the registry’s not-found exception before creating a project. CWE-693, CWE-703.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/python/feast/registry_server.py` around lines 1748 - 1780, Update the
protection logic around should_protect and the existing-project lookup: treat an
unset FEAST_PROTECTED_PROJECT_ENV as a no-op, only remove PROTECTED_PROJECT_TAG
when the environment explicitly specifies false, and catch the registry-specific
project-not-found exception rather than all exceptions before creating a
project. Preserve protection tagging for explicit true and avoid mutating the
registry on registry/auth failures.
Comment on lines
+347
to
+391
| class TestInitWorkerFreshnessMonitoring: | ||
| """init_worker_freshness_monitoring mirrors init_worker_monitoring's gating.""" | ||
|
|
||
| def test_starts_daemon_thread_when_freshness_enabled(self): | ||
| import feast.metrics as m | ||
|
|
||
| m._config = m._MetricsFlags( | ||
| enabled=True, | ||
| resource=False, | ||
| request=False, | ||
| online_features=False, | ||
| push=False, | ||
| materialization=False, | ||
| freshness=True, | ||
| ) | ||
| mock_store = MagicMock() | ||
|
|
||
| with patch("feast.metrics.threading.Thread") as mock_thread_cls: | ||
| m.init_worker_freshness_monitoring(mock_store) | ||
|
|
||
| mock_thread_cls.assert_called_once_with( | ||
| target=m.monitor_freshness, args=(mock_store, 30), daemon=True | ||
| ) | ||
| mock_thread_cls.return_value.start.assert_called_once() | ||
|
|
||
| def test_does_not_start_thread_when_freshness_disabled(self): | ||
| import feast.metrics as m | ||
|
|
||
| m._config = m._MetricsFlags( | ||
| enabled=True, | ||
| resource=False, | ||
| request=False, | ||
| online_features=False, | ||
| push=False, | ||
| materialization=False, | ||
| freshness=False, | ||
| ) | ||
| mock_store = MagicMock() | ||
|
|
||
| with patch("feast.metrics.threading.Thread") as mock_thread_cls: | ||
| m.init_worker_freshness_monitoring(mock_store) | ||
|
|
||
| mock_thread_cls.assert_not_called() | ||
|
|
||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the module-global metrics configuration after each test.
These tests leave feast.metrics._config mutated, making later tests order-dependent. Use pytest's monkeypatch.setattr or a fixture that restores _config during teardown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/python/tests/unit/test_metrics.py` around lines 347 - 391, Restore
feast.metrics._config after each test in TestInitWorkerFreshnessMonitoring by
using pytest monkeypatch.setattr or an equivalent fixture that automatically
reverts the module-global value during teardown. Update both tests’ direct
_config assignments while preserving their existing flag values and assertions.
Comment on lines
+486
to
+499
| def test_freshness_monitoring_deferred_same_as_resource_monitoring(self): | ||
| """start_freshness_monitoring must mirror start_resource_monitoring exactly.""" | ||
| from types import SimpleNamespace | ||
|
|
||
| mock_store = MagicMock() | ||
| mock_store.config = SimpleNamespace( | ||
| feature_server=SimpleNamespace(metrics=SimpleNamespace(enabled=True)), | ||
| ) | ||
|
|
||
| mock_fm = self._call_start_server(mock_store, cli_metrics=False) | ||
| _, kwargs = mock_fm.start_metrics_server.call_args | ||
| assert ( | ||
| kwargs["start_resource_monitoring"] == kwargs["start_freshness_monitoring"] | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the expected platform-specific values.
The equality assertion passes when both flags are incorrectly True or incorrectly False. On Windows both should be True; on non-Windows both should be False. Assert each flag against sys.platform == "win32" so this test detects the intended deferral regression.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/python/tests/unit/test_metrics.py` around lines 486 - 499, Update
test_freshness_monitoring_deferred_same_as_resource_monitoring to assert
kwargs["start_resource_monitoring"] and kwargs["start_freshness_monitoring"]
individually against whether sys.platform equals "win32", rather than asserting
only their equality. Preserve the existing mock setup and call flow.
|
@jyejare: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Srihari1192
approved these changes
Jul 28, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jyejare, Srihari1192 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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:
Which issue(s) this PR fixes:
Checks
git commit -s)Testing Strategy
Misc
Summary by CodeRabbit
New Features
Bug Fixes
Tests