fix: implement FileAuditKV.MultiSave instead of returning "not implemented" - #521
Conversation
…ented"
FileAuditKV.MultiSave always returned errors.New("not implemented")
instead of delegating to the wrapped MetaKV client, unlike every other
method on FileAuditKV (Save, Remove, etc.), which log/record the audit
entry and then delegate.
FileAuditKV is the default write wrapper whenever an audit log file is
open (the normal case), so this broke two call sites:
- `repair segment-storage-layout` fails at the metadata-backup step
before any repair logic runs.
- `restore` silently no-ops: its MultiSave error is only printed
inside a goroutine and never propagated, so a restore can report
success while writing zero keys to etcd.
Mirror the Save() pattern: write audit records around the batch, then
delegate to the wrapped client's MultiSave and return its result.
Fixes milvus-io#520
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: lokeshramchand-ctrl <lokeshramchand@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lokeshramchand-ctrl The full list of commands accepted by this bot can be found 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
|
|
/assign @congqixia - Please look into this and let me know if any edits required |
| return errors.Newf("keys and values length mismatch, keys: %d, values: %d", len(keys), len(values)) | ||
| } | ||
| c.writeHeader(models.AuditOpType_OpPut, int32(len(keys))) | ||
| err := c.cli.MultiSave(ctx, keys, values) |
There was a problem hiding this comment.
states/kv/kv_audit.go line:58
Medium ---- This delegation is correct and faithfully mirrors the Save() pattern, but the stated motivation (repair/restore broken by this stub) does not match the client wiring at head. repair segment-storage-layout runs on ComponentRepair, which is constructed with the unwrapped client (states/instance.go:182 passes cli, and states/backup_mock_connect.go:126 builds the client with plain kv.NewEtcdKV), so its MultiSave calls never reach FileAuditKV. On the TiKV connection the "not implemented" error actually comes from txnTiKV.MultiSave (states/kv/kv.go:476), which this PR does not touch, so that repair path stays broken; restore/load-backup likewise writes through the raw etcd client. Could you confirm which path you reproduced the failure on, and whether fixing txnTiKV.MultiSave (or wiring the components through the audit wrapper) belongs in this PR?
There was a problem hiding this comment.
Good catch, and you're right on both counts.
I reproduced this on the restore/load-backup path (restoreEtcdFromBackV2 in states/etcd_restore.go:118, which writes via state.client — the audit-wrapped kv), not on repair. On repair specifically, the actual bug was one step earlier: GetInstanceState in states/instance.go constructed ComponentRepair/ComponentRemove/ComponentShow/ComponentSet with the raw cli instead of the audit-wrapped kv, so those components' writes never reached FileAuditKV at all — the MultiSave fix alone wouldn't have helped them.
I've pushed a follow-up commit (731e2c8) that wires all four components through the audit-wrapped kv, so repair segment-storage-layout now goes through FileAuditKV.MultiSave as intended. txnTiKV.MultiSave (states/kv/kv.go:476) is a separate, still-unimplemented method on a different backend — happy to file that as its own follow-up issue rather than scope-creep this PR, unless you'd prefer it bundled here too.
| require.Len(t, fake.multiSaveCalls, 1) | ||
| assert.Equal(t, keys, fake.multiSaveCalls[0][0]) | ||
| assert.Equal(t, values, fake.multiSaveCalls[0][1]) | ||
| assert.Equal(t, "v1", fake.data["k1"]) |
There was a problem hiding this comment.
states/kv/kv_audit_test.go line:108
Low ---- The three subtests verify delegation, error propagation, and length rejection, but none ever reads the audit file back, so the actual purpose of FileAuditKV - writing the OpPut/OpPutBefore/OpPutAfter headers plus the key/value records - is untested. A regression in the file-writing path (wrong headers, wrong entry counts, dropped records) would pass all three tests. Consider a case that replays the temp file by parsing the length-prefixed AuditHeader records and asserts header counts and key/value contents, so the on-disk format is locked in.
There was a problem hiding this comment.
Agreed, added in 731e2c8. Two new subtests in states/kv/kv_audit_test.go replay the raw file (length-prefixed records, decoded via proto.Unmarshal into AuditHeader / mvccpb.KeyValue) and assert on it directly:
writes OpPut/OpPutBefore/OpPutAfter headers and key/value records: on success, asserts the exact 5-record sequence (OpPut header with EntriesNum=2, OpPutBefore header, the two key/value records with correct key/value bytes, OpPutAfter header).writes only OpPutAfter header when underlying save fails: on failure, asserts only the OpPut + OpPutAfter headers are written and no key/value records leak through.
That locks in the on-disk format so a regression in header ordering/counts or dropped records would now fail the test.
|
@lokeshramchand-ctrl Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
ComponentRepair, ComponentRemove, ComponentShow, and ComponentSet were constructed with the raw etcd client instead of the FileAuditKV wrapper, so their writes (e.g. repair segment-storage-layout) bypassed audit logging entirely and never reached the previously-fixed MultiSave path. Also adds a regression test that replays the on-disk audit log format (length-prefixed AuditHeader + key/value records) to lock in the OpPut/OpPutBefore/OpPutAfter sequence, per review feedback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: lokeshramchand-ctrl <lokeshramchand@gmail.com>
731e2c8 to
907cbfa
Compare
3 participants
Summary
FileAuditKV.MultiSavewas a stub that always returnederrors.New("not implemented")instead of delegating to the wrappedMetaKVclient, unlike every other method onFileAuditKV(Save,Remove, etc.), which write an audit record and then delegate.FileAuditKVis the default write wrapper whenever an audit log file is open (the normal case), so this broke two live call sites:repair segment-storage-layoutfails 100% of the time at the metadata-backup step, before any repair logic runs.restoresilently no-ops: itsMultiSaveerror is only printed inside a goroutine and never propagated, so a restore can report success while writing zero keys to etcd.Save()pattern: write audit records for the batch, delegate toc.cli.MultiSave, and return its result. Also guards against mismatchedkeys/valuesslice lengths.Fixes #520
Test plan
go build ./states/kv/...go vet ./states/kv/...states/kv/kv_audit_test.gocovering: delegation + persistence, error propagation from the wrapped client, and mismatched key/value length rejection — all pass