Actually starting to use the deallocator to clean up services

**- What I did**

This patch allows users to choose to make removals of services and networks
asynchronous, which means that they only get deleted once all the containers
belonging to them or using them are safely shut down.

**- How I did it**

A previous patch (#2759) introduced a
new component, the deallocator, responsible for cleaning up services and
service-level resources. This patch is actually starting to make use of that
component.

Since the deallocator used to rely on the reaper deleting the tasks belonging
to services that had been marked for removal, a previous version of this patch
was modifying the task reaper quite heavily to also keep track of such services
(needed since otherwise the reaper would fail to clean up all of them, instead
keeping some for history tracking purposes). However, it soon appeared that
this was not the best approach:
 * this creates a hidden coupling between the reaper and the deallocator
 * it's also not the best user experience to suddenly remove all of a service's
   task history while shutting down, for not apparent reason to the user

For these reasons, this patch instead amends the deallocator to also look at tasks status when keeping track of how many alive tasks remain to a service.

Part of this test was already reviewed in
#2778, which subsequently got reverted
following the discussion in moby/moby#38525.
The main difference between this PR and #2778 is that asynchronous removal is
an opt-in behaviour here instead of changing the existing behaviour.

**- How to test it**

Updated tests.

**- Description for the changelog**

Services & networks can be deleted asynchronously, in which case the request
for their removal simply marks them for removal, and they are then actually
deleted only when all their tasks are actually shut down.

Signed-off-by: Jean Rouge <rougej+github@gmail.com>

api/api.pb.txt

@@ -7048,6 +7048,13 @@ file {
       type: TYPE_STRING
       json_name: "serviceId"
     }
+    field {
+      name: "with_soft_delete"
+      number: 2
+      label: LABEL_OPTIONAL
+      type: TYPE_BOOL
+      json_name: "withSoftDelete"
+    }
   }
   message_type {
     name: "RemoveServiceResponse"
@@ -7199,6 +7206,13 @@ file {
       type: TYPE_STRING
       json_name: "networkId"
     }
+    field {
+      name: "with_soft_delete"
+      number: 3
+      label: LABEL_OPTIONAL
+      type: TYPE_BOOL
+      json_name: "withSoftDelete"
+    }
   }
   message_type {
     name: "RemoveNetworkResponse"

api/control.proto

@@ -300,6 +300,10 @@ message UpdateServiceResponse {
 
 message RemoveServiceRequest {
 	string service_id = 1;
+	// WithSoftDelete marks whether the user has requested a soft, ie asynchronous removal
+	// In that case, the service will only be removed after all of its containers
+	// have safely shut down, instead of being removed right away
+	bool with_soft_delete = 2;
 }
 
 message RemoveServiceResponse {
@@ -342,6 +346,10 @@ message GetNetworkResponse {
 message RemoveNetworkRequest {
 	string name = 1;
 	string network_id = 2;
+	// WithSoftDelete marks whether the user has requested a soft, ie asynchronous removal
+	// In that case, the network will only be removed after all the containers using it
+	// have safely shut down, instead of being removed right away
+	bool with_soft_delete = 3;
 }
 
 message RemoveNetworkResponse {}

cmd/swarmctl/network/inspect.go

@@ -51,6 +51,10 @@ func printNetworkSummary(network *api.Network) {
 	common.FprintfIfNotEmpty(w, "ID\t: %s\n", network.ID)
 	common.FprintfIfNotEmpty(w, "Name\t: %s\n", spec.Annotations.Name)
 
+	if network.PendingDelete {
+		common.FprintfIfNotEmpty(w, "[Network %s marked for removal]\n", spec.Annotations.Name)
+	}
+
 	fmt.Fprintln(w, "Spec:\t")
 	if len(spec.Annotations.Labels) > 0 {
 		fmt.Fprintln(w, "  Labels:\t")

cmd/swarmctl/service/inspect.go

@@ -22,12 +22,17 @@ func printServiceSummary(service *api.Service, running int) {
 	w := tabwriter.NewWriter(os.Stdout, 8, 8, 8, ' ', 0)
 	defer w.Flush()
 
-	task := service.Spec.Task
+	spec := service.Spec
 	common.FprintfIfNotEmpty(w, "ID\t: %s\n", service.ID)
-	common.FprintfIfNotEmpty(w, "Name\t: %s\n", service.Spec.Annotations.Name)
-	if len(service.Spec.Annotations.Labels) > 0 {
+	common.FprintfIfNotEmpty(w, "Name\t: %s\n", spec.Annotations.Name)
+
+	if service.PendingDelete {
+		common.FprintfIfNotEmpty(w, "[Service %s marked for removal]\n", spec.Annotations.Name)
+	}
+
+	if len(spec.Annotations.Labels) > 0 {
 		fmt.Fprintln(w, "Labels\t")
-		for k, v := range service.Spec.Annotations.Labels {
+		for k, v := range spec.Annotations.Labels {
 			fmt.Fprintf(w, "  %s\t: %s\n", k, v)
 		}
 	}
@@ -51,7 +56,8 @@ func printServiceSummary(service *api.Service, running int) {
 
 	fmt.Fprintln(w, "Template\t")
 	fmt.Fprintln(w, " Container\t")
-	ctr := service.Spec.Task.GetContainer()
+	task := spec.Task
+	ctr := task.GetContainer()
 	common.FprintfIfNotEmpty(w, "  Image\t: %s\n", ctr.Image)
 	common.FprintfIfNotEmpty(w, "  Command\t: %q\n", strings.Join(ctr.Command, " "))
 	common.FprintfIfNotEmpty(w, "  Args\t: [%s]\n", strings.Join(ctr.Args, ", "))
@@ -90,7 +96,7 @@ func printServiceSummary(service *api.Service, running int) {
 			printResources(w, res.Limits)
 		}
 	}
-	if len(service.Spec.Task.Networks) > 0 {
+	if len(spec.Task.Networks) > 0 {
 		fmt.Fprint(w, "  Networks:")
 		for _, n := range service.Spec.Task.Networks {
 			fmt.Fprintf(w, " %s", n.Target)

manager/controlapi/network.go

@@ -164,7 +164,7 @@ func (s *Server) RemoveNetwork(ctx context.Context, request *api.RemoveNetworkRe
 
 	var (
 		n  *api.Network
-		rm = s.removeNetwork
+		rm = func(networkID string) error { return s.removeNetwork(networkID, request.WithSoftDelete) }
 	)
 
 	s.store.View(func(tx store.ReadTx) {
@@ -192,15 +192,26 @@ func (s *Server) RemoveNetwork(ctx context.Context, request *api.RemoveNetworkRe
 	return &api.RemoveNetworkResponse{}, nil
 }
 
-func (s *Server) removeNetwork(id string) error {
+func (s *Server) removeNetwork(id string, withSoftDelete bool) error {
 	return s.store.Update(func(tx store.Tx) error {
 		services, err := store.FindServices(tx, store.ByReferencedNetworkID(id))
 		if err != nil {
 			return status.Errorf(codes.Internal, "could not find services using network %s: %v", id, err)
 		}
 
-		if len(services) != 0 {
-			return status.Errorf(codes.FailedPrecondition, "network %s is in use by service %s", id, services[0].ID)
+		var stillInUseByService *api.Service
+		if withSoftDelete {
+			for _, service := range services {
+				if !service.PendingDelete {
+					stillInUseByService = service
+					break
+				}
+			}
+		} else if len(services) != 0 {
+			stillInUseByService = services[0]
+		}
+		if stillInUseByService != nil {
+			return status.Errorf(codes.FailedPrecondition, "network %s is in use by service %s", id, stillInUseByService.ID)
 		}
 
 		tasks, err := store.FindTasks(tx, store.ByReferencedNetworkID(id))
@@ -214,6 +225,14 @@ func (s *Server) removeNetwork(id string) error {
 			}
 		}
 
+		if withSoftDelete {
+			network := store.GetNetwork(tx, id)
+			if network == nil {
+				return status.Errorf(codes.NotFound, "network %s not found", id)
+			}
+			network.PendingDelete = true
+			return store.UpdateNetwork(tx, network)
+		}
 		return store.DeleteNetwork(tx, id)
 	})
 }

manager/controlapi/network_test.go

@@ -191,11 +191,30 @@ func TestRemoveNetworkWithAttachedService(t *testing.T) {
 	assert.NoError(t, err)
 	assert.NotEqual(t, nr.Network, nil)
 	assert.NotEqual(t, nr.Network.ID, "")
-	createServiceInNetwork(t, ts, "name", "image", nr.Network.ID, 1)
+	createServiceInNetwork(t, ts, "service1", "image", nr.Network.ID, 1)
 	_, err = ts.Client.RemoveNetwork(context.Background(), &api.RemoveNetworkRequest{NetworkID: nr.Network.ID})
 	assert.Error(t, err)
 }
 
+func TestRemoveNetworkWithAttachedServiceMarkedForRemoval(t *testing.T) {
+	ts := newTestServer(t)
+	defer ts.Stop()
+	nr, err := ts.Client.CreateNetwork(context.Background(), &api.CreateNetworkRequest{
+		Spec: createNetworkSpec("testnet5"),
+	})
+	assert.NoError(t, err)
+	assert.NotEqual(t, nr.Network, nil)
+	assert.NotEqual(t, nr.Network.ID, "")
+	service := createServiceInNetwork(t, ts, "service2", "image", nr.Network.ID, 1)
+	// then let's delete the service
+	r, err := ts.Client.RemoveService(context.Background(), &api.RemoveServiceRequest{ServiceID: service.ID, WithSoftDelete: true})
+	assert.NoError(t, err)
+	assert.NotNil(t, r)
+	// now we should be able to delete the network
+	_, err = ts.Client.RemoveNetwork(context.Background(), &api.RemoveNetworkRequest{NetworkID: nr.Network.ID, WithSoftDelete: true})
+	assert.NoError(t, err)
+}
+
 func TestCreateNetworkInvalidDriver(t *testing.T) {
 	ts := newTestServer(t)
 	defer ts.Stop()

manager/controlapi/service.go

@@ -722,6 +722,7 @@ func (s *Server) GetService(ctx context.Context, request *api.GetServiceRequest)
 // - Returns `NotFound` if the Service is not found.
 // - Returns `InvalidArgument` if the ServiceSpec is malformed.
 // - Returns `Unimplemented` if the ServiceSpec references unimplemented features.
+// - Returns `FailedPrecondition` if the Service is marked for removal
 // - Returns an error if the update fails.
 func (s *Server) UpdateService(ctx context.Context, request *api.UpdateServiceRequest) (*api.UpdateServiceResponse, error) {
 	if request.ServiceID == "" || request.ServiceVersion == nil {
@@ -755,6 +756,12 @@ func (s *Server) UpdateService(ctx context.Context, request *api.UpdateServiceRe
 			return status.Errorf(codes.NotFound, "service %s not found", request.ServiceID)
 		}
 
+		// we couldn't do this any sooner, as we do need to be holding the lock
+		// when checking for this flag
+		if service.PendingDelete {
+			return status.Errorf(codes.FailedPrecondition, "service %s is marked for removal", request.ServiceID)
+		}
+
 		// It's not okay to update Service.Spec.Networks on its own.
 		// However, if Service.Spec.Task.Networks is also being
 		// updated, that's okay (for example when migrating from the
@@ -848,6 +855,15 @@ func (s *Server) RemoveService(ctx context.Context, request *api.RemoveServiceRe
 	}
 
 	err := s.store.Update(func(tx store.Tx) error {
+		if request.WithSoftDelete {
+			service := store.GetService(tx, request.ServiceID)
+			if service == nil {
+				return store.ErrNotExist
+			}
+			// mark service for removal
+			service.PendingDelete = true
+			return store.UpdateService(tx, service)
+		}
 		return store.DeleteService(tx, request.ServiceID)
 	})
 	if err != nil {

manager/controlapi/service_test.go

@@ -956,6 +956,19 @@ func TestUpdateService(t *testing.T) {
 	assert.Equal(t, codes.InvalidArgument, testutils.ErrorCode(err))
 }
 
+func TestUpdateServiceMarkedForRemoval(t *testing.T) {
+	ts := newTestServer(t)
+	defer ts.Stop()
+
+	service := createService(t, ts, "name", "image", 1)
+	r, err := ts.Client.RemoveService(context.Background(), &api.RemoveServiceRequest{ServiceID: service.ID, WithSoftDelete: true})
+	assert.NoError(t, err)
+	assert.NotNil(t, r)
+
+	_, err = ts.Client.UpdateService(context.Background(), &api.UpdateServiceRequest{ServiceID: service.ID, Spec: &service.Spec, ServiceVersion: &service.Meta.Version})
+	assert.Equal(t, codes.FailedPrecondition, testutils.ErrorCode(err))
+}
+
 func TestServiceUpdateRejectNetworkChange(t *testing.T) {
 	ts := newTestServer(t)
 	defer ts.Stop()