From d64c2b6f4e0504ba9c46e1c72b3b6ce17f0a1261 Mon Sep 17 00:00:00 2001 From: Santiago Date: Thu, 30 Nov 2023 15:36:41 +0100 Subject: [PATCH] Alerting: Implement ApplyConfig in the forked Alertmanager (#78684) * Alerting: Add a sync interval for ApplyConfig in remote secondary mode * remove out of scope code * remove parentheses after CleanUp for consistency in test comments * Add comment to ApplyConfig --- .../remote/forked_alertmanager_test.go | 33 ++++++++++++++++--- .../remote_secondary_forked_alertmanager.go | 13 ++++++-- 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/pkg/services/ngalert/remote/forked_alertmanager_test.go b/pkg/services/ngalert/remote/forked_alertmanager_test.go index e89d9a89e2b..8a6dd36259c 100644 --- a/pkg/services/ngalert/remote/forked_alertmanager_test.go +++ b/pkg/services/ngalert/remote/forked_alertmanager_test.go @@ -5,7 +5,9 @@ import ( "errors" "testing" + "github.com/grafana/grafana/pkg/infra/log" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" + "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/notifier" "github.com/grafana/grafana/pkg/services/ngalert/notifier/alertmanager_mock" "github.com/stretchr/testify/mock" @@ -21,6 +23,27 @@ func TestForkedAlertmanager_ModeRemoteSecondary(t *testing.T) { ctx := context.Background() expErr := errors.New("test error") + t.Run("ApplyConfig", func(tt *testing.T) { + // ApplyConfig should be called in both Alertmanagers. + internal, remote, forked := genTestAlertmanagers(tt, modeRemoteSecondary) + internal.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Twice() + remote.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Twice() + require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{})) + require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{})) + + // An error in the remote Alertmanager should not be returned. + internal, remote, forked = genTestAlertmanagers(tt, modeRemoteSecondary) + internal.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Once() + remote.EXPECT().ApplyConfig(ctx, mock.Anything).Return(expErr).Once() + require.NoError(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{})) + + // An error in the internal Alertmanager should be returned. + internal, remote, forked = genTestAlertmanagers(tt, modeRemoteSecondary) + internal.EXPECT().ApplyConfig(ctx, mock.Anything).Return(expErr).Once() + remote.EXPECT().ApplyConfig(ctx, mock.Anything).Return(nil).Once() + require.Error(tt, forked.ApplyConfig(ctx, &models.AlertConfiguration{}), expErr) + }) + t.Run("SaveAndApplyConfig", func(tt *testing.T) { // SaveAndApplyConfig should only be called on the remote Alertmanager. // State and configuration are updated on an interval. @@ -243,16 +266,16 @@ func TestForkedAlertmanager_ModeRemoteSecondary(t *testing.T) { }) t.Run("CleanUp", func(tt *testing.T) { - // CleanUp() should be called only in the internal Alertmanager, + // CleanUp should be called only in the internal Alertmanager, // there's no cleanup to do in the remote one. - internal, _, forked := genTestAlertmanagers(tt, modeRemotePrimary) + internal, _, forked := genTestAlertmanagers(tt, modeRemoteSecondary) internal.EXPECT().CleanUp().Once() forked.CleanUp() }) t.Run("StopAndWait", func(tt *testing.T) { // StopAndWait should be called on both Alertmanagers. - internal, remote, forked := genTestAlertmanagers(tt, modeRemotePrimary) + internal, remote, forked := genTestAlertmanagers(tt, modeRemoteSecondary) internal.EXPECT().StopAndWait().Once() remote.EXPECT().StopAndWait().Once() forked.StopAndWait() @@ -475,7 +498,7 @@ func TestForkedAlertmanager_ModeRemotePrimary(t *testing.T) { }) t.Run("CleanUp", func(tt *testing.T) { - // CleanUp() should be called only in the internal Alertmanager, + // CleanUp should be called only in the internal Alertmanager, // there's no cleanup to do in the remote one. internal, _, forked := genTestAlertmanagers(tt, modeRemotePrimary) internal.EXPECT().CleanUp().Once() @@ -515,7 +538,7 @@ func genTestAlertmanagers(t *testing.T, mode int) (*alertmanager_mock.Alertmanag remote := alertmanager_mock.NewAlertmanagerMock(t) if mode == modeRemoteSecondary { - return internal, remote, NewRemoteSecondaryForkedAlertmanager(internal, remote) + return internal, remote, NewRemoteSecondaryForkedAlertmanager(log.NewNopLogger(), internal, remote) } return internal, remote, NewRemotePrimaryForkedAlertmanager(internal, remote) } diff --git a/pkg/services/ngalert/remote/remote_secondary_forked_alertmanager.go b/pkg/services/ngalert/remote/remote_secondary_forked_alertmanager.go index d913a89f16e..1349ac165f0 100644 --- a/pkg/services/ngalert/remote/remote_secondary_forked_alertmanager.go +++ b/pkg/services/ngalert/remote/remote_secondary_forked_alertmanager.go @@ -3,25 +3,34 @@ package remote import ( "context" + "github.com/grafana/grafana/pkg/infra/log" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/notifier" ) type RemoteSecondaryForkedAlertmanager struct { + log log.Logger + internal notifier.Alertmanager remote notifier.Alertmanager } -func NewRemoteSecondaryForkedAlertmanager(internal, remote notifier.Alertmanager) *RemoteSecondaryForkedAlertmanager { +func NewRemoteSecondaryForkedAlertmanager(l log.Logger, internal, remote notifier.Alertmanager) *RemoteSecondaryForkedAlertmanager { return &RemoteSecondaryForkedAlertmanager{ + log: l, internal: internal, remote: remote, } } +// ApplyConfig will only log errors for the remote Alertmanager and ensure we delegate the call to the internal Alertmanager. +// We don't care about errors in the remote Alertmanager in remote secondary mode. func (fam *RemoteSecondaryForkedAlertmanager) ApplyConfig(ctx context.Context, config *models.AlertConfiguration) error { - return nil + if err := fam.remote.ApplyConfig(ctx, config); err != nil { + fam.log.Error("Error applying config to the remote Alertmanager", "err", err) + } + return fam.internal.ApplyConfig(ctx, config) } // SaveAndApplyConfig is only called on the internal Alertmanager when running in remote secondary mode.