Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken swarm commands with Kubernetes defined as orchestrator #1137

Merged
merged 4 commits into from
Jun 22, 2018

Conversation

silvin-lubecki
Copy link
Contributor

- What I did

All swarm commands (node, service, etc...) were erroring out if the orchestrator was defined as Kubernetes. I moved the orchestrator flag, the environment variable and the config file value to more locally values regarding stack command.

- How I did it

  • Renamed DOCKER_ORCHESTRATOR to DOCKER_STACK_ORCHESTRATOR
  • Renamed config file option "orchestrator" to "stackOrchestrator"
  • "--orchestrator" flag is no more global but local to stack command and subcommands
  • Cleaned all global orchestrator code
  • Replicated Hidden flags in help and Supported flags from root command to stack command

- How to verify it

$ DOCKER_STACK_ORCHESTRATOR=kubernetes docker node ls
ID                            HOSTNAME                STATUS              AVAILABILITY        MANAGER STATUS      ENGINE VERSION
1u26o5yll4mdqxfr1o86xphb5 *   linuxkit-025000000001   Ready               Active              Leader              18.06.0-ce-dev

- Description for the changelog

  • Fixed broken swarm commands with Kubernetes defined as orchestrator
  • Renamed DOCKER_ORCHESTRATOR to DOCKER_STACK_ORCHESTRATOR
  • Renamed config file option "orchestrator" to "stackOrchestrator"
  • "--orchestrator" flag is no more global but local to stack command and subcommands

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

@silvin-lubecki
Copy link
Contributor Author

Please review with attention stack/cmd.go, and if you can make some manual tests too, it would be great 😄

Copy link
Member

@mat007 mat007 left a comment

Choose a reason for hiding this comment

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

A few comments!

