Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion states/kv/kv_audit.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,19 @@ func (c *FileAuditKV) Save(ctx context.Context, key, value string) error {
}

func (c *FileAuditKV) MultiSave(ctx context.Context, keys, values []string) error {
return errors.New("not implemented")
if len(keys) != len(values) {
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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is confirmed fixed.

if err == nil {
c.writeHeader(models.AuditOpType_OpPutBefore, int32(len(keys)))
for i, key := range keys {
c.writeKeyValue(key, values[i])
}
}
c.writeHeader(models.AuditOpType_OpPutAfter, int32(len(keys)))
return err
}

func (c *FileAuditKV) Remove(ctx context.Context, key string) error {
Expand Down
130 changes: 130 additions & 0 deletions states/kv/kv_audit_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
package kv

import (
"bufio"
"context"
"os"
"testing"

"github.com/cockroachdb/errors"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.etcd.io/etcd/api/v3/mvccpb"
)

// fakeMetaKV is a minimal in-memory MetaKV used to verify FileAuditKV
// delegates to the wrapped client instead of swallowing calls.
type fakeMetaKV struct {
data map[string]string

multiSaveErr error
multiSaveCalls [][2][]string
}

func newFakeMetaKV() *fakeMetaKV {
return &fakeMetaKV{data: map[string]string{}}
}

func (f *fakeMetaKV) Load(ctx context.Context, key string, opts ...LoadOption) (string, error) {
v, ok := f.data[key]
if !ok {
return "", errors.New("key not found")
}
return v, nil
}

func (f *fakeMetaKV) LoadWithPrefix(ctx context.Context, key string, opts ...LoadOption) ([]string, []string, error) {
return nil, nil, nil
}

func (f *fakeMetaKV) Save(ctx context.Context, key, value string) error {
f.data[key] = value
return nil
}

func (f *fakeMetaKV) MultiSave(ctx context.Context, keys, values []string) error {
f.multiSaveCalls = append(f.multiSaveCalls, [2][]string{keys, values})
if f.multiSaveErr != nil {
return f.multiSaveErr
}
for i, key := range keys {
f.data[key] = values[i]
}
return nil
}

func (f *fakeMetaKV) Remove(ctx context.Context, key string) error {
delete(f.data, key)
return nil
}

func (f *fakeMetaKV) RemoveWithPrefix(ctx context.Context, key string) error {
return nil
}

func (f *fakeMetaKV) removeWithPrevKV(ctx context.Context, key string) (*mvccpb.KeyValue, error) {
return nil, nil
}

func (f *fakeMetaKV) removeWithPrefixAndPrevKV(ctx context.Context, prefix string) ([]*mvccpb.KeyValue, error) {
return nil, nil
}

func (f *fakeMetaKV) GetAllRootPath(ctx context.Context) ([]string, error) {
return nil, nil
}

func (f *fakeMetaKV) BackupKV(base, prefix string, w *bufio.Writer, ignoreRevision bool, batchSize int64) error {
return nil
}

func (f *fakeMetaKV) WalkWithPrefix(ctx context.Context, prefix string, paginationSize int, fn func([]byte, []byte) error) error {
return nil
}

func (f *fakeMetaKV) Close() {}

func newTestFileAuditKV(t *testing.T, cli MetaKV) *FileAuditKV {
t.Helper()
f, err := os.CreateTemp(t.TempDir(), "audit-*.log")
require.NoError(t, err)
t.Cleanup(func() { f.Close() })
return NewFileAuditKV(cli, f)
}

func TestFileAuditKVMultiSave(t *testing.T) {
t.Run("delegates and persists values", func(t *testing.T) {
fake := newFakeMetaKV()
audit := newTestFileAuditKV(t, fake)

keys := []string{"k1", "k2"}
values := []string{"v1", "v2"}
err := audit.MultiSave(context.TODO(), keys, values)
require.NoError(t, err)

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"])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is confirmed fixed.

assert.Equal(t, "v2", fake.data["k2"])
})

t.Run("propagates underlying error", func(t *testing.T) {
fake := newFakeMetaKV()
fake.multiSaveErr = errors.New("injected failure")
audit := newTestFileAuditKV(t, fake)

err := audit.MultiSave(context.TODO(), []string{"k1"}, []string{"v1"})
assert.ErrorIs(t, err, fake.multiSaveErr)
assert.Empty(t, fake.data)
})

t.Run("rejects mismatched keys and values", func(t *testing.T) {
fake := newFakeMetaKV()
audit := newTestFileAuditKV(t, fake)

err := audit.MultiSave(context.TODO(), []string{"k1", "k2"}, []string{"v1"})
assert.Error(t, err)
assert.Empty(t, fake.multiSaveCalls)
})
}