Plugins on swarm by cpuguy83 · Pull Request #33575 · moby/moby

cpuguy83 · 2017-06-07T17:23:09Z

Support deploying plugins via swarm services

Modifies the services API to support deploying plugins (previously only
supported containers).
This makes use of some of the existing work around generic service
runtimes.

With this change you can curl -X POST myDocker:2375/services/create with a
PluginSpec defined in the TaskTemplate.
The plugin defined by in the PluginSpec will be deployed similar to a
"global mode" service.

Work still needed:

More tests (test constraints, others)
Deal with some race conditions in the plugin manager
Better way to filter swarm managed plugins from non-swarm managed plugins
Support enable/disable via services API?
Handle (old) task removes (ie, don't remove a plugin that has an active task)

The format of the plugin request that is sent to swarm is currently just a json encoded object specified in api/types/swarm... wondering if it would be better to change this to protobuf.

Ping @ehazlett @tiborvass

What this does not tackle:

UX, CLI is not handled here, only API's are added... even API's could maybe use some tweaks but worried about back-compat for older clients... specifically on service and task listings which currently only list services/tasks with the container runtime unless you set a runtime in the list filter.
Secrets, configs, mounts, networks, etc... this requires deeper changes to plugins and can be done separately.

cpuguy83 · 2017-06-08T18:46:22Z

Ok, I think this is ready for design review, and code is in a pretty good state.

ping @aaronlehmann

ehazlett · 2017-06-08T19:58:51Z

Sorry for the delay. +1 for pb encoding. In the "original" design, that was the intention we just left it out of the Generic Runtime PR as we didn't have anything to use yet.

aaronlehmann · 2017-06-08T21:55:50Z

wondering if it would be better to change this to protobuf.

Yes, I think protobuf would be better.

aaronlehmann · 2017-06-08T21:58:38Z

api/types/swarm/task.go

I'd suggest putting this next to ContainerSpec in the struct definition and adding a comment that they are mutually exclusive. We should probably change ContainerSpec to be a pointer (and make sure no code assumes it exists).

aaronlehmann · 2017-06-08T22:00:13Z

daemon/cluster/controllers/plugin/controller.go

Looks like tag+digest is already supported. What else needs to be done?

Should we pin this before sending off to swarm?

Pinning happens on the CLI side starting with API 1.30.

(See docker/cli#30)

Ok makes sense to do that there.

aaronlehmann · 2017-06-08T22:02:21Z

daemon/cluster/controllers/plugin/controller.go

It should be possible to pass a reference with both a tag and a digest to the pull code. This wasn't working properly earlier, but it was fixed recently.

Not sure if there are any plugin-specific issues with doing this, but ideally there shouldn't be.

So you are saying to drop the TrimNamed bit?

aaronlehmann · 2017-06-08T22:04:48Z

daemon/cluster/controllers/plugin/controller_test.go

Is 500 ms enough? I like to be very conservative with timeouts in test so that loaded machines don't cause spurious failures.

aaronlehmann · 2017-06-08T22:06:17Z

daemon/cluster/convert/service.go

I guess we'll change this to gRPC anyway, but the error needs to be checked.

aaronlehmann · 2017-06-08T22:07:52Z

daemon/cluster/services.go

Wondering if we should do this in the CLI instead of the API. No strong preference though.

I would prefer the CLI, but I didn't want to make a breaking change to the API here.

aaronlehmann · 2017-06-08T22:10:42Z

integration-cli/docker_api_swarm_service_test.go

I don't think this sleep is necessary. I don't recall any other tests that create services doing this.

Oh I copied this from the test above it. I can remove.

aaronlehmann · 2017-06-08T22:12:20Z

integration-cli/docker_api_swarm_service_test.go

I'm hoping we can avoid adding new dependencies on Docker Hub in tests; see #30626

Yeah, I keep putting this off... I'll work on something for this.

Ok, added a plugin builder + some registry integration. Check it out in integration-cli/fixtures/plugin.

aaronlehmann · 2017-06-08T22:14:04Z

plugin/events.go

I remember this truncated comment from swarmkit :)

Such a coincidence :)

This commit extends SwarmKit secret management with pluggable secret backends support, using the current cluster configuration to understand if secrets are being stored locally, or need to be retrieved from a remote secret store. The solution will use the existing docker plugin framework. The approach is to add a new `driver` parameter to existing secrets, which defines whether the values are loaded from SwarmKit or from one of the secret plugins. The loading of secrets is done using the standard docker plugin infrastructure, which is already accessible in swarmkit and used in other flows (e.g., networking) Remarks: * I've added support to mock the plugin subsystem in controlapi server, I preferred this approach over loading the full plugin subsystem. Work still needed in this CR: - [ ] More unit tests (pending initial iteration) - [ ] Customized error handling Work still needed to complete this feature: - [ ] Inject secrets as part of plugin initialization - [ ] CLI support in docker References: moby/moby#33575 Signed-off-by: liron <liron@twistlock.com>

This commit extends SwarmKit secret management with pluggable secret backends support. The solution uses the existing docker plugin framework for loading plugins and the existing SwarmKit data backend for storing them. The approach is to add a new `driver` parameter to existing secrets, which defines whether the values are taken as is or fetched from one of the secret plugins. The loading of secrets is done using the standard docker plugin infrastructure, which is already accessible in SwarmKit and used in other flows (e.g., networking). The fetched values are are stored as regular SwarmKit secrets. Remarks: * I've added support for mocking the plugin subsystem when settings up the controlapi server. I preferred this approach over loading the full plugin subsystem in UT. Work still needed in this CR: - [ ] More unit tests (pending initial iteration) - [ ] Customized error handling (e.g., customize error string for Not Found) Work still needed to complete this feature: - [ ] Inject secrets as part of plugin initialization - [ ] CLI support in docker - [ ] Docs - [ ] Support scheduling plugins in swarm moby/moby#33575 Signed-off-by: liron <liron@twistlock.com>

cpuguy83 · 2017-07-05T13:13:26Z

This is updated and addresses all comments.

diogomonica · 2017-07-05T19:21:51Z

LGTM

thaJeztah · 2017-07-06T01:39:12Z

@cpuguy83 needs a minor rebase

aaronlehmann · 2017-07-06T17:14:25Z

client/service_create.go

Minor: the client probably shouldn't allow both ContainerSpec and PluginSpec to be specified.

aaronlehmann · 2017-07-06T17:15:29Z

client/service_update.go

Minor: PluginSpec and ContainerSpec should be mutually exclusive.

Can you clarify what you want to see changed? Just a client-side check for not mixing spec types?

Yeah, right now imgPlatforms will be overridden by PluginSpec if both specs exist. I think it would be better to return an error if both exist.

aaronlehmann · 2017-07-06T17:18:26Z

daemon/cluster/controllers/plugin/controller_test.go

Seems like this goroutine could be avoided by using a context with a timeout, but perhaps part of the point of the unit test is to make sure that Wait doesn't get stuck, so maybe this external timeout makes sense.

Well, we need Wait to be called before enable. Since Wait is blocking and we need to check the error a context isn't really enough here.

aaronlehmann · 2017-07-06T17:19:17Z

daemon/cluster/convert/service.go

Why the blank line?

aaronlehmann · 2017-07-06T17:24:09Z

Only had minor comments on the last pass. LGTM after the rebase.

Enables other subsystems to watch actions for a plugin(s). This will be used specifically for implementing plugins on swarm where a swarm controller needs to watch the state of a plugin. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2017-07-06T18:27:01Z

Rebased, fixed imports, added client side validation for service spec to create/update.

yongtang · 2017-07-07T09:22:58Z

All tests passed after windowsRS1 rerun. Merging.

rogaha · 2017-07-07T12:57:34Z

💥 thanks @cpuguy83 for putting this together!

cpuguy83 · 2017-07-07T13:10:27Z

Just step 1, still need:

Support for plugins in private repos
Secrets
Networking
CLI

rogaha · 2017-07-07T13:14:06Z

@cpuguy83 do we have issues for those remaining pieces?

carlfischer1 · 2017-07-07T17:16:27Z

@rogaha @cpuguy83 for Secrets we have #30796

dnephin · 2017-07-26T20:08:13Z

client/service_update.go

+	}
+	if len(imgPlatforms) > 0 {
+		service.TaskTemplate.Placement.Platforms = imgPlatforms
+	}


This (and the other changes to this file) seem like the wrong place to handle these concerns. Every other function in this package is only concerned with translating a golang struct into an HTTP request, performing the request, and translating the response into a goland struct.

With this change we've introduced a bunch of application logic and validation that does not belong here.

I think this needs to be extracted to some other package.

