Skip to content

Commit 70f9a53

Browse files
dannykoppingdannykopping
committed
chore: adding tests
Signed-off-by: Danny Kopping <dannykopping@gmail.com>
1 parent f24aef0 commit 70f9a53

File tree

10 files changed

+306
-80
lines changed

10 files changed

+306
-80
lines changed

‎coderd/coderd.go

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1763,11 +1763,6 @@ func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, n
17631763
return nil, xerrors.Errorf("failed to create in-memory provisioner daemon: %w", err)
17641764
}
17651765

1766-
var prebuildsOrchestrator prebuilds.ReconciliationOrchestrator
1767-
if val := api.PrebuildsReconciler.Load(); val != nil {
1768-
prebuildsOrchestrator = *val
1769-
}
1770-
17711766
mux := drpcmux.New()
17721767
api.Logger.Debug(dialCtx, "starting in-memory provisioner daemon", slog.F("name", name))
17731768
logger := api.Logger.Named(fmt.Sprintf("inmem-provisionerd-%s", name))
@@ -1795,7 +1790,7 @@ func (api *API) CreateInMemoryTaggedProvisionerDaemon(dialCtx context.Context, n
17951790
Clock: api.Clock,
17961791
},
17971792
api.NotificationsEnqueuer,
1798-
prebuildsOrchestrator,
1793+
&api.PrebuildsReconciler,
17991794
)
18001795
if err != nil {
18011796
return nil, err

‎coderd/notifications/notificationstest/fake_enqueuer.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"github.com/prometheus/client_golang/prometheus"
1010

1111
"github.com/coder/coder/v2/coderd/database/dbauthz"
12+
"github.com/coder/coder/v2/coderd/notifications"
1213
"github.com/coder/coder/v2/coderd/rbac"
1314
"github.com/coder/coder/v2/coderd/rbac/policy"
1415
)
@@ -19,6 +20,12 @@ type FakeEnqueuer struct {
1920
sent []*FakeNotification
2021
}
2122