return
}
if _, ok := f.Annotations["kubernetes"]; ok && !orchestrator.HasKubernetes() {
errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker cli with kubernetes features enabled", f.Name))
Copy link
Member

Choose a reason for hiding this comment

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

nit: back quotes would remove the need to escape the double quotes:

	errs = append(errs, fmt.Sprintf(`"--%s" is only supported on a Docker cli with kubernetes features enabled`, f.Name))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I just moved the code but you're right I'll fix it.

@@ -10,7 +10,7 @@ import (
"github.com/spf13/cobra"
)

func newPsCommand(dockerCli command.Cli) *cobra.Command {
func newPsCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think common needs to be a pointer here as it's always non-nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's because newPsCommand is called during command instantiation, but the common.orchestrator value will be updated in the stack/cmd.go:PersistentPreRunE. That's why it is passed as a pointer and not as a copied value.

@@ -13,7 +13,7 @@ import (
"vbom.ml/util/sortorder"
)

func newListCommand(dockerCli command.Cli) *cobra.Command {
func newListCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, it doesn't seem common can ever be nil.

)

var errUnsupportedAllOrchestrator = fmt.Errorf(`no orchestrator specified: use either "kubernetes" or "swarm"`)

type commonOptions struct {
orchestrator command.Orchestrator
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this type? Can't we use command.Orchestrator directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to update the value while resolving the orchestrator (conf file -> env -> flag), but we need to wait for the PersistentPreRun to be able to read the orchestrator flag value.

@@ -14,6 +15,10 @@ import (
"gotest.tools/golden"
)

var (
orchestrator = commonOptions{orchestrator: command.OrchestratorSwarm}
Copy link
Member

Choose a reason for hiding this comment

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

orchestrator is actually a commonOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I know, I was lazy... It was a first attempt with orchestrator, didn't want to rename 😅

@@ -121,13 +121,19 @@ func reformatDate(buildTime string) string {
return buildTime
}

// nolint: gocyclo
Copy link
Member

Choose a reason for hiding this comment

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

Really? :p

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, don't know why...

kubeVersion := getKubernetesVersion(dockerCli, opts.kubeConfig)
var kubeVersion *kubernetesVersion
if orchestrator.HasKubernetes() {
kubeVersion = getKubernetesVersion(opts.kubeConfig)
Copy link
Member

Choose a reason for hiding this comment

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

nit: or we could pass orchestrator to getKubernetesVersion instead of kubeConfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why instead of? I think getKubernetesVersion needs the kubeConfig to instantiate the kube client.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry got mixed up, I meant instead of the dockerCli which was passed before.

@@ -46,7 +46,7 @@ func AddNamespaceFlag(flags *flag.FlagSet) {
}

// WrapCli wraps command.Cli with kubernetes specifics
func WrapCli(dockerCli command.Cli, opts Options) (*KubeCli, error) {
func WrapCli(dockerCli command.Cli, opts Options, orchestrator command.Orchestrator) (*KubeCli, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Can't orchestrator be put inside Options somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try that!

@silvin-lubecki
Copy link
Contributor Author

The metalinter is not happy, nor the unit testing... I'll fix this!

@silvin-lubecki
Copy link
Contributor Author

PTAL

@antonybichon17
Copy link

I see all those testOrchestrator being removed but nothing replacing them... or am I missing something?

@silvin-lubecki
Copy link
Contributor Author

Tests were moved here

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

left some comments for discussion; trying to give it a spin, but nuked my k8s, lol

@@ -17,10 +17,25 @@ const (
OrchestratorAll = Orchestrator("all")
Copy link
Member

Choose a reason for hiding this comment

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

Looks like these constants can be un-exported? (and the GoDoc removed)

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind; I see they're used in a test; can be fixed, but no urgency

return command.GetStackOrchestrator(orchestratorFlag, config.StackOrchestrator)
}

func hideFlag(cmd *cobra.Command, orchestrator command.Orchestrator) {
Copy link
Member

Choose a reason for hiding this comment

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

hideOrchestrationFlags perhaps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Built: Wed May 30 22:21:05 2018
OS/Arch: linux/amd64
Experimental: true
Stack Orchestrator: swarm
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this now, a bit concerned how this will scale if support for more options is added 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked with @thaJeztah @dhiltgen and it seems more desirable to just get rid of the Orchestrator line for now and revisit.

@@ -17,10 +17,25 @@ const (
OrchestratorAll = Orchestrator("all")
orchestratorUnset = Orchestrator("unset")

defaultOrchestrator = OrchestratorSwarm
envVarDockerOrchestrator = "DOCKER_ORCHESTRATOR"
Copy link
Member

Choose a reason for hiding this comment

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

We may have to think about printing a warning if someone has this variable set.

I realize so far it's been experimental, so we're allowed to change things, but it's definitely possible people had this option set, and now it's being ignored.

Of course, to further complicate stuff; some day it may find its way back, once k8s integration on the cli is complete 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added the warning!

newPsCommand(dockerCli),
newRemoveCommand(dockerCli),
newServicesCommand(dockerCli),
newDeployCommand(dockerCli, &opts),
Copy link
Member

Choose a reason for hiding this comment

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

It's a pity the change has such a deep impact. Don't see another solution at this time though 😕

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh 😓

@@ -29,7 +29,7 @@ Client:{{if ne .Platform.Name ""}} {{.Platform.Name}}{{end}}
Built: {{.BuildTime}}
OS/Arch: {{.Os}}/{{.Arch}}
Experimental: {{.Experimental}}
Orchestrator: {{.Orchestrator}}
Stack Orchestrator: {{.StackOrchestrator}}
Copy link
Member

Choose a reason for hiding this comment

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

Still on the fence wether we should always print this, or only if any orchestrator is enabled

Copy link
Member

@thaJeztah thaJeztah Jun 21, 2018

Choose a reason for hiding this comment

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

Looks like we may want to; here's against a host that doesn't have swarm (or kube) enabled;

Client:
 Version:            18.06.0-dev
 API version:        1.37 (downgraded from 1.38)
 Go version:         go1.10.3
 Git commit:         ba061224
 Built:              Thu Jun 21 20:48:25 2018
 OS/Arch:            linux/amd64
 Experimental:       false
 Stack Orchestrator: swarm

Server:
 Engine:
  Version:      18.05.0-ce
  API version:  1.37 (minimum version 1.12)
  Go version:   go1.10.1
  Git commit:   f150324
  Built:        Wed May  9 22:20:16 2018
  OS/Arch:      linux/amd64
  Experimental: true

Output for all may be a bit confusing as well, but don't have better suggestions for that at this point

 Stack Orchestrator: all

@thaJeztah
Copy link
Member

Giving this a spin;

Using "all" orchestrator:

$ docker stack deploy --orchestrator=swarm -c docker-compose.yml foobar
Creating network foobar_default
Creating service foobar_web

$ docker stack ls
NAME                SERVICES            ORCHESTRATOR       NAMESPACE
foobar              1                   Swarm
$ docker stack deploy -c docker-compose.yml foobar 
no orchestrator specified: use either "kubernetes" or "swarm"

$ docker stack deploy --orchestrator=kubernetes -c docker-compose.yml foobar
Waiting for the stack to be stable and running...
web: Ready		[pod status: 1/1 ready, 0/1 pending, 0/1 failed]

Stack foobar is stable and running

$ docker stack ls
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
foobar              1                   Kubernetes          default
foobar              1                   Swarm               
something           1                   Swarm               

This is something we can improve (i.e.; if it's non-ambiguous, show the result);

$ docker stack ps something
no orchestrator specified: use either "kubernetes" or "swarm"
$ docker stack ps --orchestrator=kubernetes foobar
ID                  NAME                          IMAGE               NODE                 DESIRED STATE       CURRENT STATE           ERROR               PORTS
1fc7d698-75a        foobar_web-789778cdbf-8tshh   nginx:alpine        docker-for-desktop   Running             Running 3 minutes ago                       *:0->80/tcp

$ docker stack ps --orchestrator=swarm foobar
ID                  NAME                IMAGE               NODE                    DESIRED STATE       CURRENT STATE           ERROR               PORTS
otpw084mgv9f        foobar_web.1        nginx:alpine        linuxkit-025000000001   Running             Running 6 minutes ago    

(actually curious how we deal with overlaps in published ports)

$ docker stack services foobar --orchestrator=kubernetes

ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
1fd9a4d4-75a        foobar_web          replicated          1/1                 nginx:alpine        *:8080->80/tcp

$ docker stack services foobar --orchestrator=swarm

ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
9x0iam28mlsl        foobar_web          replicated          1/1                 nginx:alpine        *:8080->80/tcp

* Renaming DOCKER_ORCHESTRATOR to DOCKER_STACK_ORCHESTRATOR
* Renaming config file option "orchestrator" to "stackOrchestrator"
* "--orchestrator" flag is no more global but local to stack command and subcommands
* Cleaning all global orchestrator code
* Replicating Hidden flags in help and Supported flags from root command to stack command

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
@thaJeztah thaJeztah force-pushed the stack-orchestrator branch 3 times, most recently from 312408d to 87c39da Compare June 22, 2018 00:53
@thaJeztah
Copy link
Member

I squashed @silvin-lubecki's commits, and added an extra commit that (at least for now) removes the "Orchestration" information from docker info output until we have a better design for that

ping @dhiltgen @andrewhsu @vdemeester PTAL

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dhiltgen dhiltgen left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -78,7 +77,7 @@ type clientVersion struct {
Arch string
BuildTime string `json:",omitempty"`
Experimental bool
Orchestrator string `json:",omitempty"`
StackOrchestrator string `json:",omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@thaJeztah it looks like this got left in (presumably to keep the changes to a minimum) - seems OK, but we should clean this up as part of a follow up pass.

Copy link
Member

Choose a reason for hiding this comment

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

Good one; was actually doubting whether or not we wanted to keep it; perhaps better to remove it yes, as it will show up in docker version --format '{{json .}}'

I'll grab my laptop and update

The output of this information can be confusing,
so removing until we have a better design for this.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The `-k` shorthand was alreaady removed from other
commands, so best to be consistent.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the stack-orchestrator branch from 87c39da to f0a8598 Compare June 22, 2018 06:35
@thaJeztah
Copy link
Member

@dhiltgen @silvin-lubecki @vdemeester I pushed some additional updates;

  • Removed the StackOrchestrator from the version struct (as discussed above Fix broken swarm commands with Kubernetes defined as orchestrator #1137 (comment))
  • Updated the documentation, and fixed some minor issues with the USAGE command
    • note that the command-reference documentation on docs.docker.com is generated from a YAML file, so changes the markdown for individual commands is only to make GitHub match reality
  • Removed the -k shorthand on docker version; this shorthand was already removed (or not added) to any of the other commands. this is a new change

Remaining changes

  • documentation likely needs additional work (already being discussed on slack)
  • Bash/Fish/zsh completion scripts do not have all the options

cmd := &cobra.Command{
Use: "stack",
Use: "stack [OPTIONS]",
Copy link
Member

Choose a reason for hiding this comment

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

this output isn't actually used for commands that have subcommands, but good to have in case that situation changes in future

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

SGTM 💦

@@ -102,7 +102,7 @@ func managementSubCommands(cmd *cobra.Command) []*cobra.Command {
var usageTemplate = `Usage:

{{- if not .HasSubCommands}} {{.UseLine}}{{end}}
{{- if .HasSubCommands}} {{ .CommandPath}} COMMAND{{end}}
{{- if .HasSubCommands}} {{ .CommandPath}}{{- if .HasAvailableFlags}} [OPTIONS]{{end}} COMMAND{{end}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Isn't this change unrelated ? 😝

Copy link
Member

Choose a reason for hiding this comment

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

The docker stack command itself now has options, but there was no [OPTIONS] printed 😞

newPsCommand(dockerCli),
newRemoveCommand(dockerCli),
newServicesCommand(dockerCli),
newDeployCommand(dockerCli, &opts),
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh 😓

@silvin-lubecki
Copy link
Contributor Author

PTAL

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

SGTM 🐸

@andrewhsu
Copy link
Contributor

@silvin-lubecki talked with @dhiltgen and @thaJeztah ... would prefer to have this PR without 8fb2e7d. the DOCKER_ORCHESTRATOR env var was in experimental mode so we don't have to treat that as a supported interface that needs migration.

if still strongly desired for inclusion, i'd suggest putting into a new PR.

@thaJeztah thaJeztah force-pushed the stack-orchestrator branch from 8fb2e7d to c2c317c Compare June 22, 2018 18:56
@@ -43,13 +42,7 @@ func NewStackCommand(dockerCli command.Cli) *cobra.Command {
}
defaultHelpFunc := cmd.HelpFunc()
cmd.SetHelpFunc(func(cmd *cobra.Command, args []string) {
config := cliconfig.LoadDefaultConfigFile(dockerCli.Err()) // dockerCli is not yet initialized, but we only need config file here
Copy link
Member

Choose a reason for hiding this comment

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

Modified the last commit, and removed the warning, but kept these changes

Copy link
Member

Choose a reason for hiding this comment

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

Discussing with @dhiltgen - dropping this commit for now, and keeping it for #1139

@thaJeztah
Copy link
Member

Moved the warning to #1139

@thaJeztah thaJeztah force-pushed the stack-orchestrator branch from c2c317c to f0a8598 Compare June 22, 2018 20:12
Copy link
Contributor

@dhiltgen dhiltgen left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants