Handling Volume at early stage #626

surajnarwade · 2017-05-30T13:02:32Z

It will resolve #544 as well as refactor volume handling part.

surajnarwade · 2017-05-30T13:09:15Z

this PR is still in WIP, so I will add tests later.
@cdrage @kadel @surajssd suggestions needed

cdrage · 2017-06-13T13:53:01Z

@surajnarwade DaemonSet's aren't working. See: https://travis-ci.org/kubernetes-incubator/kompose/jobs/237507721#L398

Please get current tests to pass as well 😄 before we add new tests!

surajssd · 2017-06-15T09:27:56Z

@surajnarwade please rebase

surajnarwade · 2017-06-15T10:28:23Z

@surajssd @cdrage done with rebase and tests are green now, needs your review please :)

cdrage

Couple questions:

Do we have tests that already cover volumes (no need to add more unit tests?)

Is it possible copy some of the functionality of: https://github.com/docker/cli/blob/master/cli/compose/types/types.go#L243 in order to better reflect the libraries we're using for parsing? (May be diffifcult since you know.. we use libcompose for parsing v1v2 but docker/cli for v3)

cdrage · 2017-06-15T13:29:22Z

pkg/kobject/kobject.go

@@ -92,6 +92,7 @@ type ServiceConfig struct {
 	MemLimit        yaml.MemStringorInt `compose:"mem_limit" bundle:""`
 	TmpFs           []string            `compose:"tmpfs" bundle:""`
 	Dockerfile      string              `compose:"dockerfile" bundle:""`
+	Vols            []Volumes           `compose:"" bundle:""`


Why Vols not Volumes?

Ideally we try and replicate / go by how they do it in libcompose :) See: https://github.com/docker/libcompose/blob/56b0613aac7d6d524d29dacb6b4221b5e4618d45/config/types.go#L70

Volumes is already there https://github.com/kubernetes-incubator/kompose/blob/master/pkg/kobject/kobject.go#L70 for libcompose mapping, Vols is new struct for handling volumes.

I think it is not a good idea to have both Volumes and Vols, please remove the []string and rename Vols to Volumes.

What I mean here is that []Volumes was added to get rid of []string ?

either I can swap names but we can't remove []string

cdrage · 2017-06-16T14:21:30Z

pkg/kobject/kobject.go

+	Container  string
+	Mode       string
+	PVCName    string
+}


I don't like how many variables there are. It may be better copying how they do it within Docker/CLI so it's easier for us to be compatible with them. See: https://github.com/docker/cli/blob/master/cli/compose/types/types.go#L243 or perhaps even import and use the type.

@cdrage I think this is needed for doing the whole volumesFrom thing reliably, since these will be resolved once to save computation later.

This is a bit confusing for me also. Maybe it would be nice to have some comments describing what some of the variables are for.

cdrage · 2017-06-16T14:22:07Z

pkg/transformer/kubernetes/kubernetes.go

-		}
-
-		log.Debug("Volume name %s", volumeName)
+	//POC work ############################


remove this comment

cdrage · 2017-06-16T14:23:27Z

pkg/loader/compose/v1v2.go

+			}
+		}
+	}
+


Is it possible to refactor this into it's own function (parseV1V2Volumes maybe?)

cdrage · 2017-06-16T14:24:24Z

pkg/transformer/kubernetes/kubernetes.go

