Handle multiple stack api versions #899

silvin-lubecki · 2018-02-22T14:58:23Z

- What I did
I integrated the new version of the Stack API (v1beta2). The CLI can now communicates with both versions.

⚠️ This PR has a dependency on #898 ⚠️

- How I did it
I added a common interface between v1beta1 and v1beta2 stack clients and one common object stack, so the code in the commands remains the same.

- How to verify it
Execute a docker version command against latest docker for mac, it should output at the end:

 Orchestrator:
   Kubernetes:  v1.9.2
   Stack:       v1beta2

- Description for the changelog

Add support for the new Stack API for Kubernetes v1beta2

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

codecov-io · 2018-02-22T15:04:56Z

Codecov Report

❗ No coverage uploaded for pull request base (master@99bd7ed). Click here to learn what that means.
The diff coverage is 23.17%.

@@            Coverage Diff            @@
##             master     #899   +/-   ##
=========================================
  Coverage          ?   53.51%           
=========================================
  Files             ?      266           
  Lines             ?    16891           
  Branches          ?        0           
=========================================
  Hits              ?     9040           
  Misses            ?     7248           
  Partials          ?      603

vdemeester

I think we can clean more code. There is quite a few code in the kubernetes api package that are dead code (in terms of client's code) — like all the Clone functions, Discovery, etc..
We should re-enable linters of kubernetes package(s)
I wonder what is bringing golang-lru and the other new vendoring code 🤔

vdemeester · 2018-02-23T13:34:19Z

cli/command/stack/kubernetes/stack.go

+
+		fileName := path.Base(config.File)
+		content, err := ioutil.ReadFile(config.File)
+		if err != nil {


Do we want to group errors ?

If a file can't read (but not the first one), a configMap will be created in k8s and not used (orphan) — and will probably cause errors next time we do the docker stack deploy, right ?

I think we should "read all", and if there is no error, "create all". Then, if one of the creation fails, what should we do with the previous one created ?

Right, I'll fix that.

I propose to fix this in a follow-up, as we may remove all configmaps and secrets if any error occurs, even while deploying the stack.

vdemeester · 2018-02-23T13:35:14Z

cli/command/stack/kubernetes/stack.go

+
+		fileName := path.Base(secret.File)
+		content, err := ioutil.ReadFile(secret.File)
+		if err != nil {


Same question here

I'll fix that too.

vdemeester · 2018-02-23T13:39:43Z

cli/command/stack/kubernetes/stackclient.go

+	stacks composev1beta2.StackInterface
+}
+
+func (c *KubeCli) newStackV1Beta2() (stackClient, error) {


I would rather have func newStackV1Beta2(config *rest.Config, namespace string) … here (not tying it up with KubeCli)

vdemeester · 2018-02-23T13:41:17Z

cli/command/stack/kubernetes/stackclient.go

+	stacks composev1beta1.StackInterface
+}
+
+func (c *KubeCli) newStackV1Beta1() (stackClient, error) {


I would rather have func newStackV1Beta1(config *rest.Config, namespace string) … here (not tying it up with KubeCli)

vdemeester · 2018-02-23T13:42:28Z

kubernetes/README.md

@@ -1,4 +0,0 @@
-# Kubernetes client libraries


Can we keep the README ? 👼

vdemeester · 2018-02-23T13:43:09Z

kubernetes/client/clientset/clientset.go

 	glog "github.com/golang/glog"
 	discovery "k8s.io/client-go/discovery"
 	rest "k8s.io/client-go/rest"
 	flowcontrol "k8s.io/client-go/util/flowcontrol"
 )

+// Interface defines the methods a compose kube client should have
+// FIXME(vdemeester) is it required ?


Ah 😅 I need to find the answer for it 😛

Yes please 😄

vdemeester · 2018-02-23T13:44:14Z

kubernetes/client/clientset/clientset.go

-// Deprecated: Compose retrieves the default version of ComposeClient.
-// Please explicitly pick a version.
+// Compose retrieves the default version of ComposeClient.
+// deprecated: please explicitly pick a version.
 func (c *Clientset) Compose() composev1beta1.ComposeV1beta1Interface {


Hum even if it's deprecated, should we return ComposeV1beta2Client here instead 👼

Or I may remove it...

thaJeztah · 2018-03-12T23:40:21Z

looks like this needs a rebase

silvin-lubecki · 2018-03-13T10:54:40Z

Done @thaJeztah

thaJeztah · 2018-03-23T12:08:50Z

@silvin-lubecki @vdemeester what's the status on this one? I think there's some outstanding review comments, but perhaps I looked wrong 😅

silvin-lubecki · 2018-03-27T14:10:15Z

I think I addressed all @vdemeester comments, but I'd be pleased to hear your comments @thaJeztah 😄
Also #903 depends on this PR.

vdemeester

LGTM 🐸

vdemeester · 2018-03-28T09:43:39Z

cli/command/stack/kubernetes/loader.go

 )

-// LoadStack loads a stack from a Compose config, with a given name.
-func LoadStack(name string, cfg composetypes.Config) (*apiv1beta1.Stack, error) {
+func loadStack(name string, cfg composetypes.Config) (stack, error) {
 	res, err := yaml.Marshal(cfg)


we only need to do that in v1beta1 case right ? We shouldn't even "try" to do it all the time.

silvin-lubecki · 2018-03-28T11:35:53Z

@vdemeester PTAL

vdemeester · 2018-03-29T08:22:32Z

cli/command/stack/kubernetes/stackclient.go

@@ -100,6 +103,18 @@ func verify(services corev1.ServiceInterface, stackName string, service string)
 	return nil
 }

+func (s *stackV1Beta1) LoadStack(name string, cfg composetypes.Config) (stack, error) {


nit: not sure if LoadStack is the correct name for those too 👼

vdemeester

LGTM 🦁

silvin-lubecki · 2018-04-16T13:03:08Z

@thaJeztah PTAL

thaJeztah

left some comments and questions

thaJeztah · 2018-04-24T16:12:18Z

cli/command/stack/kubernetes/convert.go

+}
+
+func fromComposeServiceConfig(s composeTypes.ServiceConfig) v1beta2.ServiceConfig {
+	var userID *int64


I think we use an int elsewhere (uids are 32-bit max IIRC (https://serverfault.com/a/585435))

This userID is used in the compose controller as a Kubernetes SecurityContext.RunAsUser (https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/core/v1/types.go#L4889) which is an *int64. And by the way the user is declared as a string in the Compose ServiceConfig type.

thaJeztah · 2018-04-24T16:25:19Z

cli/command/stack/kubernetes/convert.go

+		numerical, err := strconv.Atoi(s.User)
+		if err == nil {
+			unixUserID := int64(numerical)
+			userID = &unixUserID


This is a nice (clean) approach as well;

cli/cli/compose/convert/service.go

Lines 384 to 386 in 236a847

func uint32Ptr(value uint32) *uint32 {

return &value

}

Right, fixed!

thaJeztah · 2018-04-24T16:30:51Z

cli/command/stack/kubernetes/convert.go

+	return c
+}
+
+func fromComposeHealthcheck(h *composeTypes.HealthCheckConfig) *v1beta2.HealthCheckConfig {


Was comparing these to their "swarm mode" counterparts, and wondering if we need validation/error-handling for these conversions (could be a follow-up), e.g. comparing to

cli/cli/compose/convert/service.go

Lines 401 to 437 in 236a847

func convertHealthcheck(healthcheck *composetypes.HealthCheckConfig) (*container.HealthConfig, error) {

if healthcheck == nil {

return nil, nil

}

var (

timeout, interval, startPeriod time.Duration

retries int

)

if healthcheck.Disable {

if len(healthcheck.Test) != 0 {

return nil, errors.Errorf("test and disable can't be set at the same time")

}

return &container.HealthConfig{

Test: []string{"NONE"},

}, nil

}

if healthcheck.Timeout != nil {

timeout = *healthcheck.Timeout

}

if healthcheck.Interval != nil {

interval = *healthcheck.Interval

}

if healthcheck.StartPeriod != nil {

startPeriod = *healthcheck.StartPeriod

}

if healthcheck.Retries != nil {

retries = int(*healthcheck.Retries)

}

return &container.HealthConfig{

Test: healthcheck.Test,

Timeout: timeout,

Interval: interval,

Retries: retries,

StartPeriod: startPeriod,

}, nil

}

Right, I add it, as it is an error and not a warning (which are in the next PR).

After a second thought, the Disable flag is just not used on Kubernetes compose controller, see https://github.com/docker/cli/pull/899/files#diff-9f8dfaca00498f5f3543480c12fec02aR198 , so I don't know if we really need to error on this...

thaJeztah · 2018-04-24T16:54:54Z

cli/command/stack/kubernetes/stack.go

+func (s *stack) createFileBasedSecrets(secrets corev1.SecretInterface) error {
+	for name, secret := range s.spec.Secrets {
+		if secret.File == "" {
+			continue


Do we need to print a warning in this case?

Perhaps also if other options (permissions/ownership) is set?

(oh, just realise thats probably on the service, not on creation of the secret)

thaJeztah · 2018-04-24T16:56:36Z

cli/command/stack/kubernetes/stack.go

+			return err
+		}
+
+		if _, err := secrets.Create(toSecret(s.name, name, fileName, content)); err != nil {


Same as for "config": what happens if a secret already exists?

thaJeztah · 2018-04-26T22:36:05Z

kubernetes/client/clientset/typed/compose/v1beta1/stack.go

 		Namespace(c.ns).
 		Resource("stacks").
 		Name(stack.Name).
 		Body(stack).
 		Do().
 		Into(result)
-	return
+	return result, err
 }

 // UpdateStatus was generated because the type contains a Status member.
 // Add a +genclientstatus=false comment above the type to avoid generating UpdateStatus().


Looks like UpdateStatus() isn't used? should we remove this? (same for the other API version)

Same answer 😄

thaJeztah · 2018-04-26T22:41:01Z

kubernetes/compose/v1beta1/doc.go

@@ -1,11 +1,10 @@
-// Package v1beta1 holds the v1beta1 versions of our stack structures.


Hm, why was the Package description move further down? The first line is what will be documented as package description

Don't really know, I'll revert it.

thaJeztah · 2018-04-26T22:42:49Z

kubernetes/compose/v1beta1/stack_test.go

@@ -0,0 +1 @@
+package v1beta1


This file is here to get representable coverage?

Yep exactly.

thaJeztah · 2018-04-26T22:43:52Z

kubernetes/compose/v1beta2/doc.go

+// backward compatibility by support multiple concurrent versions
+// of the same resource
+
+// Package v1beta2 is the second version of the stack, containing a structured spec


Same question here for positioning of the Package description (should likely be the first line in this file

thaJeztah · 2018-04-26T22:45:30Z

vendor.conf

@@ -83,3 +83,4 @@ k8s.io/client-go kubernetes-1.8.2
 k8s.io/kubernetes v1.8.2
 k8s.io/kube-openapi 61b46af70dfed79c6d24530cd23b41440a7f22a5
 vbom.ml/util 928aaa586d7718c70f4090ddf83f2b34c16fdc8d
+github.com/hashicorp/golang-lru 0a025b7e63adc15a622f29b0b2c4c3848243bbf6


nit: please add a newline

silvin-lubecki · 2018-04-27T16:45:31Z

PTAL @thaJeztah
I also rebased onto master.

vieux · 2018-04-30T09:04:32Z

LGTM ping @thaJeztah

thaJeztah

Looks like there's a linting failure;

Successfully built 4fd3db512de8
Successfully tagged cli-linter:11601
kubernetes/compose/v1beta1/doc.go:7:1:warning: package comment should be of the form "Package v1beta1 ..." (golint)
Exited with code 1

but looks good otherwise

mat007 · 2018-04-30T09:23:08Z

Linter should be happy now, thanks @thaJeztah !

…s and use it in kubernetes stack commands * Add kubernetes Stack API v1beta2 client * Upgrade v1beta1 client to remove generated code Signed-off-by: Silvin Lubecki <[email protected]>

thaJeztah

LGTM!

silvin-lubecki requested review from dnephin and vdemeester as code owners February 22, 2018 14:58

GordonTheTurtle added the status/0-triage label Feb 22, 2018

silvin-lubecki mentioned this pull request Feb 22, 2018

WIP Handle multiple compose versions #854

Closed

vdemeester added status/1-design-review and removed status/0-triage labels Feb 22, 2018

silvin-lubecki mentioned this pull request Feb 22, 2018

Print Stack API and Kubernetes versions in version command #898

Merged

vdemeester requested changes Feb 23, 2018

View reviewed changes

silvin-lubecki force-pushed the handle-multiple-stack-api-versions branch 2 times, most recently from 38df84c to 96b7779 Compare February 26, 2018 00:41

silvin-lubecki mentioned this pull request Feb 26, 2018

Add warnings client side before deployment on Kubernetes #903

Merged

vdemeester added the area/kubernetes label Feb 27, 2018

silvin-lubecki force-pushed the handle-multiple-stack-api-versions branch from 96b7779 to b26d649 Compare February 28, 2018 22:21

silvin-lubecki force-pushed the handle-multiple-stack-api-versions branch from b26d649 to eb91268 Compare March 13, 2018 10:47

silvin-lubecki force-pushed the handle-multiple-stack-api-versions branch from eb91268 to e89100b Compare March 27, 2018 13:59

vdemeester approved these changes Mar 28, 2018

View reviewed changes

vdemeester added status/2-code-review and removed status/1-design-review labels Mar 28, 2018

vdemeester requested changes Mar 28, 2018

View reviewed changes

vdemeester reviewed Mar 29, 2018

View reviewed changes

silvin-lubecki force-pushed the handle-multiple-stack-api-versions branch from 48bf2ad to 92ba690 Compare March 30, 2018 09:04

vdemeester approved these changes Mar 30, 2018

View reviewed changes

thaJeztah requested changes Apr 26, 2018

View reviewed changes

silvin-lubecki force-pushed the handle-multiple-stack-api-versions branch from 92ba690 to ece1de4 Compare April 27, 2018 16:45

thaJeztah requested changes Apr 30, 2018

View reviewed changes

mat007 force-pushed the handle-multiple-stack-api-versions branch from ece1de4 to dc3d8d7 Compare April 30, 2018 09:22

Add a common interface between different Kubernetes Stack API version…

f958c66

…s and use it in kubernetes stack commands * Add kubernetes Stack API v1beta2 client * Upgrade v1beta1 client to remove generated code Signed-off-by: Silvin Lubecki <[email protected]>

mat007 force-pushed the handle-multiple-stack-api-versions branch from dc3d8d7 to f958c66 Compare April 30, 2018 09:27

thaJeztah approved these changes Apr 30, 2018

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Apr 30, 2018

thaJeztah merged commit 7ad3036 into docker:master Apr 30, 2018

GordonTheTurtle added this to the 18.05.0 milestone Apr 30, 2018

thaJeztah modified the milestones: 18.05.0, 18.06.0 Apr 30, 2018

silvin-lubecki deleted the handle-multiple-stack-api-versions branch May 16, 2018 07:54

silvin-lubecki mentioned this pull request Jun 1, 2018

[k8s] support multiple version of the kube compose controller api #799

Closed

vdemeester mentioned this pull request Jun 19, 2018

Unsupported Compose File Version 3.6 error reported for Kubernetes while worked for Swarm docker/for-mac#3014

Closed

2 tasks

	func convertHealthcheck(healthcheck composetypes.HealthCheckConfig) (container.HealthConfig, error) {
	if healthcheck == nil {
	return nil, nil
	}
	var (
	timeout, interval, startPeriod time.Duration
	retries int
	)
	if healthcheck.Disable {
	if len(healthcheck.Test) != 0 {
	return nil, errors.Errorf("test and disable can't be set at the same time")
	}
	return &container.HealthConfig{
	Test: []string{"NONE"},
	}, nil

	}
	if healthcheck.Timeout != nil {
	timeout = *healthcheck.Timeout
	}
	if healthcheck.Interval != nil {
	interval = *healthcheck.Interval
	}
	if healthcheck.StartPeriod != nil {
	startPeriod = *healthcheck.StartPeriod
	}
	if healthcheck.Retries != nil {
	retries = int(*healthcheck.Retries)
	}
	return &container.HealthConfig{
	Test: healthcheck.Test,
	Timeout: timeout,
	Interval: interval,
	Retries: retries,
	StartPeriod: startPeriod,
	}, nil
	}

		@@ -1,11 +1,10 @@
		// Package v1beta1 holds the v1beta1 versions of our stack structures.

Handle multiple stack api versions #899

Handle multiple stack api versions #899

Conversation

silvin-lubecki commented Feb 22, 2018

codecov-io commented Feb 22, 2018 • edited Loading

Codecov Report

vdemeester left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Mar 12, 2018

silvin-lubecki commented Mar 13, 2018

thaJeztah commented Mar 23, 2018

silvin-lubecki commented Mar 27, 2018

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvin-lubecki commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

silvin-lubecki commented Apr 16, 2018

thaJeztah left a 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

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

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

silvin-lubecki commented Apr 27, 2018

vieux commented Apr 30, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

mat007 commented Apr 30, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 22, 2018 •

edited

Loading