From 07a340ba9dedaed0ed9a03b21c6079eb90c0be4f Mon Sep 17 00:00:00 2001 From: nagamocha3000 Date: Wed, 13 Oct 2021 23:11:56 +0300 Subject: [PATCH 1/3] Add test for deadlock on field recreation --- index_test.go | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/index_test.go b/index_test.go index 890d481d1..2837d8409 100644 --- a/index_test.go +++ b/index_test.go @@ -15,10 +15,15 @@ package pilosa_test import ( + "context" + "fmt" + "math/rand" "reflect" "testing" + "time" - "github.com/molecula/featurebase/v2" + pilosa "github.com/molecula/featurebase/v2" + "github.com/molecula/featurebase/v2/disco" "github.com/molecula/featurebase/v2/pql" "github.com/molecula/featurebase/v2/test" "github.com/molecula/featurebase/v2/testhook" @@ -275,3 +280,68 @@ func isNotFoundError(err error) bool { _, ok := root.(pilosa.NotFoundError) return ok } + +func TestIndex_RecreateFieldOnRestart(t *testing.T) { + c := test.MustRunCluster(t, 1) + defer c.Close() + + // create index + indexName := fmt.Sprintf("idx_%d", rand.Uint64()) + holder := c.GetHolder(0) + index, err := holder.CreateIndex(indexName, pilosa.IndexOptions{ + Keys: false, + }) + if err != nil { + t.Fatal(err) + } + defer index.Close() + + // // create field + fieldName := fmt.Sprintf("field_%d", rand.Uint64()) + _, err = c.GetNode(0).API.CreateField(context.Background(), indexName, fieldName, + pilosa.OptFieldTypeDefault()) + if err != nil { + t.Fatal(err) + } + + // set value + _, err = c.GetNode(0).API.Query(context.Background(), &pilosa.QueryRequest{ + Index: indexName, + Query: fmt.Sprintf(`Set(1, %s=1)`, fieldName), + }) + if err != nil { + t.Fatal(err) + } + + // restart node + node := c.GetNode(0) + if err := node.Reopen(); err != nil { + t.Fatal(err) + } + if err := c.AwaitState(disco.ClusterStateNormal, 10*time.Second); err != nil { + t.Fatalf("restarting cluster: %v", err) + } + + // delete field + err = c.GetNode(0).API.DeleteField(context.Background(), indexName, fieldName) + if err != nil { + t.Fatal(err) + } + + // recreate field + errCh := make(chan error) + go func() { + _, err := c.GetNode(0).API.CreateField(context.Background(), indexName, + fieldName, pilosa.OptFieldTypeDefault()) + errCh <- err + }() + select { + case <-time.After(10 * time.Second): + t.Fatalf("recreating field took too long") + case <-errCh: + if err != nil { + t.Fatal(err) + } + } + +} From bce008df26dd9d752b80dbccd0a2907940c67ba3 Mon Sep 17 00:00:00 2001 From: nagamocha3000 Date: Thu, 14 Oct 2021 17:04:09 +0300 Subject: [PATCH 2/3] Fix deadlock on delete then recreate field after node restart --- dbshard.go | 4 ++++ index.go | 3 +++ index_test.go | 12 +++++++++--- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/dbshard.go b/dbshard.go index 606fe75f2..a86cc4ab7 100644 --- a/dbshard.go +++ b/dbshard.go @@ -1228,6 +1228,10 @@ func (vs *FieldView2Shards) String() (r string) { return } +func (vs *FieldView2Shards) removeField(name string) { + delete(vs.m, name) +} + // Note: cannot call this during migration, because // it only ever returns the green shards if we are in blue-green. func (per *DBPerShard) GetFieldView2ShardsMapForIndex(idx *Index) (vs *FieldView2Shards, err error) { diff --git a/index.go b/index.go index 83a4e8e4a..4cfd301b2 100644 --- a/index.go +++ b/index.go @@ -805,6 +805,9 @@ func (i *Index) DeleteField(name string) error { // Remove reference. delete(i.fields, name) + // remove shard metadata for field + i.fieldView2shard.removeField(name) + // Delete the field from etcd as the system of record. if err := i.Schemator.DeleteField(context.TODO(), i.name, name); err != nil { return errors.Wrapf(err, "deleting field from etcd: %s/%s", i.name, name) diff --git a/index_test.go b/index_test.go index 2837d8409..34fedc098 100644 --- a/index_test.go +++ b/index_test.go @@ -18,6 +18,7 @@ import ( "context" "fmt" "math/rand" + "os" "reflect" "testing" "time" @@ -281,6 +282,10 @@ func isNotFoundError(err error) bool { return ok } +// Ensure that after node/cluster restart, deleting and recreating a field +// does not cause a deadlock +// This is a regression test after a customer experienced the same deadlock. +// For details, check out https://molecula.atlassian.net/browse/CORE-919 func TestIndex_RecreateFieldOnRestart(t *testing.T) { c := test.MustRunCluster(t, 1) defer c.Close() @@ -296,7 +301,7 @@ func TestIndex_RecreateFieldOnRestart(t *testing.T) { } defer index.Close() - // // create field + // create field fieldName := fmt.Sprintf("field_%d", rand.Uint64()) _, err = c.GetNode(0).API.CreateField(context.Background(), indexName, fieldName, pilosa.OptFieldTypeDefault()) @@ -337,8 +342,9 @@ func TestIndex_RecreateFieldOnRestart(t *testing.T) { }() select { case <-time.After(10 * time.Second): - t.Fatalf("recreating field took too long") - case <-errCh: + t.Logf("recreating field took too long") + os.Exit(1) + case err := <-errCh: if err != nil { t.Fatal(err) } From c6b708933274b46f3b311be04393e43a32c127e0 Mon Sep 17 00:00:00 2001 From: nagamocha3000 Date: Thu, 14 Oct 2021 17:53:24 +0300 Subject: [PATCH 3/3] Add comment as to why we are using os.Exit instead of panic --- index_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/index_test.go b/index_test.go index 34fedc098..f1a12b55c 100644 --- a/index_test.go +++ b/index_test.go @@ -342,6 +342,13 @@ func TestIndex_RecreateFieldOnRestart(t *testing.T) { }() select { case <-time.After(10 * time.Second): + // We have to use os.Exit here instead of t.Fatal or panic since + // on panic, deferred statements are still ran. Given that + // we have deferred cluster.Close(), it deadlocks on the same + // issue this test is, well, is testing on. + // With os.Exit, the process exits at that point without running the + // deferred actions. This is more of a work-around fix to make the + // test meaningful on timeout. t.Logf("recreating field took too long") os.Exit(1) case err := <-errCh: