Generic Task Runtime support #2014

aluzzardi · 2017-03-07T04:49:13Z

This mostly needs naming & datastructure validation.

The PR includes:

A new runtime type, custom, along with a CustomSpec
API validation, Store indexing, API filters

Both services and tasks can be filtered by runtime. Custom runtimes can be filtered by their custom type (e.g. runtime=plugin for plugins), while built-in types can be filtered by what they are (e.g. runtime=container).

/cc @ehazlett @aaronlehmann @stevvooe

aaronlehmann · 2017-03-07T04:52:20Z

api/specs.proto

@@ -128,6 +130,11 @@ message TaskSpec {
 	uint64 force_update = 9;
 }

+message CustomSpec {
+	string name = 1;


I suppose this is describing which kind of spec is enclosed - i.e. plugin, container etc (perhaps with some com.docker. scheme) - not an arbitrary name. If so, maybe kind is a better name? I would also have suggested type, but that's a reserved word.

aaronlehmann · 2017-03-07T04:53:26Z

manager/controlapi/service.go

+	}
+
+	if payload.TypeUrl == "" || len(payload.Value) == 0 {
+		return grpc.Errorf(codes.InvalidArgument, "Custom runtime has malformed payload")


Specific errors for missing TypeUrl and Value might be useful.

aaronlehmann · 2017-03-07T04:54:49Z

manager/state/store/services.go

+					Name:         indexRuntime,
+					AllowMissing: true,
+					Indexer:      serviceIndexerByRuntime{},
+				},


Just curious - why is it necessary to index by runtime? I don't think this was part of the original plugin runtime PR.

Discussed IRL - this is to allow filtering by container for things such as docker service ls

aaronlehmann · 2017-03-07T04:56:28Z

api/ca.pb.go

