Remote Alertmanager: Remove comparison before sending the state (#104930)

* Remote Alertmanager: Remove comparison before sending the state

* fix test

* fix test

pkg/services/ngalert/notifier/multiorg_alertmanager_remote_test.go

@@ -137,12 +137,12 @@ func TestMultiorgAlertmanager_RemoteSecondaryMode(t *testing.T) {
 		require.Equal(t, fakeAM.config, lastConfig)
 		require.Equal(t, fakeAM.state, lastState)
 
-		// Syncing after the sync interval elapses should update both config and state.
+		// Syncing after the sync interval elapses should update the config, not the state.
 		require.Eventually(t, func() bool {
 			require.NoError(t, moa.LoadAndSyncAlertmanagersForOrgs(ctx))
-			return fakeAM.config != lastConfig && fakeAM.state != lastState
+			return fakeAM.config != lastConfig && fakeAM.state == lastState
 		}, 15*time.Second, 300*time.Millisecond)
-		lastConfig, lastState = fakeAM.config, fakeAM.state
+		lastConfig = fakeAM.config
 	}
 
 	// It should send config and state on shutdown.
@@ -154,13 +154,12 @@ func TestMultiorgAlertmanager_RemoteSecondaryMode(t *testing.T) {
 			OrgID:                     1,
 			LastApplied:               time.Now().Unix(),
 		}))
-		require.NoError(t, kvStore.Set(ctx, 1, "alertmanager", notifier.SilencesFilename, "lwEKhgEKAWESFxIJYWxlcnRuYW1lGgp0ZXN0X2FsZXJ0EiMSDmdyYWZhbmFfZm9sZGVyGhF0ZXN0X2FsZXJ0X2ZvbGRlchoMCPuEkbAGEK3AhM8CIgwIi6GRsAYQrcCEzwIqCwiAkrjDmP7///8BQgxHcmFmYW5hIFRlc3RKDFRlc3QgU2lsZW5jZRIMCIuhkbAGEK3AhM8ClwEKhgEKAWISFxIJYWxlcnRuYW1lGgp0ZXN0X2FsZXJ0EiMSDmdyYWZhbmFfZm9sZGVyGhF0ZXN0X2FsZXJ0X2ZvbGRlchoMCPuEkbAGEK3AhM8CIgwIi6GRsAYQrcCEzwIqCwiAkrjDmP7///8BQgxHcmFmYW5hIFRlc3RKDFRlc3QgU2lsZW5jZRIMCIuhkbAGEK3AhM8C"))
-		require.NoError(t, kvStore.Set(ctx, 1, "alertmanager", notifier.NotificationLogFilename, "OAopCgZncm91cEESEgoJcmVjZWl2ZXJBEgV0ZXN0MyoLCNmEkbAGEOzO0BUSCwjpoJGwBhDsztAVOAopCgZncm91cEISEgoJcmVjZWl2ZXJCEgV0ZXN0MyoLCNmEkbAGEOzO0BUSCwjpoJGwBhDsztAV"))
 
 		// Both state and config should be updated when shutting the Alertmanager down.
 		moa.StopAndWait()
-		require.NotEqual(t, fakeAM.config, lastConfig)
-		require.NotEqual(t, fakeAM.state, lastState)
+		require.Eventually(t, func() bool {
+			return fakeAM.config != lastConfig && fakeAM.state != lastState
+		}, 15*time.Second, 300*time.Millisecond)
 	}
 }
 

pkg/services/ngalert/remote/alertmanager.go

@@ -216,7 +216,7 @@ func (am *Alertmanager) ApplyConfig(ctx context.Context, config *models.AlertCon
 		}
 		am.log.Debug("Completed readiness check for remote Alertmanager, starting state upload", "url", am.url)
 
