Better namespace experience with Kubernetes by silvin-lubecki · Pull Request #991 · docker/cli

silvin-lubecki · 2018-04-09T14:51:37Z

- What I did
I redesigned a better UX for namespaces with Kubernetes:

the default namespace defined in the ~/.kube/config is now used with all stack commands
a NAMESPACE column is added for docker stack ls command on Kubernetes orchestrator only
a --all-namespaces flag is added for docker stack ls command on Kubernetes orchestrator only

- How I did it

For the default namespace, I modified the kubernetes/config.go:NewKubernetesConfig function to return a lower level config object, which can resolve an optional namespace defined in the kubernetes configuration file.
For the NAMESPACE column, I modified the formatter/stack.go:NewStackFormat function, which is now almost no-op. We now have two default format, a Kubernetes one with a NAMESPACE column, and a Swarm one without it, so I let the caller choosing the appropriate default table format. Buy the way now these two default format are exported. A closer look will be appreciated 😄
For the --all-namespaces flag, I patched the Stack client to handle the all-namespaces option on List command. All stack clients are namespaced, but with Kubernetes, setting the namespace to empty string means all namespaces.

- How to verify it

# Use Kubernetes by default for every following commands
$ export DOCKER_ORCHESTRATOR=kubernetes
# Deploy a stack on default namespace
$ docker stack deploy mystack -c docker-compose.yml
...
# List the stacks in the default namespace, it will show a new NAMESPACE column
$ docker stack ls
NAME SERVICES ORCHESTRATOR NAMESPACE
mystack 3 Kubernetes default
# Swarm hasn't any concept of namespace, so no NAMESPACE column
$ DOCKER_ORCHESTRATOR=swarm docker stack ls
NAME SERVICES ORCHESTRATOR
# Create a namespace on Kubernetes:
$ kubectl create namespace mynamespace
namespace "mynamespace" created
# Deploy a stack on that namespace
$ docker stack deploy mystack --namespace mynamespace -c docker-compose.yml
...
# List all stacks in all namespaces using --all-namespace flag
$ docker stack ls --all-namespaces
NAME SERVICES ORCHESTRATOR NAMESPACE
mystack 3 Kubernetes default
mystack 1 Kubernetes mynamespace
# Edit your ~/.kube/config file to add a different default namespace to your current context
$ cat ~/.kube/config
...
contexts:
- context:
cluster: docker-for-desktop-cluster
namespace: mynamespace
user: docker-for-desktop
name: docker-for-desktop
...
# List again the stacks in the default namespace
$ docker stack ls
NAME SERVICES ORCHESTRATOR NAMESPACE
mystack 3 Kubernetes mynamespace

- Description for the changelog

Fix using namespace defined in ~/.kube/config for stack commands
Add a NAMESPACE column for docker stack ls command
Add a --all-namespaces flag for docker stack ls command

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

⚠️ Depends on #973⚠️

thaJeztah · 2018-04-09T19:20:25Z

@silvin-lubecki looks like there's a linting issue

cli/command/stack/kubernetes/client.go:80:47:warning: exported method Stacks returns unexported type kubernetes.stackClient, which can be annoying to use (golint)
Exited with code 1

silvin-lubecki · 2018-04-09T21:13:25Z

Yes, I'm on it, thanks @thaJeztah!

silvin-lubecki · 2018-04-10T09:35:19Z

PTAL

mat007 · 2018-04-30T10:30:44Z

Rebased!

vieux · 2018-04-30T18:42:50Z

@silvin-lubecki what's the easiest way for me to test this ?

mat007 · 2018-05-02T13:29:56Z

@vieux silvin is on holiday, I'm carrying his PRs meanwhile.
The most simple means to install Kubernetes would be either through Docker for Desktop (Mac and Windows) or probably UCP (linux), after which following the «How to verify it» steps should work.
In Docker for Desktop there is only a checkbox to toggle in order to enable and install Kubernetes, I'm afraid I don't know how to proceed for UCP and linux, maybe @vdemeester has some experience with this?

