From 9689ae6001734eca537a472a911a2b7d23935302 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 19 Dec 2024 11:15:15 +0000 Subject: [PATCH 1/3] chore(coderd/coderdtest): wait for provisioner daemons to be connected --- coderd/coderdtest/coderdtest.go | 32 +++++++++++++++++++ coderd/templateversions_test.go | 4 +-- .../coderd/coderdenttest/coderdenttest.go | 2 ++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 7c1e6a4962a8c..53d54f7ef1119 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -559,6 +559,8 @@ func NewWithAPI(t testing.TB, options *Options) (*codersdk.Client, io.Closer, *c var provisionerCloser io.Closer = nopcloser{} if options.IncludeProvisionerDaemon { provisionerCloser = NewTaggedProvisionerDaemon(t, coderAPI, "test", options.ProvisionerDaemonTags) + // Wait for the provisioner daemon to be ready before continuing. + AwaitProvisionerDaemonsConnected(t, coderAPI) } client := codersdk.New(serverURL) t.Cleanup(func() { @@ -570,6 +572,36 @@ func NewWithAPI(t testing.TB, options *Options) (*codersdk.Client, io.Closer, *c return client, provisionerCloser, coderAPI } +// AwaitProvisionerDaemonsConnected waits for the provisioner daemon to connect. +func AwaitProvisionerDaemonsConnected(t testing.TB, api *coderd.API) { + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) + defer cancel() + for { + select { + case <-ctx.Done(): + t.Fatal("provisioner daemon did not connect in time") + case <-time.After(testutil.IntervalFast): + // nolint:gocritic // used for testing only + daemons, err := api.Database.GetProvisionerDaemons(dbauthz.AsSystemReadProvisionerDaemons(ctx)) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + t.Logf("no provisioner daemons found yet") + continue + } + require.NoError(t, err) + } + if len(daemons) == 0 { + t.Logf("no provisioner daemons found yet") + continue + } + for _, daemon := range daemons { + t.Logf("found provisioner daemon %q", daemon.Name) + } + return + } + } +} + // ProvisionerdCloser wraps a provisioner daemon as an io.Closer that can be called multiple times type ProvisionerdCloser struct { mu sync.Mutex diff --git a/coderd/templateversions_test.go b/coderd/templateversions_test.go index d8377821245bf..1a67508880188 100644 --- a/coderd/templateversions_test.go +++ b/coderd/templateversions_test.go @@ -172,8 +172,8 @@ func TestPostTemplateVersionsByOrganization(t *testing.T) { require.Equal(t, provisionersdk.ScopeOrganization, version.Job.Tags[provisionersdk.TagScope]) if assert.Equal(t, version.Job.Status, codersdk.ProvisionerJobPending) { assert.NotNil(t, version.MatchedProvisioners) - assert.Equal(t, version.MatchedProvisioners.Available, 1) - assert.Equal(t, version.MatchedProvisioners.Count, 1) + assert.Equal(t, 1, version.MatchedProvisioners.Available) + assert.Equal(t, 1, version.MatchedProvisioners.Count) assert.True(t, version.MatchedProvisioners.MostRecentlySeen.Valid) } diff --git a/enterprise/coderd/coderdenttest/coderdenttest.go b/enterprise/coderd/coderdenttest/coderdenttest.go index 0d44937e4a82d..45e15107edae8 100644 --- a/enterprise/coderd/coderdenttest/coderdenttest.go +++ b/enterprise/coderd/coderdenttest/coderdenttest.go @@ -121,6 +121,8 @@ func NewWithAPI(t *testing.T, options *Options) ( var provisionerCloser io.Closer = nopcloser{} if options.IncludeProvisionerDaemon { provisionerCloser = coderdtest.NewProvisionerDaemon(t, coderAPI.AGPL) + // Wait for provisioner daemons to be connected befroe continuing + coderdtest.AwaitProvisionerDaemonsConnected(t, coderAPI.AGPL) } t.Cleanup(func() { From 904b857742b21168c86bbfea970e0d284459b77d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 19 Dec 2024 11:25:24 +0000 Subject: [PATCH 2/3] typo fix --- enterprise/coderd/coderdenttest/coderdenttest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/coderdenttest/coderdenttest.go b/enterprise/coderd/coderdenttest/coderdenttest.go index 45e15107edae8..a19830e19ce2a 100644 --- a/enterprise/coderd/coderdenttest/coderdenttest.go +++ b/enterprise/coderd/coderdenttest/coderdenttest.go @@ -121,7 +121,7 @@ func NewWithAPI(t *testing.T, options *Options) ( var provisionerCloser io.Closer = nopcloser{} if options.IncludeProvisionerDaemon { provisionerCloser = coderdtest.NewProvisionerDaemon(t, coderAPI.AGPL) - // Wait for provisioner daemons to be connected befroe continuing + // Wait for provisioner daemons to be connected before continuing coderdtest.AwaitProvisionerDaemonsConnected(t, coderAPI.AGPL) } From d2e25074c8026741a4c12502f69a16c1a5608eb7 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 19 Dec 2024 14:18:10 +0000 Subject: [PATCH 3/3] address Spike's comments --- coderd/coderdtest/coderdtest.go | 38 +++---------------- .../coderd/coderdenttest/coderdenttest.go | 2 - provisionerd/provisionerd.go | 23 ++++++++--- 3 files changed, 24 insertions(+), 39 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 53d54f7ef1119..bd1ed740a7ce7 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -559,8 +559,6 @@ func NewWithAPI(t testing.TB, options *Options) (*codersdk.Client, io.Closer, *c var provisionerCloser io.Closer = nopcloser{} if options.IncludeProvisionerDaemon { provisionerCloser = NewTaggedProvisionerDaemon(t, coderAPI, "test", options.ProvisionerDaemonTags) - // Wait for the provisioner daemon to be ready before continuing. - AwaitProvisionerDaemonsConnected(t, coderAPI) } client := codersdk.New(serverURL) t.Cleanup(func() { @@ -572,36 +570,6 @@ func NewWithAPI(t testing.TB, options *Options) (*codersdk.Client, io.Closer, *c return client, provisionerCloser, coderAPI } -// AwaitProvisionerDaemonsConnected waits for the provisioner daemon to connect. -func AwaitProvisionerDaemonsConnected(t testing.TB, api *coderd.API) { - ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) - defer cancel() - for { - select { - case <-ctx.Done(): - t.Fatal("provisioner daemon did not connect in time") - case <-time.After(testutil.IntervalFast): - // nolint:gocritic // used for testing only - daemons, err := api.Database.GetProvisionerDaemons(dbauthz.AsSystemReadProvisionerDaemons(ctx)) - if err != nil { - if errors.Is(err, sql.ErrNoRows) { - t.Logf("no provisioner daemons found yet") - continue - } - require.NoError(t, err) - } - if len(daemons) == 0 { - t.Logf("no provisioner daemons found yet") - continue - } - for _, daemon := range daemons { - t.Logf("found provisioner daemon %q", daemon.Name) - } - return - } - } -} - // ProvisionerdCloser wraps a provisioner daemon as an io.Closer that can be called multiple times type ProvisionerdCloser struct { mu sync.Mutex @@ -663,6 +631,7 @@ func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, name string, assert.NoError(t, err) }() + connectedCh := make(chan struct{}) daemon := provisionerd.New(func(dialCtx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) { return coderAPI.CreateInMemoryTaggedProvisionerDaemon(dialCtx, name, []codersdk.ProvisionerType{codersdk.ProvisionerTypeEcho}, provisionerTags) }, &provisionerd.Options{ @@ -672,7 +641,12 @@ func NewTaggedProvisionerDaemon(t testing.TB, coderAPI *coderd.API, name string, Connector: provisionerd.LocalProvisioners{ string(database.ProvisionerTypeEcho): sdkproto.NewDRPCProvisionerClient(echoClient), }, + InitConnectionCh: connectedCh, }) + // Wait for the provisioner daemon to connect before continuing. + // Users of this function tend to assume that the provisioner is connected + // and ready to use when that may not strictly be the case. + <-connectedCh closer := NewProvisionerDaemonCloser(daemon) t.Cleanup(func() { _ = closer.Close() diff --git a/enterprise/coderd/coderdenttest/coderdenttest.go b/enterprise/coderd/coderdenttest/coderdenttest.go index a19830e19ce2a..0d44937e4a82d 100644 --- a/enterprise/coderd/coderdenttest/coderdenttest.go +++ b/enterprise/coderd/coderdenttest/coderdenttest.go @@ -121,8 +121,6 @@ func NewWithAPI(t *testing.T, options *Options) ( var provisionerCloser io.Closer = nopcloser{} if options.IncludeProvisionerDaemon { provisionerCloser = coderdtest.NewProvisionerDaemon(t, coderAPI.AGPL) - // Wait for provisioner daemons to be connected before continuing - coderdtest.AwaitProvisionerDaemonsConnected(t, coderAPI.AGPL) } t.Cleanup(func() { diff --git a/provisionerd/provisionerd.go b/provisionerd/provisionerd.go index 4cf2436ea1267..e3b8da8bfe2d9 100644 --- a/provisionerd/provisionerd.go +++ b/provisionerd/provisionerd.go @@ -60,6 +60,7 @@ type Options struct { UpdateInterval time.Duration LogBufferInterval time.Duration Connector Connector + InitConnectionCh chan struct{} // only to be used in tests } // New creates and starts a provisioner daemon. @@ -84,6 +85,9 @@ func New(clientDialer Dialer, opts *Options) *Server { mets := NewMetrics(reg) opts.Metrics = &mets } + if opts.InitConnectionCh == nil { + opts.InitConnectionCh = make(chan struct{}) + } ctx, ctxCancel := context.WithCancel(context.Background()) daemon := &Server{ @@ -93,11 +97,12 @@ func New(clientDialer Dialer, opts *Options) *Server { clientDialer: clientDialer, clientCh: make(chan proto.DRPCProvisionerDaemonClient), - closeContext: ctx, - closeCancel: ctxCancel, - closedCh: make(chan struct{}), - shuttingDownCh: make(chan struct{}), - acquireDoneCh: make(chan struct{}), + closeContext: ctx, + closeCancel: ctxCancel, + closedCh: make(chan struct{}), + shuttingDownCh: make(chan struct{}), + acquireDoneCh: make(chan struct{}), + initConnectionCh: opts.InitConnectionCh, } daemon.wg.Add(2) @@ -115,6 +120,11 @@ type Server struct { wg sync.WaitGroup + // initConnectionCh will receive when the daemon connects to coderd for the + // first time. + initConnectionCh chan struct{} + initConnectionOnce sync.Once + // mutex protects all subsequent fields mutex sync.Mutex // closeContext is canceled when we start closing. @@ -231,6 +241,9 @@ connectLoop: } p.opts.Logger.Info(p.closeContext, "successfully connected to coderd") retrier.Reset() + p.initConnectionOnce.Do(func() { + close(p.initConnectionCh) + }) // serve the client until we are closed or it disconnects for {