-		if err := am.CompareAndSendState(ctx); err != nil {
+		if err := am.SendState(ctx); err != nil {
 			return fmt.Errorf("unable to upload the state to the remote Alertmanager: %w", err)
 		}
 		am.log.Debug("Completed state upload to remote Alertmanager", "url", am.url)
@@ -327,22 +327,22 @@ func (am *Alertmanager) sendConfiguration(ctx context.Context, decrypted *apimod
 	return nil
 }
 
-// CompareAndSendState gets the Alertmanager's internal state and compares it with the remote Alertmanager's one.
-// If the states are different, it updates the remote Alertmanager's state with that of the internal Alertmanager.
-func (am *Alertmanager) CompareAndSendState(ctx context.Context) error {
+// SendState gets the Alertmanager's internal state and sends it to the remote Alertmanager.
+func (am *Alertmanager) SendState(ctx context.Context) error {
+	am.metrics.StateSyncsTotal.Inc()
+
 	state, err := am.getFullState(ctx)
 	if err != nil {
+		am.metrics.StateSyncErrorsTotal.Inc()
 		return err
 	}
 
-	if am.shouldSendState(ctx, state) {
-		am.metrics.StateSyncsTotal.Inc()
-		if err := am.mimirClient.CreateGrafanaAlertmanagerState(ctx, state); err != nil {
-			am.metrics.StateSyncErrorsTotal.Inc()
-			return err
-		}
-		am.metrics.LastStateSync.SetToCurrentTime()
+	if err := am.mimirClient.CreateGrafanaAlertmanagerState(ctx, state); err != nil {
+		am.metrics.StateSyncErrorsTotal.Inc()
+		return err
 	}
+
+	am.metrics.LastStateSync.SetToCurrentTime()
 	return nil
 }
 
@@ -677,16 +677,3 @@ func (am *Alertmanager) shouldSendConfig(ctx context.Context, hash [16]byte) boo
 	}
 	return md5.Sum(rawRemote) != hash
 }
-
-// shouldSendState compares the remote Alertmanager state with our local one.
-// It returns true if the states are different.
-func (am *Alertmanager) shouldSendState(ctx context.Context, state string) bool {
-	rs, err := am.mimirClient.GetGrafanaAlertmanagerState(ctx)
-	if err != nil {
-		// Log the error and return true so we try to upload our state anyway.
-		am.log.Warn("Unable to get the remote Alertmanager state for comparison, sending the state without comparing", "err", err)
-		return true
-	}
-
-	return rs.State != state
-}

pkg/services/ngalert/remote/forked_alertmanager_test.go

@@ -44,13 +44,12 @@ func TestForkedAlertmanager_ModeRemoteSecondary(t *testing.T) {
 
 		{
 			// If the remote Alertmanager is ready and the sync interval has elapsed,
-			// the forked Alertmanager should sync state and configuration on the remote Alertmanager
+			// the forked Alertmanager should sync the configuration on the remote Alertmanager
 			// and call ApplyConfig only on the internal Alertmanager.
 			internal, remote, forked := genTestAlertmanagersWithSyncInterval(tt, modeRemoteSecondary, 0)
 			internal.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Twice()
 			remote.EXPECT().Ready().Return(true).Twice()
 			remote.EXPECT().CompareAndSendConfiguration(ctx, mock.Anything).Return(nil).Twice()
-			remote.EXPECT().CompareAndSendState(ctx).Return(nil).Twice()
 			require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{}))
 			require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{}))
 		}
@@ -58,7 +57,7 @@ func TestForkedAlertmanager_ModeRemoteSecondary(t *testing.T) {
 		{
 			// An error in the remote Alertmanager should not be returned,
 			// but it should result in the forked Alertmanager trying to sync
-			// configuration and state in the next call to ApplyConfig, regardless of the sync interval.
+			// the configuration in the next call to ApplyConfig, regardless of the sync interval.
 			internal, remote, forked := genTestAlertmanagersWithSyncInterval(tt, modeRemoteSecondary, 10*time.Minute)
 			internal.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Twice()
 			remote.EXPECT().Ready().Return(false).Twice()
@@ -71,15 +70,6 @@ func TestForkedAlertmanager_ModeRemoteSecondary(t *testing.T) {
 			internal.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Twice()
 			remote.EXPECT().Ready().Return(true).Twice()
 			remote.EXPECT().CompareAndSendConfiguration(ctx, mock.Anything).Return(expErr).Twice()
-			remote.EXPECT().CompareAndSendState(ctx).Return(nil).Twice()
-			require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{}))
-			require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{}))
-
-			internal, remote, forked = genTestAlertmanagersWithSyncInterval(tt, modeRemoteSecondary, 10*time.Minute)
-			internal.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Twice()
-			remote.EXPECT().Ready().Return(true).Twice()
-			remote.EXPECT().CompareAndSendConfiguration(ctx, mock.Anything).Return(nil).Twice()
-			remote.EXPECT().CompareAndSendState(ctx).Return(expErr).Twice()
 			require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{}))
 			require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{}))
 		}
