enhance: support ARN auth for external collection storage reads - #537
enhance: support ARN auth for external collection storage reads#537congqixia wants to merge 3 commits into
Conversation
show manifest and scan-binlog build object store clients from Milvus component configuration, so they cannot read external collection data files whose role_arn/external_id credentials are stored on the collection schema (external_source + external_spec.extfs), not in minio config. Add a shared layer in states/ossutil that parses the external spec, builds an external store from collection credentials, and routes each manifest file to the internal or external store. A single manifest can mix external data files with internal function-output files such as sparse vectors generated from varchar columns. - show manifest: label files with backend and resolved object key - scan-binlog: read external segments via manifest-driven scan - inspect-parquet --external: route per-file through the resolver Add unit tests for extfs parsing and mixed-manifest path routing. Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: congqixia 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 |
|
Tick the box to add this pull request to the merge queue (same as
|
Rewrite the show manifest output selection as a switch to satisfy the gocritic ifElseChain rule, and add the blank lines gofumpt requires between the resolver stub methods in the external store tests. Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
| fmt.Printf("failed to resolve manifest file path %s: %s\n", f.Path, err.Error()) | ||
| continue | ||
| } | ||
| if err := scanExternalParquetFile(ctx, store, objectKey, col2Field, fields, pk, filters, scanTask, segment.GetID(), batchIdx); err != nil { |
There was a problem hiding this comment.
Scanning each column-group file independently does not preserve the logical row contract. External source fields and generated function-output fields can live in separate column groups, while the virtual PK is not stored in either file and must be derived from the segment ID and global row offset. As written, pk remains at its zero value, function-output groups can be counted as additional rows, and row resets for every Arrow batch. This makes count, locate, dedup, $pk expressions, and delete filtering incorrect. Please merge column groups by their row ranges (or reuse a manifest-aware reader), compute the global offset and virtual PK, and invoke Scan once per logical row.
| store, objectKey, _, err := resolver.Resolve(f.Path, "_data") | ||
| if err != nil { | ||
| fmt.Printf("failed to resolve manifest file path %s: %s\n", f.Path, err.Error()) | ||
| continue |
There was a problem hiding this comment.
Please propagate this error instead of printing it and continuing. The same applies to the file scan error below. An invalid ARN, missing object, or Parquet decoding failure currently produces a successful command with zero or partial results, which is especially dangerous for a diagnostic command. If best-effort scanning is required, it should be explicitly enabled and the skipped files and rows must be reported in the summary.
| location := ExternalSourceLocation{ | ||
| Scheme: u.Scheme, | ||
| Host: u.Host, | ||
| Bucket: parts[0], |
There was a problem hiding this comment.
This parser only supports the Milvus-form URI (scheme://endpoint/bucket/key), but Milvus also accepts the AWS form (s3://bucket/key) and derives the endpoint from cloud_provider and region. For example, s3://my-bucket/root/file is currently parsed as endpoint my-bucket, bucket root, and root path file. Please reuse or mirror the canonical pkg/v3/util/externalspec two-form URI contract instead of always treating the URI host as the endpoint.
| return err | ||
| } | ||
| if collection.GetProto().GetSchema().GetExternalSource() != "" { | ||
| externalStore, externalLocation, err = ossutil.NewResolvedExternalObjectStoreFromCollection(ctx, collection, p.SkipBucketCheck) |
There was a problem hiding this comment.
show manifest only reads the manifest from Milvus internal storage and needs the external source location solely to annotate paths. Constructing the external client here performs a bucket existence check and also rejects non-Parquet external formats, so the diagnostic command fails precisely when external credentials or connectivity are broken. Please parse the source/spec without connecting to the external bucket and create the client lazily only in commands that actually read external objects.
| continue | ||
| } | ||
| arr := rec.Column(idx) | ||
| if arr == nil || arr.IsNull(row) { |
There was a problem hiding this comment.
Skipping null cells removes the field from values, which changes the existing scan contract: a nullable field should be represented as fieldID: nil, not as a missing column. Expressions over nullable fields will otherwise fail or produce incorrect results. Please preserve null values and return an explicit error when a non-null value cannot be deserialized to the requested Milvus type instead of silently dropping it.
| // printManifestWithResolver prints manifest entries annotated with the storage | ||
| // backend and the resolved object key for each file. It supports manifests that | ||
| // mix external data files with internal function-output files. | ||
| func printManifestWithResolver(m *manifest, resolver *ossutil.ManifestPathResolver) { |
There was a problem hiding this comment.
This duplicates the existing manifest renderer and already causes output drift: the resolver version omits column-group metadata, stats metadata, V6 index details, index file keys, and index properties. Please keep a single renderer and make path resolution an optional annotation step so external manifests retain all existing diagnostic output while adding backend and object-key information.
Merge column groups by global row offset so overlapping external data and internal function-output files contribute to the same logical rows, and derive the virtual PK from segment ID plus offset. Errors now abort the scan instead of printing and continuing, and null cells are kept as nil values while deserialization failures surface explicitly. Support AWS-form external source URIs (scheme://bucket/key) with the endpoint derived from cloud provider and region, and parse the external location lazily in show manifest so the diagnostic does not require a working external bucket. Unify the manifest renderer so resolver-based output retains all fields. Signed-off-by: Congqi Xia <congqi.xia@zilliz.com>
3 participants
show manifest and scan-binlog build object store clients from Milvus component configuration, so they cannot read external collection data files whose role_arn/external_id credentials are stored on the collection schema (external_source + external_spec.extfs), not in minio config.
Add a shared layer in states/ossutil that parses the external spec, builds an external store from collection credentials, and routes each manifest file to the internal or external store. A single manifest can mix external data files with internal function-output files such as sparse vectors generated from varchar columns.
Add unit tests for extfs parsing and mixed-manifest path routing.