vdemeester

LGTM 🐸

vdemeester · 2018-05-03T08:26:52Z

On non docker-for-desktop, you need to have orchestrator: "kubernetes" and experimental: "enabled" in the client configuration ($HOME/.docker/config.json) — and a kube configuration pointing to a live kubernetes cluster.

thaJeztah · 2018-05-08T15:00:42Z

+
+	configNamespace, _, err := clientConfig.Namespace()
+	if err != nil {
+ return nil, err


Looks like this code is executed (possibly showing an error) even if the namespace is overridden through opts.Namespace. Retrieving the namespace from the config should probably be done conditional (i.e., if there's no --namespace option set)?

Good point, I'll invert the condition below, but the code in here is likely going to change when I address your next comment about --namespace.

thaJeztah · 2018-05-08T15:00:58Z

+ return nil, err
+	}
+	cli.kubeNamespace = configNamespace
+	if opts.Namespace != "default" {


What if opts.Namespace is an empty string? (also; I'm not a big fan of magic words, such as "default" for default values)
What if I run docker stack ls --namespace=default; will the specified ("default") namespace be ignored? (perhaps kubernetes.NewOptions() should use flags.Changed() to detect if the flag was set (even if it's with the default value
cli/cli/command/stack/kubernetes/cli.go
Lines 31 to 41 in 854aad8
// NewOptions returns an Options initialized with command line flags
funcNewOptions(flags*flag.FlagSet) Options {
varoptsOptions
ifnamespace, err:=flags.GetString("namespace"); err==nil {
opts.Namespace=namespace
}
ifkubeConfig, err:=flags.GetString("kubeconfig"); err==nil {
opts.Config=kubeConfig
}
returnopts
}
)

Running a complete set of (manual) tests with both kubectl get stacks and docker stack ls reveals a few discrepancies. I'll sort that out.

thaJeztah · 2018-05-08T15:01:19Z

 // NewKubernetesConfig resolves the path to the desired Kubernetes configuration file, depending
 // environment variable and command line flag.
-func NewKubernetesConfig(configFlag string) (*restclient.Config, error) {
+func NewKubernetesConfig(configFlag string) clientcmd.ClientConfig {


not new, but the configFlag variable is a bit confusing (i.e., it's not the flag itself, but a custom path, which happens to be passed through a flag). Perhaps renamed this to (e.g.) configPath

Yes, changing this.

thaJeztah · 2018-05-08T15:01:50Z

-func NewKubernetesConfig(configFlag string) (*restclient.Config, error) {
+func NewKubernetesConfig(configFlag string) clientcmd.ClientConfig {
 	kubeConfig := configFlag
 	if kubeConfig == "" {


The exact same code is also in cli.WrapCli() (and looks to be redundant). I suggest extracting this to a function, e.g.
funcConfigPath(kubeconfigPathstring) string { ifkubeconfigPath!="" { returnkubeconfigPath } ifconfig:=os.Getenv("KUBECONFIG"); config!="" { returnconfig } returnfilepath.Join(homedir.Get(), ".kube/config") }
Is a non-existing path reason for an error?

Well actually WrapCli calls NewKubernetesConfig right after the duplicated code so this looks like the code was moved/factorized and not removed…
Fixed!

thaJeztah · 2018-05-08T15:15:15Z

 // NewStackFormat returns a format for use with a stack Context
 func NewStackFormat(source string) Format {
-	switch source {
-	case TableFormatKey:


This is a change in behaviour; before this change:
docker stack ls --format=table NAME SERVICES foobar 1
After this change:
docker stack ls --format=table # (no output)
Perhaps remove this function, and in swarm/kubernetes do;
iflen(format) ==0||format==formatter.TableFormatKey { format=formatter.KubernetesStackTableFormat } stackCtx:= formatter.Context{ Output: dockerCli.Out(), Format: formatter.Format(format), }

Indeed, fixing this!

thaJeztah · 2018-05-08T15:25:05Z

@@ -17,7 +17,7 @@ func RunList(dockerCli *KubeCli, opts options.List) error {
 	}
 	format := opts.Format
 	if len(format) == 0 {


See my other comment above

thaJeztah · 2018-05-08T15:25:16Z

@@ -25,7 +25,7 @@ func RunList(dockerCli command.Cli, opts options.List) error {
 	}
 	format := opts.Format
 	if len(format) == 0 {


See my other comment above

thaJeztah · 2018-05-08T15:29:46Z

 	flags := cmd.Flags()
 	flags.StringVar(&opts.Format, "format", "", "Pretty-print stacks using a Go template")
+	flags.BoolVarP(&opts.AllNamespaces, "all-namespaces", "", false, "List stacks among all Kubernetes namespaces")
+	flags.SetAnnotation("all-namespaces", "kubernetes", nil)


Does this have to be gated by API-version as well? (possibly not because it was experimental so far)

Yes it was experimental and also targeting only Kubernetes and UCP. The behavior toward the Docker daemon is supposed to not have been changed.

thaJeztah · 2018-05-08T15:37:04Z


 	flags := cmd.Flags()
 	flags.StringVar(&opts.Format, "format", "", "Pretty-print stacks using a Go template")
+	flags.BoolVarP(&opts.AllNamespaces, "all-namespaces", "", false, "List stacks among all Kubernetes namespaces")


Wonder if we considered
using --namespace=all (e.g.) to show all namespaces (which would be consistent with --orchestrator=all), also allowing listing stack in a single namespace --namespace=<something>
showing all namespaces by default, but use a --filter namespace=foo to limit the output

I believe what drives the current design is to mimic what kubectl does in order to not confuse Kubernetes users.

thaJeztah · 2018-05-08T15:38:26Z

Oh, this also needs a documentation update to document the .Namespace placeholder for --format

mat007 · 2018-05-09T07:33:14Z

For the record, here is the manual test suite I ran against kubectl, obviously this is highly dependent on the services deployed beforehand…

+ kubectl.exe config set-context docker-for-desktop --namespace=tata
Context "docker-for-desktop" modified.
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace
NAME NAMESPACE
dtc tata
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --namespace=default
NAME NAMESPACE
blaaaaaa default
blaaaaaaaaa default
dtcccccc default
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --namespace=
NAME NAMESPACE
dtc tata
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --namespace=tata
NAME NAMESPACE
dtc tata
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --namespace=bla
NAME NAMESPACE
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --all-namespaces
NAME NAMESPACE
blaaaaaa default
blaaaaaaaaa default
dtcccccc default
dtc tata
dtc tutu
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --all-namespaces --namespace=bla
NAME NAMESPACE
blaaaaaa default
blaaaaaaaaa default
dtcccccc default
dtc tata
dtc tutu
+ kubectl.exe config set-context docker-for-desktop --namespace=
Context "docker-for-desktop" modified.
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace
NAME NAMESPACE
blaaaaaa default
blaaaaaaaaa default
dtcccccc default
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --namespace=default
NAME NAMESPACE
blaaaaaa default
blaaaaaaaaa default
dtcccccc default
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --namespace=
NAME NAMESPACE
blaaaaaa default
blaaaaaaaaa default
dtcccccc default
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --namespace=tata
NAME NAMESPACE
dtc tata
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --namespace=bla
NAME NAMESPACE
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --all-namespaces
NAME NAMESPACE
blaaaaaa default
blaaaaaaaaa default
dtcccccc default
dtc tata
dtc tutu
+ kubectl get stacks --output=custom-columns=NAME:.metadata.name,NAMESPACE:.metadata.namespace --all-namespaces --namespace=bla
NAME NAMESPACE
blaaaaaa default
blaaaaaaaaa default
dtcccccc default
dtc tata
dtc tutu

mat007 · 2018-05-09T07:35:26Z

And here is the matching docker stack ls test suite output (after I fixed the issues with --namespace):

+ kubectl.exe config set-context docker-for-desktop --namespace=tata
Context "docker-for-desktop" modified.
+ ./build/docker.exe stack ls
NAME SERVICES ORCHESTRATOR NAMESPACE
dtc 5 Kubernetes tata
+ ./build/docker.exe stack ls --namespace=default
NAME SERVICES ORCHESTRATOR NAMESPACE
blaaaaaa 1 Kubernetes default
blaaaaaaaaa 1 Kubernetes default
dtcccccc 5 Kubernetes default
+ ./build/docker.exe stack ls --namespace=
NAME SERVICES ORCHESTRATOR NAMESPACE
dtc 5 Kubernetes tata
+ ./build/docker.exe stack ls --namespace=tata
NAME SERVICES ORCHESTRATOR NAMESPACE
dtc 5 Kubernetes tata
+ ./build/docker.exe stack ls --namespace=bla
NAME SERVICES ORCHESTRATOR NAMESPACE
+ ./build/docker.exe stack ls --all-namespaces
NAME SERVICES ORCHESTRATOR NAMESPACE
blaaaaaa 1 Kubernetes default
blaaaaaaaaa 1 Kubernetes default
dtc 5 Kubernetes tata
dtc 5 Kubernetes tutu
dtcccccc 5 Kubernetes default
+ ./build/docker.exe stack ls --all-namespaces --namespace=bla
NAME SERVICES ORCHESTRATOR NAMESPACE
blaaaaaa 1 Kubernetes default
blaaaaaaaaa 1 Kubernetes default
dtc 5 Kubernetes tata
dtc 5 Kubernetes tutu
dtcccccc 5 Kubernetes default
+ kubectl.exe config set-context docker-for-desktop --namespace=
Context "docker-for-desktop" modified.
+ ./build/docker.exe stack ls
NAME SERVICES ORCHESTRATOR NAMESPACE
blaaaaaa 1 Kubernetes default
blaaaaaaaaa 1 Kubernetes default
dtcccccc 5 Kubernetes default
+ ./build/docker.exe stack ls --namespace=default
NAME SERVICES ORCHESTRATOR NAMESPACE
blaaaaaa 1 Kubernetes default
blaaaaaaaaa 1 Kubernetes default
dtcccccc 5 Kubernetes default
+ ./build/docker.exe stack ls --namespace=
NAME SERVICES ORCHESTRATOR NAMESPACE
blaaaaaa 1 Kubernetes default
blaaaaaaaaa 1 Kubernetes default
dtcccccc 5 Kubernetes default
+ ./build/docker.exe stack ls --namespace=tata
NAME SERVICES ORCHESTRATOR NAMESPACE
dtc 5 Kubernetes tata
+ ./build/docker.exe stack ls --namespace=bla
NAME SERVICES ORCHESTRATOR NAMESPACE
+ ./build/docker.exe stack ls --all-namespaces
NAME SERVICES ORCHESTRATOR NAMESPACE
blaaaaaa 1 Kubernetes default
blaaaaaaaaa 1 Kubernetes default
dtc 5 Kubernetes tata
dtc 5 Kubernetes tutu
dtcccccc 5 Kubernetes default
+ ./build/docker.exe stack ls --all-namespaces --namespace=bla
NAME SERVICES ORCHESTRATOR NAMESPACE
blaaaaaa 1 Kubernetes default
blaaaaaaaaa 1 Kubernetes default
dtc 5 Kubernetes tata
dtc 5 Kubernetes tutu
dtcccccc 5 Kubernetes default

mat007 · 2018-05-09T08:04:11Z

@thaJeztah I believe I have addressed all your comments. I added small commits so it remains easy for you to check the changes, but afterwards if you would rather have me collapse them or merge them with the bigger commits from @silvin-lubecki I'll do it.

thaJeztah

LGTM, but if you can combine some of the commits, that'd be good

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

…elongs Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

…rnetes orchestrator Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

…rator, to list all stacks in all namespaces. Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

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

mat007 · 2018-05-09T15:46:24Z

Done, thanks for your help!

silvin-lubecki requested a review from vdemeester as a code owner April 9, 2018 14:51

GordonTheTurtle added the status/0-triage label Apr 9, 2018

silvin-lubecki changed the title ~~Better namespace experience with Kubernetes~~[WIP] Better namespace experience with KubernetesApr 9, 2018

silvin-lubecki force-pushed the kubernetes-namespace branch from a17f07f to b03b1c0Compare April 9, 2018 16:08

silvin-lubecki force-pushed the kubernetes-namespace branch from b03b1c0 to e3fb0c4Compare April 10, 2018 09:27

silvin-lubecki changed the title ~~[WIP] Better namespace experience with Kubernetes~~Better namespace experience with KubernetesApr 10, 2018

vdemeester added status/2-code-review and removed status/0-triage labels Apr 10, 2018

mat007 force-pushed the kubernetes-namespace branch from e3fb0c4 to 27f8396Compare April 30, 2018 10:29

mat007 mentioned this pull request Apr 30, 2018
Better stack list experience with Kubernetes and Swarm #1031
Merged

vdemeester approved these changes May 3, 2018
View reviewed changes

thaJeztah requested changes May 8, 2018
View reviewed changes

thaJeztah added the area/kubernetes label May 8, 2018

thaJeztah approved these changes May 9, 2018
View reviewed changes

silvin-lubecki added 5 commits May 9, 2018 17:30

Fix using the namespace specified in the kubeconfig file.
f0aa126
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Clean useless named imports
c23c148
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Move stack client creation from KubeCli to client factory, where it b…
31dccff
…elongs Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Add namespace column for docker stack ls command while targeting Kube…
65526a2
…rnetes orchestrator Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Add --all-namespaces to docker stack ls command on Kubernetes orchest…
f097831
…rator, to list all stacks in all namespaces. Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>

Fix stack ls behaviour with Kubernetes and variout --namespace flag v…
2af66be
…alues Signed-off-by: Mathieu Champlon <mathieu.champlon@docker.com>

mat007 force-pushed the kubernetes-namespace branch from a82c9f1 to 2af66beCompare May 9, 2018 15:41

wsong approved these changes May 9, 2018
View reviewed changes

thaJeztah merged commit 552ee50 into docker:masterMay 9, 2018

GordonTheTurtle added this to the 18.06.0 milestone May 9, 2018

silvin-lubecki deleted the kubernetes-namespace branch May 18, 2018 12:27

	// NewOptions returns an Options initialized with command line flags
	funcNewOptions(flags*flag.FlagSet) Options {
	varoptsOptions
	ifnamespace, err:=flags.GetString("namespace"); err==nil {
	opts.Namespace=namespace
	}
	ifkubeConfig, err:=flags.GetString("kubeconfig"); err==nil {
	opts.Config=kubeConfig
	}
	returnopts
	}

Uh oh!

Conversation

silvin-lubecki commented Apr 9, 2018

Uh oh!

thaJeztah commented Apr 9, 2018

Uh oh!

silvin-lubecki commented Apr 9, 2018

Uh oh!

silvin-lubecki commented Apr 10, 2018

Uh oh!

mat007 commented Apr 30, 2018

Uh oh!

vieux commented Apr 30, 2018

Uh oh!

mat007 commented May 2, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester commented May 3, 2018

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!

thaJeztahMay 8, 2018• 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!

thaJeztah commented May 8, 2018

Uh oh!

mat007 commented May 9, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mat007 commented May 9, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mat007 commented May 9, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

mat007 commented May 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

thaJeztahMay 8, 2018•
edited
Loading

mat007 commented May 9, 2018•
edited
Loading

mat007 commented May 9, 2018•
edited
Loading