Skip to content

fix: use background context for inmem provisionerd #11545

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 10, 2024
Merged

fix: use background context for inmem provisionerd #11545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cli/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1371,10 +1371,10 @@ func newProvisionerDaemon(
connector[string(database.ProvisionerTypeTerraform)] = sdkproto.NewDRPCProvisionerClient(terraformClient)
}

return provisionerd.New(func(ctx context.Context) (proto.DRPCProvisionerDaemonClient, error) {
return provisionerd.New(func(dialCtx context.Context) (proto.DRPCProvisionerDaemonClient, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I don't think this rename is required as I feel this is the default way we should think about contexts, unless explicitly specified otherwise. But there's also no harm so feel free.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear from here that this function argument is dialer, so I thought it would be a nice reminder.

// This debounces calls to listen every second. Read the comment
// in provisionerdserver.go to learn more!
return coderAPI.CreateInMemoryProvisionerDaemon(ctx, name)
return coderAPI.CreateInMemoryProvisionerDaemon(dialCtx, name)
}, &provisionerd.Options{
Logger: logger.Named(fmt.Sprintf("provisionerd-%s", name)),
UpdateInterval: time.Second,
Expand Down
24 changes: 18 additions & 6 deletions coderd/coderd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1174,7 +1174,7 @@ func compressHandler(h http.Handler) http.Handler {

// CreateInMemoryProvisionerDaemon is an in-memory connection to a provisionerd.
// Useful when starting coderd and provisionerd in the same process.
func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context, name string) (client proto.DRPCProvisionerDaemonClient, err error) {
func (api *API) CreateInMemoryProvisionerDaemon(dialCtx context.Context, name string) (client proto.DRPCProvisionerDaemonClient, err error) {
tracer := api.TracerProvider.Tracer(tracing.TracerName)
clientSession, serverSession := drpc.MemTransportPipe()
defer func() {
Expand All @@ -1185,7 +1185,7 @@ func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context, name string
}()

//nolint:gocritic // in-memory provisioners are owned by system
daemon, err := api.Database.UpsertProvisionerDaemon(dbauthz.AsSystemRestricted(ctx), database.UpsertProvisionerDaemonParams{
daemon, err := api.Database.UpsertProvisionerDaemon(dbauthz.AsSystemRestricted(dialCtx), database.UpsertProvisionerDaemonParams{
Name: name,
CreatedAt: dbtime.Now(),
Provisioners: []database.ProvisionerType{
Expand All @@ -1201,7 +1201,7 @@ func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context, name string
}

mux := drpcmux.New()
api.Logger.Info(ctx, "starting in-memory provisioner daemon", slog.F("name", name))
api.Logger.Info(dialCtx, "starting in-memory provisioner daemon", slog.F("name", name))
logger := api.Logger.Named(fmt.Sprintf("inmem-provisionerd-%s", name))
srv, err := provisionerdserver.NewServer(
api.ctx, // use the same ctx as the API
Expand Down Expand Up @@ -1238,13 +1238,25 @@ func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context, name string
if xerrors.Is(err, io.EOF) {
return
}
logger.Debug(ctx, "drpc server error", slog.Error(err))
logger.Debug(dialCtx, "drpc server error", slog.Error(err))
},
},
)
// in-mem pipes aren't technically "websockets" but they have the same properties as far as the
// API is concerned: they are long-lived connections that we need to close before completing
// shutdown of the API.
api.WebsocketWaitMutex.Lock()
api.WebsocketWaitGroup.Add(1)
api.WebsocketWaitMutex.Unlock()
go func() {
err := server.Serve(ctx, serverSession)
logger.Info(ctx, "provisioner daemon disconnected", slog.Error(err))
defer api.WebsocketWaitGroup.Done()
// here we pass the background context, since we want the server to keep serving until the
// client hangs up. If we, say, pass the API context, then when it is canceled, we could
// drop a job that we locked in the database but never passed to the provisionerd. The
// provisionerd is local, in-mem, so there isn't a danger of losing contact with it and
// having a dead connection we don't know the status of.
err := server.Serve(context.Background(), serverSession)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity check: will this result in provisioner jobs not being cancelled on shutdown? And is that really what we want?

Let's say a provisioner job (terraform) is running and it will take 5 minutes, I imagine we would like to ensure that job is cancelled gracefully, so we only wait for the cleanup but not the full 5 minutes. Is that what will happen or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in cli/server.go we explicitly close the provisionerDaemons before closing the API. Closing the provisioner daemon fails the active job. That's not exactly graceful, but at least it leaves the job in an understandable state.

logger.Info(dialCtx, "provisioner daemon disconnected", slog.Error(err))
// close the sessions, so we don't leak goroutines serving them.
_ = clientSession.Close()
_ = serverSession.Close()
Expand Down
4 changes: 2 additions & 2 deletions coderd/coderdtest/coderdtest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,8 +532,8 @@ func NewProvisionerDaemon(t testing.TB, coderAPI *coderd.API) io.Closer {
assert.NoError(t, err)
}()

daemon := provisionerd.New(func(ctx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) {
return coderAPI.CreateInMemoryProvisionerDaemon(ctx, "test")
daemon := provisionerd.New(func(dialCtx context.Context) (provisionerdproto.DRPCProvisionerDaemonClient, error) {
return coderAPI.CreateInMemoryProvisionerDaemon(dialCtx, "test")
}, &provisionerd.Options{
Logger: coderAPI.Logger.Named("provisionerd").Leveled(slog.LevelDebug),
UpdateInterval: 250 * time.Millisecond,
Expand Down