For docker stack ls make an error if Swarm and Kubernetes hosts do not match #1035

mat007 · 2018-05-03T14:38:28Z

- What I did

Issued an error for docker stack ls if Swarm and Kubernetes hosts do not match when --orchestrator=all

- How I did it

By comparing the host names:

if the host names are the same then we're good
if the docker daemon is reached using a scheme different than tcp (e.g. fd, npipe or unix) and Kubernetes host name/address is a loopback, we're good too
otherwise report an error

- How to verify it

Having both Docker for Desktop with Kubernetes and UCP installed and configured:

$ kubectl config use-context docker-for-desktop
Switched to context "docker-for-desktop".

$ docker stack ls --orchestrator=all
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
dtc                 5                   Kubernetes          tata
dtc                 5                   Kubernetes          tutu
wall-e              2                   Swarm

$ kubectl config use-context ucp_34.209.72.126:6443_admin
Switched to context "ucp_34.209.72.126:6443_admin".

$ docker stack ls --orchestrator=all
Swarm and Kubernetes hosts do not match, check DOCKER_HOST and Kubernetes context

- Description for the changelog

Issue an error for --orchestrator=all when working on mismatched Swarm and Kubernetes hosts

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

thaJeztah · 2018-05-07T09:06:27Z

Renamed to "WIP", because this depends on #1031 to be merged first

silvin-lubecki

LGTM, thanks @mat007 !

vdemeester

SGTM :fallen_leaf:
cc @thaJeztah

thaJeztah · 2018-05-18T10:54:08Z

Ah, yes, I was looking at this one a couple of times, trying to grasp the "local socket" situation; let me have a look again

thaJeztah · 2018-05-18T12:19:34Z

cli/command/stack/kubernetes/cli.go

+			}
+		}
+	}
+	return fmt.Errorf("Swarm and Kubernetes hosts do not match, check DOCKER_HOST and Kubernetes context")


So, wondering if we should error, or just print a warning. For example, running inside my development container, this means I can't use --orchestrator=all;

cat ~/.kube/config | grep server: server: https://192.168.65.2:6443 docker stack ls Swarm and Kubernetes hosts do not match, check DOCKER_HOST and Kubernetes context docker --orchestrator=kubernetes stack ls NAME SERVICES ORCHESTRATOR NAMESPACE cat ~/.kube/config | grep server: server: https://host.docker.internal:6443 docker stack ls Swarm and Kubernetes hosts do not match, check DOCKER_HOST and Kubernetes context docker --orchestrator=kubernetes stack ls NAME SERVICES ORCHESTRATOR NAMESPACE

We should also have a way to find out what we're using, e.g. docker version doesn't show which server(s) I'm connected to ;

Client: Version: 18.06.0-dev API version: 1.37 Go version: go1.10.2 Git commit: 6dc89d02 Built: Fri May 18 11:15:43 2018 OS/Arch: linux/amd64 Experimental: true Orchestrator: all 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 Kubernetes: Version: v1.9.6 StackAPI: v1beta2

Maybe it should land to docker info instead?

Yes, docker info definitely, and likely the error/warning should include the information as well;

Swarm and Kubernetes hosts do not match, check DOCKER_HOST and Kubernetes context

(i.e, it should mention what it found)

I agree we should try to get more details exposed (most likely in info) but there may be some other wrinkles we need to work out to get that fully wired up. A follow up PR perhaps?

If we could improve the error message to give a little more contextual information that might help users get themselves unstuck. Perhaps something like this?

return fmt.Errorf("Swarm and Kubernetes hosts do not match - DOCKER_HOST=%s and kubernetes context=%s - update DOCKER_HOST or use 'kubectl config use-context' to match", daemonEndpoint.Hostname(), kubeEndpoint.Hostname())

@dhiltgen question is: should it be a fatal error? In the example I gave, swarm and kubernetes are actually on the same host, but we're not able to detect that (because we don't know that 192.168.65.2 is actually localhost).

My worry is that this will be a blocker for users (for example, when running a docker CLI inside a container, which is a fairly common situation for CI)

Instead of returning an error here, we could print a warning, and just use the host.

I think folks were concerned about subtle failure modes with miswired configurations leading to worse confusion. My $0.02 is a hard fail is OK as long as there's enough detail in the error message so the user can figure it out and correct the problem.

so the user can figure it out and correct the problem.

That's exactly the problem: the user's configuration is correct, so there is no workaround for the user.

It's a "best effort" detection to notify the user in situations where (possibly) swarm and kubernetes don't match, but there's many scenarios where we will be wrong.

mat007 · 2018-05-20T13:06:59Z

I updated the message and turned it into a warning log, @thaJeztah @dhiltgen just tell me if we rather want an error instead and I'll change it back.

