[WIP] Add support for server-side stacks by dhiltgen · Pull Request #1721 · docker/cli

dhiltgen · 2019-03-06T20:28:21Z

This PR is currently marked WIP, because work in the underlying docker/stacks repository is still ongoing.

Contacts for this PR are myself, @dperny and @alexmavr

- What I did

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

The new server-side stacks support aims to move as much business logic dealing with stacks to the server side as possible in order to achieve a consistent experience across all clients (CLI, GUI, etc.) while bringing the benefits of a 1st-class knowledge of stacks on the server with a reconciler to ensure the actual state matches the desired state for the stack.

- How I did it

Update vendoring to include the new stack types, and client API definitions for server side handling of stacks operations
Move existing client-side logic into "legacy" subdirs for eventual deprecation and removal.
Toggle client-side vs. server-side logic based on API version
New client-side logic leveraging the server-side implementations for stack operations

- How to verify it

Combine with [WIP] Add support for server-side stacks moby/moby#38832 for end-to-end usage
Unit test coverage is included in this PR
end-to-end tests are being developed in the https://github.com/docker/stacks repo for inclusion in the broader end-to-end Docker platform tests

- Description for the changelog

Add support for server-side stacks

- A picture of a cute animal (not mandatory but encouraged)

This is my dog Trinity, playing along with her human siblings

Signed-off-by: Daniel Hiltgen <[email protected]>

This change moves the legacy client-side implementations for kubernetes and swarm stacks into new sub-packages for eventual deprecation/removal in the future. The new implementation relies on the server side implementatoin introduced in the upcoming Docker platform version. Signed-off-by: Daniel Hiltgen <[email protected]>

Signed-off-by: Daniel Hiltgen <[email protected]>

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]>

thaJeztah · 2019-03-06T23:27:09Z

 vbom.ml/util 256737ac55c46798123f754ab7d2c784e2c71783
+
+#get libnetwork packages
+github.com/docker/libnetwork 1a06131fb8a047d919f7deaf02a4c414d7884b83


curious why all these dependencies are now needed on the client side 🤔

Yeah, I was bummed this pulled in so much but it comes from the code just below this comment block: https://github.com/docker/cli/pull/1721/files#diff-04999fb6f01d1478d54e2713a0b41a15R113

I also noticed here that we're pulling in the backend;

cli/vendor/github.com/docker/stacks/pkg/interfaces/interfaces.go

Lines 6 to 37 in 99c7e42

"github.com/docker/docker/api/server/router/network"

dockerTypes "github.com/docker/docker/api/types"

"github.com/docker/docker/api/types/events"

"github.com/docker/docker/api/types/filters"

"github.com/docker/docker/api/types/swarm"

"github.com/docker/stacks/pkg/types"

)

// StacksBackend is the backend handler for Stacks within the engine.

// It is consumed by the API handlers, and by the Reconciler.

type StacksBackend interface {

CreateStack(types.StackCreate) (types.StackCreateResponse, error)

GetStack(id string) (types.Stack, error)

ListStacks() ([]types.Stack, error)

UpdateStack(id string, spec types.StackSpec, version uint64) error

DeleteStack(id string) error

// The following operations are only used by the Reconciler and not

// exposed via the Stacks API.

GetSwarmStack(id string) (SwarmStack, error)

ListSwarmStacks() ([]SwarmStack, error)

ParseComposeInput(input types.ComposeInput) (*types.StackCreate, error)

}

// SwarmResourceBackend is a subset of the swarm.Backend interface,

// combined with the network.ClusterBackend interface. It includes all

// methods required to validate, provision and update manipulate Swarm

// stacks and their referenced resources.

