Add a lockedManagerAction method to Cluster…

… in order to remove duplication.
Each time we update a cluster object, we do some common
operations (lock, verify it's on a manager, get the request context,
and the update). This introduce a method and refactor few
update/remove method that allows to duplicate less code.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>

daemon/cluster/cluster.go

@@ -386,3 +386,18 @@ func detectLockedError(err error) error {
 	}
 	return err
 }
+
+func (c *Cluster) lockedManagerAction(fn func(ctx context.Context, state nodeState) error) error {
+	c.mu.RLock()
+	defer c.mu.RUnlock()
+
+	state := c.currentNodeState()
+	if !state.IsActiveManager() {
+		return c.errNoManager(state)
+	}
+
+	ctx, cancel := c.getRequestContext()
+	defer cancel()
+
+	return fn(ctx, state)
+}

daemon/cluster/networks.go

@@ -48,19 +48,16 @@ func (c *Cluster) getNetworks(filters *swarmapi.ListNetworksRequest_Filters) ([]
 
 // GetNetwork returns a cluster network by an ID.
 func (c *Cluster) GetNetwork(input string) (apitypes.NetworkResource, error) {
-	c.mu.RLock()
+	var network *swarmapi.Network
-	defer c.mu.RUnlock()
 
-	state := c.currentNodeState()
+	if err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-	if !state.IsActiveManager() {
+		n, err := getNetwork(ctx, state.controlClient, input)
-		return apitypes.NetworkResource{}, c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
-	network, err := getNetwork(ctx, state.controlClient, input)
 		if err != nil {
+			return err
+		}
+		network = n
+		return nil
+	}); err != nil {
 		return apitypes.NetworkResource{}, err
 	}
 	return convert.BasicNetworkFromGRPC(*network), nil
@@ -224,44 +221,30 @@ func (c *Cluster) DetachNetwork(target string, containerID string) error {
 
 // CreateNetwork creates a new cluster managed network.
 func (c *Cluster) CreateNetwork(s apitypes.NetworkCreateRequest) (string, error) {
-	c.mu.RLock()
-	defer c.mu.RUnlock()
-
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return "", c.errNoManager(state)
-	}
-
 	if runconfig.IsPreDefinedNetwork(s.Name) {
 		err := fmt.Errorf("%s is a pre-defined network and cannot be created", s.Name)
 		return "", apierrors.NewRequestForbiddenError(err)
 	}
 
-	ctx, cancel := c.getRequestContext()
+	var resp *swarmapi.CreateNetworkResponse
-	defer cancel()
+	if err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-
 		networkSpec := convert.BasicNetworkCreateToGRPC(s)
 		r, err := state.controlClient.CreateNetwork(ctx, &swarmapi.CreateNetworkRequest{Spec: &networkSpec})
 		if err != nil {
+			return err
+		}
+		resp = r
+		return nil
+	}); err != nil {
 		return "", err
 	}
 
-	return r.Network.ID, nil
+	return resp.Network.ID, nil
 }
 
 // RemoveNetwork removes a cluster network.
 func (c *Cluster) RemoveNetwork(input string) error {
-	c.mu.RLock()
+	return c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-	defer c.mu.RUnlock()
-
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
 		network, err := getNetwork(ctx, state.controlClient, input)
 		if err != nil {
 			return err
@@ -269,6 +252,7 @@ func (c *Cluster) RemoveNetwork(input string) error {
 
 		_, err = state.controlClient.RemoveNetwork(ctx, &swarmapi.RemoveNetworkRequest{NetworkID: network.ID})
 		return err
+	})
 }
 
 func (c *Cluster) populateNetworkID(ctx context.Context, client swarmapi.ControlClient, s *types.ServiceSpec) error {

daemon/cluster/nodes.go

@@ -6,6 +6,7 @@ import (
 	types "github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/daemon/cluster/convert"
 	swarmapi "github.com/docker/swarmkit/api"
+	"golang.org/x/net/context"
 )
 
 // GetNodes returns a list of all nodes known to a cluster.
@@ -43,34 +44,25 @@ func (c *Cluster) GetNodes(options apitypes.NodeListOptions) ([]types.Node, erro
 
 // GetNode returns a node based on an ID.
 func (c *Cluster) GetNode(input string) (types.Node, error) {
-	c.mu.RLock()
+	var node *swarmapi.Node
-	defer c.mu.RUnlock()
 
-	state := c.currentNodeState()
+	if err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-	if !state.IsActiveManager() {
+		n, err := getNode(ctx, state.controlClient, input)
-		return types.Node{}, c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
-	node, err := getNode(ctx, state.controlClient, input)
 		if err != nil {
+			return err
+		}
+		node = n
+		return nil
+	}); err != nil {
 		return types.Node{}, err
 	}
+
 	return convert.NodeFromGRPC(*node), nil
 }
 
 // UpdateNode updates existing nodes properties.
 func (c *Cluster) UpdateNode(input string, version uint64, spec types.NodeSpec) error {
-	c.mu.RLock()
+	return c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-	defer c.mu.RUnlock()
-
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return c.errNoManager(state)
-	}
-
 		nodeSpec, err := convert.NodeSpecToGRPC(spec)
 		if err != nil {
 			return apierrors.NewBadRequestError(err)
@@ -95,21 +87,12 @@ func (c *Cluster) UpdateNode(input string, version uint64, spec types.NodeSpec)
 			},
 		)
 		return err
