Enable HATracker memberlist#7284
Conversation
pkg/ha/ha_tracker.go
Outdated
| EnableStartupSync bool `yaml:"enable_startup_sync"` | ||
|
|
||
| KVStore kv.Config `yaml:"kvstore" doc:"description=Backend storage to use for the ring. Please be aware that memberlist is not supported by the HA tracker since gossip propagation is too slow for HA purposes."` | ||
| KVStore kv.Config `yaml:"kvstore" doc:"description=Backend storage to use for the ring. Please be aware that memberlist support in the HA tracker is currently experimental since gossip propagation is too slow for HA purposes."` |
There was a problem hiding this comment.
We should reword it again. since gossip propagation is too slow for HA purposes is more for why we didn't allow memberlist before
|
|
||
| // Tracker kv store only supports consul and etcd. | ||
| storeAllowedList := []string{"consul", "etcd"} | ||
| if slices.Contains(storeAllowedList, cfg.KVStore.Store) { |
There was a problem hiding this comment.
This PR should only just extend allow list to memberlist instead of allowing all stores
|
@yeya24 |
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
SungJin1212
force-pushed
the
Enable-HATracker-memberlist
branch
from
0a901c6 to
b958c62
Compare
| expectedErr: fmt.Errorf("invalid HATracker KV store type: %s", "memberlist"), | ||
| expectedErr: nil, | ||
| }, | ||
| "should pass with multi kv store": { |
There was a problem hiding this comment.
should we also have an integration test for multi kv store?
There was a problem hiding this comment.
I updated TestHATrackerWithMultiKV to the latest commit
CHANGELOG.md
Outdated
| * Metrics: Renamed `cortex_parquet_queryable_cache_*` to `cortex_parquet_cache_*`. | ||
| * Flags: Renamed `-querier.parquet-queryable-shard-cache-size` to `-querier.parquet-shard-cache-size` and `-querier.parquet-queryable-shard-cache-ttl` to `-querier.parquet-shard-cache-ttl`. | ||
| * Config: Renamed `parquet_queryable_shard_cache_size` to `parquet_shard_cache_size` and `parquet_queryable_shard_cache_ttl` to `parquet_shard_cache_ttl`. | ||
| * [FEATURE] HATracker: Add experimental support for memberlist as a KV store backend. #7284 |
| expectedRingMembers, expectedTombstones) | ||
| } | ||
|
|
||
| func TestHATrackerWithMemberlist(t *testing.T) { |
There was a problem hiding this comment.
This integration test is fine. do you think another test with 2 cortex instances would be valuable? I am thinking sending samples to an instance and ensuring the other instance has the same HA leader
|
|
||
| // RemoveTombstones is a no-op for ReplicaDesc. | ||
| func (d *ReplicaDesc) RemoveTombstones(_ time.Time) (total, removed int) { | ||
| // No-op: HATracker manages tombstones via cleanupOldReplicas |
There was a problem hiding this comment.
I think cleanOldReplicas calls delete that is not implemented for memberlist.
Line 458 in ac39439
can you add a test for cleaning old replicas?
There was a problem hiding this comment.
Oh, we need to implement it. thanks
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
Signed-off-by: SungJin1212 <tjdwls1201@gmail.com>
3 participants
This PR enables memberlist on HATracker.
It includes the implementations of a
mergeableinterface ofReplicaDesc.Which issue(s) this PR fixes:
Fixes #7220
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]