restore: decide index replay from what the secondary backup carries

[czs007]

issue: #1167

Follow-up to #1168.

Problem

IndexInfo splits into two groups by source:

source	fields
DescribeIndex (always present)	field_name index_name index_type params index_id
etcd, only with --backup_index_extra	field_id create_time type_params index_params user_index_params is_auto_index min_index_version max_index_version

createIndexes loops over the index infos and broadcasts each one unconditionally — it was written assuming the etcd half is always there, with no check and no fallback. So a backup taken without the flag broadcasts an index on FieldID 0 (RowIDField, which is never indexed), and the target ends up with an index entry that gates every future import of that collection:

• the import job finishes importing and then stays in IndexBuilding; the client polls [state=Importing] [progress=80] and never stops, because waitBulkInsertState only returns on Completed or Failed (_bulkInsertTimeout logs a warning and keeps going);
• DataCoord index tasks fail with parse magic number failed, expected: 16775868, actual: 827474256 — PAR1, the index node reading the storage-v2 packed column file of field 0 as a v1 binlog;
• DescribeIndex on the restored collection fails with failed to get collection field: 0, so the bogus entry is hard to even inspect.

Nothing required that message to be sent. Not sending it is strictly better: with no index entry the import job has no index task to run and completes.

Change

Whether indexes are part of a secondary restore is settled when the backup is created, not at restore time. So decide it from the backup as a whole, once, in Task.Execute before any client is created or any DDL is broadcast:

backup	behaviour
every index carries the extra attributes	replay create index as before — unchanged
none does	the backup holds no index information a verbatim replay can use: restore the collections and their data without indexes, naming the left-out indexes up front
some do, some do not	the attributes are collected for every index at once or for none, so the meta is inconsistent — fail before anything is restored

Nothing is reconstructed on the client side. The missing attributes are what makes the DDL replay match the source cluster, and create_time in particular does not exist anywhere in a default backup; guessing them would silently put an index with different parameters on the replica, which is worse than not having one.

restore secondary --help now states what --backup_index_extra buys.

Tests

TestReplayIndex covers all three outcomes plus empty index list, empty collection list, and index_params missing on its own; TestIndexNames covers the message formatting.

Open question, deliberately not answered here

A collection restored without indexes still gets the AlterLoadConfig broadcast when the source was loaded. Whether the target can load a collection that has no index on its vector field is a server-side question I have not verified — if it cannot, the load stage needs the same treatment. Pointers welcome; I can follow up with a two-cluster test.

Not in scope

• Recovering an existing backup. The attributes can still be read from the source cluster's etcd, so a command that backfills them into an already-taken backup would make those backups fully restorable, indexes included. Restore-time is too late for it: restore secondary is configured against the target (CI rewrites milvus.grpc.port to the downstream before restoring), and --source_cluster_id is an identifier, not a connection — so this belongs on the source side, as its own command.
• The root cause is on the backup side. --backup_index_extra defaults to false and is documented nowhere, so a default create silently produces a backup that cannot replay indexes. Note it also gates the dynamic-field backup, which reuses the etcd client created only for it (core/backup/task.go:515) — the same silent gap, under a flag name that does not suggest it. @huanghaoyuanhhy's --use secondary preset ([Bug]: restore secondary broadcasts CreateIndex with FieldID 0 when the backup was taken without --backup_index_extra #1167 (comment)) addresses this; happy to follow up once that issue is opened.

Relation to the earlier review

@huanghaoyuanhhy proposed rejecting such backups outright (#1167 (comment)). This keeps his constraint — no client-side reconstruction, ever — but declines to make an existing backup unusable for secondary restore: it still restores the collections, the data and the CDC linkage, only without indexes, and says so up front. Rejecting outright would leave every backup taken before the contract was known with no path at all, and the data those backups hold is not the part that is missing.

[sre-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: czs007
To complete the pull request process, please assign lentitude2tk after the PR has been reviewed.
You can assign the PR to them by writing /assign @lentitude2tk in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov-commenter]

Codecov Report

❌ Patch coverage is 61.70213% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.78%. Comparing base (c0c1a96) to head (0757304).

Files with missing lines	Patch %	Lines
core/restore/secondary/task.go	0.00%	15 Missing ⚠️
core/restore/secondary/coll_ddl_task.go	0.00%	3 Missing ⚠️

❌ Your patch status has failed because the patch coverage (61.70%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project status has failed because the head coverage (42.78%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1175      +/-   ##
==========================================
+ Coverage   42.69%   42.78%   +0.08%     
==========================================
  Files         134      134              
  Lines       12415    12459      +44     
==========================================
+ Hits         5301     5330      +29     
- Misses       6710     6725      +15     
  Partials      404      404              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[huanghaoyuanhhy]

Suggest failing the "none has extra" branch as well, rather than skipping indexes:

1. The degrade path does not actually work. Answering the open question: a collection without indexes cannot be loaded — loadCollectionTask.PreExecute fails with ErrIndexNotFound when DescribeIndex finds no index (milvus internal/proxy/task.go). A loaded source is the common DR case, so skipping indexes just fails later, at the AlterLoadConfig broadcast, with a less obvious error. And even for unloaded sources it yields a secondary that can never be loaded without a manual CreateIndex and cannot serve after failover. Reporting success on a silently diverged secondary is worse than failing — the failure is visible, the divergence is not.
2. The backups this path is meant to rescue barely exist. Building a secondary requires the backup to be taken around a flush all and CDC to read the WAL from the backup checkpoint; once the primary's WAL is GC'd past that point, no secondary can be built from it regardless of indexes. Backups taken before the --backup_index_extra contract was known have almost certainly lost that window. The realistic workflow is always made-to-order: take a fresh backup with the flag when building a secondary — and failing fast on a missing flag catches the misconfiguration at exactly the right moment.
3. Precedent in this package: checkDynamicField already fails the restore for the dynamic-field gap gated by the same flag. The index gap is the same shape; the handling should be the same.

So replayIndex should return an error when indexes exist but none carries the extra info, naming the indexes and pointing at --backup_index_extra, same as the inconsistent case.