+	})
 }
 
 // RemoveNode removes a node from a cluster
 func (c *Cluster) RemoveNode(input string, force bool) error {
-	c.mu.RLock()
+	return c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-	defer c.mu.RUnlock()
-
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
 		node, err := getNode(ctx, state.controlClient, input)
 		if err != nil {
 			return err
@@ -117,4 +100,5 @@ func (c *Cluster) RemoveNode(input string, force bool) error {
 
 		_, err = state.controlClient.RemoveNode(ctx, &swarmapi.RemoveNodeRequest{NodeID: node.ID, Force: force})
 		return err
+	})
 }

daemon/cluster/secrets.go

@@ -5,23 +5,21 @@ import (
 	types "github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/daemon/cluster/convert"
 	swarmapi "github.com/docker/swarmkit/api"
+	"golang.org/x/net/context"
 )
 
 // GetSecret returns a secret from a managed swarm cluster
 func (c *Cluster) GetSecret(input string) (types.Secret, error) {
-	c.mu.RLock()
+	var secret *swarmapi.Secret
-	defer c.mu.RUnlock()
 
-	state := c.currentNodeState()
+	if err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-	if !state.IsActiveManager() {
+		s, err := getSecret(ctx, state.controlClient, input)
-		return types.Secret{}, c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
-	secret, err := getSecret(ctx, state.controlClient, input)
 		if err != nil {
+			return err
+		}
+		secret = s
+		return nil
+	}); err != nil {
 		return types.Secret{}, err
 	}
 	return convert.SecretFromGRPC(secret), nil
@@ -61,41 +59,26 @@ func (c *Cluster) GetSecrets(options apitypes.SecretListOptions) ([]types.Secret
 
 // CreateSecret creates a new secret in a managed swarm cluster.
 func (c *Cluster) CreateSecret(s types.SecretSpec) (string, error) {
-	c.mu.RLock()
+	var resp *swarmapi.CreateSecretResponse
-	defer c.mu.RUnlock()
+	if err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return "", c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
 		secretSpec := convert.SecretSpecToGRPC(s)
 
 		r, err := state.controlClient.CreateSecret(ctx,
 			&swarmapi.CreateSecretRequest{Spec: &secretSpec})
 		if err != nil {
+			return err
+		}
+		resp = r
+		return nil
+	}); err != nil {
 		return "", err
 	}
-
+	return resp.Secret.ID, nil
-	return r.Secret.ID, nil
 }
 
 // RemoveSecret removes a secret from a managed swarm cluster.
 func (c *Cluster) RemoveSecret(input string) error {
-	c.mu.RLock()
+	return c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-	defer c.mu.RUnlock()
-
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
 		secret, err := getSecret(ctx, state.controlClient, input)
 		if err != nil {
 			return err
@@ -107,22 +90,13 @@ func (c *Cluster) RemoveSecret(input string) error {
 
 		_, err = state.controlClient.RemoveSecret(ctx, req)
 		return err
+	})
 }
 
 // UpdateSecret updates a secret in a managed swarm cluster.
 // Note: this is not exposed to the CLI but is available from the API only
 func (c *Cluster) UpdateSecret(id string, version uint64, spec types.SecretSpec) error {
-	c.mu.RLock()
+	return c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-	defer c.mu.RUnlock()
-
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
 		secretSpec := convert.SecretSpecToGRPC(spec)
 
 		_, err := state.controlClient.UpdateSecret(ctx,
@@ -134,4 +108,5 @@ func (c *Cluster) UpdateSecret(id string, version uint64, spec types.SecretSpec)
 				Spec: &secretSpec,
 			})
 		return err
+	})
 }

daemon/cluster/services.go

