From 948e60691e523022f88e7f8129f02106a0f8826c Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Mon, 14 Nov 2016 18:08:24 -0800 Subject: [PATCH] Return warnings from service create and service update when digest pinning fails Modify the service update and create APIs to return optional warning messages as part of the response. Populate these messages with an informative reason when digest resolution fails. This is a small API change, but significantly improves the UX. The user can now get immediate feedback when they've specified a nonexistent image or unreachable registry. Signed-off-by: Aaron Lehmann --- api/server/router/swarm/backend.go | 4 +- api/server/router/swarm/cluster_routes.go | 11 +++-- api/swagger.yaml | 23 ++++++++-- api/types/client.go | 4 +- api/types/service_update_response.go | 12 +++++ api/types/volume.go | 2 +- api/types/volume/volumes_create.go | 2 +- cli/command/service/create.go | 4 ++ cli/command/service/scale.go | 6 ++- cli/command/service/update.go | 6 ++- cli/command/stack/deploy.go | 9 +++- client/interface.go | 2 +- client/service_update.go | 11 ++++- client/service_update_test.go | 6 +-- daemon/cluster/cluster.go | 44 ++++++++++++------- docs/reference/api/docker_remote_api.md | 1 + docs/reference/api/docker_remote_api_v1.25.md | 13 +++++- docs/reference/api/docker_remote_api_v1.26.md | 13 +++++- hack/generate-swagger-api.sh | 3 +- 19 files changed, 132 insertions(+), 44 deletions(-) create mode 100644 api/types/service_update_response.go diff --git a/api/server/router/swarm/backend.go b/api/server/router/swarm/backend.go index c65d705c5c..33840f0d30 100644 --- a/api/server/router/swarm/backend.go +++ b/api/server/router/swarm/backend.go @@ -18,8 +18,8 @@ type Backend interface { UnlockSwarm(req types.UnlockRequest) error GetServices(basictypes.ServiceListOptions) ([]types.Service, error) GetService(string) (types.Service, error) - CreateService(types.ServiceSpec, string) (string, error) - UpdateService(string, uint64, types.ServiceSpec, string, string) error + CreateService(types.ServiceSpec, string) (*basictypes.ServiceCreateResponse, error) + UpdateService(string, uint64, types.ServiceSpec, string, string) (*basictypes.ServiceUpdateResponse, error) RemoveService(string) error ServiceLogs(context.Context, string, *backend.ContainerLogsConfig, chan struct{}) error GetNodes(basictypes.NodeListOptions) ([]types.Node, error) diff --git a/api/server/router/swarm/cluster_routes.go b/api/server/router/swarm/cluster_routes.go index 3ccbe2ba8b..10250da24f 100644 --- a/api/server/router/swarm/cluster_routes.go +++ b/api/server/router/swarm/cluster_routes.go @@ -166,15 +166,13 @@ func (sr *swarmRouter) createService(ctx context.Context, w http.ResponseWriter, // Get returns "" if the header does not exist encodedAuth := r.Header.Get("X-Registry-Auth") - id, err := sr.backend.CreateService(service, encodedAuth) + resp, err := sr.backend.CreateService(service, encodedAuth) if err != nil { logrus.Errorf("Error creating service %s: %v", service.Name, err) return err } - return httputils.WriteJSON(w, http.StatusCreated, &basictypes.ServiceCreateResponse{ - ID: id, - }) + return httputils.WriteJSON(w, http.StatusCreated, resp) } func (sr *swarmRouter) updateService(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { @@ -194,11 +192,12 @@ func (sr *swarmRouter) updateService(ctx context.Context, w http.ResponseWriter, registryAuthFrom := r.URL.Query().Get("registryAuthFrom") - if err := sr.backend.UpdateService(vars["id"], version, service, encodedAuth, registryAuthFrom); err != nil { + resp, err := sr.backend.UpdateService(vars["id"], version, service, encodedAuth, registryAuthFrom) + if err != nil { logrus.Errorf("Error updating service %s: %v", vars["id"], err) return err } - return nil + return httputils.WriteJSON(w, http.StatusOK, resp) } func (sr *swarmRouter) removeService(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { diff --git a/api/swagger.yaml b/api/swagger.yaml index d6567259e3..28f099aecf 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -2218,6 +2218,16 @@ definitions: Deleted: description: "The image ID of an image that was deleted" type: "string" + ServiceUpdateResponse: + type: "object" + properties: + Warnings: + description: "Optional warning messages" + type: "array" + items: + type: "string" + example: + Warning: "unable to pin image doesnotexist:latest to digest: image library/doesnotexist:latest not found" ContainerSummary: type: "array" items: @@ -6875,8 +6885,12 @@ paths: ID: description: "The ID of the created service." type: "string" + Warning: + description: "Optional warning message" + type: "string" example: ID: "ak7w3gjqoa3kuz8xcpnyy0pvl" + Warning: "unable to pin image doesnotexist:latest to digest: image library/doesnotexist:latest not found" 409: description: "name conflicts with an existing service" schema: @@ -6998,10 +7012,14 @@ paths: /services/{id}/update: post: summary: "Update a service" - operationId: "PostServicesUpdate" + operationId: "ServiceUpdate" + consumes: ["application/json"] + produces: ["application/json"] responses: 200: description: "no error" + schema: + $ref: "#/definitions/ImageDeleteResponse" 404: description: "no such service" schema: @@ -7065,8 +7083,7 @@ paths: description: "A base64-encoded auth configuration for pulling from private registries. [See the authentication section for details.](#section/Authentication)" type: "string" - tags: - - "Services" + tags: [Service] /tasks: get: summary: "List tasks" diff --git a/api/types/client.go b/api/types/client.go index 67e4446c61..78a000cfad 100644 --- a/api/types/client.go +++ b/api/types/client.go @@ -285,10 +285,12 @@ type ServiceCreateOptions struct { } // ServiceCreateResponse contains the information returned to a client -// on the creation of a new service. +// on the creation of a new service. type ServiceCreateResponse struct { // ID is the ID of the created service. ID string + // Warnings is a set of non-fatal warning messages to pass on to the user. + Warnings []string `json:",omitempty"` } // Values for RegistryAuthFrom in ServiceUpdateOptions diff --git a/api/types/service_update_response.go b/api/types/service_update_response.go new file mode 100644 index 0000000000..74ea64b1bb --- /dev/null +++ b/api/types/service_update_response.go @@ -0,0 +1,12 @@ +package types + +// This file was generated by the swagger tool. +// Editing this file might prove futile when you re-run the swagger generate command + +// ServiceUpdateResponse service update response +// swagger:model ServiceUpdateResponse +type ServiceUpdateResponse struct { + + // Optional warning messages + Warnings []string `json:"Warnings"` +} diff --git a/api/types/volume.go b/api/types/volume.go index f2db0cda7b..da4f8ebd9c 100644 --- a/api/types/volume.go +++ b/api/types/volume.go @@ -11,7 +11,7 @@ type Volume struct { // Required: true Driver string `json:"Driver"` - // A mapping of abitrary key/value data set on this volume. + // User-defined key/value metadata. // Required: true Labels map[string]string `json:"Labels"` diff --git a/api/types/volume/volumes_create.go b/api/types/volume/volumes_create.go index 5f9513a227..679c16006f 100644 --- a/api/types/volume/volumes_create.go +++ b/api/types/volume/volumes_create.go @@ -19,7 +19,7 @@ type VolumesCreateBody struct { // Required: true DriverOpts map[string]string `json:"DriverOpts"` - // A mapping of arbitrary key/value data to set on the volume. + // User-defined key/value metadata. // Required: true Labels map[string]string `json:"Labels"` diff --git a/cli/command/service/create.go b/cli/command/service/create.go index 061a36f06c..96c9f36da1 100644 --- a/cli/command/service/create.go +++ b/cli/command/service/create.go @@ -90,6 +90,10 @@ func runCreate(dockerCli *command.DockerCli, opts *serviceOptions) error { return err } + for _, warning := range response.Warnings { + fmt.Fprintln(dockerCli.Err(), warning) + } + fmt.Fprintf(dockerCli.Out(), "%s\n", response.ID) return nil } diff --git a/cli/command/service/scale.go b/cli/command/service/scale.go index ea30265bd7..cf89e90273 100644 --- a/cli/command/service/scale.go +++ b/cli/command/service/scale.go @@ -82,11 +82,15 @@ func runServiceScale(dockerCli *command.DockerCli, serviceID string, scale uint6 serviceMode.Replicated.Replicas = &scale - err = client.ServiceUpdate(ctx, service.ID, service.Version, service.Spec, types.ServiceUpdateOptions{}) + response, err := client.ServiceUpdate(ctx, service.ID, service.Version, service.Spec, types.ServiceUpdateOptions{}) if err != nil { return err } + for _, warning := range response.Warnings { + fmt.Fprintln(dockerCli.Err(), warning) + } + fmt.Fprintf(dockerCli.Out(), "%s scaled to %d\n", serviceID, scale) return nil } diff --git a/cli/command/service/update.go b/cli/command/service/update.go index d2639a62db..20a4fc5708 100644 --- a/cli/command/service/update.go +++ b/cli/command/service/update.go @@ -133,11 +133,15 @@ func runUpdate(dockerCli *command.DockerCli, flags *pflag.FlagSet, serviceID str updateOpts.RegistryAuthFrom = types.RegistryAuthFromSpec } - err = apiClient.ServiceUpdate(ctx, service.ID, service.Version, *spec, updateOpts) + response, err := apiClient.ServiceUpdate(ctx, service.ID, service.Version, *spec, updateOpts) if err != nil { return err } + for _, warning := range response.Warnings { + fmt.Fprintln(dockerCli.Err(), warning) + } + fmt.Fprintf(dockerCli.Out(), "%s\n", serviceID) return nil } diff --git a/cli/command/stack/deploy.go b/cli/command/stack/deploy.go index 683f0cad35..63adeacd62 100644 --- a/cli/command/stack/deploy.go +++ b/cli/command/stack/deploy.go @@ -408,15 +408,20 @@ func deployServices( if sendAuth { updateOpts.EncodedRegistryAuth = encodedAuth } - if err := apiClient.ServiceUpdate( + response, err := apiClient.ServiceUpdate( ctx, service.ID, service.Version, serviceSpec, updateOpts, - ); err != nil { + ) + if err != nil { return err } + + for _, warning := range response.Warnings { + fmt.Fprintln(dockerCli.Err(), warning) + } } else { fmt.Fprintf(out, "Creating service %s\n", name) diff --git a/client/interface.go b/client/interface.go index 7a3ebe8b4c..d46720e6c7 100644 --- a/client/interface.go +++ b/client/interface.go @@ -124,7 +124,7 @@ type ServiceAPIClient interface { ServiceInspectWithRaw(ctx context.Context, serviceID string) (swarm.Service, []byte, error) ServiceList(ctx context.Context, options types.ServiceListOptions) ([]swarm.Service, error) ServiceRemove(ctx context.Context, serviceID string) error - ServiceUpdate(ctx context.Context, serviceID string, version swarm.Version, service swarm.ServiceSpec, options types.ServiceUpdateOptions) error + ServiceUpdate(ctx context.Context, serviceID string, version swarm.Version, service swarm.ServiceSpec, options types.ServiceUpdateOptions) (types.ServiceUpdateResponse, error) ServiceLogs(ctx context.Context, serviceID string, options types.ContainerLogsOptions) (io.ReadCloser, error) TaskInspectWithRaw(ctx context.Context, taskID string) (swarm.Task, []byte, error) TaskList(ctx context.Context, options types.TaskListOptions) ([]swarm.Task, error) diff --git a/client/service_update.go b/client/service_update.go index 8e03f7f483..afa94d47e2 100644 --- a/client/service_update.go +++ b/client/service_update.go @@ -1,6 +1,7 @@ package client import ( + "encoding/json" "net/url" "strconv" @@ -10,7 +11,7 @@ import ( ) // ServiceUpdate updates a Service. -func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version swarm.Version, service swarm.ServiceSpec, options types.ServiceUpdateOptions) error { +func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version swarm.Version, service swarm.ServiceSpec, options types.ServiceUpdateOptions) (types.ServiceUpdateResponse, error) { var ( headers map[string][]string query = url.Values{} @@ -28,7 +29,13 @@ func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version query.Set("version", strconv.FormatUint(version.Index, 10)) + var response types.ServiceUpdateResponse resp, err := cli.post(ctx, "/services/"+serviceID+"/update", query, service, headers) + if err != nil { + return response, err + } + + err = json.NewDecoder(resp.body).Decode(&response) ensureReaderClosed(resp) - return err + return response, err } diff --git a/client/service_update_test.go b/client/service_update_test.go index 081649f492..76bea176bf 100644 --- a/client/service_update_test.go +++ b/client/service_update_test.go @@ -19,7 +19,7 @@ func TestServiceUpdateError(t *testing.T) { client: newMockClient(errorMock(http.StatusInternalServerError, "Server error")), } - err := client.ServiceUpdate(context.Background(), "service_id", swarm.Version{}, swarm.ServiceSpec{}, types.ServiceUpdateOptions{}) + _, err := client.ServiceUpdate(context.Background(), "service_id", swarm.Version{}, swarm.ServiceSpec{}, types.ServiceUpdateOptions{}) if err == nil || err.Error() != "Error response from daemon: Server error" { t.Fatalf("expected a Server Error, got %v", err) } @@ -64,12 +64,12 @@ func TestServiceUpdate(t *testing.T) { } return &http.Response{ StatusCode: http.StatusOK, - Body: ioutil.NopCloser(bytes.NewReader([]byte("body"))), + Body: ioutil.NopCloser(bytes.NewReader([]byte("{}"))), }, nil }), } - err := client.ServiceUpdate(context.Background(), "service_id", updateCase.swarmVersion, swarm.ServiceSpec{}, types.ServiceUpdateOptions{}) + _, err := client.ServiceUpdate(context.Background(), "service_id", updateCase.swarmVersion, swarm.ServiceSpec{}, types.ServiceUpdateOptions{}) if err != nil { t.Fatal(err) } diff --git a/daemon/cluster/cluster.go b/daemon/cluster/cluster.go index add9d33569..b6c7b2f2ff 100644 --- a/daemon/cluster/cluster.go +++ b/daemon/cluster/cluster.go @@ -1050,12 +1050,12 @@ func (c *Cluster) imageWithDigestString(ctx context.Context, image string, authC } // CreateService creates a new service in a managed swarm cluster. -func (c *Cluster) CreateService(s types.ServiceSpec, encodedAuth string) (string, error) { +func (c *Cluster) CreateService(s types.ServiceSpec, encodedAuth string) (*apitypes.ServiceCreateResponse, error) { c.RLock() defer c.RUnlock() if !c.isActiveManager() { - return "", c.errNoManager() + return nil, c.errNoManager() } ctx, cancel := c.getRequestContext() @@ -1063,17 +1063,17 @@ func (c *Cluster) CreateService(s types.ServiceSpec, encodedAuth string) (string err := c.populateNetworkID(ctx, c.client, &s) if err != nil { - return "", err + return nil, err } serviceSpec, err := convert.ServiceSpecToGRPC(s) if err != nil { - return "", err + return nil, err } ctnr := serviceSpec.Task.GetContainer() if ctnr == nil { - return "", fmt.Errorf("service does not use container tasks") + return nil, fmt.Errorf("service does not use container tasks") } if encodedAuth != "" { @@ -1087,11 +1087,15 @@ func (c *Cluster) CreateService(s types.ServiceSpec, encodedAuth string) (string logrus.Warnf("invalid authconfig: %v", err) } } + + resp := &apitypes.ServiceCreateResponse{} + // pin image by digest if os.Getenv("DOCKER_SERVICE_PREFER_OFFLINE_IMAGE") != "1" { digestImage, err := c.imageWithDigestString(ctx, ctnr.Image, authConfig) if err != nil { logrus.Warnf("unable to pin image %s to digest: %s", ctnr.Image, err.Error()) + resp.Warnings = append(resp.Warnings, fmt.Sprintf("unable to pin image %s to digest: %s", ctnr.Image, err.Error())) } else { logrus.Debugf("pinning image %s by digest: %s", ctnr.Image, digestImage) ctnr.Image = digestImage @@ -1100,10 +1104,11 @@ func (c *Cluster) CreateService(s types.ServiceSpec, encodedAuth string) (string r, err := c.client.CreateService(ctx, &swarmapi.CreateServiceRequest{Spec: &serviceSpec}) if err != nil { - return "", err + return nil, err } - return r.Service.ID, nil + resp.ID = r.Service.ID + return resp, nil } // GetService returns a service based on an ID or name. @@ -1126,12 +1131,12 @@ func (c *Cluster) GetService(input string) (types.Service, error) { } // UpdateService updates existing service to match new properties. -func (c *Cluster) UpdateService(serviceIDOrName string, version uint64, spec types.ServiceSpec, encodedAuth string, registryAuthFrom string) error { +func (c *Cluster) UpdateService(serviceIDOrName string, version uint64, spec types.ServiceSpec, encodedAuth string, registryAuthFrom string) (*apitypes.ServiceUpdateResponse, error) { c.RLock() defer c.RUnlock() if !c.isActiveManager() { - return c.errNoManager() + return nil, c.errNoManager() } ctx, cancel := c.getRequestContext() @@ -1139,22 +1144,22 @@ func (c *Cluster) UpdateService(serviceIDOrName string, version uint64, spec typ err := c.populateNetworkID(ctx, c.client, &spec) if err != nil { - return err + return nil, err } serviceSpec, err := convert.ServiceSpecToGRPC(spec) if err != nil { - return err + return nil, err } currentService, err := getService(ctx, c.client, serviceIDOrName) if err != nil { - return err + return nil, err } newCtnr := serviceSpec.Task.GetContainer() if newCtnr == nil { - return fmt.Errorf("service does not use container tasks") + return nil, fmt.Errorf("service does not use container tasks") } if encodedAuth != "" { @@ -1168,14 +1173,14 @@ func (c *Cluster) UpdateService(serviceIDOrName string, version uint64, spec typ ctnr = currentService.Spec.Task.GetContainer() case apitypes.RegistryAuthFromPreviousSpec: if currentService.PreviousSpec == nil { - return fmt.Errorf("service does not have a previous spec") + return nil, fmt.Errorf("service does not have a previous spec") } ctnr = currentService.PreviousSpec.Task.GetContainer() default: - return fmt.Errorf("unsupported registryAuthFromValue") + return nil, fmt.Errorf("unsupported registryAuthFromValue") } if ctnr == nil { - return fmt.Errorf("service does not use container tasks") + return nil, fmt.Errorf("service does not use container tasks") } newCtnr.PullOptions = ctnr.PullOptions // update encodedAuth so it can be used to pin image by digest @@ -1191,11 +1196,15 @@ func (c *Cluster) UpdateService(serviceIDOrName string, version uint64, spec typ logrus.Warnf("invalid authconfig: %v", err) } } + + resp := &apitypes.ServiceUpdateResponse{} + // pin image by digest if os.Getenv("DOCKER_SERVICE_PREFER_OFFLINE_IMAGE") != "1" { digestImage, err := c.imageWithDigestString(ctx, newCtnr.Image, authConfig) if err != nil { logrus.Warnf("unable to pin image %s to digest: %s", newCtnr.Image, err.Error()) + resp.Warnings = append(resp.Warnings, fmt.Sprintf("unable to pin image %s to digest: %s", newCtnr.Image, err.Error())) } else if newCtnr.Image != digestImage { logrus.Debugf("pinning image %s by digest: %s", newCtnr.Image, digestImage) newCtnr.Image = digestImage @@ -1212,7 +1221,8 @@ func (c *Cluster) UpdateService(serviceIDOrName string, version uint64, spec typ }, }, ) - return err + + return resp, err } // RemoveService removes a service from a managed swarm cluster. diff --git a/docs/reference/api/docker_remote_api.md b/docs/reference/api/docker_remote_api.md index f5652cba80..979b499369 100644 --- a/docs/reference/api/docker_remote_api.md +++ b/docs/reference/api/docker_remote_api.md @@ -171,6 +171,7 @@ This section lists each version from latest to oldest. Each listing includes a * `POST /containers/create` now takes `StopTimeout` field. * `POST /services/create` and `POST /services/(id or name)/update` now accept `Monitor` and `MaxFailureRatio` parameters, which control the response to failures during service updates. * `POST /services/(id or name)/update` now accepts a `ForceUpdate` parameter inside the `TaskTemplate`, which causes the service to be updated even if there are no changes which would ordinarily trigger an update. +* `POST /services/create` and `POST /services/(id or name)/update` now return a `Warnings` array. * `GET /networks/(name)` now returns field `Created` in response to show network created time. * `POST /containers/(id or name)/exec` now accepts an `Env` field, which holds a list of environment variables to be set in the context of the command execution. * `GET /volumes`, `GET /volumes/(name)`, and `POST /volumes/create` now return the `Options` field which holds the driver specific options to use for when creating the volume. diff --git a/docs/reference/api/docker_remote_api_v1.25.md b/docs/reference/api/docker_remote_api_v1.25.md index 3728ae7b80..5ab89bd8d2 100644 --- a/docs/reference/api/docker_remote_api_v1.25.md +++ b/docs/reference/api/docker_remote_api_v1.25.md @@ -5262,7 +5262,8 @@ image](#create-an-image) section for more details. Content-Type: application/json { - "ID":"ak7w3gjqoa3kuz8xcpnyy0pvl" + "ID": "ak7w3gjqoa3kuz8xcpnyy0pvl", + "Warnings": ["unable to pin image doesnotexist:latest to digest: image library/doesnotexist:latest not found"] } **Status codes**: @@ -5628,6 +5629,16 @@ image](#create-an-image) section for more details. - **404** – no such service - **500** – server error +**Example response**: + + HTTP/1.1 200 OK + Content-Type: application/json + + { + "Warnings": ["unable to pin image doesnotexist:latest to digest: image library/doesnotexist:latest not found"] + } + + ### Get service logs `GET /services/(id or name)/logs` diff --git a/docs/reference/api/docker_remote_api_v1.26.md b/docs/reference/api/docker_remote_api_v1.26.md index 9a260f7b7f..9bcd4d0257 100644 --- a/docs/reference/api/docker_remote_api_v1.26.md +++ b/docs/reference/api/docker_remote_api_v1.26.md @@ -5262,7 +5262,8 @@ image](#create-an-image) section for more details. Content-Type: application/json { - "ID":"ak7w3gjqoa3kuz8xcpnyy0pvl" + "ID": "ak7w3gjqoa3kuz8xcpnyy0pvl", + "Warnings": ["unable to pin image doesnotexist:latest to digest: image library/doesnotexist:latest not found"] } **Status codes**: @@ -5628,6 +5629,16 @@ image](#create-an-image) section for more details. - **404** – no such service - **500** – server error +**Example response**: + + HTTP/1.1 200 OK + Content-Type: application/json + + { + "Warnings": ["unable to pin image doesnotexist:latest to digest: image library/doesnotexist:latest not found"] + } + + ### Get service logs `GET /services/(id or name)/logs` diff --git a/hack/generate-swagger-api.sh b/hack/generate-swagger-api.sh index d46eaa33de..a8e9f818a7 100755 --- a/hack/generate-swagger-api.sh +++ b/hack/generate-swagger-api.sh @@ -8,7 +8,8 @@ swagger generate model -f api/swagger.yaml \ -n ImageSummary \ -n Plugin -n PluginDevice -n PluginMount -n PluginEnv -n PluginInterfaceType \ -n ErrorResponse \ - -n IdResponse + -n IdResponse \ + -n ServiceUpdateResponse swagger generate operation -f api/swagger.yaml \ -t api -a types -m types -C api/swagger-gen.yaml \