@@ -325,7 +315,7 @@ func TestForkedAlertmanager_ModeRemoteSecondary(t *testing.T) {
 			internal.EXPECT().StopAndWait().Once()
 			remote.EXPECT().StopAndWait().Once()
 			remote.EXPECT().CompareAndSendConfiguration(mock.Anything, mock.Anything).Return(nil).Once()
-			remote.EXPECT().CompareAndSendState(mock.Anything).Return(nil).Once()
+			remote.EXPECT().SendState(mock.Anything).Return(nil).Once()
 			forked.StopAndWait()
 		}
 
@@ -336,7 +326,7 @@ func TestForkedAlertmanager_ModeRemoteSecondary(t *testing.T) {
 			internal.EXPECT().StopAndWait().Once()
 			remote.EXPECT().StopAndWait().Once()
 			remote.EXPECT().CompareAndSendConfiguration(mock.Anything, mock.Anything).Return(expErr).Once()
-			remote.EXPECT().CompareAndSendState(mock.Anything).Return(expErr).Once()
+			remote.EXPECT().SendState(mock.Anything).Return(expErr).Once()
 			forked.StopAndWait()
 		}
 
@@ -350,7 +340,7 @@ func TestForkedAlertmanager_ModeRemoteSecondary(t *testing.T) {
 
 			internal.EXPECT().StopAndWait().Once()
 			remote.EXPECT().StopAndWait().Once()
-			remote.EXPECT().CompareAndSendState(mock.Anything).Return(expErr).Once()
+			remote.EXPECT().SendState(mock.Anything).Return(expErr).Once()
 			forked.StopAndWait()
 		}
 	})

pkg/services/ngalert/remote/mock/remoteAlertmanager.go

@@ -125,12 +125,12 @@ func (_c *RemoteAlertmanagerMock_CompareAndSendConfiguration_Call) RunAndReturn(
 	return _c
 }
 
