Better namespace experience with Kubernetes #991

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

cli/command/stack/kubernetes/cli.go

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

cli/command/stack/kubernetes/cli.go

+		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

func NewOptions(flags *flag.FlagSet) Options {

var opts Options

if namespace, err := flags.GetString("namespace"); err == nil {

opts.Namespace = namespace

}

if kubeConfig, err := flags.GetString("kubeconfig"); err == nil {

opts.Config = kubeConfig

}

return opts

}

)

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

kubernetes/config.go

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

kubernetes/config.go

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

func ConfigPath(kubeconfigPath string) string { if kubeconfigPath != "" { return kubeconfigPath } if config := os.Getenv("KUBECONFIG"); config != "" { return config } return filepath.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

cli/command/formatter/stack.go

 // 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;

if len(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

cli/command/stack/kubernetes/list.go

 		return err
 	}
 	format := opts.Format
 	if len(format) == 0 {


See my other comment above

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

cli/command/stack/swarm/list.go

 		return err
 	}
 	format := opts.Format
 	if len(format) == 0 {


See my other comment above

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

cli/command/stack/list.go

 	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

cli/command/stack/list.go


 	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 <[email protected]>

…elongs Signed-off-by: Silvin Lubecki <[email protected]>

…rnetes orchestrator Signed-off-by: Silvin Lubecki <[email protected]>

…rator, to list all stacks in all namespaces. Signed-off-by: Silvin Lubecki <[email protected]>

…alues Signed-off-by: Mathieu Champlon <[email protected]>

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 Kubernetes Apr 9, 2018

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

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

silvin-lubecki changed the title ~~[WIP] Better namespace experience with Kubernetes~~ Better namespace experience with Kubernetes Apr 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 27f8396 Compare 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 <[email protected]>

Clean useless named imports

c23c148

Signed-off-by: Silvin Lubecki <[email protected]>

Move stack client creation from KubeCli to client factory, where it b…

31dccff

…elongs Signed-off-by: Silvin Lubecki <[email protected]>

Add namespace column for docker stack ls command while targeting Kube…

65526a2

…rnetes orchestrator Signed-off-by: Silvin Lubecki <[email protected]>

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 <[email protected]>

Fix stack ls behaviour with Kubernetes and variout --namespace flag v…

2af66be

…alues Signed-off-by: Mathieu Champlon <[email protected]>

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

wsong approved these changes May 9, 2018

View reviewed changes

thaJeztah merged commit 552ee50 into docker:master May 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
	func NewOptions(flags *flag.FlagSet) Options {
	var opts Options
	if namespace, err := flags.GetString("namespace"); err == nil {
	opts.Namespace = namespace
	}
	if kubeConfig, err := flags.GetString("kubeconfig"); err == nil {
	opts.Config = kubeConfig
	}
	return opts
	}

Better namespace experience with Kubernetes #991

Better namespace experience with Kubernetes #991

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!

thaJeztah May 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

thaJeztah May 8, 2018 •

edited

Loading

mat007 commented May 9, 2018 •

edited

Loading

mat007 commented May 9, 2018 •

edited

Loading