api: Centralize defaults and add InsertDefaults flag by aaronlehmann · Pull Request #1744 · moby/swarmkit

aaronlehmann · 2016-11-11T19:33:33Z

Currently, there is a problem with visibility of defaults. The best way
to create a service is to only specify fields that you care about,
because then the manager can apply appropriate defaults for the other
fields. However, there's no way to know which values are actually being
used for those fields. The spec is under the user's control, so if they
don't specify a value (for example, Spec.Task.Restart.Delay), this
field will show up as blank when the service is retrieved.

We don't want to break the spec ownership model, but we do want a way to
communicate actual values in use back to the client. For this, we add an
InsertDefaults flag to the ControlAPI which allows fetching a version
of the service with defaults inserted into empty fields.

This also has the benefit of centralizing defaults. Default values for
these fields that were defined in various places around the code are now
centralized in github.com/docker/swarmkit/api/defaults.

Right now this flag is only is only available for Service, but as we
encounter situations where other objects have default values which need
to be exposed, we should add this to other objects as well.

cc @aluzzardi @stevvooe

codecov-io · 2016-11-11T19:41:59Z

Codecov Report

Merging #1744 into master will increase coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1744      +/-   ##
==========================================
+ Coverage   54.33%   54.35%   +0.01%     
==========================================
  Files         111      111              
  Lines       19323    19327       +4     
==========================================
+ Hits        10499    10505       +6     
+ Misses       7559     7557       -2     
  Partials     1265     1265

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 970b45a...27d3eb8. Read the comment docs.

aaronlehmann · 2016-11-16T00:41:07Z

manager/controlapi/service.go

 		return nil, grpc.Errorf(codes.NotFound, "service %s not found", request.ServiceID)
 	}

+	service.InterpolatedSpec = defaults.InterpolateService(&service.Spec)


add to ListServices as well

dongluochen · 2017-02-23T19:32:47Z

Right now InterpolatedSpec is only added to Service, but if we move
forward with this, it would make sense to add it to other objects as
well.

This is a decision to make. Inconsistency in different objects would cause confusion. Can we list the alternatives and what Docker has been using in other places?

aaronlehmann · 2017-02-23T19:34:18Z

This is a decision to make. Inconsistency in different objects would cause confusion. Can we list the alternatives and what Docker has been using in other places?

I only meant that I haven't gone through the effort of adding this to other objects because the design hasn't been finalized yet. I think it should be part of every object.

dongluochen · 2017-02-23T19:44:33Z

Since swarmkit is part of Docker, if we are going to implement this, should objects in other Docker kit follow?

aaronlehmann · 2017-02-23T19:47:41Z

To the extent that other objects have Specs and follow the declarative model, sure. I think swarmkit objects are essentially the only ones that allow updates like this, so it's up to us to define the model.

aaronlehmann · 2017-02-24T01:20:48Z

Rebased (fairly involved one, since so many things changed since this was opened - new deepcopy, native protobuf types, orchestrator changes, updater changes, etc.)

Would love to get this going again

dongluochen · 2017-02-24T02:16:23Z

api/defaults/service.go

+	},
+	Update: &api.UpdateConfig{
+		FailureAction: api.UpdateConfig_PAUSE,
+		Monitor:       gogotypes.DurationProto(5 * time.Second),


There are more parameters like parallelism, update-delay, etc.

The default value of UpdateDelay is 0, so it's not necessary to include it here. You're right that Parallelism should be included. It used to default to 0, but now defaults to 1. Fixing.

dongluochen · 2017-02-24T20:08:47Z

api/objects.proto

+	// and it is useful for that to return detailed information about
+	// what values are actually being used to run the service. In the
+	// future, we should add add InterpolatedSpec to other objects as well.
+	ServiceSpec interpolated_spec = 7;


I have concern to add another ServiceSpec in service object. It might confuse developer which one to update.

I'd welcome any other suggestions.

I actually wanted to support updating based on InterpolatedSpec as a feature. It would let the end user lock in the current default values.

If it's only generated on the fly, it's ok.

Expanded the comment to clarify.

I agree with this design.

Do you think the user should be able to view both the service spec they provided, and the full spec on inspect? As a user, maybe I don't want to always see the defaults.

This will be the behavior with docker inspect <servicename>, which just dumps the JSON. I think docker inspect --pretty <servicename> should show values from the interpolated spec.

That sounds right.

dongluochen · 2017-02-24T22:02:36Z

LGTM

nishanttotla · 2017-02-24T22:19:10Z

The main question about this approach is whether we want to solve the
defaults visibility problem by using this approach where defaults are
substituted in, or by encouraging the client to set all fields instead
of leaving some empty. There are advantages and disadvantages to each
approach. The benefit of doing this client-side is that changes to
default values won't affect existing services retroactively. However,
expecting the client to fill in all fields with good defaults is
expecting a lot, and would be a barrier to centralizing defaults. I
think the best approach is to apply defaults on the manager side, but be
conservative about changing defaults, knowing that it may have
unexpected side effects.

I don't think the client should have to set all fields, from a usability perspective. It makes it harder to change defaults later for the user of the API, and for us to update protos.

aluzzardi · 2017-02-25T01:47:56Z

What bothers me is I don't recall ever seeing one API doing this.

How is the world dealing wit h this right now? I'm sure we're not the first ones struggling with API defaults :)