23+
var _ notifications.Enqueuer = &FakeEnqueuer{}
24+
25+
func NewFakeEnqueuer() notifications.Enqueuer {
26+
return &FakeEnqueuer{}
27+
}
28+
2229
type FakeNotification struct {
2330
UserID, TemplateID uuid.UUID
2431
Labels map[string]string

‎coderd/prebuilds/api.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ type ReconciliationOrchestrator interface {
3131

3232
// TrackResourceReplacement handles a pathological situation whereby a terraform resource is replaced due to drift,
3333
// which can obviate the whole point of pre-provisioning a prebuilt workspace.
34+
// See more detail at https://coder.com/docs/admin/templates/extending-templates/prebuilt-workspaces.md#preventing-resource-replacement.
3435
TrackResourceReplacement(ctx context.Context, workspaceID, buildID, claimantID uuid.UUID, replacements []*sdkproto.ResourceReplacement)
3536
}
3637

‎coderd/provisionerdserver/provisionerdserver.go

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ type server struct {
109109
UserQuietHoursScheduleStore *atomic.Pointer[schedule.UserQuietHoursScheduleStore]
110110
DeploymentValues *codersdk.DeploymentValues
111111
NotificationsEnqueuer notifications.Enqueuer
112-
PrebuildsOrchestrator prebuilds.ReconciliationOrchestrator
112+
PrebuildsOrchestrator *atomic.Pointer[prebuilds.ReconciliationOrchestrator]
113113

114114
OIDCConfig promoauth.OAuth2Config
115115

@@ -164,7 +164,7 @@ func NewServer(lifecycleCtx context.Context,
164164
deploymentValues *codersdk.DeploymentValues,
165165
options Options,
166166
enqueuer notifications.Enqueuer,
167-
prebuildsOrchestrator prebuilds.ReconciliationOrchestrator,
167+
prebuildsOrchestrator *atomic.Pointer[prebuilds.ReconciliationOrchestrator],
168168
) (proto.DRPCProvisionerDaemonServer, error) {
169169
// Fail-fast if pointers are nil
170170
if lifecycleCtx == nil {
@@ -1731,10 +1731,13 @@ func (s *server) CompleteJob(ctx context.Context, completed *proto.CompletedJob)
17311731
})
17321732
}
17331733

1734-
// Track resource replacements, if there are any.
1735-
if resourceReplacements := completed.GetWorkspaceBuild().GetResourceReplacements(); len(resourceReplacements) > 0 {
1736-
// Fire and forget.
1737-
go s.PrebuildsOrchestrator.TrackResourceReplacement(context.Background(), workspace.ID, workspaceBuild.ID, input.PrebuildClaimedByUser, resourceReplacements)
1734+
if s.PrebuildsOrchestrator != nil {
1735+
// Track resource replacements, if there are any.
1736+
orchestrator := s.PrebuildsOrchestrator.Load()
1737+
if resourceReplacements := completed.GetWorkspaceBuild().GetResourceReplacements(); orchestrator != nil && len(resourceReplacements) > 0 {
1738+
// Fire and forget.
1739+
go (*orchestrator).TrackResourceReplacement(context.Background(), workspace.ID, workspaceBuild.ID, input.PrebuildClaimedByUser, resourceReplacements)
1740+
}
17381741
}
17391742

17401743
msg, err := json.Marshal(wspubsub.WorkspaceEvent{

‎coderd/provisionerdserver/provisionerdserver_test.go

Lines changed: 112 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1747,6 +1747,109 @@ func TestCompleteJob(t *testing.T) {
17471747
})
17481748
}
17491749
})
1750+
1751+
t.Run("PrebuiltWorkspaceClaimWithResourceReplacements", func(t *testing.T) {
1752+
t.Parallel()
1753+
1754+
ctx := testutil.Context(t, testutil.WaitLong)
1755+
1756+
// Given: a mock prebuild orchestrator which stores calls to TrackResourceReplacement.
1757+
done := make(chan struct{})
1758+
orchestrator := &mockPrebuildsOrchestrator{
1759+
ReconciliationOrchestrator: agplprebuilds.DefaultReconciler,
1760+
done: done,
1761+
}
1762+
srv, db, ps, pd := setup(t, false, &overrides{
1763+
prebuildsOrchestrator: orchestrator,
1764+
})
1765+
1766+
// Given: a workspace build which simulates claiming a prebuild.
1767+
user := dbgen.User(t, db, database.User{})
1768+
template := dbgen.Template(t, db, database.Template{
1769+
Name: "template",
1770+
Provisioner: database.ProvisionerTypeEcho,
1771+
OrganizationID: pd.OrganizationID,
1772+
})
1773+
file := dbgen.File(t, db, database.File{CreatedBy: user.ID})
1774+
workspaceTable := dbgen.Workspace(t, db, database.WorkspaceTable{
1775+
TemplateID: template.ID,
1776+
OwnerID: user.ID,
1777+
OrganizationID: pd.OrganizationID,
1778+
})
1779+
version := dbgen.TemplateVersion(t, db, database.TemplateVersion{
1780+
OrganizationID: pd.OrganizationID,
1781+
TemplateID: uuid.NullUUID{
1782+
UUID: template.ID,
1783+
Valid: true,
1784+
},
1785+
JobID: uuid.New(),
1786+
})
1787+
build := dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{
1788+
WorkspaceID: workspaceTable.ID,
1789+
InitiatorID: user.ID,
1790+
TemplateVersionID: version.ID,
1791+
Transition: database.WorkspaceTransitionStart,
1792+
Reason: database.BuildReasonInitiator,
1793+
})
1794+
job := dbgen.ProvisionerJob(t, db, ps, database.ProvisionerJob{
1795+
FileID: file.ID,
1796+
InitiatorID: user.ID,
1797+
Type: database.ProvisionerJobTypeWorkspaceBuild,
1798+
Input: must(json.Marshal(provisionerdserver.WorkspaceProvisionJob{
1799+
WorkspaceBuildID: build.ID,
1800+
1801+
// Mark the job as a prebuilt workspace claim.
1802+
PrebuildClaimedByUser: uuid.New(),
1803+
IsPrebuild: false,
1804+
})),
1805+
OrganizationID: pd.OrganizationID,
1806+
})
1807+
_, err := db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{
1808+
OrganizationID: pd.OrganizationID,
1809+
WorkerID: uuid.NullUUID{
1810+
UUID: pd.ID,
1811+
Valid: true,
1812+
},
1813+
Types: []database.ProvisionerType{database.ProvisionerTypeEcho},
1814+
})
1815+
require.NoError(t, err)
1816+
1817+
// When: a replacement is encountered.
1818+
replacements := []*sdkproto.ResourceReplacement{
1819+
{
1820+
Resource: "docker_container[0]",
1821+
Paths: []string{"env"},
1822+
},
1823+
}
1824+
1825+
// Then: CompleteJob makes a call to TrackResourceReplacement.
1826+
_, err = srv.CompleteJob(ctx, &proto.CompletedJob{
1827+
JobId: job.ID.String(),
1828+
Type: &proto.CompletedJob_WorkspaceBuild_{
1829+
WorkspaceBuild: &proto.CompletedJob_WorkspaceBuild{
1830+
State: []byte{},
1831+
ResourceReplacements: replacements,
1832+
},
1833+
},
1834+
})
1835+
require.NoError(t, err)
1836+
1837+
// Then: the replacements are as we expected.
1838+
testutil.RequireReceive(ctx, t, done)
1839+
require.Equal(t, replacements, orchestrator.replacements)
1840+
})
1841+
}
1842+
1843+
type mockPrebuildsOrchestrator struct {
1844+
agplprebuilds.ReconciliationOrchestrator
1845+
1846+
replacements []*sdkproto.ResourceReplacement
1847+
done chan struct{}
1848+
}
1849+
1850+
func (m *mockPrebuildsOrchestrator) TrackResourceReplacement(_ context.Context, _, _, _ uuid.UUID, replacements []*sdkproto.ResourceReplacement) {
1851+
m.replacements = replacements
1852+
m.done <- struct{}{}
17501853
}
17511854

