8000 chore: set notifications manager before enterprise server initializes… · coder/coder@5f62702 · GitHub
[go: up one dir, main page]

Skip to content

Commit 5f62702

Browse files
committed
chore: set notifications manager before enterprise server initializes API
avoid all this pointer juggling Signed-off-by: Danny Kopping <dannykopping@gmail.com>
1 parent 6d1c3ea commit 5f62702

File tree

8 files changed

+48
-64
lines changed

8 files changed

+48
-64
lines changed

cli/server.go

Lines changed: 31 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -928,6 +928,37 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
928928
options.StatsBatcher = batcher
929929
defer closeBatcher()
930930

931+
// Manage notifications.
932+
var (
933+
notificationsCfg = options.DeploymentValues.Notifications
934+
notificationsManager *notifications.Manager
935+
)
936+
937+
metrics := notifications.NewMetrics(options.PrometheusRegistry)
938+
helpers := templateHelpers(options)
939+
940+
// The enqueuer is responsible for enqueueing notifications to the given store.
941+
enqueuer, err := notifications.NewStoreEnqueuer(notificationsCfg, options.Database, helpers, logger.Named("notifications.enqueuer"), quartz.NewReal())
942+
if err != nil {
943+
return xerrors.Errorf("failed to instantiate notification store enqueuer: %w", err)
944+
}
945+
options.NotificationsEnqueuer = enqueuer
946+
947+
// The notification manager is responsible for:
948+
// - creating notifiers and managing their lifecycles (notifiers are responsible for dequeueing/sending notifications)
949+
// - keeping the store updated with status updates
950+
notificationsManager, err = notifications.NewManager(notificationsCfg, options.Database, options.Pubsub, helpers, metrics, logger.Named("notifications.manager"))
951+
if err != nil {
952+
return xerrors.Errorf("failed to instantiate notification manager: %w", err)
953+
}
954+
955+
// nolint:gocritic // We need to run the manager in a notifier context.
956+
notificationsManager.Run(dbauthz.AsNotifier(ctx))
957+
958+
// Run report generator to distribute periodic reports.
959+
notificationReportGenerator := reports.NewReportGenerator(ctx, logger.Named("notifications.report_generator"), options.Database, options.NotificationsEnqueuer, quartz.NewReal())
960+
defer notificationReportGenerator.Close()
961+
931962
// We use a separate coderAPICloser so the Enterprise API
932963
// can have its own close functions. This is cleaner
933964
// than abstracting the Coder API itself.
@@ -975,37 +1006,6 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
9751006
return xerrors.Errorf("write config url: %w", err)
9761007
}
9771008

978-
// Manage notifications.
979-
var (
980-
notificationsCfg = options.DeploymentValues.Notifications
981-
notificationsManager *notifications.Manager
982-
)
983-
984-
metrics := notifications.NewMetrics(options.PrometheusRegistry)
985-
helpers := templateHelpers(options)
986-
987-
// The enqueuer is responsible for enqueueing notifications to the given store.
988-
enqueuer, err := notifications.NewStoreEnqueuer(notificationsCfg, options.Database, helpers, logger.Named("notifications.enqueuer"), quartz.NewReal())
989-
if err != nil {
990-
return xerrors.Errorf("failed to instantiate notification store enqueuer: %w", err)
991-
}
992-
options.NotificationsEnqueuer = enqueuer
993-
994-
// The notification manager is responsible for:
995-
// - creating notifiers and managing their lifecycles (notifiers are responsible for dequeueing/sending notifications)
996-
// - keeping the store updated with status updates
997-
notificationsManager, err = notifications.NewManager(notificationsCfg, options.Database, options.Pubsub, helpers, metrics, logger.Named("notifications.manager"))
998-
if err != nil {
999-
return xerrors.Errorf("failed to instantiate notification manager: %w", err)
1000-
}
1001-
1002-
// nolint:gocritic // We need to run the manager in a notifier context.
1003-
notificationsManager.Run(dbauthz.AsNotifier(ctx))
1004-
1005-
// Run report generator to distribute periodic reports.
1006-
notificationReportGenerator := reports.NewReportGenerator(ctx, logger.Named("notifications.report_generator"), options.Database, options.NotificationsEnqueuer, quartz.NewReal())
1007-
defer notificationReportGenerator.Close()
1008-
10091009
// Since errCh only has one buffered slot, all routines
10101010
// sending on it must be wrapped in a select/default to
10111011
// avoid leaving dangling goroutines waiting for the