@@ -2062,7 +2062,7 @@ func init() { proto.RegisterFile("ca.proto", fileDescriptorCa) }

 var fileDescriptorCa = []byte{
 	// 610 bytes of a gzipped FileDescriptorProto
-	0x1f, 0x8b, 0x08, 0x00, 0x00, 0x09, 0x6e, 0x88, 0x02, 0xff, 0x94, 0x54, 0xcd, 0x6e, 0xd3, 0x40,
+	0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x94, 0x54, 0xcd, 0x6e, 0xd3, 0x40,


There are some unrelated changes to generated .pb.go files happening here. I think #1968 needs to be merged first. Can you please revive that one?

stevvooe · 2017-03-07T22:24:22Z

api/specs.proto

@@ -101,6 +102,7 @@ message TaskSpec {
 	oneof runtime {
 		NetworkAttachmentSpec attachment = 8;
 		ContainerSpec container = 1;
+		CustomSpec custom = 10;


The word "custom" is probably a little narrow. "dynamic" or something in that area would would be more apt. Basically, the goal is to decouple the swarmkit schema from the runtime schema.

Why not just embed the any field here directly and use the url to name the runtime?

We thought about that and ruled in favor for a wrapper message for a few reason, but still open for debate.

the Url field of the Any type is supposed to represent the encoding, not the "type"

https://github.com/google/protobuf/blob/master/src/google/protobuf/any.proto#L113

// A URL/resource name whose content describes the type of the // serialized protocol buffer message.

The type in the URL won't be "friendly", e.g., it's going to be a long, namespaced versioned type. The type in the spec will be akin to a top-level type (e.g. "plugin", or "unikernel")

By wrapping it into a *Spec, we leave ourselves open to provide additional supporting fields (version? controller name? whatever)

Thoughts?

Ping @stevvooe

@aluzzardi I think I understand the encoding issue. The bigger issue here is that the thing is called "custom", but will likely be used most commonly. Custom usually denotes something that is exceptional or bespoke, where as we are trying to communicate that this merely a generic type holder.

Maybe RuntimeSpec?

Yes, I don't like CustomSpec / custom either. I was just pointing out why it's a wrapper object.

We thought about RuntimeSpec, the problem however is that we already have a oneof runtime in TaskSpec, e.g.

oneof runtime { RuntimeSpec runtime = 10;

It's a bit weird to have a runtime inside a runtime.

Also, this won't compile I believe - Spec.GetRuntime() would be defined twice

Suggestions?

/cc @stevvooe @aaronlehmann

aluzzardi · 2017-03-09T18:34:33Z

/cc @ijc25

ijc · 2017-03-10T10:17:12Z

manager/controlapi/service.go

+			return err
+		}
+	case *api.TaskSpec_Custom:
+		if err := validateCustomSpec(taskSpec.GetCustom()); err != nil {


Nit: I think it's nicer if functions which match a naming pattern follow a pattern for their arguments/returns to so either all take the type associated with $1 in validate(.*)Spec or all take the generic type and cast themselves. Here you have one of each. Since this is internal and the functions are all pretty near each other it's less important than if it were a public API though.

ijc · 2017-03-10T10:19:00Z

manager/controlapi/service_test.go

+		Custom: &api.CustomSpec{
+			Name: runtime,
+			Payload: &gogotypes.Any{
+				TypeUrl: "com.docker.custom.runtime",


Should perhaps be com.docker.swarmkit.custom.runtime to be properly namespaced? (maybe then the custom becomes redundant?) and/or maybe there should be a spec in there somewhere?

OTOH I just spotted this was in service_test.go so it doesn't really matter.

Should we not follow the conventions laid out in the protobuf specification?

Here are some examples: https://developers.google.com/protocol-buffers/docs/reference/csharp/class/google/protobuf/well-known-types/any.

The proposed format in this PR is not a url.

It looks like we need something like type.docker.com/docker.swarmkit.v1.PluginSpec to be compliant but I am not sure where they have this specified.

This is a unit test

tiborvass · 2017-03-15T17:48:44Z

Ping @aluzzardi

codecov · 2017-03-16T23:58:19Z

Codecov Report

Merging #2014 into master will increase coverage by 0.11%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #2014      +/-   ##
==========================================
+ Coverage   53.86%   53.97%   +0.11%     
==========================================
  Files         111      111              
  Lines       19233    19317      +84     
==========================================
+ Hits        10360    10427      +67     
- Misses       7609     7616       +7     
- Partials     1264     1274      +10

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 0c052c7...11b975f. Read the comment docs.

aluzzardi · 2017-03-16T23:59:54Z

Fixed most comments.

The only remaining part is how to name this.

Anyone has suggestions?

Remember it's going to fit here:

message TaskSpec {
  oneof runtime {
    <Something>Spec something;
    ContainerSpec container;
    [...]
  }
}

/cc @ehazlett @aaronlehmann @stevvooe @ijc25 @tiborvass

aaronlehmann · 2017-03-17T00:22:12Z

OpaqueSpec?

aluzzardi · 2017-03-17T01:50:41Z

ThirdPartyRuntimeSpec?

ExtendedRuntimeSpec

ExtensionRuntimeSpec

ExternalRuntimeSpec

[...] getting somewhere

aaronlehmann · 2017-03-17T04:05:48Z

I think ExtensionRuntimeSpec or ExternalRuntimeSpec are good.

tiborvass · 2017-03-17T06:49:28Z

ForeignRuntimeSpec, AlienRuntimeSpec, ResidentAlienRuntimeSpec (sorry :P)

What's wrong with AnyRuntimeSpec ? I also like OpaqueRuntimeSpec except my mind is OCD about the ambiguity of associativity: Opaque-RuntimeSpec vs OpaqueRuntime-Spec :P

Happens to not be a problem with Any, because it doesn't matter whether it's Any-RuntimeSpec or AnyRuntime-Spec. I know this is the weakest argument ever. Other than that, I simply like the simplicity of AnyRuntimeSpec.

ehazlett · 2017-03-17T15:43:52Z

ExternalRuntimeSpec sgtm

aluzzardi · 2017-03-17T23:34:01Z

@stevvooe proposed GenericRuntimeSpec, which I also like.

ExternalRuntimeSpec vs GenericRuntimeSpec everyone?

tiborvass · 2017-03-17T23:36:18Z

@aluzzardi I like GenericRuntimeSpec, but I don't know what's wrong with AnyRuntimeSpec which is shorter.

aluzzardi · 2017-03-18T00:27:09Z

@tiborvass hahah I figured you like AnyRuntimeSpec :)

I'm not too fond - it's like hungarian notation for datatypes.

tiborvass · 2017-03-18T00:29:58Z

Lol that must be why I like it then haha 🇭🇺 Alright I give my points to GenericRuntimeSpec

…

On Fri, Mar 17, 2017 at 5:27 PM Andrea Luzzardi ***@***.***> wrote: @tiborvass <https://github.com/tiborvass> hahah I figured you like AnyRuntimeSpec :) I'm not too fond - it's like hungarian notation for datatypes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2014 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAye-6bEpd2MfMP66L_wUeB-3gmaMZIpks5rmyTfgaJpZM4MVCg-> .

aluzzardi · 2017-03-18T00:40:28Z

Updated to GenericRuntimeSpec and "generic runtime".

Going once, going twice...

aaronlehmann · 2017-03-18T01:01:27Z

api/control.proto

@@ -191,6 +191,8 @@ message ListTasksRequest {
 		repeated docker.swarmkit.v1.TaskState desired_states = 6;
 		// NamePrefixes matches all objects with the given prefixes
 		repeated string name_prefixes = 7;
+		repeated string runtimes = 9;


Do you think it makes sense to make this TaskSpec.runtime instead of a string, so we can select by type instead of using a string describing the type?

I'm not sure it makes sense, but just wanted to share the idea.

The problem is it works for built-in types (Container, NetworkAttachment) but not for Generic.

For Generic, we index by GenericRuntime.Kind.

Basically, we assume runtimes are indexed by a string. Container and NetworkAttachment are special cases.

Makes sense?

Makes sense. Matches what I did in the draft of the external store API.

aaronlehmann · 2017-03-18T01:02:21Z

api/naming/naming.go

+	case *api.TaskSpec_Generic:
+		return strings.ToLower(r.Generic.Kind)
+	default:
+		return "none"


I would return an error here instead of "none"

It used to panic. But we have so much code in our unit tests that just pass a service (or task) with no runtime. I'm not sure it was worth the hassle of fixing it all.

Thoughts?

I don't think it should panic. But I think returning an error is the right thing to do. Returning "none" here means that passing "none" as the filter parameter would return unrecognized runtimes, which would be a weird undocumented quirk in the API.

Fair enough - fixed it.

Could you double check the way I use the indexing?

AllowMissing: true plus returning false if naming.Runtime yields an error.

Also, changed the control API filters to properly handle errors. If a filter was specified, then we skip the invalid runtime. If not, then we include it.

aaronlehmann · 2017-03-18T01:03:50Z

Overall LGTM. Left some non-blocking comments.

aluzzardi · 2017-03-20T20:40:07Z

Addressed the remaining comment. PTAL

aaronlehmann · 2017-03-20T20:43:21Z

manager/controlapi/task.go

@@ -123,6 +125,14 @@ func (s *Server) ListTasks(ctx context.Context, request *api.ListTasksRequest) (
 				return filterContains(e.NodeID, request.Filters.NodeIDs)
 			},
 			func(e *api.Task) bool {
+				r, err := naming.Runtime(e.Spec)


Very minor: I would try to avoid this call to Runtime when no runtime filter is specified. Could we have something above like:

if len(request.Filters.Runtimes) == 0 { return true }

and then the return below can be simplified to return false.

👍

Much cleaner

aaronlehmann · 2017-03-20T20:44:10Z

manager/state/store/services.go

+	s := obj.(serviceEntry)
+	r, err := naming.Runtime(s.Spec.Task)
+	if err != nil {
+		return false, []byte{}, nil


Can be return false, nil, nil

aaronlehmann · 2017-03-20T20:44:28Z

manager/state/store/tasks.go

+	t := obj.(taskEntry)
+	r, err := naming.Runtime(t.Spec)
+	if err != nil {
+		return false, []byte{}, nil


Can be return false, nil, nil

aluzzardi · 2017-03-20T20:55:37Z

Thanks - updated

aaronlehmann · 2017-03-20T21:00:37Z

LGTM

aaronlehmann · 2017-03-21T18:53:19Z

@aluzzardi: Needs a rebase.

aluzzardi · 2017-03-21T23:35:27Z

Rebased

aaronlehmann · 2017-03-21T23:45:17Z

CI failing - the wrapper types serviceEntry and taskEntry no longer exist

aluzzardi · 2017-03-22T01:11:32Z

I tried to jedi merge/commit :) Fixing

Signed-off-by: Andrea Luzzardi <[email protected]>

aluzzardi · 2017-03-22T01:14:07Z

By the way: in that code, we have a bunch of

	s, ok := obj.(*api.Service)
	if !ok {
		panic("unexpected type passed to FromObject")
	}

I added mine simply as s := obj.(*api.Service) - go will already panic if we cast the wrong object, so I don't see a point of doing a manual check just to panic. Was there a reason?

aaronlehmann · 2017-03-22T01:15:32Z

It's absolutely fine to rely on the implicit panic. This came up here:

#2039 (comment)

aaronlehmann · 2017-03-22T01:16:13Z

I'm going to open a PR to remove those checks just to simplify the code and preempt further questions about why the checks exist.

aaronlehmann reviewed Mar 7, 2017

View reviewed changes

stevvooe reviewed Mar 7, 2017

View reviewed changes

aluzzardi force-pushed the any-runtime branch from f9851ec to b51fb91 Compare March 9, 2017 02:00

ijc reviewed Mar 10, 2017

View reviewed changes

ehazlett mentioned this pull request Mar 10, 2017

Swarm-wide plugin deployment and lifecycle management moby/moby#31401

Open

aluzzardi force-pushed the any-runtime branch from b51fb91 to 6f38065 Compare March 16, 2017 23:57

aluzzardi force-pushed the any-runtime branch from 6f38065 to 373a77d Compare March 17, 2017 01:15

aluzzardi force-pushed the any-runtime branch from 373a77d to ef04dbd Compare March 17, 2017 23:31

aluzzardi force-pushed the any-runtime branch 2 times, most recently from ed3e02d to b9d0cfa Compare March 18, 2017 00:34

aluzzardi changed the title ~~Custom Task Runtime support~~ Generic Task Runtime support Mar 18, 2017

aluzzardi force-pushed the any-runtime branch 2 times, most recently from 653b126 to 3b6e027 Compare March 18, 2017 00:39

aaronlehmann reviewed Mar 18, 2017

View reviewed changes

aluzzardi force-pushed the any-runtime branch from 3b6e027 to ecb7b48 Compare March 20, 2017 20:05

aaronlehmann reviewed Mar 20, 2017

View reviewed changes

aluzzardi force-pushed the any-runtime branch from ecb7b48 to 5b377de Compare March 20, 2017 20:55

aluzzardi force-pushed the any-runtime branch from 5b377de to 0eefd8d Compare March 21, 2017 23:33

Support for generic runtimes.

11b975f

Signed-off-by: Andrea Luzzardi <[email protected]>

aluzzardi force-pushed the any-runtime branch from 0eefd8d to 11b975f Compare March 22, 2017 01:13

aluzzardi merged commit 8e323f8 into moby:master Mar 22, 2017

aluzzardi deleted the any-runtime branch March 22, 2017 01:34

ehazlett mentioned this pull request Mar 22, 2017

Support Swarmkit Generic Task Runtime moby/moby#32030

Merged

aaronlehmann mentioned this pull request Mar 23, 2017

Add plugins #1545

Closed

Generic Task Runtime support #2014

Generic Task Runtime support #2014

Conversation

aluzzardi commented Mar 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aluzzardi Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

aluzzardi commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiborvass commented Mar 15, 2017

codecov bot commented Mar 16, 2017 • edited Loading

Codecov Report

aluzzardi commented Mar 16, 2017

aaronlehmann commented Mar 17, 2017

aluzzardi commented Mar 17, 2017 • edited Loading

aaronlehmann commented Mar 17, 2017

tiborvass commented Mar 17, 2017

ehazlett commented Mar 17, 2017

aluzzardi commented Mar 17, 2017

tiborvass commented Mar 17, 2017

aluzzardi commented Mar 18, 2017

tiborvass commented Mar 18, 2017 via email

aluzzardi commented Mar 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Mar 18, 2017

aluzzardi commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aluzzardi commented Mar 20, 2017

aaronlehmann commented Mar 20, 2017

aaronlehmann commented Mar 21, 2017

aluzzardi commented Mar 21, 2017

aaronlehmann commented Mar 21, 2017

aluzzardi commented Mar 22, 2017

aluzzardi commented Mar 22, 2017

aaronlehmann commented Mar 22, 2017

aaronlehmann commented Mar 22, 2017

aluzzardi commented Mar 7, 2017 •

edited

Loading

aluzzardi Mar 16, 2017 •

edited

Loading

codecov bot commented Mar 16, 2017 •

edited

Loading

aluzzardi commented Mar 17, 2017 •

edited

Loading