@@ -59,19 +59,15 @@ func (c *Cluster) GetServices(options apitypes.ServiceListOptions) ([]types.Serv
 
 // GetService returns a service based on an ID or name.
 func (c *Cluster) GetService(input string) (types.Service, error) {
-	c.mu.RLock()
+	var service *swarmapi.Service
-	defer c.mu.RUnlock()
+	if err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-
+		s, err := getService(ctx, state.controlClient, input)
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return types.Service{}, c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
-	service, err := getService(ctx, state.controlClient, input)
 		if err != nil {
+			return err
+		}
+		service = s
+		return nil
+	}); err != nil {
 		return types.Service{}, err
 	}
 	return convert.ServiceFromGRPC(*service), nil
@@ -79,30 +75,21 @@ func (c *Cluster) GetService(input string) (types.Service, error) {
 
 // CreateService creates a new service in a managed swarm cluster.
 func (c *Cluster) CreateService(s types.ServiceSpec, encodedAuth string) (*apitypes.ServiceCreateResponse, error) {
-	c.mu.RLock()
+	var resp *apitypes.ServiceCreateResponse
-	defer c.mu.RUnlock()
+	err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return nil, c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
 		err := c.populateNetworkID(ctx, state.controlClient, &s)
 		if err != nil {
-		return nil, err
+			return err
 		}
 
 		serviceSpec, err := convert.ServiceSpecToGRPC(s)
 		if err != nil {
-		return nil, apierrors.NewBadRequestError(err)
+			return apierrors.NewBadRequestError(err)
 		}
 
 		ctnr := serviceSpec.Task.GetContainer()
 		if ctnr == nil {
-		return nil, errors.New("service does not use container tasks")
+			return errors.New("service does not use container tasks")
 		}
 
 		if encodedAuth != "" {
@@ -117,7 +104,7 @@ func (c *Cluster) CreateService(s types.ServiceSpec, encodedAuth string) (*apity
 			}
 		}
 
-	resp := &apitypes.ServiceCreateResponse{}
+		resp = &apitypes.ServiceCreateResponse{}
 
 		// pin image by digest
 		if os.Getenv("DOCKER_SERVICE_PREFER_OFFLINE_IMAGE") != "1" {
@@ -135,44 +122,39 @@ func (c *Cluster) CreateService(s types.ServiceSpec, encodedAuth string) (*apity
 
 		r, err := state.controlClient.CreateService(ctx, &swarmapi.CreateServiceRequest{Spec: &serviceSpec})
 		if err != nil {
-		return nil, err
+			return err
 		}
 
 		resp.ID = r.Service.ID
-	return resp, nil
+		return nil
+	})
+	return resp, err
 }
 
 // UpdateService updates existing service to match new properties.
 func (c *Cluster) UpdateService(serviceIDOrName string, version uint64, spec types.ServiceSpec, encodedAuth string, registryAuthFrom string) (*apitypes.ServiceUpdateResponse, error) {
-	c.mu.RLock()
+	var resp *apitypes.ServiceUpdateResponse
-	defer c.mu.RUnlock()
 
-	state := c.currentNodeState()
+	err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-	if !state.IsActiveManager() {
-		return nil, c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
 
 		err := c.populateNetworkID(ctx, state.controlClient, &spec)
 		if err != nil {
-		return nil, err
+			return err
 		}
 
 		serviceSpec, err := convert.ServiceSpecToGRPC(spec)
 		if err != nil {
-		return nil, apierrors.NewBadRequestError(err)
+			return apierrors.NewBadRequestError(err)
 		}
 
 		currentService, err := getService(ctx, state.controlClient, serviceIDOrName)
 		if err != nil {
-		return nil, err
+			return err
 		}
 
 		newCtnr := serviceSpec.Task.GetContainer()
 		if newCtnr == nil {
-		return nil, errors.New("service does not use container tasks")
+			return errors.New("service does not use container tasks")
 		}
 
 		if encodedAuth != "" {
@@ -186,14 +168,14 @@ func (c *Cluster) UpdateService(serviceIDOrName string, version uint64, spec typ
 				ctnr = currentService.Spec.Task.GetContainer()
 			case apitypes.RegistryAuthFromPreviousSpec:
 				if currentService.PreviousSpec == nil {
-				return nil, errors.New("service does not have a previous spec")
+					return errors.New("service does not have a previous spec")
 				}
 				ctnr = currentService.PreviousSpec.Task.GetContainer()
 			default:
-			return nil, errors.New("unsupported registryAuthFrom value")
+				return errors.New("unsupported registryAuthFrom value")
 			}
 			if ctnr == nil {
-			return nil, errors.New("service does not use container tasks")
+				return errors.New("service does not use container tasks")
 			}
 			newCtnr.PullOptions = ctnr.PullOptions
 			// update encodedAuth so it can be used to pin image by digest
@@ -236,23 +218,14 @@ func (c *Cluster) UpdateService(serviceIDOrName string, version uint64, spec typ
 				},
 			},
 		)
-
+		return err
+	})
 	return resp, err
 }
 
 // RemoveService removes a service from a managed swarm cluster.
 func (c *Cluster) RemoveService(input string) error {
-	c.mu.RLock()
+	return c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-	defer c.mu.RUnlock()
-
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
 		service, err := getService(ctx, state.controlClient, input)
 		if err != nil {
 			return err
@@ -260,6 +233,7 @@ func (c *Cluster) RemoveService(input string) error {
 
 		_, err = state.controlClient.RemoveService(ctx, &swarmapi.RemoveServiceRequest{ServiceID: service.ID})
 		return err
+	})
 }
 
 // ServiceLogs collects service logs and writes them back to `config.OutStream`