-
-		log.Debug("Volume name %s", volumeName)
+	//POC work ############################
+	for _, vol1 := range service.Vols {


please change vol1 variable name to something better, why not volume like before?

surajnarwade · 2017-06-20T04:21:47Z

@cdrage , in order to answer your question regarding copying functionality from docker/cli, it can be used moreover for root level volumes key but not the thing that I am working on.

cdrage · 2017-06-20T13:50:41Z

@surajnarwade I believe that you can use similar type.

See how we "copy" from libcompose in kobject.go in terms of ServiceConfig. This should be the same in terms of Volumes. It makes sense to try to keep the underlying code similar to upstream code.

What do you think @kadel ? Also regarding the many variables (see #626 (diff))

surajssd · 2017-06-21T05:20:49Z

pkg/kobject/kobject.go

+	VFrom      string
+	VolumeName string
+	Host       string
+	Container  string


Is this container mount Path ?

I mean I am confused on MountPath vs Container ?

mountpath can be host:container, hence container is that directory, for example, /abc

surajssd · 2017-06-21T05:25:15Z

pkg/loader/compose/v1v2.go

+
+func handleVolume(svcName string, komposeObject kobject.KomposeObject) (volume []kobject.Volumes) {
+
+	if komposeObject.ServiceConfigs[svcName].VolumesFrom != nil {


please put in lots of comments since it is hard to understand what is happening here

After certain time even the programmer forgets why s/he wrote a piece of code! :-D

comments not just here but in other parts of code

surajssd · 2017-06-21T05:28:19Z

pkg/loader/compose/v1v2.go

+
+	if komposeObject.ServiceConfigs[svcName].VolumesFrom != nil {
+		for _, depVol := range komposeObject.ServiceConfigs[svcName].VolumesFrom {
+


nit: s//n/

surajssd · 2017-06-21T05:28:48Z

pkg/loader/compose/v1v2.go

+	if komposeObject.ServiceConfigs[svcName].VolumesFrom != nil {
+		for _, depVol := range komposeObject.ServiceConfigs[svcName].VolumesFrom {
+
+			dVols := handleVolume(depVol, komposeObject)


also why this recursive call is made, needs an explanation

surajssd · 2017-06-21T05:33:53Z

pkg/loader/compose/v1v2.go

+				cVols = append(cVols, vol)
+			}
+			for _, cv := range cVols {
+				i, dv := getVol(cv, dVols)


nit for general practices reason, s/i/ok ?

surajssd · 2017-06-21T05:34:56Z

pkg/loader/compose/v1v2.go

+			}
+			for _, cv := range cVols {
+				i, dv := getVol(cv, dVols)
+				if i == true {


also in a conditional where you are using boolean values no need to check it with booleans! essentially I mean s/i == true/i

surajssd · 2017-06-21T05:35:51Z

pkg/loader/compose/v1v2.go

+					dv.VFrom = dv.SvcName
+					volume = append(volume, dv)
+				}
+


nit: s//n/

surajssd · 2017-06-21T05:36:05Z

pkg/loader/compose/v1v2.go

+				}
+
+			}
+


nit: s//n/

surajssd · 2017-06-21T05:36:22Z

pkg/loader/compose/v1v2.go

+				MountPath: v,
+			}
+			volume = append(volume, vol)
+


nit: s//n/

surajssd · 2017-06-21T05:39:24Z

pkg/loader/compose/v1v2.go

+	return false
+}
+
+func getVol(tofind kobject.Volumes, dVols []kobject.Volumes) (bool, kobject.Volumes) {


This is a generic function that searches array of volumes without any bias on what the array of volumes is so maybe name it better, like s/dVols/vols/

surajssd · 2017-06-21T05:41:55Z

pkg/loader/compose/v1v2.go

+}
+
+func checkvolpresent(tofind kobject.Volumes, cVols []kobject.Volumes) bool {
+	for _, cv := range cVols {


I don't see a reason to have this function? Because getVol does similar thing but with inverted functionality can we use just one?

but getVol returns with extra Volumes struct

surajnarwade · 2017-07-17T18:30:11Z

@cdrage , added new example

cdrage

For the most part, this is all just renaming as well as moving things around, rather than refactoring.

Other than my one nitpick, LGTM.

cdrage · 2017-07-17T19:09:07Z

script/test/fixtures/volume-mounts/volumes-from/docker-compose.yml

@@ -17,4 +17,4 @@ services:
    volumes:
      - /www/public
    volumes_from:
-      - web
+      - web


added space I think?

I don't think so

you've got to do git checkout script/test/fixtures/volume-mounts/volumes-from/docker-compose.yml to revert it back to normal. you've modified something here (newline maybe?)

@cdrage already resolved that change, so it's outdated now :)

cdrage · 2017-07-17T19:10:22Z

Let's get @kadel 's quick approval then merge.

cdrage · 2017-07-18T14:45:34Z

LGTM!

kadel · 2017-07-19T13:03:09Z

pkg/loader/compose/v1v2.go

+}
+
+// for current volumes
+func checkVolPresent(toFind kobject.Volumes, Vols []kobject.Volumes) bool {


Shouldn't this be named checkVolNotPresent?

kadel · 2017-07-19T13:09:52Z

It will resolve #544 as well as refactor volume handling part.

this is not resolving #544 is it?
Isn't this meant to be just ground work for resolving #544 in the future?

surajnarwade · 2017-07-20T05:26:36Z

@kadel , it will completely resolve #544

kadel · 2017-07-20T08:58:22Z

@kadel , it will completely resolve #544

I've tested that and result is still the same :-(

surajnarwade · 2017-07-20T13:59:20Z

@kadel , problem is occuring due to permissions of volumes in the example mentioned in the issue.

kadel · 2017-07-20T14:21:54Z

@kadel , problem is occuring due to permissions of volumes in the example mentioned in the issue.

yeh, but it should work even with permissions.
If volumes are the same, only permissions differ that kompose has to set readOnly for ro volumes. Right now it puts duplicate volume there.

kadel · 2017-07-20T14:37:08Z

pkg/loader/compose/v1v2.go

+			return nil, errors.Wrapf(err, "could not parse volume %q: %v", vn, err)
+		}
+		v.SvcName = svcName
+		v.MountPath = vn


vn contains full path with permissions. like ./certs:/etc/nginx/certs:ro.
Shouldn't permission be stripped from it before saving it to MountPath?

It might solve problem with duplicate volumes with different permissions #626 (comment)

it can be quickly fixed like this: v.MountPath = fmt.Sprintf("%s:%s", v.Host, v.Container)

This made me realize that we don't even need v.MountPath as it can be always assembled from v.Host + v.Container and if needed v.Mode

surajnarwade · 2017-07-24T12:08:48Z

@kadel , now it works

kadel · 2017-07-24T12:51:55Z

@surajnarwade getting closer ;-)

But permissions are still not correct:-(

letsencrypt Deployment has /etc/nginx/certs mounted as readOnly, even if docker-compose says rw

          - mountPath: /etc/nginx/certs
            name: proxy-claim2
            readOnly: true

cdrage · 2017-07-24T16:55:38Z

@kadel Odd that this is not caught in a failing test, did @surajnarwade override it or is there actually no tests written for checking if the readOnly value has been enabled?

kadel · 2017-07-25T07:49:42Z

@kadel Odd that this is not caught in a failing test, did @surajnarwade override it or is there actually no tests written for checking if the readOnly value has been enabled?

It looks like there is no test case checking that :-(

There should be a test testing this case: One volume mounted to two different containers, in one container as ro, in other as rw

kadel · 2017-07-25T08:40:06Z

pkg/loader/compose/v1v2.go

+// for dependent volumes, returns true and the respective volume if mountpath are same
+func getVol(toFind kobject.Volumes, Vols []kobject.Volumes) (bool, kobject.Volumes) {
+	for _, dv := range Vols {
+		if toFind.MountPath == dv.MountPath && toFind.Mode == dv.Mode {


toFind.Mode == dv.Mode shouldn't be here. It bring back issue with duplicate volumeMounts

kadel · 2017-07-25T15:22:11Z

I've created simple test file:

version: "2"

services:
  foo:
    image: quay.io/tomkral/sleeper
    volumes:
     - ./data:/opt/data:ro
    
  bar:
    image: quay.io/tomkral/sleeper
    volumes_from: 
     - foo
    volumes:
     - ./data:/opt/data:rw

using this I've found weird bug
If I use this docker-compose file I Deploymnet for bar service doesn't have any volumeMounts.

Once I add second volume for bar service:

    volumes:
     - ./data:/opt/data:rw
     - /tmp

generated Deployment suddenly has both volumeMounts

kadel · 2017-07-25T15:27:08Z

I think that for this docker-compose

version: "2"

services:
  foo:
    image: quay.io/tomkral/sleeper
    volumes:
     - ./data:/opt/data:ro
    
  bar:
    image: quay.io/tomkral/sleeper
    volumes_from: 
     - foo
    volumes:
     - ./data:/opt/data:rw

correct output should be

apiVersion: v1
items:
- apiVersion: v1
  kind: Service
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: bar
    name: bar
  spec:
    clusterIP: None
    ports:
    - name: headless
      port: 55555
      targetPort: 0
    selector:
      io.kompose.service: bar
  status:
    loadBalancer: {}
- apiVersion: v1
  kind: Service
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: foo
    name: foo
  spec:
    clusterIP: None
    ports:
    - name: headless
      port: 55555
      targetPort: 0
    selector:
      io.kompose.service: foo
  status:
    loadBalancer: {}
- apiVersion: extensions/v1beta1
  kind: Deployment
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: bar
    name: bar
  spec:
    replicas: 1
    strategy:
      type: Recreate
    template:
      metadata:
        creationTimestamp: null
        labels:
          io.kompose.service: bar
      spec:
        containers:
        - image: quay.io/tomkral/sleeper
          name: bar
          resources: {}
          volumeMounts:
          - mountPath: /opt/data
            name: foo-claim0
        restartPolicy: Always
        volumes:
        - name: foo-claim0
          persistentVolumeClaim:
            claimName: foo-claim0
  status: {}
- apiVersion: extensions/v1beta1
  kind: Deployment
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: foo
    name: foo
  spec:
    replicas: 1
    strategy:
      type: Recreate
    template:
      metadata:
        creationTimestamp: null
        labels:
          io.kompose.service: foo
      spec:
        containers:
        - image: quay.io/tomkral/sleeper
          name: foo
          resources: {}
          volumeMounts:
          - mountPath: /opt/data
            name: foo-claim0
            readOnly: true
        restartPolicy: Always
        volumes:
        - name: foo-claim0
          persistentVolumeClaim:
            claimName: foo-claim0
            readOnly: true
  status: {}
- apiVersion: v1
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: foo-claim0
    name: foo-claim0
  spec:
    accessModes:
    - ReadWriteOnce
    resources:
      requests:
        storage: 100Mi
  status: {}
kind: List
metadata: {}

To sum this up with the current state of this PR I see two issues.

readOnly flag is set for volumeMount that has explicitly specified rw in docker-compose
missing volume if there is only one volume with volume_from defined

It will resolve kubernetes#544 as well as refactor volume handling part.

kadel · 2017-07-27T14:19:37Z

It looks like all the problems were solved in the last update of this PR. 🎉

Grat work @surajnarwade ! 👍

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 30, 2017

surajnarwade changed the title ~~Handling Volume at early stage~~ [WIP]Handling Volume at early stage May 30, 2017

surajssd mentioned this pull request Jun 5, 2017

Add v3 support of Docker Compose #600

Merged

surajnarwade force-pushed the handle_volume branch 2 times, most recently from a7a045b to ede9e62 Compare June 15, 2017 09:10

surajssd added the rebase needed label Jun 15, 2017

surajnarwade force-pushed the handle_volume branch 2 times, most recently from 2695d87 to 56a1d60 Compare June 15, 2017 09:47

cdrage requested changes Jun 16, 2017

View reviewed changes

surajnarwade force-pushed the handle_volume branch from 56a1d60 to c9bff6f Compare June 20, 2017 04:30

surajnarwade changed the title ~~[WIP]Handling Volume at early stage~~ Handling Volume at early stage Jun 20, 2017

surajssd added review needed and removed rebase needed labels Jun 20, 2017

surajssd reviewed Jun 21, 2017

View reviewed changes

pkg/loader/compose/v1v2.go Outdated

}

}

Copy link

Member

surajssd Jun 21, 2017 •

edited

Choose a reason for hiding this comment

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

nit: s//n/

surajnarwade reacted with thumbs up emoji

surajssd reviewed Jun 21, 2017

View reviewed changes

surajnarwade force-pushed the handle_volume branch from aaf9276 to 5b5dcdb Compare July 17, 2017 18:16

cdrage approved these changes Jul 17, 2017

View reviewed changes

surajnarwade force-pushed the handle_volume branch from 5b5dcdb to fd88428 Compare July 18, 2017 04:25

surajnarwade mentioned this pull request Jul 18, 2017

Ignore Volume if similar mountpath found #560

Closed

kadel reviewed Jul 19, 2017

View reviewed changes

surajnarwade force-pushed the handle_volume branch from fd88428 to e0c1622 Compare July 20, 2017 05:33

kadel reviewed Jul 20, 2017

View reviewed changes

surajnarwade force-pushed the handle_volume branch from e0c1622 to 2097ff7 Compare July 24, 2017 06:24

surajnarwade force-pushed the handle_volume branch from 2097ff7 to b8768de Compare July 25, 2017 06:09

kadel reviewed Jul 25, 2017

View reviewed changes

surajnarwade force-pushed the handle_volume branch from b8768de to 07ea646 Compare July 25, 2017 10:43

Handling Volume at early stage

d5a5f42

It will resolve kubernetes#544 as well as refactor volume handling part.

surajnarwade force-pushed the handle_volume branch from 07ea646 to d5a5f42 Compare July 26, 2017 14:29

kadel approved these changes Jul 27, 2017

View reviewed changes

kadel merged commit 3b2ef30 into kubernetes:master Jul 27, 2017


		func handleVolume(svcName string, komposeObject kobject.KomposeObject) (volume []kobject.Volumes) {

		if komposeObject.ServiceConfigs[svcName].VolumesFrom != nil {


		if komposeObject.ServiceConfigs[svcName].VolumesFrom != nil {
		for _, depVol := range komposeObject.ServiceConfigs[svcName].VolumesFrom {

Handling Volume at early stage #626

Handling Volume at early stage #626

Conversation

surajnarwade commented May 30, 2017

surajnarwade commented May 30, 2017

cdrage commented Jun 13, 2017

surajssd commented Jun 15, 2017

surajnarwade commented Jun 15, 2017

cdrage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd Jun 21, 2017 • edited

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

surajnarwade commented Jun 20, 2017

cdrage commented Jun 20, 2017

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

surajssd Jun 21, 2017 • edited

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

surajssd Jun 21, 2017 • edited

Choose a reason for hiding this comment

surajssd Jun 21, 2017 • edited

Choose a reason for hiding this comment

surajssd Jun 21, 2017 • edited

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

surajnarwade commented Jul 17, 2017

cdrage 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

cdrage commented Jul 17, 2017

cdrage commented Jul 18, 2017

Choose a reason for hiding this comment

kadel commented Jul 19, 2017

surajnarwade commented Jul 20, 2017

kadel commented Jul 20, 2017

surajnarwade commented Jul 20, 2017

kadel commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented Jul 24, 2017

kadel commented Jul 24, 2017

cdrage commented Jul 24, 2017

kadel commented Jul 25, 2017

Choose a reason for hiding this comment

kadel commented Jul 25, 2017

kadel commented Jul 25, 2017 • edited

kadel commented Jul 27, 2017

surajssd Jun 21, 2017 •

edited

surajssd Jun 21, 2017 •

edited

surajssd Jun 21, 2017 •

edited

surajssd Jun 21, 2017 •

edited

surajssd Jun 21, 2017 •

edited

kadel commented Jul 25, 2017 •

edited