issue: #53307
Protocol dependency: milvus-io/milvus-proto#668 (merged).
## What changed
- Reuse the existing RPCs to expose retained segment lifecycle metadata
and direct compaction sources; retain the existing sealed/loaded segment
reporting contract without adding Growing segments to
GetQuerySegmentInfo.
- Expose collection-scoped retained compaction tasks, with complete task
identity, public type/state enums, source/target sets, and failure
reasons, while preserving manual job-ID lookup.
- Preserve failure reasons across worker cleanup without changing task
cleanup or metadata GC semantics.
- Add seven **L3-only** data-integrity workloads: fixed V2/V3 Insert,
fixed V2/V3 Import, V2 add/drop-field evolution through MixCompaction
with schema bump disabled, V3 schema evolution, and V2-to-V3-to-V2
storage transitions.
- Validate deterministic full-row/full-field data against an independent
oracle, fenced persistence/serving checkpoints, and actual lineage
transitions, allowing either automatic or manual compaction.
- Provide `tests/scripts/ci_compaction_integrity.sh` as a dedicated,
single-invocation, serial entry; leave the shared
`tests/scripts/ci_e2e.sh` unchanged from the PR base.
- Retain successful audit records only for the dedicated workload
marker, preserving ordinary tests' JSON, HTML, and worker-report
formats.
## Compatibility and execution boundary
- Omitted segment states preserve the existing persistent-state
behavior; task and dropped-segment history are subject to existing
retention/GC, not a fixed response-size or record-count limit.
- These workloads are not added to per-PR E2E or ordinary Nightly
pipelines.
- The dedicated job must own the deployment/config namespace
exclusively, install a compatible PyMilvus build, and provide connection
options plus a unique `CI_LOG_PATH`.
- The entry enforces `-n 0`, the L3 workload marker, and explicit
opt-in, while forwarding connection options, filters, and pytest's exit
status.
- Insert/Import workloads use three rounds by default; insert load units
are batches, not a promise of a physical segment count, and dense vector
dimensions are 16.
## Observability semantics and follow-ups
- **Segment listing is not an atomic snapshot.** The Proxy first calls
`GetSegmentsByStates`, then `GetSegmentInfo`; returned details are
filtered again against the requested states (or the legacy default:
Flushing, Flushed, Sealed). A segment whose fetched state no longer
matches is omitted, and a segment becoming eligible after ID enumeration
may be absent. Consumers must not treat a single response as a
consistent lifecycle snapshot.
- **Existing lookup errors are preserved.** `IncludeUnHealthy` is
enabled only when Dropped is explicitly requested. In the default
healthy-only path, a selected source becoming Dropped before detail
lookup still returns `SegmentNotFound`, as before; that error is not
converted into a successful partial list. A history segment being GC'd
between the two RPCs can also fail the lookup.
- **Aggregate `Completed` means no running tasks, not universal
success.** This preserves the existing aggregation semantics:
all-failed/all-timeout retained tasks, or an empty task set, can report
Completed. Inspect each task's `state` and `failure_reason` for its
outcome; proving an actual data transition additionally requires
lineage, active-set adoption, and serving-set evidence. This PR does not
redefine task retry or cleanup behavior.
- **History capacity is a follow-up.** Dropped-segment queries enumerate
the retained history and fetch its details, while collection-scoped
compaction queries return retained tasks across triggers. Neither path
currently provides pagination or a fixed count/size cap, so scan, copy,
and response costs grow with the retained record count despite
retention/GC. Pagination and capacity controls for both paths are
deferred; they must preserve the ability to enumerate complete retained
history rather than silently truncate lineage or task evidence.
## Previously recorded verification
The results below are earlier recorded runs, including the original
six-workload suite before the V2 DDL workload was added; the
documentation/review-response update did not rerun tests.
- 22 server-independent CI boundary tests passed, covering
ordinary/special audit output, worker report merging, scoped execution
guards, and the dedicated entry's argument/exit-code behavior.
- 20 differential checks against the PR-base logging plugin produced
byte-identical ordinary worker JSON, report JSON, and HTML across all
five result outcomes.
- 14 existing DataIntegrity and etcd-helper unit tests passed.
- The three original L1 DataIntegrity parameter instances passed with
`-n 2 --dist loadgroup` in 59.39 seconds; their merged successful JSON
records retained the original metadata-only shape.
- Ruff lint/format, shell syntax, and scoped PR-base whitespace checks
passed; the shared runner is byte-identical to the PR base.
- The new dedicated entry ran all six workloads serially against the
existing local cluster: **6 passed in 3373.52 seconds**, with three
rounds for each Insert/Import workload and the storage-transition
workload, plus five DDL checkpoints over 25,000 rows.
- All six successful reports retained their audit evidence (403
structured events), including every expected round/DDL checkpoint, full
segment/task snapshots, serving frontiers, and matching
expected/retrieved PK digests.
## Design and job entry
- [Data-integrity design
contract](https://github.com/MrPresent-Han/milvus/blob/codex/compaction-e2e-observability/docs/design-docs/design_docs/compaction_data_integrity_e2e_design.md)
- [Dedicated serial
entry](https://github.com/MrPresent-Han/milvus/blob/codex/compaction-e2e-observability/tests/scripts/ci_compaction_integrity.sh)
Signed-off-by: MrPresent-Han <chun.han@gmail.com>
Co-authored-by: MrPresent-Han <chun.han@gmail.com>
issue: #52114
Makes `ruff check` and `ruff format --check` pass over all of `tests/`,
so the `Ruff (changed files only)` job stops failing PRs for violations
they did not introduce (most recently #51903, which got 69 errors from
an 8-line change).
Almost all of this is mechanical. The behavioural changes are called out
individually below: one `NameError` fix, three shadowed duplicate
definitions, 13 assertion gaps, and two undefined names that only became
visible once star imports were expanded.
### Layout
Seven reviewable commits:
| commit | what | files |
|---|---|---|
| 1 | `ruff format` | 170 |
| 2 | `ruff check --fix` (safe fixes only, no `--unsafe-fixes`) | 156 |
| 3 | F821 fix + `tests/ruff.toml` | 4 |
| 4 | three shadowed duplicate definitions | 4 |
| 5 | close 13 assertion gaps behind F841 | 7 |
| 6 | expand star imports, re-enable F403/F405/F821 | 33 |
| 7 | enable E402, E712, E722 | 16 |
1745 violations → 0. Only `tests/` is touched.
### The undefined-name bug
`chaos/test_chaos_memory_stress.py`:
`TestMemoryStressReplica.prepare_collection` referenced bare `nb` and
`dim`. Those are class attributes, never module-level names, so the
fixture raised `NameError` the moment it ran — and the line directly
above it already used `self.dim`. Changed to `self.nb` / `self.dim`.
This was the only F821 in `tests/`; **the rule stays enabled**.
### Three shadowed duplicate definitions
Three places define the same name twice in the same scope. Python keeps
the later one, so the earlier definition is dead while still reading as
live code — a test that looks like it exists and never runs.
**`restful_client/testcases/test_vector_operations.py` —
`TestDeleteVector`**
```python
@pytest.mark.L0
class TestDeleteVector(TestBase): # line 778 — shadowed, never ran
def test_delete_vector_default(...) # 2x2 parametrized
@pytest.mark.L1
class TestDeleteVector(TestBase): # line 838 — wins
def test_delete_vector_with_invalid_api_key(...)
def test_delete_vector_with_invalid_collection_name(...)
def test_delete_vector_with_non_primary_key(...)
```
The second holds only negative cases, and the file already uses
`TestInsertVectorNegative` / `TestSearchVectorNegative`, so this is a
copy whose class name was never updated. Renamed it to
`TestDeleteVectorNegative`.
**This is the one to look at:** it brings back an `L0` case, so 4
parametrizations of `test_delete_vector_default` start running in PR CI.
They have never executed, so if the restful delete path has drifted
since they were written, this is where it surfaces.
**`python_client/testcases/test_compaction.py` —
`test_compact_after_delete`**
Defined twice in `TestCompactionParams`, covering different things: the
shadowed one (L3, `@parametrize("delete_pos", [1, tmp_nb // 2])`) covers
*partial* deletes plus a compaction-plan check; the surviving one (L2)
deletes everything. There is already a separate
`test_compact_after_delete_all`, so partial-delete coverage existed only
in the shadowed case and was not running. Renamed it to
`test_compact_after_delete_partial`; the surviving definition is
untouched.
**`python_client/utils/util_pymilvus.py` — `add_vector_field`**
Defined twice with different signatures:
```python
def add_vector_field(entities, is_normal=False): # line 455 — shadowed
...
entities.append(field)
return entities
def add_vector_field(nb, dimension=default_dim): # line 527 — wins
field_name = gen_unique_str()
field = {...} # built, then discarded
return field_name # returns only the name
```
Removed the second — both the broken one and the one that was winning.
Grepped the whole repo: **neither definition has a caller**; every
`add_vector_field` hit is something else
(`test_milvus_client_add_vector_field` test methods, the
`Op.add_vector_field` enum member in `chaos/checker.py`, a `Checker`
method of the same name). Dead-code removal, not an API change.
**Net effect on what executes:** `test_delete_vector_default` ×4
(**L0**) and `test_compact_after_delete_partial` ×2 (L3) start running;
nothing stops running; the 3 negative cases keep running under the new
class name. Computed by parsing each file before and after and resolving
shadowing the way Python does, not by reading the diff.
F811 is now clean across `tests/` with no per-file suppressions.
### Star imports expanded, F403/F405/F821 re-enabled
33 of 307 files used `from utils.util_pymilvus import *` and friends.
Inside those, pyflakes cannot tell a typo from a star-imported name — it
downgrades everything unresolved to F405, and **F821 never fires**. The
`NameError` fixed in commit 3 was only catchable because that file
happens not to use a star import.
The 379 F405 hits were only **58 distinct names**. All are now explicit,
resolved by walking the star modules in source order (later import wins,
matching Python). Names that `util_pymilvus` merely re-exports come from
their real source now — `DataType` from `pymilvus`, `np` from `numpy`,
`log` from `utils.util_log` — rather than being laundered through it.
Turning F821 on immediately caught **two real undefined names**, both in
`rate_limit/test_rate_limit.py` (L3, so it has never run in CI):
```python
for collection_num in range(collectionRateLimit): # no such name
# ^ the parametrize arg is `collection_rate_limit`, and it is
# a string ("10"/"500"), so range() needs int().
# The same test uses the correct name 30 lines below.
mil.uninstall(release_name, namespace=NAMESPACE) # no such name
# ^ MilvusOperator.uninstall has exactly this signature, and other suites do the
# same under `mic`/`milvus_op` — the instantiation was just missing.
```
Both fixed. `test_milvus_client_data_integrity_new.py` needed 15 `noqa`
markers instead: its `prepare_data` fixture injects those names via
`globals()[field] = ...`, which no static analysis can follow. That
block has other problems — a `float is None` check against the builtin,
an always-true `if "is null" or "IS NULL" in expression`, leftover
`binbin_debug1` logging — but rewriting fixture logic does not belong
here; noted in #52116.
### 13 assertion gaps closed
Of ~80 F841 hits, most are harmless scratch values. 13 were a call whose
result was bound and then never checked. Highlights:
- `test_milvus_client_database.py`: two tests apply a non-existent
property key (`data.replica.number`, note `data.` not `database.`),
describe the database, and drop it without checking anything. Their
docstrings say `actual: run successfully` / `nothing changed` — the
intent was to prove nothing changed. Now asserted.
- `test_milvus_client_alias.py`: two assertions were commented out,
**and they were commented out for a reason** — `list_aliases()[0]`
returns a dict, so `assert alias in aliases` would have tested dict
keys. Rewritten against `[0]["aliases"]` (how the rest of the file does
it) and re-enabled. In the skipped `alias_default` test the calls were
also missing the required `collection_name` argument.
The remaining ~65 F841 stay, and the rule stays off.
### E402, E712, E722 also enabled
**E722** (37 sites): mechanical `except:` → `except Exception:`. Checked
the two ways this could change behaviour, both empty: no bare-except
try-block can raise `SystemExit`/`KeyboardInterrupt`, and **none calls
`pytest.skip`/`fail`/`xfail`**. That second one matters — pytest's
`Skipped` derives from `BaseException`, so a bare except silently eats
it while `except Exception` would not. 0 of 37, so behaviour is
unchanged.
**E402** (5 sites): imports that were just in the wrong place — `import
re` sat 70 lines into `check/param_check.py`, and the two
`resource_group` modules imported `rich` after their module-level
constants. Nothing to do with `sys.path`, which is what an earlier
version of this description wrongly claimed.
**E712** (5 sites), gone through one at a time rather than
bulk-rewritten:
| site | value | change |
|---|---|---|
| `test_milvus_client_partition.py` | `has_partition()` | → `is False` —
stricter than `not result`, which would pass on `None` |
| `test_restart.py` | JSON status flag | → `is True` |
| `test_restart.py` | local `break_flag` | → plain truth test, it is a
real bool |
| `test_query.py` | wrapper `check_result` | → `is True` |
| `test_milvus_client_ttl.py` | `hit.get(bool_field)` | **kept as `==
False` + noqa** |
That last one is the reason this rule cannot be bulk-fixed: `.get()`
returns `None` for a missing field, so `not ...` would pass exactly
where the assertion should fail, and `is False` would break on a numpy
bool.
### What ruff.toml still disables
| rule | n | reason |
|---|---|---|
| F841 | ~65 | tests deliberately keep call results; the 13 that were
genuine assertion gaps are fixed above |
| UP031 | 95 | pure style rewrite that catches no bugs, and 27 sites use
`% (...)` tuples where it is not equivalent |
Everything else pyflakes/pycodestyle checks is on, including `F401`,
`F811` and `F821`, so new code cannot introduce dead, duplicate or
undefined names. Tracked in #52116.
### Verification
Not just "ruff is green" — the mechanical passes were checked for
semantic drift:
- **Format pass:** parsed all 307 files before and after and compared
ASTs with string constants normalized away → **zero structural
differences**. The only AST-visible deltas are 146 string-constant
changes, every one whitespace-only (docstring indentation / trailing
whitespace); zero non-whitespace string changes.
- **Import removals:** 142 imports were dropped. For each, checked
whether the bound name still appears anywhere in its file. 115 have no
remaining reference at all. The 27 that still matched textually were
each read by hand and are all comments (`# pdb.set_trace()`, the
commented-out checker dict in `test_load_with_checker.py`), docstring
prose (`"input random collection name"`, `"Tuple[List[Dict]]"`), string
literals (`"Collection ID"`, the `"config"` dict key), a different
binding (`from time import sleep`), or attribute access through another
module (`ct.CaseLabel`, `np.random`). **No real reference was removed.**
- **Python version:** the pyupgrade rewrites emit `datetime.UTC` (3.11+)
and `X | None` (3.10+). `tests/.python-version` is 3.12,
`tests/scripts/python-env.sh` defaults to 3.12, and
`tests/docker/Dockerfile` is `python:3.12-bullseye`.
- **`compileall`** over all 307 files under Python 3.12 passes.
- No external reference to `TestDeleteVector`,
`test_compact_after_delete`, or `add_vector_field` exists anywhere in
the repo (`*.py`, `*.sh`, `*.yaml`, `*.json`).
### Conflict surface
Of the 100 currently-open PRs, 9 touch `tests/*.py` and only **2**
overlap with this change: #51903 (3 of its 5 files) and #51856 (1 file).
The other 7 touch files that were already format-clean. Worth merging
promptly to keep that number small.
`.pre-commit-config.yaml` needs no change — it already runs with
`--config=tests/ruff.toml`, so local hooks and CI stay in sync.
---------
Signed-off-by: xiaofanluan <xf@hjjaq.com>
Co-authored-by: xiaofanluan <xf@hjjaq.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
## What changed
- Add Chaos Mesh `container-kill` schedules for Milvus components and
dependencies.
- Add container-kill coverage for etcd, MinIO, Pulsar, and Kafka targets
to match pod-kill coverage.
- Resolve real pod container names before creating container-kill
resources, which is required for release-prefixed Pulsar containers.
- Add selected-pod container-kill support for etcd leader/follower chaos
scenarios.
## Validation
- `python3 -m py_compile tests/python_client/chaos/test_chaos_apply.py
tests/python_client/chaos/test_chaos_apply_to_determined_pod.py
tests/python_client/utils/util_k8s.py`
- Parsed 17 container-kill YAML/template files with PyYAML.
- `git diff --check`
- Verified 4am Chaos Mesh v2.8.0 CRD supports `container-kill` and
`containerNames`.
- Server-side dry-run passed for static container-kill manifests against
4am Chaos Mesh, excluding Pulsar static YAML because Pulsar container
names are filled dynamically at runtime.
- Server-side dry-run passed for a Pulsar container-kill Schedule after
filling real containerNames.
- Server-side dry-run passed for etcd selected-pod PodChaos after
filling real containerNames.
Fixes#49798
Signed-off-by: yanliang567 <yanliang.qiao@zilliz.com>
/kind improvement
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
- Core invariant: tests' persistence of EventRecords and RequestRecords
must be append-safe under concurrent writers; this PR replaces Parquet
with JSONL and uses per-file locks and explicit buffer flushes to
guarantee atomic, append-safe writes (EventRecords uses event_lock +
append per line; RequestRecords buffers under request_lock and flushes
to file when threshold or on sink()).
- Logic removed/simplified and rationale: DataFrame-based parquet
append/read logic (pyarrow/fastparquet) and implicit parquet buffering
were removed in favor of simple line-oriented JSON writes and explicit
buffer management. The complex Parquet append/merge paths were redundant
because parquet append under concurrent test-writer patterns caused
corruption; JSONL removes the append-mode complexity and the
parquet-specific buffering/serialization code.
- Why no data loss or behavior regression (concrete code paths):
EventRecords.insert writes a complete JSON object per event under
event_lock to /tmp/ci_logs/event_records_*.jsonl and get_records_df
reads every JSON line under the same lock (or returns an empty DataFrame
with the same schema on FileNotFound/Error), preserving all fields
event_name/event_status/event_ts. RequestRecords.insert appends to an
in-memory buffer under request_lock and triggers _flush_buffer() when
len(buffer) >= 100; _flush_buffer() writes each buffered JSON line to
/tmp/ci_logs/request_records_*.jsonl and clears the buffer; sink() calls
_flush_buffer() under request_lock before get_records_df() reads the
file — ensuring all buffered records are persisted before reads. Both
read paths handle FileNotFoundError and exceptions by returning empty
DataFrames with identical column schemas, so external callers see the
same API and no silent record loss.
- Enhancement summary (concrete): Replaces flaky Parquet append/read
with JSONL + explicit locking and deterministic flush semantics,
removing the root cause of parquet append corruption in tests while
keeping the original DataFrame-based analysis consumers unchanged
(get_records_df returns equivalent schemas).
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Signed-off-by: zhuwenxing <wenxing.zhu@zilliz.com>