17521855
func TestInsertWorkspacePresetsAndParameters(t *testing.T) {
@@ -2632,6 +2735,7 @@ type overrides struct {
26322735
heartbeatInterval time.Duration
26332736
auditor audit.Auditor
26342737
notificationEnqueuer notifications.Enqueuer
2738+
prebuildsOrchestrator agplprebuilds.ReconciliationOrchestrator
26352739
}
26362740

26372741
func setup(t *testing.T, ignoreLogErrors bool, ov *overrides) (proto.DRPCProvisionerDaemonServer, database.Store, pubsub.Pubsub, database.ProvisionerDaemon) {
@@ -2713,6 +2817,13 @@ func setup(t *testing.T, ignoreLogErrors bool, ov *overrides) (proto.DRPCProvisi
27132817
})
27142818
require.NoError(t, err)
27152819

2820+
prebuildsOrchestrator := ov.prebuildsOrchestrator
2821+
if prebuildsOrchestrator == nil {
2822+
prebuildsOrchestrator = agplprebuilds.DefaultReconciler
2823+
}
2824+
var op atomic.Pointer[agplprebuilds.ReconciliationOrchestrator]
2825+
op.Store(&prebuildsOrchestrator)
2826+
27162827
srv, err := provisionerdserver.NewServer(
27172828
ov.ctx,
27182829
&url.URL{},
@@ -2740,7 +2851,7 @@ func setup(t *testing.T, ignoreLogErrors bool, ov *overrides) (proto.DRPCProvisi
27402851
HeartbeatFn: ov.heartbeatFn,
27412852
},
27422853
notifEnq,
2743-
agplprebuilds.DefaultReconciler,
2854+
&op,
27442855
)
27452856
require.NoError(t, err)
27462857
return srv, db, ps, daemon

‎enterprise/coderd/prebuilds/metricscollector.go

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,30 +16,42 @@ import (
1616
"github.com/coder/coder/v2/coderd/prebuilds"
1717
)
1818

19+
const (
20+
namespace = "coderd_prebuilt_workspaces_"
21+
22+
MetricCreatedCount = namespace + "created_total"
23+
MetricFailedCount = namespace + "failed_total"
24+
MetricClaimedCount = namespace + "claimed_total"
25+
MetricResourceReplacementsCount = namespace + "resource_replacements_total"
26+
MetricDesiredGauge = namespace + "desired"
27+
MetricRunningGauge = namespace + "running"
28+
MetricEligibleGauge = namespace + "eligible"
29+
)
30+
1931
var (
2032
labels = []string{"template_name", "preset_name", "organization_name"}
2133
createdPrebuildsDesc = prometheus.NewDesc(
22-
"coderd_prebuilt_workspaces_created_total",
34+
MetricCreatedCount,
2335
"Total number of prebuilt workspaces that have been created to meet the desired instance count of each "+
2436
"template preset.",
2537
labels,
2638
nil,
2739
)
2840
failedPrebuildsDesc = prometheus.NewDesc(
29-
"coderd_prebuilt_workspaces_failed_total",
41+
MetricFailedCount,
3042
"Total number of prebuilt workspaces that failed to build.",
3143
labels,
3244
nil,
3345
)
3446
claimedPrebuildsDesc = prometheus.NewDesc(
35-
"coderd_prebuilt_workspaces_claimed_total",
47+
MetricClaimedCount,
3648
"Total number of prebuilt workspaces which were claimed by users. Claiming refers to creating a workspace "+
3749
"with a preset selected for which eligible prebuilt workspaces are available and one is reassigned to a user.",
3850
labels,
3951
nil,
4052
)
4153
resourceReplacementsDesc = prometheus.NewDesc(
42-
"coderd_prebuilt_workspaces_resource_replacements_total",
54+
MetricResourceReplacementsCount,
4355
"Total number of prebuilt workspaces whose resource(s) got replaced upon being claimed. "+
4456
"In Terraform, drift on immutable attributes results in resource replacement. "+
4557
"This represents a worst-case scenario for prebuilt workspaces because the pre-provisioned resource "+
@@ -49,20 +61,20 @@ var (
4961
nil,
5062
)
5163
desiredPrebuildsDesc = prometheus.NewDesc(
52-
"coderd_prebuilt_workspaces_desired",
64+
MetricDesiredGauge,
5365
"Target number of prebuilt workspaces that should be available for each template preset.",
5466
labels,
5567
nil,
5668
)
5769
runningPrebuildsDesc = prometheus.NewDesc(
58-
"coderd_prebuilt_workspaces_running",
70+
MetricRunningGauge,
5971
"Current number of prebuilt workspaces that are in a running state. These workspaces have started "+
6072
"successfully but may not yet be claimable by users (see coderd_prebuilt_workspaces_eligible).",
6173
labels,
6274
nil,
6375
)
6476
eligiblePrebuildsDesc = prometheus.NewDesc(
65-
"coderd_prebuilt_workspaces_eligible",
77+
MetricEligibleGauge,
6678
"Current number of prebuilt workspaces that are eligible to be claimed by users. These are workspaces that "+
6779
"have completed their build process with their agent reporting 'ready' status.",
6880
labels,
@@ -162,5 +174,10 @@ func (mc *MetricsCollector) trackResourceReplacement(orgName, templateName, pres
162174
if _, ok := mc.replacementsCounter[key]; !ok {
163175
mc.replacementsCounter[key] = &atomic.Int64{}
164176
}
177+
178+
// We only track _that_ a resource replacement occurred, not how many.
179+
// Just one is enough to ruin a prebuild, but we can't know apriori which replacement would cause this.
180+
// For example, say we have 2 replacements: a docker_container and a null_resource; we don't know which one might
181+
// cause an issue (or indeed if either would), so we just track the replacement.
165182
mc.replacementsCounter[key].Add(1)
166183
}

0 commit comments

Comments
 (0)