service create is using CLI defaults rather than leaving the fields empty

[aluzzardi]

The CLI is creating a spec by doing something like:

    service = swarm.ServiceSpec{
        TaskTemplate: swarm.TaskSpec{
                        [...]
            Resources:     opts.resources.ToResourceRequirements(),
            RestartPolicy: opts.restartPolicy.ToRestartPolicy(),
            Placement: &swarm.Placement{
                Constraints: opts.constraints,
            },
        },
        Mode: swarm.ServiceMode{},
        UpdateConfig: &swarm.UpdateConfig{
            Parallelism: opts.update.parallelism,
            Delay:       opts.update.delay,
        },
        Networks:     convertNetworks(opts.networks),
        EndpointSpec: opts.endpoint.ToEndpointSpec(),
    }

This is pretty bad because it hardcodes the CLI defaults into the server. We have no way to make a distinction between a user wanting something versus a user leaving the defaults.

For example, the user runs a docker service create foo, no arguments.
Rather than leaving RestartPolicy to null, we're filling it up with stuff:

func (r *restartPolicyOptions) ToRestartPolicy() *swarm.RestartPolicy {
    return &swarm.RestartPolicy{
        Condition:   swarm.RestartPolicyCondition(r.condition),
        Delay:       r.delay.Value(),
        MaxAttempts: r.maxAttempts.Value(),
        Window:      r.window.Value(),
    }
}

We're incapable to know whether the user wanted something to be done about restart policies or if it's just the CLI setting defaults.

[thaJeztah]

related / similar: #24958

Having the client set defaults is something that's been bugging me for a while; perhaps we should have a tracking issue to fix that throughout

One challenge though, is that the usage output of the CLI shows the defaults as set in the client, and doesn't know about defaults that are set server-side (in the daemon)

[aaronlehmann]

I'd like to get this fixed for 1.13.

The first step is changing the CLI (and json<->grpc conversion) to leave empty any fields that aren't explicitly specified by the user. This may take some minor engine API changes to make certain things in the spec pointers, if they are embedded structs today.

The main issue after that is visibility into the defaults. This matters in two places: seeing an existing service's parameters (i.e. service inspect), and CLI usage output.

@aluzzardi: For the former, what do you think about having controlapi's GetService also return a second version of the spec that has defaults inserted in all empty fields. We could call this InterpolatedSpec. For example, if the actual spec has no RestartPolicy, InterpolatedSpec would have a RestartPolicy filled with default values. service inspect would show the interpolated spec.

The main downside I can see is that people could confuse Spec and InterpolatedSpec, and accidentally do a service update based on InterpolatedSpec. But now that I think more about it, this is actually a useful feature. If you update a service using InterpolatedSpec, you freeze in place the current defaults, and they won't evolve when the managers upgrade to a new software version that may change defaults. This seems like useful behavior.

For the CLI usage output, I don't think it's worth the complexity of fetching defaults from the leader through an RPC. There's a lot that can go wrong here, and I think it's important to provide useful help output even when there isn't a usable swarm setup. One possibility for this is to provide a github.com/docker/swarmkit/defaults package as a source of truth for default values. It's not perfect because there's no guarantee that your client is linked to the same version of the package as the engine that acts as the leader and interprets empty values, but it seems reasonable if we want the CLI usage output to show defaults.

[thaJeztah]

A similar change was made to logging driver settings a while back (see #21153 / #21118, and the discussion on #22575)

For that change, I was in doubt what the correct behavior would be,
as using the "merged" configuration would make it impossible to change daemon
defaults for existing containers (and the same would probably apply to services here).

On the other hand; not using the "merged" configuration (so, not baking it in the
service definition), could potentially break existing services if the daemon
configuration is updated.

I still think we need a global design document / guideline for this; also taking
into account how to deal with new options. For example; if #26421
gets merged (just as example);

• should existing service-definitions be automatically updated?
• should the defaults be merged at runtime?
• do we need a general "migration" callback, that checks if service definitions
  need to be updated (so that we can decide on a per-feature base)?

[aaronlehmann]

I still think we need a global design document / guideline for this; also taking
into account how to deal with new options. For example; if #26421
gets merged (just as example);

• should existing service-definitions be automatically updated?
• should the defaults be merged at runtime?
• do we need a general "migration" callback, that checks if service definitions need to be updated (so that we can decide on a per-feature base)?

I think dealing with new options is straightforward. The SwarmKit model is that the spec is never modified except by the user. So if a spec is missing some new option, the default will automatically apply. This should be fine as long as we pick sensible defaults. In particular, for new features we'll want to err on the side of disabling them by default, so it doesn't affect the behavior of existing services.

[aaronlehmann]

I started working on this in SwarmKit: moby/swarmkit#1744