type SwarmResourceBackend interface {

network.ClusterBackend

Many of these should not be in the CLI (and might not even work on macOS, Windows)

If those dependencies pop up in the client code (libnetwork, hashcorp/memberlist, consul, etcd ?) it means the packages in docker/stack are not loosely coupled. There should be no reason for the cli to pull those dependencies 😓

A small package split / reorganization on docker/stack is required for this to get in docker/cli

Agreed, the stacks/pkg/interfaces package is meant for use only by components that are internal to the engine. We'll try to unwrangle why it's being referenced here

Signed-off-by: Daniel Hiltgen <[email protected]>

codecov-io · 2019-03-08T15:55:17Z

Codecov Report

Merging #1721 into master will increase coverage by 0.06%.
The diff coverage is 78.39%.

@@            Coverage Diff             @@
##           master    #1721      +/-   ##
==========================================
+ Coverage   56.16%   56.22%   +0.06%     
==========================================
  Files         306      309       +3     
  Lines       21016    21389     +373     
==========================================
+ Hits        11803    12026     +223     
- Misses       8359     8491     +132     
- Partials      854      872      +18

silvin-lubecki · 2019-03-12T14:04:44Z

I dont' understand why all the github.com/docker/stacks code is not in the github.com/moby/moby repository, as we move the logic from the client to the server side? @dhiltgen ?

dperny · 2019-03-12T14:33:14Z

same reason swarmkit is not in the moby/moby repository. more modularity.

This shifts the env variable substitution logic client side as we wont be able to complete the server side implementation in time for this release. It also adds a new inspect command. Signed-off-by: Daniel Hiltgen <[email protected]>

silvin-lubecki · 2019-03-12T15:27:06Z

@dperny I don't see why you can't have the same modularity with a package inside moby/moby? 🤔
A package is a package, just moving it to its own repo won't add more modularity.

vdemeester

I do agree with @silvin-lubecki on the repository… There is also a couple of things that seems to be weird / a pain

docker/stacks copies the compose structs (here). We really should extract those structs/functions in a standalone repository that would act as a library (for example docker/libcompose 👼 — that has been dead for ages) that other project would use (docker/app, docker/compose-on-kubernetes, docker/stacks, docker/cli, moby/moby, and most likely a lot others outside of docker/moby's reach…)
the upstream repository has a really weird line in the README.md, that is really not welcoming for contributors : "This repository is solely maintained by Docker, Inc." — I'm really really not sure how to take this sentence… Especially given the next point where we basically render the whole client-side compose implementation deprecated for this docker/stacks code — feels like we are going from "anybody can work of this compose impl." to "just docker inc. can"…
We can't move packages as easy as what's done here cli/command/stack/* -> cli/command/stack/legacy/* : projects depends on it (docker/app is one example but it's most likely not the only one). Same goes for cli/cli/compose -> cli/cli/legacy/compose… Moreover, it doesn't really make sense to me as they're not really in the way of the other packages that are on another repository…
If we really want to move these packages, we have to use go aliases so that it doesn't break project that depends on it (and at very least doesn't break them in one day…).
This is adding quite some duplication (of existing feature that could be.. refactor to be easier to use if they do not fit the new workflow put into place in this PR) — that looks a bit like a case of NYIH
Design wise, I feel this adds TONS of complexity. I feel like the "train" for server-side compose file passed quite some time ago (but I might be wrong here). And I agree with @cpuguy83 on the moby/moby PR. Not sure the current client structs in docker/cli are the correct abstraction for a server side stack implementation.

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.

vdemeester · 2019-03-12T17:49:05Z

 	"strings"

-	"github.com/docker/cli/cli/compose/convert"
+	"github.com/docker/cli/cli/legacy/compose/convert"


vdemeester · 2019-03-12T17:51:53Z

+	return nil
+}
+
+func loadDotEnv(workingDir string) (map[string]string, error) {


I feel like these pieces of code are common across moby/moby and docker/cli codebase… we may want to share some parts…

vdemeester · 2019-03-12T17:59:40Z

+	"github.com/docker/cli/cli/legacy/compose/loader"
+	"github.com/docker/cli/cli/legacy/compose/schema"
+	composeTypes "github.com/docker/cli/cli/legacy/compose/types"
+	composetypes "github.com/docker/cli/cli/legacy/compose/types"


there is one too much here

vdemeester · 2019-03-12T18:01:51Z

+	"github.com/docker/stacks/pkg/types"
+)
+
+func substituteProperties(input *types.ComposeInput, workingDir string) error {


Why is this here ? Why not reusing code from cli/cli/compose package ? This adds yet another implementation of this variable substitution…

thaJeztah · 2019-03-14T12:16:23Z

the upstream repository has a really weird line in the README.md, that is really not welcoming for contributors

@vdemeester I opened docker-archive/stacks#51 to make that messaging a bit more friendly (YOLO) 😇😄

dhiltgen · 2019-03-14T18:48:21Z

Lets close this until we sort out what the path forward is with moby/moby#38872

Daniel Hiltgen added 3 commits March 6, 2019 12:03

Vendoring bumps for stacks

69c8c8a

Signed-off-by: Daniel Hiltgen <[email protected]>

Temporarily comment out the task listing code

3882ae0

Signed-off-by: Daniel Hiltgen <[email protected]>

dhiltgen requested a review from silvin-lubecki as a code owner March 6, 2019 20:28

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

Daniel Hiltgen added 2 commits March 6, 2019 13:51

lint/validate cleanups

99c7e42

Signed-off-by: Daniel Hiltgen <[email protected]>

thaJeztah reviewed Mar 6, 2019

View reviewed changes

Daniel Hiltgen added 2 commits March 8, 2019 07:54

Remove extranious vendoring deps - SQUASH ME!

20c6e6c

Signed-off-by: Daniel Hiltgen <[email protected]>

Switch to using the legacy compose conversion

8ea53a8

Signed-off-by: Daniel Hiltgen <[email protected]>

andrewhsu mentioned this pull request Mar 8, 2019

server-side stacks #1725

Closed

1 task

Daniel Hiltgen added 2 commits March 12, 2019 07:52

Bump docker+stacks vendoring - SQUASH ME

d637d02

Add client side env and inspect

d05d61d

This shifts the env variable substitution logic client side as we wont be able to complete the server side implementation in time for this release. It also adds a new inspect command. Signed-off-by: Daniel Hiltgen <[email protected]>

vdemeester added status/1-design-review and removed status/0-triage labels Mar 12, 2019

vdemeester requested changes Mar 12, 2019

View reviewed changes

dhiltgen closed this Mar 14, 2019

	"github.com/docker/docker/api/server/router/network"
	dockerTypes "github.com/docker/docker/api/types"
	"github.com/docker/docker/api/types/events"
	"github.com/docker/docker/api/types/filters"
	"github.com/docker/docker/api/types/swarm"

	"github.com/docker/stacks/pkg/types"
	)

	// StacksBackend is the backend handler for Stacks within the engine.
	// It is consumed by the API handlers, and by the Reconciler.
	type StacksBackend interface {
	CreateStack(types.StackCreate) (types.StackCreateResponse, error)
	GetStack(id string) (types.Stack, error)
	ListStacks() ([]types.Stack, error)
	UpdateStack(id string, spec types.StackSpec, version uint64) error
	DeleteStack(id string) error

	// The following operations are only used by the Reconciler and not
	// exposed via the Stacks API.
	GetSwarmStack(id string) (SwarmStack, error)
	ListSwarmStacks() ([]SwarmStack, error)

	ParseComposeInput(input types.ComposeInput) (*types.StackCreate, error)
	}

	// SwarmResourceBackend is a subset of the swarm.Backend interface,
	// combined with the network.ClusterBackend interface. It includes all
	// methods required to validate, provision and update manipulate Swarm
	// stacks and their referenced resources.
	type SwarmResourceBackend interface {
	network.ClusterBackend

Conversation

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

Uh oh!

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!

codecov-io commented Mar 8, 2019

Codecov Report

Uh oh!

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

Uh oh!

Uh oh!

dperny commented Mar 12, 2019

Uh oh!

silvin-lubecki commented Mar 12, 2019

Uh oh!

vdemeester left a comment • 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!

thaJeztah commented Mar 14, 2019

Uh oh!

dhiltgen commented Mar 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dhiltgen commented Mar 6, 2019 •

edited

Loading

silvin-lubecki commented Mar 12, 2019 •

edited

Loading

vdemeester left a comment •

edited

Loading