- Notifications
You must be signed in to change notification settings - Fork1k
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1371,10 +1371,10 @@ func newProvisionerDaemon( | ||
| connector[string(database.ProvisionerTypeTerraform)] = sdkproto.NewDRPCProvisionerClient(terraformClient) | ||
| } | ||
| return provisionerd.New(func(dialCtx context.Context) (proto.DRPCProvisionerDaemonClient, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. It's not clear from here that this function argument is | ||
| // This debounces calls to listen every second. Read the comment | ||
| // in provisionerdserver.go to learn more! | ||
| return coderAPI.CreateInMemoryProvisionerDaemon(dialCtx, name) | ||
| }, &provisionerd.Options{ | ||
| Logger: logger.Named(fmt.Sprintf("provisionerd-%s", name)), | ||
| UpdateInterval: time.Second, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(dialCtx context.Context, name string) (client proto.DRPCProvisionerDaemonClient, err error) { | ||
| tracer := api.TracerProvider.Tracer(tracing.TracerName) | ||
| clientSession, serverSession := drpc.MemTransportPipe() | ||
| defer func() { | ||
| @@ -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(dialCtx), database.UpsertProvisionerDaemonParams{ | ||
| Name: name, | ||
| CreatedAt: dbtime.Now(), | ||
| Provisioners: []database.ProvisionerType{ | ||
| @@ -1201,7 +1201,7 @@ func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context, name string | ||
| } | ||
| mux := drpcmux.New() | ||
| 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 | ||
| @@ -1238,13 +1238,25 @@ func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context, name string | ||
| if xerrors.Is(err, io.EOF) { | ||
| return | ||
| } | ||
| 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() { | ||
| 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||