coderd/coderd.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,11 @@ import (
3838
"tailscale.com/util/singleflight"
3939

4040
"cdr.dev/slog"
41-
"github.com/coder/coder/v2/codersdk/drpcsdk"
4241
"github.com/coder/quartz"
4342
"github.com/coder/serpent"
4443

44+
"github.com/coder/coder/v2/codersdk/drpcsdk"
45+
4546
"github.com/coder/coder/v2/coderd/ai"
4647
"github.com/coder/coder/v2/coderd/cryptokeys"
4748
"github.com/coder/coder/v2/coderd/entitlements"

coderd/notifications/enqueuer.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ type StoreEnqueuer struct {
5050
}
5151

5252
// NewStoreEnqueuer creates an Enqueuer implementation which can persist notification messages in the store.
53-
func NewStoreEnqueuer(cfg codersdk.NotificationsConfig, store Store, helpers template.FuncMap, log slog.Logger, clock quartz.Clock) (Enqueuer, error) {
53+
func NewStoreEnqueuer(cfg codersdk.NotificationsConfig, store Store, helpers template.FuncMap, log slog.Logger, clock quartz.Clock) (*StoreEnqueuer, error) {
5454
var method database.NotificationMethod
5555
// TODO(DanielleMaywood):
5656
// Currently we do not want to allow setting `inbox` as the default notification method.
@@ -203,7 +203,7 @@ func (s *StoreEnqueuer) buildPayload(metadata database.FetchNewMessageMetadataRo
203203
type NoopEnqueuer struct{}
204204

205205
// NewNoopEnqueuer builds a NoopEnqueuer which is used to fulfill the contract for enqueuing notifications, if ExperimentNotifications is not set.
206-
func NewNoopEnqueuer() Enqueuer {
206+
func NewNoopEnqueuer() *NoopEnqueuer {
207207
return &NoopEnqueuer{}
208208
}
209209

coderd/notifications/notificationstest/fake_enqueuer.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ type FakeEnqueuer struct {
2222

2323
var _ notifications.Enqueuer = &FakeEnqueuer{}
2424

25-
func NewFakeEnqueuer() notifications.Enqueuer {
25+
func NewFakeEnqueuer() *FakeEnqueuer {
2626
return &FakeEnqueuer{}
2727
}
2828

enterprise/coderd/coderd.go

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"strconv"
1111
"strings"
1212
"sync"
13-
"sync/atomic"
1413
"time"
1514

1615
"github.com/coder/quartz"
@@ -20,7 +19,6 @@ import (
2019
"github.com/coder/coder/v2/coderd/database"
2120
"github.com/coder/coder/v2/coderd/entitlements"
2221
"github.com/coder/coder/v2/coderd/idpsync"
23-
"github.com/coder/coder/v2/coderd/notifications"
2422
agplportsharing "github.com/coder/coder/v2/coderd/portsharing"
2523
agplprebuilds "github.com/coder/coder/v2/coderd/prebuilds"
2624
"github.com/coder/coder/v2/coderd/rbac/policy"
@@ -1166,11 +1164,7 @@ func (api *API) setupPrebuilds(featureEnabled bool) (agplprebuilds.Reconciliatio
11661164
return agplprebuilds.DefaultReconciler, agplprebuilds.DefaultClaimer
11671165
}
11681166

1169-
// api.NotificationsEnqueuer may not be fully setup by the time the reconciler is created; ensure it can be updated
1170-
// later safely and keep a reference to the underlying value.
1171-
var enqueuer atomic.Pointer[notifications.Enqueuer]
1172-
enqueuer.Store(&api.NotificationsEnqueuer)
11731167
reconciler := prebuilds.NewStoreReconciler(api.Database, api.Pubsub, api.DeploymentValues.Prebuilds,
1174-
api.Logger.Named("prebuilds"), quartz.NewReal(), api.PrometheusRegistry, &enqueuer)
1168+
api.Logger.Named("prebuilds"), quartz.NewReal(), api.PrometheusRegistry, api.NotificationsEnqueuer)
11751169
return reconciler, prebuilds.NewEnterpriseClaimer(api.Database)
11761170
}

enterprise/coderd/prebuilds/metricscollector.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package prebuilds
33
import (
44
"context"
55
"fmt"
6+
"sync"
67
"sync/atomic"
78
"time"
89

enterprise/coderd/prebuilds/reconcile.go

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,7 @@ type StoreReconciler struct {
4444
clock quartz.Clock
4545
registerer prometheus.Registerer
4646
metrics *MetricsCollector
47-
// Kept as an atomic.Pointer so the implementation can swap at runtime;
48-
// a plain interface value would be copied into the struct and wouldn't
49-
// reflect later assignments. The enqueuer starts as a NoopEnqueuer and is later changed,
50-
// and refactoring the order of operations would be too onerous.
51-
notifEnq *atomic.Pointer[notifications.Enqueuer]
47+
notifEnq notifications.Enqueuer
5248

5349
cancelFn context.CancelCauseFunc
5450
running atomic.Bool
@@ -65,7 +61,7 @@ func NewStoreReconciler(store database.Store,
6561
logger slog.Logger,
6662
clock quartz.Clock,
6763
registerer prometheus.Registerer,
68-
notifEnq *atomic.Pointer[notifications.Enqueuer],
64+
notifEnq notifications.Enqueuer,
6965
) *StoreReconciler {
7066
reconciler := &StoreReconciler{
7167
store: store,
@@ -713,7 +709,7 @@ func (c *StoreReconciler) trackResourceReplacement(ctx context.Context, workspac
713709
}
714710

715711
// Send notification to template admins.
716-
if c.notifEnq == nil || c.notifEnq.Load() == nil {
712+
if c.notifEnq == nil {
717713
c.logger.Warn(ctx, "notification enqueuer not set, cannot send resource replacement notification(s)")
718714
return nil
719715
}
@@ -732,7 +728,7 @@ func (c *StoreReconciler) trackResourceReplacement(ctx context.Context, workspac
732728

733729
var notifErr error
734730
for _, templateAdmin := range templateAdmins {
735-
if _, err := (*c.notifEnq.Load()).EnqueueWithData(ctx, templateAdmin.ID, notifications.TemplateWorkspaceResourceReplaced,
731+
if _, err := c.notifEnq.EnqueueWithData(ctx, templateAdmin.ID, notifications.TemplateWorkspaceResourceReplaced,
736732
map[string]string{
737733
"org": org.Name,
738734
"workspace": workspace.Name,

enterprise/coderd/prebuilds/reconcile_test.go

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"database/sql"
66
"fmt"
77
"sync"
8-
"sync/atomic"
98
"testing"
109
"time"
1110

@@ -951,10 +950,9 @@ func TestTrackResourceReplacement(t *testing.T) {
951950
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelDebug)
952951
db, ps := dbtestutil.NewDB(t)
953952

954-
ep := newFakeEnqueuer()
953+
fakeEnqueuer := newFakeEnqueuer()
955954
registry := prometheus.NewRegistry()
956-
reconciler := prebuilds.NewStoreReconciler(db, ps, codersdk.PrebuildsConfig{}, logger, clock, registry, ep)
957-
fakeEnqueuer := (*ep.Load()).(*notificationstest.FakeEnqueuer)
955+
reconciler := prebuilds.NewStoreReconciler(db, ps, codersdk.PrebuildsConfig{}, logger, clock, registry, fakeEnqueuer)
958956

959957
// Given: a template admin to receive a notification.
960958
templateAdmin := dbgen.User(t, db, database.User{
@@ -1025,18 +1023,12 @@ func TestTrackResourceReplacement(t *testing.T) {
10251023
require.EqualValues(t, 1, metric.GetCounter().GetValue())
10261024
}
10271025

1028-
func newNoopEnqueuer() *atomic.Pointer[notifications.Enqueuer] {
1029-
var ep atomic.Pointer[notifications.Enqueuer]
1030-
enqueuer := notifications.NewNoopEnqueuer()
1031-
ep.Store(&enqueuer)
1032-
return &ep
1026+
func newNoopEnqueuer() *notifications.NoopEnqueuer {
1027+
return notifications.NewNoopEnqueuer()
10331028
}
10341029

1035-
func newFakeEnqueuer() *atomic.Pointer[notifications.Enqueuer] {
1036-
var ep atomic.Pointer[notifications.Enqueuer]
1037-
fakeEnqueuer := notificationstest.NewFakeEnqueuer()
1038-
ep.Store(&fakeEnqueuer)
1039-
return &ep
1030+
func newFakeEnqueuer() *notificationstest.FakeEnqueuer {
1031+
return notificationstest.NewFakeEnqueuer()
10401032
}
10411033

10421034
// nolint:revive // It's a control flag, but this is a test.

0 commit comments

Comments
 (0)
0