ui: fix store never removed from /stores page bug (#2339)

* ui: fix store never removed from /stores page bug

We need to update `LastCheck` only if the error is non-nil. That field
is used in the cleanup function to know when to remove the StoreAPI from
the UI. If we always update it, even if an error has happened, that
means that `--store.unhealthy-timeout` is never respected.

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

* query: fix storeset Update() test

Now let's start with a proper state where LastCheck is not 0 at the
beginning and we have 2 active stores, 3 store statuses just like the
original author had intended.

Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>

CHANGELOG.md

@@ -17,6 +17,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel
 - [#2231](https://github.com/thanos-io/thanos/pull/2231) Bucket Web - Sort chunks by thanos.downsample.resolution for better grouping
 - [#2254](https://github.com/thanos-io/thanos/pull/2254) Bucket: Fix metrics registered multiple times in bucket replicate
 - [#2271](https://github.com/thanos-io/thanos/pull/2271) Bucket Web: Fixed Issue #2260 bucket passes null when storage is empty
+- [#2339](https://github.com/thanos-io/thanos/pull/2339) Query: fix a bug where `--store.unhealthy-timeout` was never respected
 
 ### Added
 

pkg/query/storeset.go

@@ -457,10 +457,9 @@ func (s *StoreSet) updateStoreStatus(store *storeRef, err error) {
 	}
 
 	status.LastError = err
-	status.LastCheck = time.Now()
 
 	if err == nil {
-
+		status.LastCheck = time.Now()
 		mint, maxt := store.TimeRange()
 		status.LabelSets = store.LabelSets()
 		status.StoreType = store.StoreType()

pkg/query/storeset_test.go

@@ -174,9 +174,6 @@ func TestStoreSet_Update(t *testing.T) {
 
 	discoveredStoreAddr := stores.StoreAddresses()
 
-	// Start with one not available.
-	stores.CloseOne(discoveredStoreAddr[2])
-
 	// Testing if duplicates can cause weird results.
 	discoveredStoreAddr = append(discoveredStoreAddr, discoveredStoreAddr[0])
 	storeSet := NewStoreSet(nil, nil, func() (specs []StoreSpec) {
@@ -188,6 +185,12 @@ func TestStoreSet_Update(t *testing.T) {
 	storeSet.gRPCInfoCallTimeout = 2 * time.Second
 	defer storeSet.Close()
 
+	// Initial update.
+	storeSet.Update(context.Background())
+
+	// Start with one not available.
+	stores.CloseOne(discoveredStoreAddr[2])
+
 	// Should not matter how many of these we run.
 	storeSet.Update(context.Background())
 	storeSet.Update(context.Background())