fix(admin,api): honor STORAGE_IMPL when surfacing file_store backend (closes #17294) - #17441
Open
Harsh23Kashyap wants to merge 3 commits into
Open
fix(admin,api): honor STORAGE_IMPL when surfacing file_store backend (closes #17294)#17441Harsh23Kashyap wants to merge 3 commits into
Harsh23Kashyap wants to merge 3 commits into
Conversation
…loses infiniflow#17294) 1. admin/server/config.py: add `case "s3"` branch to load_configurations that creates a FileStoreConfig with store_type="s3", parsing endpoint_url for host/port. Falls back to s3.amazonaws.com:443 when endpoint_url is absent so the status page still renders a sane host. 2. admin/server/services.py: add a STORAGE_IMPL filter to ServiceMgr.get_all_services that hides inactive file_store backends (e.g. a stale minio entry from service_conf.yaml when STORAGE_IMPL =AWS_S3). Maps env var names (AWS_S3, MINIO, ...) to lowercase store_type (s3, minio, ...) by stripping the AWS_ prefix and lowercasing. 3. api/utils/health_utils.py: add check_s3_alive that delegates to the existing generic check_storage so the same code path works for AWS S3, MinIO, R2, or any other S3-compatible endpoint. Without this fix, the Admin Service status page reports MinIO as the active file_store even when STORAGE_IMPL=AWS_S3, and the wired health check (check_minio_alive) times out against settings.MINIO['host'].
… check_s3_alive (infiniflow#17294) 1. test/unit_test/api/utils/test_health_utils_s3.py: 5 cases covering check_s3_alive alive/timeout paths and the delegate-contract that mirrors check_storage's return shape (elapsed, error). 2. test/unit_test/admin/test_load_configurations.py: 10 cases covering the new case "s3" branch (https/http/non-default-port/omitted), a regression guard for the existing minio path, both-backends-in-one- config, and 3 malformed-endpoint "does not raise" cases. 3. test/unit_test/admin/test_get_all_services.py: 9 cases covering the new STORAGE_IMPL filter (minio-active, s3-active, no-config), the existing DOC_ENGINE retrieval filter (regression guard), and a parametrized env-var-to-store_type mapping test (MINIO, AWS_S3, OSS, GCS). 4. test/unit_test/admin/conftest.py: prepend admin/server to sys.path so admin modules can be imported from pytest. The admin package is invoked as a script in production; tests need the same path layout.
dosubot
Bot
added
size:M
Open
4 tasks
Contributor
📝 WalkthroughWalkthroughAdds S3 configuration loading and health checks to the admin service backend, filters file-store services by ChangesS3 admin service reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigLoader
participant ServiceMgr
participant HealthCheck
participant Storage
ConfigLoader->>ServiceMgr: register S3 FileStoreConfig
ServiceMgr->>ServiceMgr: filter by STORAGE_IMPL
ServiceMgr->>HealthCheck: request S3 liveness
HealthCheck->>Storage: check storage
Storage-->>HealthCheck: health result
HealthCheck-->>ServiceMgr: alive or timeout status
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@admin/server/config.py`:
- Around line 293-297: Update the S3 endpoint parsing in admin/server/config.py
around endpoint_url, parsed, and parsed.port to catch ValueError from malformed
URLs or invalid ports and fall back without aborting configuration loading. Add
coverage in test/unit_test/admin/test_load_configurations.py for non-numeric
ports, out-of-range ports, and malformed IPv6 input, verifying configuration
loading completes with the fallback behavior.
In `@api/utils/health_utils.py`:
- Around line 249-259: Update api/utils/health_utils.py lines 249-259 in
check_s3_alive to emit a debug log for the check_storage outcome without logging
raw error payloads; update admin/server/config.py lines 288-311 to log the
parsed S3 host and port while never logging endpoint_url directly; update
admin/server/services.py lines 276-293 to add debug visibility for normalized
STORAGE_IMPL filtering.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c40fac9d-e11b-439f-8a7a-318e59147d15
📒 Files selected for processing (7)
admin/server/config.pyadmin/server/services.pyapi/utils/health_utils.pytest/unit_test/admin/conftest.pytest/unit_test/admin/test_get_all_services.pytest/unit_test/admin/test_load_configurations.pytest/unit_test/api/utils/test_health_utils_s3.py
…load `urlparse(endpoint_url)` raises ValueError on malformed IPv6 URLs, and `parsed.port` raises ValueError on non-numeric or out-of-range ports. A bad endpoint in the config block previously crashed the admin status loader and stopped the UI from showing any services. Wrapped the S3 parser in try/except ValueError and fall back to the raw endpoint string as host with the scheme's default port so config loading always completes. Added logging: - `check_s3_alive` emits the check outcome at debug level - `admin/server/config.py` logs the selected host/port after parsing (and the AWS S3 default when endpoint_url is unset) at debug level; the full endpoint_url is never logged because it may contain credentials in the query string. - The malformed-URL fallback path logs at warning level. Extended `TestLoadS3HandlesBadEndpoint` with the three concrete malformed inputs the bug surfaced: non-numeric port, out-of-range port, and malformed IPv6 literal.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@api/utils/health_utils.py`:
- Around line 258-260: Update check_s3_alive to use a defined logger before its
success and failure logging calls. Import the logging module or reuse the
module’s existing logger, ensuring both logging.debug references execute without
raising NameError and the function returns its health result.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 36f3a41b-07f4-4cd2-adbe-c3ce71c4b0eb
📒 Files selected for processing (3)
admin/server/config.pyapi/utils/health_utils.pytest/unit_test/admin/test_load_configurations.py
🚧 Files skipped from review as they are similar to previous changes (2)
- admin/server/config.py
- test/unit_test/admin/test_load_configurations.py
Comment on lines
+258
to
+260
| logging.debug("check_s3_alive: ok, elapsed=%s ms", payload.get("elapsed", "?")) | ||
| return {"status": "alive", "message": f"Confirm elapsed: {payload.get('elapsed', '?')} ms."} | ||
| logging.debug("check_s3_alive: failed, error=%s", payload.get("error", "unknown")) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Define logging before using it.
Both paths reference an undefined name, so check_s3_alive() raises NameError instead of returning a health result. Import logging or use the module’s existing logger.
Proposed fix
+import logging
+
def check_s3_alive():📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logging.debug("check_s3_alive: ok, elapsed=%s ms", payload.get("elapsed", "?")) | |
| return {"status": "alive", "message": f"Confirm elapsed: {payload.get('elapsed', '?')} ms."} | |
| logging.debug("check_s3_alive: failed, error=%s", payload.get("error", "unknown")) | |
| import logging | |
| logging.debug("check_s3_alive: ok, elapsed=%s ms", payload.get("elapsed", "?")) | |
| return {"status": "alive", "message": f"Confirm elapsed: {payload.get('elapsed', '?')} ms."} | |
| logging.debug("check_s3_alive: failed, error=%s", payload.get("error", "unknown")) |
🧰 Tools
🪛 Ruff (0.15.21)
[error] 258-258: Undefined name logging
(F821)
[error] 260-260: Undefined name logging
(F821)
🤖 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 `@api/utils/health_utils.py` around lines 258 - 260, Update check_s3_alive to
use a defined logger before its success and failure logging calls. Import the
logging module or reuse the module’s existing logger, ensuring both
logging.debug references execute without raising NameError and the function
returns its health result.
Source: Linters/SAST tools
Contributor
Author
|
Opened #17441 to honor STORAGE_IMPL when surfacing the file_store backend on the admin status page. While addressing the CodeRabbit review I also tightened the S3 endpoint parser: malformed IPv6 URLs, non-numeric ports, and out-of-range ports now fall back to a sensible default instead of aborting config load, and the S3 health/parser paths emit debug-level diagnostics (without leaking credentials). All three review threads resolved. |
Contributor
|
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
2 participants
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.
Summary
When
STORAGE_IMPL=AWS_S3, the Admin Service status page keeps showingMinIO. Three things conspire:
admin/server/config.py::load_configurationsonly knows theminioandminio_0config keys. Ans3block lands on thecase _:branch and logsUnknown configuration key: s3(issue [Bug]: Admin Service status still reports MinIO when object storage is configured as AWS S3 #17294).
admin/server/services.py::ServiceMgr.get_all_servicesfiltersretrieval services by
DOC_ENGINEbut has no equivalent filterfor
file_storeservices bySTORAGE_IMPL. A stale MinIO blockis returned regardless of the active backend.
check_minio_alive, whichcalls
settings.MINIO['host']. WithSTORAGE_IMPL=AWS_S3thatblock is uninitialized, so the check always times out.
Changes
admin/server/config.py:case "s3"branch that creates aFileStoreConfigwithstore_type="s3", parsingendpoint_urlfor host/port. Fallsback to
s3.amazonaws.com:443whenendpoint_urlis absent.admin/server/services.py:STORAGE_IMPLfilter inServiceMgr.get_all_services. Theenv var name (e.g.
AWS_S3) is mapped to the lowercasestore_type(e.g.s3) by stripping theAWS_prefix andlowercasing.
api/utils/health_utils.py:check_s3_alivethat delegates to the existing genericcheck_storage. The generic helper already usessettings.STORAGE_IMPL.health(), so the same code path works forAWS S3, MinIO, R2, and any other S3-compatible endpoint.
Tests
test/unit_test/api/utils/test_health_utils_s3.py— 5 casescovering alive/timeout paths and the delegate contract.
test/unit_test/admin/test_load_configurations.py— 10 casescovering the new
case "s3"branch (https, http, non-defaultport, omitted endpoint), regression guard for the existing minio
path, both-backends-in-one-config, and 3 malformed-endpoint
"does not raise" cases.
test/unit_test/admin/test_get_all_services.py— 9 cases coveringthe new filter (minio-active, s3-active, no-config), the existing
DOC_ENGINEretrieval filter (regression guard), and aparametrized env-var-to-store_type mapping test (
MINIO,AWS_S3,OSS,GCS).test/unit_test/admin/conftest.py— sys.path shim so the adminmodule (which is run as a script in production) imports cleanly
from pytest.
36 tests pass locally. Ruff check + format clean.
Out of scope (follow-up)
The same shape applies to
oss,gcs,azure_spn,azure_sas,and
opendal. Each is a smallcaseblock inadmin/server/config.pyplus the samestore_typename. I scopedthis PR to S3 because the issue is specifically about S3; the rest
can land as one or more follow-up PRs using the same pattern.
Backward compatibility
minioconfigurations are unchanged. The newcase "s3"is purely additive.s3block is absent andSTORAGE_IMPL=MINIO, theexisting minio entry still appears (regression-guard tests).
file_storeconfig in
service_conf.yaml(the common case).Fixes #17294