fix/service delete deadline (#227)

* fix: service container wait

only apply if the destroy grace period is larger than 0s. Interpret also the wait result chan and errors

* chore(service): explains why we ignore certain errors

* chore: readds terraform-plugin-docs

* docs(service): explains the behavior if the grace period is 0s

* fix(service): ignore certain errors on wait error

* refactor: introduce more speaking error contains ignorable message method

because it's more precise on what's happening in the code

Author: mavogel

docs/resources/service.md

@@ -354,7 +354,7 @@ Optional:
 - **privileges** (Block List, Max: 1) Security options for the container (see [below for nested schema](#nestedblock--task_spec--container_spec--privileges))
 - **read_only** (Boolean) Mount the container's root filesystem as read only
 - **secrets** (Block Set) References to zero or more secrets that will be exposed to the service (see [below for nested schema](#nestedblock--task_spec--container_spec--secrets))
-- **stop_grace_period** (String) Amount of time to wait for the container to terminate before forcefully removing it (ms|s|m|h)
+- **stop_grace_period** (String) Amount of time to wait for the container to terminate before forcefully removing it (ms|s|m|h). If not specified or '0s' the destroy will not check if all tasks/containers of the service terminate.
 - **stop_signal** (String) Signal to stop the container
 - **user** (String) The user inside the container
 

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/docker/go-connections v0.4.0
 	github.com/docker/go-units v0.4.0
 	github.com/hashicorp/go-cty v1.4.1-0.20200414143053-d3edf31b6320
+	github.com/hashicorp/terraform-plugin-docs v0.4.0 // indirect
 	github.com/hashicorp/terraform-plugin-sdk/v2 v2.6.1
 	github.com/mattn/go-colorable v0.1.8 // indirect
 	github.com/mitchellh/go-homedir v1.1.0

go.sum

@@ -122,3 +122,15 @@ func testCheckLabelMap(name string, partialKey string, expectedLabels map[string
 		return nil
 	}
 }
+
+// containsIgnorableErrorMessage checks if the error message contains one of the
+// message to ignore. Returns true if so, false otherwise (also if no ignorable message is given)
+func containsIgnorableErrorMessage(errorMsg string, ignorableErrorMessages ...string) bool {
+	for _, ignorableErrorMessage := range ignorableErrorMessages {
+		if strings.Contains(errorMsg, ignorableErrorMessage) {
+			return true
+		}
+	}
+
+	return false
+}

internal/provider/helpers.go

@@ -355,7 +355,7 @@ func resourceDockerContainerCreate(ctx context.Context, d *schema.ResourceData,
 	// Still support the deprecated properties
 	if v, ok := d.GetOk("networks"); ok {
 		if err := client.NetworkDisconnect(ctx, "bridge", retContainer.ID, false); err != nil {
-			if !strings.Contains(err.Error(), "is not connected to the network bridge") {
+			if !containsIgnorableErrorMessage(err.Error(), "is not connected to the network bridge") {
 				return diag.Errorf("Unable to disconnect the default network: %s", err)
 			}
 		}
@@ -375,7 +375,7 @@ func resourceDockerContainerCreate(ctx context.Context, d *schema.ResourceData,
 	// But overwrite them with the future ones, if set
 	if v, ok := d.GetOk("networks_advanced"); ok {
 		if err := client.NetworkDisconnect(ctx, "bridge", retContainer.ID, false); err != nil {
-			if !strings.Contains(err.Error(), "is not connected to the network bridge") {
+			if !containsIgnorableErrorMessage(err.Error(), "is not connected to the network bridge") {
 				return diag.Errorf("Unable to disconnect the default network: %s", err)
 			}
 		}
@@ -848,7 +848,7 @@ func resourceDockerContainerDelete(ctx context.Context, d *schema.ResourceData,
 	case waitOk := <-waitOkC:
 		log.Printf("[INFO] Container exited with code [%v]: '%s'", waitOk.StatusCode, d.Id())
 	case err := <-errorC:
-		if !(strings.Contains(err.Error(), "No such container") || strings.Contains(err.Error(), "is already in progress")) {
+		if !containsIgnorableErrorMessage(err.Error(), "No such container", "is already in progress") {
 			return diag.Errorf("Error waiting for container removal '%s': %s", d.Id(), err)
 		}
 	}

internal/provider/resource_docker_container_funcs.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"encoding/json"
 	"log"
-	"strings"
 	"time"
 
 	"github.com/docker/docker/api/types"
@@ -201,7 +200,7 @@ func resourceDockerNetworkRemoveRefreshFunc(ctx context.Context,
 		}
 
 		if err := client.NetworkRemove(ctx, networkID); err != nil {
-			if strings.Contains(err.Error(), "has active endpoints") {
+			if containsIgnorableErrorMessage(err.Error(), "has active endpoints") {
 				return networkID, "pending", nil
 			}
 			return networkID, "other", err

internal/provider/resource_docker_network_funcs.go

@@ -303,7 +303,7 @@ func resourceDockerService() *schema.Resource {
 									},
 									"stop_grace_period": {
 										Type:             schema.TypeString,
-										Description:      "Amount of time to wait for the container to terminate before forcefully removing it (ms|s|m|h)",
+										Description:      "Amount of time to wait for the container to terminate before forcefully removing it (ms|s|m|h). If not specified or '0s' the destroy will not check if all tasks/containers of the service terminate.",
 										Optional:         true,
 										Computed:         true,
 										ValidateDiagFunc: validateDurationGeq0(),

internal/provider/resource_docker_service.go

@@ -67,7 +67,7 @@ func resourceDockerServiceCreate(ctx context.Context, d *schema.ResourceData, me
 			if deleteErr := deleteService(ctx, service.ID, d, client); deleteErr != nil {
 				return diag.FromErr(deleteErr)
 			}
-			if strings.Contains(err.Error(), "timeout while waiting for state") {
+			if containsIgnorableErrorMessage(err.Error(), "timeout while waiting for state") {
 				return diag.FromErr(&DidNotConvergeError{ServiceID: service.ID, Timeout: convergeConfig.timeout})
 			}
 			return diag.FromErr(err)
@@ -205,7 +205,7 @@ func resourceDockerServiceUpdate(ctx context.Context, d *schema.ResourceData, me
 		state, err := stateConf.WaitForStateContext(ctx)
 		log.Printf("[INFO] State awaited: %v with error: %v", state, err)
 		if err != nil {
-			if strings.Contains(err.Error(), "timeout while waiting for state") {
+			if containsIgnorableErrorMessage(err.Error(), "timeout while waiting for state") {
 				return diag.FromErr(&DidNotConvergeError{ServiceID: service.ID, Timeout: convergeConfig.timeout})
 			}
 			return diag.FromErr(err)
@@ -249,7 +249,7 @@ func fetchDockerService(ctx context.Context, ID string, name string, client *cli
 func deleteService(ctx context.Context, serviceID string, d *schema.ResourceData, client *client.Client) error {
 	// get containerIDs of the running service because they do not exist after the service is deleted
 	serviceContainerIds := make([]string, 0)
-	if _, ok := d.GetOk("task_spec.0.container_spec.0.stop_grace_period"); ok {
+	if v, ok := d.GetOk("task_spec.0.container_spec.0.stop_grace_period"); ok && v.(string) != "0s" {
 		filters := filters.NewArgs()
 		filters.Add("service", d.Get("name").(string))
 		tasks, err := client.TaskList(ctx, types.TaskListOptions{
@@ -264,39 +264,58 @@ func deleteService(ctx context.Context, serviceID string, d *schema.ResourceData
 			if task.Status.ContainerStatus != nil {
 				containerID = task.Status.ContainerStatus.ContainerID
 			}
-			log.Printf("[INFO] Found container ['%s'] for destroying: '%s'", task.Status.State, containerID)
+			log.Printf("[INFO] Found container with ID ['%s'] in state '%s' for destroying", containerID, task.Status.State)
 			if strings.TrimSpace(containerID) != "" && task.Status.State != swarm.TaskStateShutdown {
 				serviceContainerIds = append(serviceContainerIds, containerID)
 			}
 		}
 	}
 
 	// delete the service
-	log.Printf("[INFO] Deleting service: '%s'", serviceID)
+	log.Printf("[INFO] Deleting service with ID: '%s'", serviceID)
 	if err := client.ServiceRemove(ctx, serviceID); err != nil {
-		return fmt.Errorf("Error deleting service %s: %s", serviceID, err)
+		return fmt.Errorf("Error deleting service with ID '%s': %s", serviceID, err)
 	}
 
 	// destroy each container after a grace period if specified
-	if v, ok := d.GetOk("task_spec.0.container_spec.0.stop_grace_period"); ok {
+	if v, ok := d.GetOk("task_spec.0.container_spec.0.stop_grace_period"); ok && v.(string) != "0s" {
 		for _, containerID := range serviceContainerIds {
-			destroyGraceSeconds, _ := time.ParseDuration(v.(string))
-			log.Printf("[INFO] Waiting for container: '%s' to exit: max %v", containerID, destroyGraceSeconds)
-			ctx, cancel := context.WithTimeout(ctx, destroyGraceSeconds)
-			// TODO why defer? see container_resource with handling return channels! why not remove then wait?
+			destroyGraceTime, _ := time.ParseDuration(v.(string))
+			log.Printf("[INFO] Waiting for container with ID: '%s' to exit: max %v", containerID, destroyGraceTime)
+			ctx, cancel := context.WithTimeout(ctx, destroyGraceTime)
+			// We defer explicitly to avoid context leaks
 			defer cancel()
-			exitCode, _ := client.ContainerWait(ctx, containerID, container.WaitConditionRemoved)
-			log.Printf("[INFO] Container exited with code [%v]: '%s'", exitCode, containerID)
+
+			containerWaitChan, containerWaitErrChan := client.ContainerWait(ctx, containerID, container.WaitConditionRemoved)
+			select {
+			case containerWaitResult := <-containerWaitChan:
+				if containerWaitResult.Error != nil {
+					// We ignore those types of errors because the container might be already removed before
+					// the containerWait returns
+					if !(containsIgnorableErrorMessage(containerWaitResult.Error.Message, "No such container")) {
+						return fmt.Errorf("failed to wait for container with ID '%s': '%v'", containerID, containerWaitResult.Error.Message)
+					}
+				}
+				log.Printf("[INFO] Container with ID '%s' exited with code '%v'", containerID, containerWaitResult.StatusCode)
+			case containerWaitErrResult := <-containerWaitErrChan:
+				// We ignore those types of errors because the container might be already removed before
+				// the containerWait returns
+				if !(containsIgnorableErrorMessage(containerWaitErrResult.Error(), "No such container")) {
+					return fmt.Errorf("error on wait for container with ID '%s': %v", containerID, containerWaitErrResult)
+				}
+			}
 
 			removeOpts := types.ContainerRemoveOptions{
 				RemoveVolumes: true,
 				Force:         true,
 			}
 
-			log.Printf("[INFO] Removing container: '%s'", containerID)
+			log.Printf("[INFO] Removing container with ID: '%s'", containerID)
 			if err := client.ContainerRemove(ctx, containerID, removeOpts); err != nil {
-				if !(strings.Contains(err.Error(), "No such container") || strings.Contains(err.Error(), "is already in progress")) {
-					return fmt.Errorf("Error deleting container %s: %s", containerID, err)
+				// We ignore those types of errors because the container might be already removed of the removal is in progress
+				// before the containerRemove call happens
+				if !containsIgnorableErrorMessage(err.Error(), "No such container", "is already in progress") {
+					return fmt.Errorf("Error deleting container with ID '%s': %s", containerID, err)
 				}
 			}
 		}

internal/provider/resource_docker_service_funcs.go

@@ -6,7 +6,6 @@ import (
 	"os"
 	"regexp"
 	"strconv"
-	"strings"
 	"testing"
 	"time"
 
@@ -1328,7 +1327,7 @@ func checkAndRemoveImages(ctx context.Context, s *terraform.State) error {
 			Force: true,
 		})
 		if err != nil {
-			if strings.Contains(err.Error(), "image is being used by running container") {
+			if containsIgnorableErrorMessage(err.Error(), "image is being used by running container") {
 				if retryDeleteCount == maxRetryDeleteCount {
 					return fmt.Errorf("could not delete image '%s' after %d retries", image.ID, maxRetryDeleteCount)
 				}

internal/provider/resource_docker_service_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"encoding/json"
 	"log"
-	"strings"
 	"time"
 
 	"github.com/docker/docker/api/types"
@@ -158,7 +157,7 @@ func resourceDockerVolumeRemoveRefreshFunc(
 		forceDelete := true
 
 		if err := client.VolumeRemove(context.Background(), volumeID, forceDelete); err != nil {
-			if strings.Contains(err.Error(), "volume is in use") { // store.IsInUse(err)
+			if containsIgnorableErrorMessage(err.Error(), "volume is in use") {
 				log.Printf("[INFO] Volume with id '%v' is still in use", volumeID)
 				return volumeID, "in_use", nil
 			}