-// CompareAndSendState provides a mock function with given fields: _a0
-func (_m *RemoteAlertmanagerMock) CompareAndSendState(_a0 context.Context) error {
+// SendState provides a mock function with given fields: _a0
+func (_m *RemoteAlertmanagerMock) SendState(_a0 context.Context) error {
 	ret := _m.Called(_a0)
 
 	if len(ret) == 0 {
-		panic("no return value specified for CompareAndSendState")
+		panic("no return value specified for SendState")
 	}
 
 	var r0 error
@@ -143,30 +143,30 @@ func (_m *RemoteAlertmanagerMock) CompareAndSendState(_a0 context.Context) error
 	return r0
 }
 
-// RemoteAlertmanagerMock_CompareAndSendState_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'CompareAndSendState'
-type RemoteAlertmanagerMock_CompareAndSendState_Call struct {
+// RemoteAlertmanagerMock_SendState_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'SendState'
+type RemoteAlertmanagerMock_SendState_Call struct {
 	*mock.Call
 }
 
-// CompareAndSendState is a helper method to define mock.On call
+// SendState is a helper method to define mock.On call
 //   - _a0 context.Context
-func (_e *RemoteAlertmanagerMock_Expecter) CompareAndSendState(_a0 interface{}) *RemoteAlertmanagerMock_CompareAndSendState_Call {
-	return &RemoteAlertmanagerMock_CompareAndSendState_Call{Call: _e.mock.On("CompareAndSendState", _a0)}
+func (_e *RemoteAlertmanagerMock_Expecter) SendState(_a0 interface{}) *RemoteAlertmanagerMock_SendState_Call {
+	return &RemoteAlertmanagerMock_SendState_Call{Call: _e.mock.On("SendState", _a0)}
 }
 
-func (_c *RemoteAlertmanagerMock_CompareAndSendState_Call) Run(run func(_a0 context.Context)) *RemoteAlertmanagerMock_CompareAndSendState_Call {
+func (_c *RemoteAlertmanagerMock_SendState_Call) Run(run func(_a0 context.Context)) *RemoteAlertmanagerMock_SendState_Call {
 	_c.Call.Run(func(args mock.Arguments) {
 		run(args[0].(context.Context))
 	})
 	return _c
 }
 
-func (_c *RemoteAlertmanagerMock_CompareAndSendState_Call) Return(_a0 error) *RemoteAlertmanagerMock_CompareAndSendState_Call {
+func (_c *RemoteAlertmanagerMock_SendState_Call) Return(_a0 error) *RemoteAlertmanagerMock_SendState_Call {
 	_c.Call.Return(_a0)
 	return _c
 }
 
-func (_c *RemoteAlertmanagerMock_CompareAndSendState_Call) RunAndReturn(run func(context.Context) error) *RemoteAlertmanagerMock_CompareAndSendState_Call {
+func (_c *RemoteAlertmanagerMock_SendState_Call) RunAndReturn(run func(context.Context) error) *RemoteAlertmanagerMock_SendState_Call {
 	_c.Call.Return(run)
 	return _c
 }

pkg/services/ngalert/remote/remote_secondary_forked_alertmanager.go

@@ -22,7 +22,7 @@ type configStore interface {
 type remoteAlertmanager interface {
 	notifier.Alertmanager
 	CompareAndSendConfiguration(context.Context, *models.AlertConfiguration) error
-	CompareAndSendState(context.Context) error
+	SendState(context.Context) error
 }
 
 type RemoteSecondaryForkedAlertmanager struct {
@@ -43,7 +43,7 @@ type RemoteSecondaryConfig struct {
 	Store  configStore
 
 	// SyncInterval determines how often we should attempt to synchronize
-	// state and configuration on the external Alertmanager.
+	// the configuration on the remote Alertmanager.
 	SyncInterval time.Duration
 }
 
@@ -89,21 +89,13 @@ func (fam *RemoteSecondaryForkedAlertmanager) ApplyConfig(ctx context.Context, c
 
 		// If the Alertmanager was marked as ready but the sync interval has elapsed, sync the Alertmanagers.
 		if time.Since(fam.lastSync) >= fam.syncInterval {
-			fam.log.Debug("Syncing configuration and state with the remote Alertmanager", "lastSync", fam.lastSync)
-			cfgErr := fam.remote.CompareAndSendConfiguration(ctx, config)
-			if cfgErr != nil {
-				fam.log.Error("Unable to upload the configuration to the remote Alertmanager", "err", cfgErr)
-			}
-
-			stateErr := fam.remote.CompareAndSendState(ctx)
-			if stateErr != nil {
-				fam.log.Error("Unable to upload the state to the remote Alertmanager", "err", stateErr)
-			}
-			fam.log.Debug("Finished syncing configuration and state with the remote Alertmanager")
-
-			if cfgErr == nil && stateErr == nil {
+			fam.log.Debug("Syncing configuration with the remote Alertmanager", "lastSync", fam.lastSync)
+			if err := fam.remote.CompareAndSendConfiguration(ctx, config); err != nil {
+				fam.log.Error("Unable to upload the configuration to the remote Alertmanager", "err", err)
+			} else {
 				fam.lastSync = time.Now()
 			}
+			fam.log.Debug("Finished syncing configuration with the remote Alertmanager")
 		}
 	}()
 
@@ -180,7 +172,7 @@ func (fam *RemoteSecondaryForkedAlertmanager) StopAndWait() {
 	// Send config and state to the remote Alertmanager.
 	// Using context.TODO() here as we think we want to allow this operation to finish regardless of time.
 	ctx := context.TODO()
-	if err := fam.remote.CompareAndSendState(ctx); err != nil {
+	if err := fam.remote.SendState(ctx); err != nil {
 		fam.log.Error("Error sending state to the remote Alertmanager while stopping", "err", err)
 	}