From 432119458b27e3ee8d5190f657e9528595b7799b Mon Sep 17 00:00:00 2001 From: Rohit Nayak <57520317+rohit-nayak-ps@users.noreply.github.com> Date: Mon, 27 May 2024 10:58:04 +0200 Subject: [PATCH] VReplication: refactor denied tables unit test, add couple more tests (#15995) Signed-off-by: Rohit Nayak --- go/vt/topo/shard_test.go | 183 ++++++++++++++++++++++++++++----------- 1 file changed, 131 insertions(+), 52 deletions(-) diff --git a/go/vt/topo/shard_test.go b/go/vt/topo/shard_test.go index ccef80944a9..9afc8d0ea78 100644 --- a/go/vt/topo/shard_test.go +++ b/go/vt/topo/shard_test.go @@ -140,69 +140,148 @@ func TestUpdateSourcePrimaryDeniedTables(t *testing.T) { func TestUpdateSourceDeniedTables(t *testing.T) { si := NewShardInfo("ks", "sh", &topodatapb.Shard{}, nil) - - // check we enforce the keyspace lock ctx := context.Background() - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, nil, false, nil); err == nil || err.Error() != "keyspace ks is not locked (no locksInfo)" { - t.Fatalf("unlocked keyspace produced wrong error: %v", err) + ctxWithLock := lockedKeyspaceContext("ks") + + type testCase struct { + name string + ctx context.Context + tabletType topodatapb.TabletType + cells []string + remove bool + tables []string + + wantError string + wantTabletControl *topodatapb.Shard_TabletControl } - ctx = lockedKeyspaceContext("ks") - // add one cell - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first"}, false, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ + // These tests update the state of the shard tablet controls, so subsequent tests + // depend on the cumulative state from the previous tests. + testCases := []testCase{ { - TabletType: topodatapb.TabletType_RDONLY, - Cells: []string{"first"}, - DeniedTables: []string{"t1", "t2"}, - }, - }) { - t.Fatalf("one cell add failed: %v", si) - } + name: "enforce keyspace lock", + ctx: ctx, + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first"}, - // remove that cell, going back - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first"}, true, nil); err != nil || len(si.TabletControls) != 0 { - t.Fatalf("going back should have remove the record: %v", si) - } - - // re-add a cell, then another with different table list to - // make sure it fails - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first"}, false, []string{"t1", "t2"}); err != nil { - t.Fatalf("one cell add failed: %v", si) - } - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, false, []string{"t2", "t3"}); err == nil || err.Error() != "trying to use two different sets of denied tables for shard ks/sh: [t1 t2] and [t2 t3]" { - t.Fatalf("different table list should fail: %v", err) - } - // add another cell, see the list grow - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, false, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ + wantError: "keyspace ks is not locked (no locksInfo)", + }, { - TabletType: topodatapb.TabletType_RDONLY, - Cells: []string{"first", "second"}, - DeniedTables: []string{"t1", "t2"}, + name: "add one cell", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first"}, + tables: []string{"t1", "t2"}, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first"}, + DeniedTables: []string{"t1", "t2"}, + }, }, - }) { - t.Fatalf("second cell add failed: %v", si) - } - - // add all cells, see the list grow to all - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first", "second", "third"}, false, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ { - TabletType: topodatapb.TabletType_RDONLY, - Cells: []string{"first", "second", "third"}, - DeniedTables: []string{"t1", "t2"}, + name: "remove the only cell", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first"}, + remove: true, + }, + { + name: "re-add cell", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first"}, + tables: []string{"t1", "t2"}, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first"}, + DeniedTables: []string{"t1", "t2"}, + }, }, - }) { - t.Fatalf("all cells add failed: %v", si) - } - - // remove one cell from the full list - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, true, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ { - TabletType: topodatapb.TabletType_RDONLY, - Cells: []string{"first", "third"}, - DeniedTables: []string{"t1", "t2"}, + name: "re-add existing cell, different tables, should fail", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first"}, + tables: []string{"t3"}, + wantError: "trying to use two different sets of denied tables for shard", }, - }) { - t.Fatalf("one cell removal from all failed: %v", si) + { + name: "add all cells, see cell list grow to all", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first", "second", "third"}, + tables: []string{"t1", "t2"}, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first", "second", "third"}, + DeniedTables: []string{"t1", "t2"}, + }, + }, + { + name: "remove one cell", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"second"}, + remove: true, + tables: []string{"t1", "t2"}, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first", "third"}, + DeniedTables: []string{"t1", "t2"}, + }, + }, + { + name: "add replica tablet type", + tabletType: topodatapb.TabletType_REPLICA, + cells: []string{"first"}, + tables: []string{"t1", "t2"}, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_REPLICA, + Cells: []string{"first"}, + DeniedTables: []string{"t1", "t2"}, + }, + }, + { + name: "confirm rdonly still stays the same, after replica was added", + tabletType: topodatapb.TabletType_RDONLY, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first", "third"}, + DeniedTables: []string{"t1", "t2"}, + }, + }, + { + name: "remove rdonly entry", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first", "third"}, + remove: true, + tables: []string{"t1", "t2"}, + }, + { + name: "remove replica entry", + tabletType: topodatapb.TabletType_REPLICA, + cells: []string{"first", "third"}, + remove: true, + tables: []string{"t1", "t2"}, + }, + } + + for _, tcase := range testCases { + t.Run(tcase.name, func(t *testing.T) { + if tcase.ctx == nil { + tcase.ctx = ctxWithLock + } + var err error + if tcase.tables != nil || tcase.cells != nil { + err = si.UpdateDeniedTables(tcase.ctx, tcase.tabletType, tcase.cells, tcase.remove, tcase.tables) + } + if tcase.wantError != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tcase.wantError) + return + } + require.NoError(t, err) + if tcase.wantTabletControl == nil { + require.Nil(t, si.GetTabletControl(tcase.tabletType)) + } else { + require.EqualValuesf(t, tcase.wantTabletControl, si.GetTabletControl(tcase.tabletType), + "want: %v, got: %v", tcase.wantTabletControl, si.GetTabletControl(tcase.tabletType)) + } + }) } }