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

@@ -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:          {},

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())
 	}
 }