From 8b7c2a459bd4a1b00709c742755636cd1b4ab7ea Mon Sep 17 00:00:00 2001 From: Santiago Date: Tue, 23 Apr 2024 14:36:40 +0200 Subject: [PATCH] Alerting: Implement SaveAndApplyDefaultConfig in the forked Alertmanager (remote primary mode) (#85668) * Alerting: Implement SaveAndApplyDefaultConfig in the forked Alertmanager (remote primary) * log the error for the internal AM instead of returning it --- .../remote/forked_alertmanager_test.go | 53 ++++++++++++------- .../remote_primary_forked_alertmanager.go | 9 +++- 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/pkg/services/ngalert/remote/forked_alertmanager_test.go b/pkg/services/ngalert/remote/forked_alertmanager_test.go index ca829cb068e..278d38efaa0 100644 --- a/pkg/services/ngalert/remote/forked_alertmanager_test.go +++ b/pkg/services/ngalert/remote/forked_alertmanager_test.go @@ -107,7 +107,7 @@ func TestForkedAlertmanager_ModeRemoteSecondary(t *testing.T) { }) t.Run("SaveAndApplyDefaultConfig", func(tt *testing.T) { - // SaveAndApplyDefaultConfig should only be called on the remote Alertmanager. + // SaveAndApplyDefaultConfig should only be called on the internal Alertmanager. // State and configuration are updated on an interval. internal, _, forked := genTestAlertmanagers(tt, modeRemoteSecondary) internal.EXPECT().SaveAndApplyDefaultConfig(ctx).Return(nil).Once() @@ -368,25 +368,42 @@ func TestForkedAlertmanager_ModeRemotePrimary(t *testing.T) { expErr := errors.New("test error") t.Run("ApplyConfig", func(tt *testing.T) { - { - // If the remote Alertmanager is not ready, ApplyConfig should be called on both Alertmanagers, - // first on the remote, then on the internal. - internal, remote, forked := genTestAlertmanagers(tt, modeRemotePrimary) - remoteCall := remote.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Once() - internal.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Once().NotBefore(remoteCall) - require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{})) + // If the remote Alertmanager is not ready, ApplyConfig should be called on both Alertmanagers, + // first on the remote, then on the internal. + internal, remote, forked := genTestAlertmanagers(tt, modeRemotePrimary) + remoteCall := remote.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Once() + internal.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Once().NotBefore(remoteCall) + require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{})) - // An error in the remote Alertmanager should be returned. - _, remote, forked = genTestAlertmanagers(tt, modeRemotePrimary) - remote.EXPECT().ApplyConfig(ctx, mock.Anything).Return(expErr).Once() - require.ErrorIs(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{}), expErr) + // An error in the remote Alertmanager should be returned. + _, remote, forked = genTestAlertmanagers(tt, modeRemotePrimary) + remote.EXPECT().ApplyConfig(ctx, mock.Anything).Return(expErr).Once() + require.ErrorIs(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{}), expErr) - // An error in the internal Alertmanager should not be returned. - internal, remote, forked = genTestAlertmanagers(tt, modeRemotePrimary) - remote.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Once() - internal.EXPECT().ApplyConfig(ctx, mock.Anything).Return(expErr).Once() - require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{})) - } + // An error in the internal Alertmanager should not be returned. + internal, remote, forked = genTestAlertmanagers(tt, modeRemotePrimary) + remote.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Once() + internal.EXPECT().ApplyConfig(ctx, mock.Anything).Return(expErr).Once() + require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{})) + }) + + t.Run("SaveAndApplyDefaultConfig", func(tt *testing.T) { + // SaveAndApplyDefaultConfig should be called on both Alertmanagers. + internal, remote, forked := genTestAlertmanagers(tt, modeRemotePrimary) + remote.EXPECT().SaveAndApplyDefaultConfig(ctx).Return(nil).Once() + internal.EXPECT().SaveAndApplyDefaultConfig(ctx).Return(nil).Once() + require.NoError(tt, forked.SaveAndApplyDefaultConfig(ctx)) + + // An error in the remote Alertmanager should be returned. + _, remote, forked = genTestAlertmanagers(tt, modeRemotePrimary) + remote.EXPECT().SaveAndApplyDefaultConfig(ctx).Return(expErr).Once() + require.ErrorIs(tt, forked.SaveAndApplyDefaultConfig(ctx), expErr) + + // An error in the internal Alertmanager should not be returned. + internal, remote, forked = genTestAlertmanagers(tt, modeRemotePrimary) + remote.EXPECT().SaveAndApplyDefaultConfig(ctx).Return(nil).Once() + internal.EXPECT().SaveAndApplyDefaultConfig(ctx).Return(expErr).Once() + require.NoError(tt, forked.SaveAndApplyDefaultConfig(ctx)) }) t.Run("GetStatus", func(tt *testing.T) { diff --git a/pkg/services/ngalert/remote/remote_primary_forked_alertmanager.go b/pkg/services/ngalert/remote/remote_primary_forked_alertmanager.go index 37b955db493..1a360e4f513 100644 --- a/pkg/services/ngalert/remote/remote_primary_forked_alertmanager.go +++ b/pkg/services/ngalert/remote/remote_primary_forked_alertmanager.go @@ -39,13 +39,18 @@ func (fam *RemotePrimaryForkedAlertmanager) ApplyConfig(ctx context.Context, con return nil } -// TODO: save the new configuration hash in memory. func (fam *RemotePrimaryForkedAlertmanager) SaveAndApplyConfig(ctx context.Context, config *apimodels.PostableUserConfig) error { return nil } -// TODO: save the new configuration hash in memory. func (fam *RemotePrimaryForkedAlertmanager) SaveAndApplyDefaultConfig(ctx context.Context) error { + if err := fam.remote.SaveAndApplyDefaultConfig(ctx); err != nil { + return fmt.Errorf("failed to send the default configuration to the remote Alertmanager: %w", err) + } + + if err := fam.internal.SaveAndApplyDefaultConfig(ctx); err != nil { + fam.log.Error("Error applying the default configuration to the internal Alertmanager", "err", err) + } return nil }