Fix deletion to use correct user ID listing and add unit test.

Signed-off-by: Steve Simpson <[email protected]>

Author: stevesg

pkg/alertmanager/multitenant.go

@@ -704,22 +704,21 @@ func (am *MultitenantAlertmanager) stopping(_ error) error {
 }
 
 // loadAlertmanagerConfigs Loads (and filters) the alertmanagers configuration from object storage, taking into consideration the sharding strategy.
+// Returns the list of all users with a configuration, and the set of users which should be configured in this instance.
 func (am *MultitenantAlertmanager) loadAlertmanagerConfigs(ctx context.Context) ([]string, map[string]alertspb.AlertConfigDesc, error) {
 	// Find all users with an alertmanager config.
-	userIDs, err := am.store.ListAllUsers(ctx)
+	allUserIDs, err := am.store.ListAllUsers(ctx)
 	if err != nil {
 		return nil, nil, errors.Wrap(err, "failed to list users with alertmanager configuration")
 	}
-	numUsersDiscovered := len(userIDs)
+	numUsersDiscovered := len(allUserIDs)
+	userIDs := make([]string, 0, len(allUserIDs))
 
 	// Filter out users not owned by this shard.
-	for i := 0; i < len(userIDs); {
-		if !am.isUserOwned(userIDs[i]) {
-			userIDs = append(userIDs[:i], userIDs[i+1:]...)
-			continue
+	for _, userID := range allUserIDs {
+		if am.isUserOwned(userID) {
+			userIDs = append(userIDs, userID)
 		}
-
-		i++
 	}
 	numUsersOwned := len(userIDs)
 
@@ -731,7 +730,7 @@ func (am *MultitenantAlertmanager) loadAlertmanagerConfigs(ctx context.Context)
 
 	am.tenantsDiscovered.Set(float64(numUsersDiscovered))
 	am.tenantsOwned.Set(float64(numUsersOwned))
-	return userIDs, configs, nil
+	return allUserIDs, configs, nil
 }
 
 func (am *MultitenantAlertmanager) isUserOwned(userID string) bool {

pkg/alertmanager/multitenant_test.go

@@ -581,23 +581,34 @@ func TestMultitenantAlertmanager_deleteUnusedRemoteUserState(t *testing.T) {
 
 	alertStore := prepareInMemoryAlertStore()
 	ringStore := consul.NewInMemoryClient(ring.GetCodec())
-	reg := prometheus.NewPedanticRegistry()
-	cfg := mockAlertmanagerConfig(t)
 
-	cfg.ShardingRing.ReplicationFactor = 1
-	cfg.ShardingRing.InstanceID = "instance"
-	cfg.ShardingRing.InstanceAddr = "127.0.0.1"
-	cfg.ShardingEnabled = true
+	createInstance := func(i int) *MultitenantAlertmanager {
+		reg := prometheus.NewPedanticRegistry()
+		cfg := mockAlertmanagerConfig(t)
 
-	// Increase state write interval so that state gets written sooner, making test faster.
-	cfg.Persister.Interval = 500 * time.Millisecond
+		cfg.ShardingRing.ReplicationFactor = 1
+		cfg.ShardingRing.InstanceID = fmt.Sprintf("instance-%d", i)
+		cfg.ShardingRing.InstanceAddr = fmt.Sprintf("127.0.0.1-%d", i)
+		cfg.ShardingEnabled = true
 
-	am, err := createMultitenantAlertmanager(cfg, nil, nil, alertStore, ringStore, nil, log.NewNopLogger(), reg)
-	require.NoError(t, err)
-	t.Cleanup(func() {
-		require.NoError(t, services.StopAndAwaitTerminated(ctx, am))
-	})
-	require.NoError(t, services.StartAndAwaitRunning(ctx, am))
+		// Increase state write interval so that state gets written sooner, making test faster.
+		cfg.Persister.Interval = 500 * time.Millisecond
+
+		am, err := createMultitenantAlertmanager(cfg, nil, nil, alertStore, ringStore, nil, log.NewLogfmtLogger(os.Stdout), reg)
+		require.NoError(t, err)
+		t.Cleanup(func() {
+			require.NoError(t, services.StopAndAwaitTerminated(ctx, am))
+		})
+		require.NoError(t, services.StartAndAwaitRunning(ctx, am))
+
+		return am
+	}
+
+	// Create two instances. With replication factor of 1, this means that only one
+	// of the instances will own the user. This tests that an instance does not delete
+	// state for users that are configured, but are owned by other instances.
+	am1 := createInstance(1)
+	am2 := createInstance(2)
 
 	// Configure the users and wait for the state persister to write some state for both.
 	{
@@ -612,7 +623,9 @@ func TestMultitenantAlertmanager_deleteUnusedRemoteUserState(t *testing.T) {
 			Templates: []*alertspb.TemplateDesc{},
 		}))
 
-		err = am.loadAndSyncConfigs(context.Background(), reasonPeriodic)
+		err := am1.loadAndSyncConfigs(context.Background(), reasonPeriodic)
+		require.NoError(t, err)
+		err = am2.loadAndSyncConfigs(context.Background(), reasonPeriodic)
 		require.NoError(t, err)
 
 		require.Eventually(t, func() bool {
@@ -624,7 +637,9 @@ func TestMultitenantAlertmanager_deleteUnusedRemoteUserState(t *testing.T) {
 
 	// Perform another sync to trigger cleanup; this should have no effect.
 	{
-		err = am.loadAndSyncConfigs(context.Background(), reasonPeriodic)
+		err := am1.loadAndSyncConfigs(context.Background(), reasonPeriodic)
+		require.NoError(t, err)
+		err = am2.loadAndSyncConfigs(context.Background(), reasonPeriodic)
 		require.NoError(t, err)
 
 		_, err = alertStore.GetFullState(context.Background(), user1)
@@ -637,7 +652,9 @@ func TestMultitenantAlertmanager_deleteUnusedRemoteUserState(t *testing.T) {
 	{
 		require.NoError(t, alertStore.DeleteAlertConfig(ctx, user1))
 
-		err = am.loadAndSyncConfigs(context.Background(), reasonPeriodic)
+		err := am1.loadAndSyncConfigs(context.Background(), reasonPeriodic)
+		require.NoError(t, err)
+		err = am2.loadAndSyncConfigs(context.Background(), reasonPeriodic)
 		require.NoError(t, err)
 
 		_, err = alertStore.GetFullState(context.Background(), user1)