Skip to content

backup: probe etcd at task start and bound every etcd read - #1218

Open
czs007 wants to merge 2 commits into
zilliztech:mainfrom
czs007:etcd-probe-timeout
Open

backup: probe etcd at task start and bound every etcd read#1218
czs007 wants to merge 2 commits into
zilliztech:mainfrom
czs007:etcd-probe-timeout

Conversation

@czs007

@czs007 czs007 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

issue: #1216

A backup run with --backup_index_extra can hang forever on its first etcd read,
silently, after every collection has already been copied. Three things combine:
the client is created without ever dialling, the reads carry no deadline, and both
etcd steps run last, so an unreachable etcd is only discovered at the end of the
backup.

Reported on a deployment where all 69 collections and the RBAC metadata were
written, and the run then stopped dead on

start to get index info from etcd  prefix=by-dev/meta/field-index/462915446997590087/

with nothing after it in a 4.9 MB log.

What changed

core/backup/etcd.go (new): etcdMeta wraps the client with the endpoints it
was built from, because clientv3 never reports which address a call was talking to
and a bare context deadline exceeded gives an operator nothing to act on.

  • probe establishes that etcd answers before the backup starts copying. One
    reachable member is enough — the client fails over, so a single down member of an
    otherwise healthy cluster must not fail a backup. An unreachable cluster reports
    every endpoint with its own error.
  • getPrefix bounds the read with a deadline: 10s, matching etcd.requestTimeout,
    the knob Milvus bounds its own etcd calls with. There is no reason for this tool
    to wait longer than the server it reads from.

core/backup/task.go: the probe runs in initClients, inside the existing
if t.option.BackupIndexExtra branch — the only case that builds an etcd client at
all — so a cluster whose etcd this tool cannot reach fails in the first seconds
rather than after the data is copied.

core/backup/coll_index_extra_task.go, core/backup/coll_dyn_field_task.go:
read through etcdMeta instead of calling kv.Get with the unbounded task context.
The dynamic field task's scan is also scoped to the backed-up collections: the key
is root-coord/fields/{collectionID}/{fieldID}, so the collection id is already the
first segment, and the previous cluster-wide read fetched every field of every
collection in order to keep at most one dynamic field per backed-up collection. The
index extra scan was scoped this way in #1055; this one was not, and its size
tracked the instance rather than the backup — which is what makes a bounded read
safe here.

Why a probe rather than a blocking dial

clientv3.New does not dial unless the config carries grpc.WithBlock(), so
construction succeeds against endpoints that do not exist. Adding WithBlock would
not be enough: it proves the TCP connection, not that the peer speaks etcd, and an
address that accepts the connection and then never answers is exactly the reported
failure. WithBlock is also deprecated. An explicit Status settles both and
doubles as the start-of-task check.

Tests

core/backup/etcd_test.go covers the probe: reachable, one member down, all
unreachable naming every endpoint, a silent endpoint that must not block, and no
endpoint configured. TestEtcdMeta_ProbeWithRealClient pins the premise of the fix
with a real clientv3 client — clientv3.New against an address nothing listens on
returns no error, and only the probe reports it. The two task tests assert that an
unreachable etcd fails their Execute with the endpoint in the message, and that
each scan asks only for the collections in the backup.

Not in this change

etcd TLS. MilvusEtcdConfig has only Endpoints and RootPath, so a deployment
whose etcd requires client certificates cannot be read by this tool at all; Milvus
itself supports etcd.ssl.enabled with tlsCert / tlsKey / tlsCACert. That is
the fourth defect in #1216 and is left open, which is why this says issue rather
than fixes.

🤖 Generated with Claude Code

A backup run with --backup_index_extra could hang forever on its first
etcd read, with nothing in the log beyond "start to get index info from
etcd". Three things had to be true at once, and all three were.

clientv3.New does not dial unless the config carries grpc.WithBlock(),
so the DialTimeout the backup passed was never exercised: construction
succeeded against endpoints that do not exist, and the failure was
deferred to the first RPC. That first RPC took the task context, which
carries no deadline, so an endpoint that accepts the connection and
never answers blocked the Get until the process was killed. And both
etcd steps -- index extra information and the dynamic field schemas --
run after every collection has been copied, so the hang landed at the
end of a backup that had otherwise finished.

etcdMeta now owns the two reads. It bounds each one with a deadline,
defaulting to 10s to match the etcd.requestTimeout Milvus bounds its own
etcd calls with, and it carries the endpoints it was built from so a
timeout names the address that did not answer -- clientv3 reports only
"context deadline exceeded", which is not actionable without a packet
capture. Its probe runs in initClients, and only when BackupIndexExtra
is on, since that is the only case that builds an etcd client at all: a
cluster whose etcd this tool cannot reach now fails in the first seconds
instead of after the data has been copied.

The probe is an explicit bounded Status rather than a blocking dial. A
dial only proves the TCP connection, and the reported failure is an
endpoint that accepts the connection and then does not speak etcd, which
a blocking dial would pass; grpc.WithBlock is also deprecated. One
reachable member is enough, so a single down member of an otherwise
healthy cluster does not fail a backup, and the error names every
endpoint that was tried.

etcd TLS remains unconfigurable; that half of zilliztech#1216 is left alone.

Related to zilliztech#1216

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
@sre-ci-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: czs007
To complete the pull request process, please assign alohaha22 after the PR has been reviewed.
You can assign the PR to them by writing /assign @alohaha22 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:

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-commenter commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.17391% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.26%. Comparing base (9b4767a) to head (beed9fa).

Files with missing lines Patch % Lines
core/backup/coll_dyn_field_task.go 69.69% 8 Missing and 2 partials ⚠️
core/backup/task.go 18.18% 9 Missing ⚠️
core/backup/coll_index_extra_task.go 83.33% 1 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (77.17%) 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 (47.26%) 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    #1218      +/-   ##
==========================================
+ Coverage   46.89%   47.26%   +0.36%     
==========================================
  Files         140      141       +1     
  Lines       12845    12877      +32     
==========================================
+ Hits         6024     6086      +62     
+ Misses       6386     6354      -32     
- Partials      435      437       +2     

☔ 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.

The dynamic field task read every field of every collection in the instance:

    prefix := fmt.Sprintf("%s/meta/root-coord/fields/", cdft.etcdRootPath)

and then threw nearly all of it away, keeping at most one dynamic field per
backed-up collection. The index extra scan was scoped this way in zilliztech#1055; this
one was not.

The size of that read tracks the instance, not the backup, so a bounded read
turns a large enough instance into a failed backup rather than a slow one --
which is a regression the deadline added alongside it would otherwise
introduce. The key is "root-coord/fields/{collectionID}/{fieldID}", so the
collection id is already the first segment and one read per backed-up
collection asks for exactly what the task uses.

parseCollIDFromFieldKey goes with it: the collection id is the loop variable
now, so there is nothing left to parse out of the key.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants