[WIP] Add support for server-side stacks by dperny · Pull Request #38832 · moby/moby

dperny · 2019-03-06T18:11:09Z

This PR is currently marked WIP, because work in the underlying docker/stacks repository is still ongoing. However, the code integrating it with the engine should be stable and ready for review, even if the underlying components themselves may change

Contacts for this PR are me, @dhiltgen, and @alexmavr

- What I did

Add support for server-side stacks by importing and wiring up the github.com/docker/stacks libraries.

- How I did it

There are three parts to this PR:

A new set of API routes and handlers, and a backend to accommodate them. These parts are created in a similar fashion to the Cluster object, in cmd/dockerd/daemon.go.
A set of methods on the Cluster object for storing the new Stacks in swarm's distributed object store. These are wired up such that the cluster can be passed to the stacks backend. These methods leverage swarmkit's new Resources and Extensions, which allows the daemon to store arbitrary data in the distributed object store. The foundation laid out here can be, in the future, extended to other kinds of resources that are external to swarmkit, but which we would like to persist in a cluster.
A new component, the stacks reconciler, which handles creating, updating, and deleting objects in order to manage the stack. It is wired up in cmd/dockerd/daemon.go, but is also included in the daemon in order to receive callbacks. It otherwise stands alone and manages its own lifecycle.

- How to verify it

The github.com/docker/stacks code is (surprisingly thoroughly) tested in its own repo. The integration code largely does not yet have tests, as this PR is still WIP and subject to lots of review and changes. There are end-to-end tests in progress which should verify that the integration all works.

- Description for the changelog

Add support for server-side stacks

- A picture of a cute animal (not mandatory but encouraged)
This is my cat, I took this picture this morning:

dperny · 2019-03-06T18:11:25Z

This initial revision builds, but likely does not pass tests.

cpuguy83 · 2019-03-06T20:59:47Z

Really glad to see this! Do you have a link to the PR introducing the API for custom resources in swarmkit?

cpuguy83 · 2019-03-06T21:01:23Z

p.s. this really would have been nice to have in the roadmap.md
This is a huge code drop without even a whisper.

dperny · 2019-03-06T21:06:57Z

I have no intention of exposing the Resource API publicly at this time. I think there's too much work involved in making it consumable by end users. However, I do believe that there are probably more things like Stacks which we might want to store in Swarmkit, so the API is available internally for new object types.

dperny · 2019-03-06T21:09:51Z

Sorry about the code drop. I actually got involved with this project relatively late in the process, after the code had been started. I'm carrying this PR because I have the most experience working in the engine like this.

alexmavr · 2019-03-06T21:22:35Z

It may be worth nothing that besides the portions vendored through this PR, the https://github.com/docker/stacks project includes components that can be used outside the scope of the moby project, such as:

A Standalone runtime which can be ran as an independent server on top of the Docker API, and is currently used primarily for testing and validation purposes
A Stack API Router which allows multiple "backend" instances of the Stacks API to be multiplexed in a single endpoint based on the target Orchestration field.
A Kubernetes backend which allows the Stack API to be surfaced on top of an instance of the https://github.com/docker/compose-on-kubernetes project.

thaJeztah · 2019-03-06T22:15:57Z


 # stacks and their deps
-github.com/docker/stacks 8e8ca77c0ac58e3f2fafbf7490340be1a91c8659 git@github.com:docker/stacks.git
+github.com/docker/stacks 68ec0f837572a4e63e4e8993c8b3504a99eb9c2e /home/dperny/go/src/github.com/docker/stacks


repo is public now, so /home/dperny/go/src/github.com/docker/stacks could be removed (I guess vndr is failing on that right now)

dhiltgen · 2019-03-06T22:22:41Z

+		Client:            c.client,
+		Version:           c.version,
+		CustomHTTPHeaders: c.customHTTPHeaders,
+	})


I should point out that we're not 100% happy with this implementation detail just yet, but what we've been aiming for is a pattern where projects integrating into Moby can define their own API and client libraries that can work in isolation, and also be integrated into the overall client library with minimal integration glue. There's more cut-and-paste code in https://github.com/docker/stacks/tree/master/pkg/client than we'd like, so ultimately where I think this should go is more of the APIs docker/docker/client are teased out into importable pkg modules that are reusable.

What are you missing? from the client package here?

Should this be passed as an op perhaps?

NewClientWithOpts(client.WithStackClient(...))

These were the two major chunks

https://github.com/docker/stacks/blob/master/pkg/client/errors.go

https://github.com/docker/stacks/blob/master/pkg/client/request.go

... and something to address https://github.com/docker/stacks/blob/master/pkg/client/client.go#L12-L31

I'm really not sure what this subclient is for.
We have the client in github.com/docker/docker/client -- what is missing from the client for you to implement a stacks client? Are you looking for a more HTTP client wrapper?

I'm really not sure what this subclient is for

It's a client library for dealing with stacks

https://github.com/docker/stacks/blob/master/pkg/client/interface.go#L10-L17

what is missing from the client for you to implement a stacks client?

We had to copy-and-paste a bit of the low-level code for interacting with a Docker REST style API server, since the functions in the docker/docker/client package were not exported. As we try to break up the monolith and fulfill the Moby vision, other projects will wind up re-inventing this same low-level http client code, so it seems like we should consider generalizing that so its reusable.

Seems like these could be refactored into a HTTP client wrapper.

@cpuguy83 in general, what are you thinking by "refactored into a HTTP client wrapper"? i'm unclear on what you mean by that.

I mean make a new package for the low level HTTP handling that can be shared by both github.com/docker/docker/client and github.com/docker/stacks/pkg/client.

dperny · 2019-03-07T16:55:09Z

I'm using new commits instead of force-pushing so that we can keep up on changes. We can squash this one way or another before we ship it.

cpuguy83 · 2019-03-07T17:31:12Z

Thinking a bit about this.
Disclaimer, I have not really gone through the stacks codebase yet.

Something that worries me is pulling in something that is completely a client side specification into the API. Of course I understand that this (at least potentially) brings much better safety guarantees than the client implementation is able to do, however the format itself is a bit problematic.

There's already many open issues about shortcomings of the (compose) format that have been left unaddressed with many heated arguments on the issue tracker. This bares a striking resemblance to the situation with Dockerfiles where we have this highly opinionated format which, historically, is notoriously difficult to introduce new features for, is tied to specific versions of Docker, and is a source of much contention and strife in the community. The solution to this was buildkit, which enables users to define their own frontend format that is translated into a common instruction set (LLB) which can then be run on any backend that understand it.

Now, I understand that the format being passed into the daemon is not the literal yaml doc, but it is basically just a conversion of it from yaml to json and as such it has the same issues... maybe even more because the lifecycle management is tied to the daemon rather than a client and as such makes it far less malleable.
I would like if we can gain the safety (for all clients) by handling the objects in the daemon but without sacrificing the flexibility the clients currently have.

This flips to the correct version numbers, which means we can turn the e2e tests back on. If the engine used during testing is from master before merging moby/moby#38832 it will incorrectly think the server-side stacks APIs are present. Signed-off-by: Daniel Hiltgen <[email protected]>

dperny · 2019-03-12T16:26:36Z

this is sort of based around a similar idea to what you seem to be describing for buildkit, actually, but sort of different

basically, there is a want to support both Kubernetes and Swarm stacks with compose. currently, as i understand, you can do that, but it's currently based around using two different sets of APIs. the goal here is to create one central stream through which you can access multiple stack providers.

the compose yaml types end up being the common language spoken across the different backends. we use them because they already exist, and there is already parsing and conversion logic for compose types to the various backend types.

cpuguy83 · 2019-03-12T16:46:39Z

A common API between the two is not the same thing at all. This is a very high level API.
Buildkit offers a means of customizing the runtime execution.

What I am saying is this PR fixes the atomicity issues of client side stacks but dramatically reduces flexibility in implementation. All this for a format that is already known to be problematic.

We could expose custom swarmkit resources to the docker API as experimental and work towards a solution where docker/stacks can be built on top of that... stacks would basically be exercising the custom resource API and gives us a chance to change as needed.
This would be in the spirit of keeping the surface area of moby from growing even more out of control.

codecov · 2019-03-12T17:22:21Z

Codecov Report

Merging #38832 into master will decrease coverage by 0.08%.
The diff coverage is 8.94%.

@@            Coverage Diff             @@
##           master   #38832      +/-   ##
==========================================
- Coverage   36.47%   36.38%   -0.09%     
==========================================
  Files         613      614       +1     
  Lines       45814    45925     +111     
==========================================
+ Hits        16709    16710       +1     
- Misses      26823    26931     +108     
- Partials     2282     2284       +2

Signed-off-by: Drew Erny <[email protected]>

Adds the plumbing to support storing server-side stacks in swarmkit's distributed object store. Includes changes to support storing and retrieving stacks using swarmkit's Resource type, and changes to support stack events. Signed-off-by: Drew Erny <[email protected]>

Adds the plumbing for running the stacks reconciler Signed-off-by: Drew Erny <[email protected]>

Co-authored-by: Drew Erny <[email protected]> Co-authored-by: Daniel Hiltgen <[email protected]> Signed-off-by: Drew Erny <[email protected]> Signed-off-by: Daniel Hiltgen <[email protected]>

Signed-off-by: Drew Erny <[email protected]>

dperny · 2019-03-12T17:35:57Z

Updated the service inspect test to ignore the Meta.Version field, which should stop that test from failing.

Rebased to resolve vendor conflict.

dperny · 2019-03-12T17:41:16Z

Well this failure is a mystery:

17:38:06 The result of hack/generate-swagger-api.sh differs
17:38:06 
17:38:06 diff --git a/api/types/container/container_wait.go b/api/types/container/container_wait.go
17:38:06 index 9e3910a6b4..06b0f02077 100644
17:38:06 --- a/api/types/container/container_wait.go
17:38:06 +++ b/api/types/container/container_wait.go
17:38:06 @@ -7,14 +7,6 @@ package container
17:38:06  // See hack/generate-swagger-api.sh
17:38:06  // ----------------------------------------------------------------------------
17:38:06  
17:38:06 -// ContainerWaitOKBodyError container waiting error, if any
17:38:06 -// swagger:model ContainerWaitOKBodyError
17:38:06 -type ContainerWaitOKBodyError struct {
17:38:06 -
17:38:06 -	// Details of an error
17:38:06 -	Message string `json:"Message,omitempty"`
17:38:06 -}
17:38:06 -
17:38:06  // ContainerWaitOKBody OK response to ContainerWait operation
17:38:06  // swagger:model ContainerWaitOKBody
17:38:06  type ContainerWaitOKBody struct {
17:38:06 @@ -27,3 +19,11 @@ type ContainerWaitOKBody struct {
17:38:06  	// Required: true
17:38:06  	StatusCode int64 `json:"StatusCode"`
17:38:06  }
17:38:06 +
17:38:06 +// ContainerWaitOKBodyError container waiting error, if any
17:38:06 +// swagger:model ContainerWaitOKBodyError
17:38:06 +type ContainerWaitOKBodyError struct {
17:38:06 +
17:38:06 +	// Details of an error
17:38:06 +	Message string `json:"Message,omitempty"`
17:38:06 +}

thaJeztah · 2019-03-12T21:08:55Z

The swagger failure is unrelated; it's due to random ordering in the generator; see #36714

justincormack · 2019-03-13T11:59:26Z

@cpuguy83 I don't know about the idea of adding fully general purpose CRDs to swarmkit, it seems like, well if you want that just use Kubernetes. Although I agree that compose has versioning and feature issues across different releases, it is a core part of the Docker platform for many. Potentially moving it server side can solve many of the problems in it being client side, and maybe lead to a way forward for a more stable version?

cpuguy83 · 2019-03-13T15:46:51Z

I don't know about the idea of adding fully general purpose CRDs to swarmkit

That's what service side stacks is built on top of here in this PR, it's just not being exposed on the Docker API.

it is a core part of the Docker platform for many

So is Dockerfile (even more so than compose). Doesn't mean we should continue down the same exact path, especially knowing the pitfalls.

Potentially moving it server side can solve many of the problems in it being client side, and maybe lead to a way forward for a more stable version?

Again, Dockerfile. Also versioning is just one part of the problem. Even so, here we'll have compose v1, v2, v3 (3.1, 3.2, ...), and then the API version (1.41).
This is a terribly bad situation that we should not be allowing to happen again.

andrewhsu · 2019-03-13T15:57:40Z

The compose versioning seems like it should be addressed, but does this PR actually intend to change the compose format yet again? I think this PR is part of a set of changes intended to be a very transparent to the end user that calls docker stacks.

Would getting the server-side stacks into the codebase be helpful for the next iteration of design changes to the stacks behaviour and perhaps the compose format? @cpuguy83 that I think needs more to flesh out.

andrewhsu · 2019-03-13T15:57:58Z

To reduce the risk of this initial PR, taking a note from DOCKER_BUILDKIT, could there also be a DOCKER_SERVER_STACKS opt-in that would activate the codepaths? I think then we would have to add something desirable for the end user to want to activate the feature...perhaps some UI change that can only be accomplished with the new server-side stacks.

cpuguy83 · 2019-03-13T16:06:49Z

That does not solve the issues I've brought up at all.
Please stop trying to shove this in as is without actually addressing anything at all.

dperny · 2019-03-13T18:43:11Z

We're gonna need some refinement before this can get anything resembling merged, so I'm closing.

GordonTheTurtle added the status/0-triage label Mar 6, 2019

dperny force-pushed the add-stacks branch 2 times, most recently from b6a024f to d5e55b1 Compare March 6, 2019 19:10

dhiltgen mentioned this pull request Mar 6, 2019

[WIP] Add support for server-side stacks docker/cli#1721

Closed

thaJeztah added status/1-design-review and removed status/0-triage labels Mar 6, 2019

thaJeztah reviewed Mar 6, 2019

View reviewed changes

dhiltgen reviewed Mar 6, 2019

View reviewed changes

andrewhsu mentioned this pull request Mar 8, 2019

server-side stacks docker/cli#1725

Closed

1 task

dperny force-pushed the add-stacks branch from e1f261b to 12edaec Compare March 11, 2019 22:54

dperny and others added 7 commits March 12, 2019 12:35

Import github.com/docker/stacks and add routes

9cfd6bf

Signed-off-by: Drew Erny <[email protected]>

Add stacks reconciler

f3af076

Adds the plumbing for running the stacks reconciler Signed-off-by: Drew Erny <[email protected]>

Add Stacks support to API client

3701ecf

Co-authored-by: Drew Erny <[email protected]> Co-authored-by: Daniel Hiltgen <[email protected]> Signed-off-by: Drew Erny <[email protected]> Signed-off-by: Daniel Hiltgen <[email protected]>

update stacks vendoring

281276b

Signed-off-by: Drew Erny <[email protected]>

address linter failures

11972f1

Signed-off-by: Drew Erny <[email protected]>

Update service test so it stops failing

64a769e

Signed-off-by: Drew Erny <[email protected]>

dperny force-pushed the add-stacks branch from b300772 to 64a769e Compare March 12, 2019 17:35

dperny closed this Mar 13, 2019

dperny mentioned this pull request Mar 13, 2019

Implement Server Side handling of compose stacks. #38872

Open

kolyshkin mentioned this pull request Oct 9, 2019

Option to skip Validate() methods generation is gone go-swagger/go-swagger#2077

Open

AkihiroSuda mentioned this pull request Jan 24, 2020

docker stack deploy api not present? #40407

Closed

thaJeztah mentioned this pull request Sep 18, 2023

[Feature Request] Custom Composition Types on Swarm #34705

Closed

Conversation

dperny commented Mar 6, 2019

Uh oh!

dperny commented Mar 6, 2019

Uh oh!

cpuguy83 commented Mar 6, 2019

Uh oh!

cpuguy83 commented Mar 6, 2019

Uh oh!

dperny commented Mar 6, 2019

Uh oh!

dperny commented Mar 6, 2019

Uh oh!

alexmavr commented Mar 6, 2019

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!

dhiltgen Mar 6, 2019 • 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!

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!

dperny commented Mar 7, 2019

Uh oh!

cpuguy83 commented Mar 7, 2019

Uh oh!

dperny commented Mar 12, 2019

Uh oh!

cpuguy83 commented Mar 12, 2019

Uh oh!

codecov Bot commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dperny commented Mar 12, 2019

Uh oh!

dperny commented Mar 12, 2019

Uh oh!

thaJeztah commented Mar 12, 2019

Uh oh!

justincormack commented Mar 13, 2019

Uh oh!

cpuguy83 commented Mar 13, 2019

Uh oh!

andrewhsu commented Mar 13, 2019

Uh oh!

andrewhsu commented Mar 13, 2019

Uh oh!

cpuguy83 commented Mar 13, 2019

Uh oh!

dperny commented Mar 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dhiltgen Mar 6, 2019 •

edited

Loading

codecov Bot commented Mar 12, 2019 •

edited

Loading