cc @thaJeztah re docker/cli#386

This is all pre-existing, just re-organized.

Ah, my mistake, looks like it was first introduced and modified in a few PRs starting around #33239

dnephin · 2017-07-26T20:09:12Z

client/service_create.go

+	}
+
+	if err := validateServiceSpec(service); err != nil {
+		return types.ServiceCreateResponse{}, err


same problem here

GordonTheTurtle added the status/0-triage label Jun 7, 2017

cpuguy83 added status/1-design-review and removed status/0-triage labels Jun 7, 2017

cpuguy83 force-pushed the plugins_on_swarm branch 4 times, most recently from 8322070 to 977fcc8 Compare June 8, 2017 02:19

cpuguy83 changed the title ~~[WIP] Plugins on swarm~~ Plugins on swarm Jun 8, 2017

cpuguy83 force-pushed the plugins_on_swarm branch 3 times, most recently from 9ab7953 to 97dec15 Compare June 8, 2017 18:37

cpuguy83 force-pushed the plugins_on_swarm branch 2 times, most recently from efcd344 to 9b02932 Compare June 8, 2017 18:54

aaronlehmann reviewed Jun 8, 2017

View reviewed changes

daemon/cluster/convert/service.go Outdated

Copy link
Copy Markdown

aaronlehmann Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we'll change this to gRPC anyway, but the error needs to be checked.

aaronlehmann reviewed Jun 8, 2017

View reviewed changes

liron-l mentioned this pull request Jun 11, 2017

Pluggable secret backend moby/swarmkit#2239

Merged

6 tasks

aaronlehmann reviewed Jul 6, 2017

View reviewed changes

daemon/cluster/convert/service.go Outdated

Copy link
Copy Markdown

aaronlehmann Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the blank line?

Make plugin emit strongly typed, consumable events

72c3bcf

Enables other subsystems to watch actions for a plugin(s). This will be used specifically for implementing plugins on swarm where a swarm controller needs to watch the state of a plugin. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

aaronlehmann approved these changes Jul 6, 2017

View reviewed changes

cpuguy83 mentioned this pull request Jul 7, 2017

Plugins on swarm with private repos #34010

Open

jamiegs mentioned this pull request Jul 13, 2017

Switch TaskSpec to use pointers for ContainerSpec so tests build successfully. fsouza/go-dockerclient#657

Merged

cyli mentioned this pull request Jul 20, 2017

ssh config files and/or private keys vieux/docker-volume-sshfs#24

Closed

dnephin reviewed Jul 26, 2017

View reviewed changes

dnephin mentioned this pull request Jul 26, 2017

Remove application logic from client/ package #34268

Open

mdlinville mentioned this pull request Jul 27, 2017

Document plugins for swarm mode docker/docs#4003

Closed

vieux mentioned this pull request Aug 9, 2017

hide swarm plugins behind experimental flag #34455

Merged

yosifkit mentioned this pull request Aug 10, 2017

Add infinit. docker-library/docs#919

Closed

andrewhsu mentioned this pull request Aug 15, 2017

WIP [17.07] update vendor of components/engine in cli docker-archive/docker-ce#195

Closed

Dimrok mentioned this pull request Aug 31, 2017

Docker Swarm Service infinit/infinit#45

Closed

thaJeztah mentioned this pull request Apr 30, 2018

Propagate managed plugins in swarm #36968

Open

thaJeztah mentioned this pull request Apr 16, 2020

ServiceCreate/ServiceUpdate: refactor and fix potential NPE #40826

Merged

thaJeztah mentioned this pull request Jul 22, 2025

API: types/plugins/logdriver must be removed from API module #50473

Closed

Conversation

cpuguy83 commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Jun 8, 2017

Uh oh!

ehazlett commented Jun 8, 2017

Uh oh!

aaronlehmann commented Jun 8, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jul 5, 2017

Uh oh!

diogomonica commented Jul 5, 2017

Uh oh!

thaJeztah commented Jul 6, 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!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Jul 6, 2017

Uh oh!

cpuguy83 commented Jul 6, 2017

Uh oh!

yongtang commented Jul 7, 2017

Uh oh!

rogaha commented Jul 7, 2017

Uh oh!

cpuguy83 commented Jul 7, 2017

Uh oh!

rogaha commented Jul 7, 2017

Uh oh!

cpuguy83 commented Jun 7, 2017 •

edited

Loading