daemon/cluster/swarm.go

@@ -187,38 +187,23 @@ func (c *Cluster) Join(req types.JoinRequest) error {
 
 // Inspect retrieves the configuration properties of a managed swarm cluster.
 func (c *Cluster) Inspect() (types.Swarm, error) {
-	c.mu.RLock()
+	var swarm *swarmapi.Cluster
-	defer c.mu.RUnlock()
+	if err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-
+		s, err := getSwarm(ctx, state.controlClient)
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return types.Swarm{}, c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
-	swarm, err := getSwarm(ctx, state.controlClient)
 		if err != nil {
+			return err
+		}
+		swarm = s
+		return nil
+	}); err != nil {
 		return types.Swarm{}, err
 	}
-
 	return convert.SwarmFromGRPC(*swarm), nil
 }
 
 // Update updates configuration of a managed swarm cluster.
 func (c *Cluster) Update(version uint64, spec types.Spec, flags types.UpdateFlags) error {
-	c.mu.RLock()
+	return c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-	defer c.mu.RUnlock()
-
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
 		swarm, err := getSwarm(ctx, state.controlClient)
 		if err != nil {
 			return err
@@ -248,34 +233,29 @@ func (c *Cluster) Update(version uint64, spec types.Spec, flags types.UpdateFlag
 			},
 		)
 		return err
+	})
 }
 
 // GetUnlockKey returns the unlock key for the swarm.
 func (c *Cluster) GetUnlockKey() (string, error) {
-	c.mu.RLock()
+	var resp *swarmapi.GetUnlockKeyResponse
-	defer c.mu.RUnlock()
+	if err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return "", c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
 		client := swarmapi.NewCAClient(state.grpcConn)
 
 		r, err := client.GetUnlockKey(ctx, &swarmapi.GetUnlockKeyRequest{})
 		if err != nil {
+			return err
+		}
+		resp = r
+		return nil
+	}); err != nil {
 		return "", err
 	}
-
+	if len(resp.UnlockKey) == 0 {
-	if len(r.UnlockKey) == 0 {
 		// no key
 		return "", nil
 	}
-
+	return encryption.HumanReadableKey(resp.UnlockKey), nil
-	return encryption.HumanReadableKey(r.UnlockKey), nil
 }
 
 // UnlockSwarm provides a key to decrypt data that is encrypted at rest.

daemon/cluster/tasks.go

@@ -6,6 +6,7 @@ import (
 	types "github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/daemon/cluster/convert"
 	swarmapi "github.com/docker/swarmkit/api"
+	"golang.org/x/net/context"
 )
 
 // GetTasks returns a list of tasks matching the filter options.
@@ -71,19 +72,15 @@ func (c *Cluster) GetTasks(options apitypes.TaskListOptions) ([]types.Task, erro
 
 // GetTask returns a task by an ID.
 func (c *Cluster) GetTask(input string) (types.Task, error) {
-	c.mu.RLock()
+	var task *swarmapi.Task
-	defer c.mu.RUnlock()
+	if err := c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
-
+		t, err := getTask(ctx, state.controlClient, input)
-	state := c.currentNodeState()
-	if !state.IsActiveManager() {
-		return types.Task{}, c.errNoManager(state)
-	}
-
-	ctx, cancel := c.getRequestContext()
-	defer cancel()
-
-	task, err := getTask(ctx, state.controlClient, input)
 		if err != nil {
+			return err
+		}
+		task = t
+		return nil
+	}); err != nil {
 		return types.Task{}, err
 	}
 	return convert.TaskFromGRPC(*task), nil