aaronlehmann · 2017-03-02T01:57:55Z

@aluzzardi: I'm not sure. This seems to be a problem that's specific to declarative APIs - otherwise the server can just fill in default values when it returns objects.

k8s manages defaults in the apiserver - see kubernetes/enhancements#55 and the issue linked from there. I couldn't find any information about how apiserver defaults are exposed to end users.

I don't think this is particularly complex. We basically have two choices: offer a version of the spec that has missing fields filled in with defaults (InterpolatedSpec in this PR), or expose the default values and let the client merge them in. I proposed the former here because I think it's simpler, but I'm happy to consider the alternative approach if there's a good reason, or any other ideas. Also, if you can point out any similar systems with declarative APIs, I can research how they are (or aren't) handling this.

aluzzardi · 2017-03-07T03:01:37Z

@aluzzardi: I'm not sure. This seems to be a problem that's specific to declarative APIs - otherwise the server can just fill in default values when it returns objects.

I think it's generic to REST APIs - PUTs your put the full resource back.

What I don't like is we're going to make the API response more complex with another thing to understand for API users. e.g. getting the interpolated spec and putting it back as the spec is bad.

Also, what happens to the CLI - if we display both it's just going be a pain to get meaningful data back.

aaronlehmann · 2017-03-07T05:06:01Z

I think it's generic to REST APIs - PUTs your put the full resource back.

The reason I think it's specific to declarative APIs is that in most other cases it's fine to fill in extra fields in the response. For example, if I PUT a very simple object and only define a few fields, there's usually no problem with the server sending me back a better-defined object that has defaults filled in. This isn't suitable for our declarative API because it's important for us not to mess with the Spec field.

What I don't like is we're going to make the API response more complex with another thing to understand for API users. e.g. getting the interpolated spec and putting it back as the spec is bad.

I understand your concerns but I'm wondering if you think there's a better way than what I'm proposing here. As I mentioned above, I see two basic approaches. We can either include something like InterpolatedSpec in API responses, or we can provide a way for the client to retrieve the defaults and merge them in client-side. A third option is to leave the problem unsolved, but I don't think that's a good outcome.

BTW, I don't think getting the InterpolatedSpec and sending it back as the spec is necessarily bad. Yes, it's bad to do it unintentionally, but if it's done intentionally it's an easy way of making the current defaults "sticky".

Also, what happens to the CLI - if we display both it's just going be a pain to get meaningful data back.

I would suggest that docker service inspect --pretty only format the interpolated spec. The user wants to see the values that actually in use. Currently this command skips anything that's not specified in the spec, which I think is really bad.

Currently docker service inspect without --pretty dumps a bunch of JSON to the terminal, which is a design I find very unfortunate. It's true that adding InterpolatedSpec would add more stuff to that JSON and might be confusing. But I think the real problem here is that we have a command that outputs unformatted JSON by default. I don't think we should let this drive our API design.

aaronlehmann · 2017-03-20T23:54:30Z

Discussed with @aluzzardi - let's change this so that instead of having InterpolatedSpec inside the Service object, there's a parameter in GetServiceRequest that returns a version of the service with defaults inserted.

The CLI can use the defaults package for its help output.

Consider clearing the Version field when the "include defaults" flag is specified, so that users can't inadvertently update based on this option.

aaronlehmann · 2017-03-21T18:51:46Z

Updated according to the above comment. PTAL.

aaronlehmann · 2017-03-24T17:48:36Z

Rebased

aluzzardi · 2017-03-27T20:25:05Z

manager/controlapi/service.go

+		service.Spec = *defaults.InterpolateService(&service.Spec)
+		// Clear version to prevent the returned service from being the
+		// basis for an update.
+		service.Meta.Version = api.Version{}


I'm having second thought about that.

It would make docker service inspect not include the version which is weird

Okay, removed that part.

Currently, there is a problem with visibility of defaults. The best way to create a service is to only specify fields that you care about, because then the manager can apply appropriate defaults for the other fields. However, there's no way to know which values are actually being used for those fields. The spec is under the user's control, so if they don't specify a value (for example, `Spec.Task.Restart.Delay`), this field will show up as blank when the service is retrieved. We don't want to break the spec ownership model, but we do want a way to communicate actual values in use back to the client. For this, we add an `InsertDefaults` flag to the ControlAPI which allows fetching a version of the service with defaults inserted into empty fields. This also has the benefit of centralizing defaults. Default values for these fields that were defined in various places around the code are now centralized in `github.com/docker/swarmkit/api/defaults`. Right now this flag is only is only available for `Service`, but as we encounter situations where other objects have default values which need to be exposed, we should add this to other objects as well. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

aluzzardi · 2017-03-30T00:59:06Z

LGTM

aaronlehmann mentioned this pull request Nov 12, 2016

service create is using CLI defaults rather than leaving the fields empty moby/moby#24959

Closed

aaronlehmann commented Nov 16, 2016

View reviewed changes

This was referenced Jan 17, 2017

add endpoint mode a default value moby/moby#30180

Merged

Add separate rollback parameters #1878

Merged

aaronlehmann mentioned this pull request Feb 23, 2017

[update] Use default parallelism of 1 #1977

Merged

aaronlehmann force-pushed the interpolated-spec branch from 4fd3c63 to e5274d2 Compare February 24, 2017 01:17

cyli mentioned this pull request Feb 24, 2017

specs: Add minimum raw security profiles needed by plugins to ContainerSpec and PluginSpec #1964

Closed

dongluochen reviewed Feb 24, 2017

View reviewed changes

aaronlehmann force-pushed the interpolated-spec branch from e5274d2 to c2a8bd3 Compare February 24, 2017 20:17

aaronlehmann force-pushed the interpolated-spec branch from c2a8bd3 to 1a4b9a1 Compare February 24, 2017 22:07

aaronlehmann force-pushed the interpolated-spec branch from 1a4b9a1 to bd68850 Compare March 21, 2017 02:00

aaronlehmann changed the title ~~api: Centralize defaults and add InterpolatedSpec~~ api: Centralize defaults and add InsertDefaults flag Mar 21, 2017

aaronlehmann force-pushed the interpolated-spec branch from bd68850 to dacc9da Compare March 21, 2017 18:51

aaronlehmann force-pushed the interpolated-spec branch from dacc9da to de7eb77 Compare March 24, 2017 17:48

aluzzardi reviewed Mar 27, 2017

View reviewed changes

aaronlehmann force-pushed the interpolated-spec branch from de7eb77 to 27d3eb8 Compare March 27, 2017 20:47

aaronlehmann merged commit 50ee2bf into moby:master Mar 30, 2017

aaronlehmann deleted the interpolated-spec branch March 30, 2017 01:11

aaronlehmann mentioned this pull request Mar 31, 2017

defaults: Add Rollback defaults #2080

Merged

Comments

Conversation

aaronlehmann commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Nov 11, 2016 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongluochen commented Feb 23, 2017

Uh oh!

aaronlehmann commented Feb 23, 2017

Uh oh!

dongluochen commented Feb 23, 2017

Uh oh!

aaronlehmann commented Feb 23, 2017

Uh oh!

aaronlehmann commented Feb 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nishanttotla Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongluochen commented Feb 24, 2017

Uh oh!

nishanttotla commented Feb 24, 2017

Uh oh!

aluzzardi commented Feb 25, 2017

Uh oh!

aaronlehmann commented Mar 2, 2017

Uh oh!

aluzzardi commented Mar 7, 2017

Uh oh!

aaronlehmann commented Mar 7, 2017

Uh oh!

aaronlehmann commented Mar 20, 2017

Uh oh!

aaronlehmann commented Mar 21, 2017

Uh oh!

aaronlehmann commented Mar 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aluzzardi commented Mar 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aaronlehmann commented Nov 11, 2016 •

edited

Loading

codecov-io commented Nov 11, 2016 •

edited by codecov bot

Loading

nishanttotla Feb 24, 2017 •

edited

Loading