Conversation
Signed-off-by: Jacob Howard <jacob.howard@docker.com>
| if errdefs.IsConflict(err) { | ||
| if err := waitForContainerToStart(ctx, dockerClient, controllerContainerName); err != nil { |
There was a problem hiding this comment.
I just had a thought; given that the "start" endpoint is effectively idempotent for containers that are already started, we could take advantage of that, and after we verified we either created the container, or got a "conflict" (already exists), we can probably just use container start with a retry;
https://github.com/moby/moby/blob/5318877858c9fc94b5cccc3cf1a75d4ec46951b8/daemon/start.go#L76-L93
func (daemon *Daemon) containerStart(ctx context.Context, daemonCfg *configStore, container *container.Container, checkpoint string, checkpointDir string, resetRestartManager bool) (retErr error) {
ctx, span := otel.Tracer("").Start(ctx, "daemon.containerStart", trace.WithAttributes(append(
labelsAsOTelAttributes(container.Config.Labels),
attribute.String("container.ID", container.ID),
attribute.String("container.Name", container.Name),
)...))
defer func() {
otelutil.RecordStatus(span, retErr)
span.End()
}()
start := time.Now()
container.Lock()
defer container.Unlock()
if resetRestartManager && container.Running { // skip this check if already in restarting step and resetRestartManager==false
return nil
}I haven't verified the approach, but let me write up a quick patch with that idea
There was a problem hiding this comment.
@xenoscopic OK; I pushed a PR that's opened against this branch (but feel free to squash it)
|
Closing in favor of #107, which incorporates these commits. |
@thaJeztah suggested a more idiomatic approach to error detection. Also we can use the container name rather than ID for inspection, so we don't even need to capture it from the error message.