What sort of output do we want in docker info?
There's already a Swarm section but nothing for Kubernetes so far.
Or do we rather have a separate Orchestrators section to group them together?

thaJeztah · 2018-05-20T13:25:36Z

cli/command/stack/kubernetes/cli.go

+			}
+		}
+	}
+	fmt.Fprintf(c.Out(), "Warning: Swarm and Kubernetes hosts do not match - DOCKER_HOST=%s and kubernetes context=%s -"+


Should probably be printed on stderr instead of stdout?

thaJeztah · 2018-05-20T13:27:11Z

cli/command/stack/kubernetes/cli.go

+		}
+	}
+	fmt.Fprintf(c.Out(), "Warning: Swarm and Kubernetes hosts do not match - DOCKER_HOST=%s and kubernetes context=%s -"+
+		" update DOCKER_HOST or use 'kubectl config use-context' to match\n", daemonEndpoint.Hostname(), kubeEndpoint.Hostname())


let me think a bit about mentioning DOCKER_HOST, because this could also be because of docker -H=<some host>

What about

"WARNING: Swarm and Kubernetes hosts do not match - docker host=%s and kubernetes context=%s -"+ " update DOCKER_HOST (or pass -H) or use 'kubectl config use-context' to match

?

Yes that looks better. Perhaps put the "docker host / kubernetes host" in brackets, and align the output for readability (as it's printed when working with docker interactively, we should make it user-friendly) e.g.;

WARNING: Swarm and Kubernetes hosts do not match (docker host=%s, kubernetes host=%s) Update $DOCKER_HOST (or pass -H), or use 'kubectl config use-context' to match.

Should we use context= or host= for k8s? "Context" may be confused with the name of the context?

host should be fine, I updated the message.

… match Signed-off-by: Mathieu Champlon <mathieu.champlon@docker.com>

thaJeztah

LGTM, thanks!

mat007 requested a review from vdemeester as a code owner May 3, 2018 14:38

GordonTheTurtle added the status/0-triage label May 3, 2018

thaJeztah added status/2-code-review area/stack area/kubernetes and removed status/0-triage labels May 7, 2018

thaJeztah changed the title ~~For docker stack ls make an error if Swarm and Kubernetes hosts do not match~~ [WIP] For docker stack ls make an error if Swarm and Kubernetes hosts do not match May 7, 2018

mat007 requested a review from thaJeztah as a code owner May 9, 2018 10:12

mat007 changed the title ~~[WIP] For docker stack ls make an error if Swarm and Kubernetes hosts do not match~~ For docker stack ls make an error if Swarm and Kubernetes hosts do not match May 15, 2018

silvin-lubecki approved these changes May 15, 2018

View reviewed changes

vdemeester approved these changes May 18, 2018

View reviewed changes

thaJeztah reviewed May 18, 2018

View reviewed changes

silvin-lubecki mentioned this pull request May 18, 2018

Remove Kubernetes commands from experimental #1068

Merged

thaJeztah requested changes May 20, 2018

View reviewed changes

For docker stack ls make an error if Swarm and Kubernetes hosts do no…

bfe9aed

… match Signed-off-by: Mathieu Champlon <mathieu.champlon@docker.com>

thaJeztah approved these changes May 22, 2018

View reviewed changes

vdemeester merged commit cc6ff56 into docker:master May 22, 2018

GordonTheTurtle added this to the 18.06.0 milestone May 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For docker stack ls make an error if Swarm and Kubernetes hosts do not match #1035

For docker stack ls make an error if Swarm and Kubernetes hosts do not match #1035

mat007 commented May 3, 2018

thaJeztah commented May 7, 2018

silvin-lubecki left a comment

vdemeester left a comment

thaJeztah commented May 18, 2018

thaJeztah May 18, 2018

thaJeztah May 18, 2018

silvin-lubecki May 18, 2018

thaJeztah May 18, 2018

dhiltgen May 18, 2018

thaJeztah May 18, 2018

dhiltgen May 19, 2018

thaJeztah May 19, 2018

mat007 commented May 20, 2018 •

edited

Loading

thaJeztah May 20, 2018

thaJeztah May 20, 2018

mat007 May 22, 2018 •

edited

Loading

thaJeztah May 22, 2018

mat007 May 22, 2018

thaJeztah left a comment

For docker stack ls make an error if Swarm and Kubernetes hosts do not match #1035

For docker stack ls make an error if Swarm and Kubernetes hosts do not match #1035

Conversation

mat007 commented May 3, 2018

thaJeztah commented May 7, 2018

silvin-lubecki left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

thaJeztah commented May 18, 2018

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

mat007 commented May 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mat007 May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

mat007 commented May 20, 2018 •

edited

Loading

mat007 May 22, 2018 •

edited

Loading