From 164dec170396525720a96d64fa47b73ec6844f90 Mon Sep 17 00:00:00 2001 From: Seebs Date: Mon, 31 Oct 2022 13:02:02 -0500 Subject: [PATCH] stop ignoring degraded/down states We think etcd's tendency to mistakenly mark nodes down may have been addressed. We can't find out without checking for it. The exact pool of methods in methodsDegraded may have bitrotted some; for instance, it didn't have PastQueries or PartitionNodes in it, but it looks like it reasonably should. We rework the Replica1/Replica2 server tests to reflect the intended semantics again. --- api.go | 50 ++++++++++++++----------------- server/server_test.go | 69 ++++++++++++++++++------------------------- 2 files changed, 51 insertions(+), 68 deletions(-) diff --git a/api.go b/api.go index 92a1bc1b7..080c1a8e5 100644 --- a/api.go +++ b/api.go @@ -118,16 +118,9 @@ func (api *API) SetAPIOptions(opts ...apiOption) error { // validAPIMethods specifies the api methods that are valid for each // cluster state. var validAPIMethods = map[disco.ClusterState]map[apiMethod]struct{}{ - disco.ClusterStateNormal: appendMap(methodsCommon, methodsNormal), - // Ideally, this would be just `appendMap(methodsCommon, methodsDegraded)`, - // but in an attempt to reduce the influence that state (determined by etcd) - // has on a node under load, this is set to effectively allow all requests - // in a DEGRADED state. - disco.ClusterStateDegraded: appendMap(methodsCommon, methodsNormal), - // Ideally, this would be just `methodsCommon`, but in an attempt to reduce - // the influence that state (determined by etcd) has on a node under load, - // this is set to effectively allow all requests in a DOWN state. - disco.ClusterStateDown: appendMap(methodsCommon, methodsNormal), + disco.ClusterStateNormal: appendMap(methodsCommon, methodsNormal), + disco.ClusterStateDegraded: appendMap(methodsCommon, methodsDegraded), + disco.ClusterStateDown: methodsCommon, } func appendMap(a, b map[apiMethod]struct{}) map[apiMethod]struct{} { @@ -3304,24 +3297,25 @@ var methodsCommon = map[apiMethod]struct{}{ apiState: {}, } -// var methodsDegraded = map[apiMethod]struct{}{ -// apiExportCSV: {}, -// apiFragmentBlockData: {}, -// apiFragmentBlocks: {}, -// apiField: {}, -// apiIndex: {}, -// apiQuery: {}, -// apiRecalculateCaches: {}, -// apiRemoveNode: {}, -// apiShardNodes: {}, -// apiSchema: {}, -// apiViews: {}, -// apiStartTransaction: {}, -// apiFinishTransaction: {}, -// apiTransactions: {}, -// apiGetTransaction: {}, -// apiActiveQueries: {}, -// } +var methodsDegraded = map[apiMethod]struct{}{ + apiExportCSV: {}, + apiFragmentBlockData: {}, + apiFragmentBlocks: {}, + apiField: {}, + apiIndex: {}, + apiQuery: {}, + apiRecalculateCaches: {}, + apiShardNodes: {}, + apiSchema: {}, + apiViews: {}, + apiStartTransaction: {}, + apiFinishTransaction: {}, + apiTransactions: {}, + apiGetTransaction: {}, + apiActiveQueries: {}, + apiPastQueries: {}, + apiPartitionNodes: {}, +} var methodsNormal = map[apiMethod]struct{}{ apiCreateField: {}, diff --git a/server/server_test.go b/server/server_test.go index e01d645ec..a4c5ba3ed 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -543,11 +543,11 @@ func TestClusteringNodesReplica1(t *testing.T) { Query: fmt.Sprintf("Row(%s=1)", fieldName), } - // check for connection refused... this used to check 'shard - // unavailable', but we since made a change to allow queries to - // nodes which the cluster *thinks* are down, but often are - // actually not. - if _, err := cluster.GetPrimary().API.Query(context.Background(), qry); !strings.Contains(err.Error(), "connection refused") { + _, err := cluster.GetPrimary().API.Query(context.Background(), qry) + if err == nil { + t.Fatalf("expected error from cluster with downed node, didn't get an error") + } + if !strings.Contains(err.Error(), "not allowed in state") { t.Fatalf("got unexpected error querying an incomplete cluster: %v", err) } } @@ -603,7 +603,7 @@ func TestClusteringNodesReplica2(t *testing.T) { } if err := others[0].Close(); err != nil { - t.Fatalf("closing third node: %v", err) + t.Fatalf("closing first node: %v", err) } err = cluster.AwaitPrimaryState(disco.ClusterStateDegraded, 30*time.Second) @@ -611,13 +611,24 @@ func TestClusteringNodesReplica2(t *testing.T) { t.Fatalf("after closing first server: %v", err) } - // We no longer support mutations or schema changes when the cluster is in - // state DEGRADED, so this test doesn't apply anymore. - // - // // confirm that cluster keeps accepting queries if replication > 1 - // if _, err := coord.API.CreateIndex(context.Background(), "anewindex", pilosa.IndexOptions{}); err != nil { - // t.Fatalf("got unexpected error creating index: %v", err) - // } + qry := &pilosa.QueryRequest{ + Index: "idx", + Query: fmt.Sprintf("Row(%s=1)", fieldName), + } + + // With only one node down, we expect a query to still work. + resp, err := coord.API.Query(context.Background(), qry) + if err != nil { + t.Fatalf("unexpected error from degraded cluster: %v", err) + } + if len(resp.Results) == 0 { + t.Fatal("got no results") + } + row, ok := resp.Results[0].(*pilosa.Row) + if !ok { + t.Fatalf("expected a *pilosa.Row, but got %T", resp.Results[0]) + } + require.Equal(t, row.Columns(), cols) // confirm that cluster stops accepting queries if 2 nodes fail and replication == 2 if err := others[1].Close(); err != nil { @@ -629,34 +640,12 @@ func TestClusteringNodesReplica2(t *testing.T) { t.Fatalf("after closing second server: %v", err) } - qry := &pilosa.QueryRequest{ - Index: "idx", - Query: fmt.Sprintf("Row(%s=1)", fieldName), + _, err = coord.API.Query(context.Background(), qry) + if err == nil { + t.Fatalf("expected a cluster with two down nodes to reject query") } - - // Because we no longer block queries when the cluster is in state DOWN, - // there are cases where a DOWN cluster can still respond to a query. In - // that case, we want the test to pass. But if the unavailable node(s) cause - // the query to result in an error, we check that it's the error we expect. - resp, err := coord.API.Query(context.Background(), qry) - if err != nil { - // check for connection refused... this used to check 'shard - // unavailable', but we since made a change to allow queries to - // nodes which the cluster *thinks* are down, but often are - // actually not. - if !strings.Contains(err.Error(), "connection refused") { - t.Fatalf("got unexpected error querying an incomplete cluster: %v", err) - } - } else { - if len(resp.Results) == 0 { - t.Fatal("got no results") - } - - row, ok := resp.Results[0].(*pilosa.Row) - if !ok { - t.Fatalf("expected a *pilosa.Row, but got %T", resp.Results[0]) - } - require.Equal(t, row.Columns(), cols) + if !strings.Contains(err.Error(), "not allowed in state") { + t.Fatalf("expected not allowed in state error, got %q", err.Error()) } }