Add volumes implementation support #88

surajssd · 2017-04-03T14:13:35Z

This PR adds support for volumes in OpenCompose.

With this there is new root level directive called volumes and new directive in container level called
mounts and also adds service level directive called emptyDirVolumes.

More info about volumes is in the proposal: https://github.com/surajssd/opencompose/blob/22207ea605580d70c6718fa87368d0385bc3cbf0/docs/proposals/volumes-proposal.md

Fixes #25

surajssd · 2017-04-04T10:50:47Z

using opencompose file:

version: 0.1-dev

services:
- name: db
  containers:
  - image: mysql
    mounts:
    - volumeName: db
      mountPath: /var/lib/mysql

- name: backup
  containers:
  - image: backup
    mounts:
    - volumeName: db
      mountPath: /app/store
      volumeSubPath: foo/bar
      readOnly: true

- name: process
  containers:
  - image: process
    mounts:
    - volumeName: temp
      mountPath: /app/data
  emptyDirVolumes:
  - name: temp

volumes:
- name: db
  size: 5Gi
  accessMode: ReadWriteMany
  storageClass: fast

converting as:

$ opencompose convert -f ex.yml -o - 
---
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  creationTimestamp: null
  labels:
    service: db
  name: db
spec:
  strategy:
    type: RollingUpdate
  template:
    metadata:
      creationTimestamp: null
      labels:
        service: db
    spec:
      containers:
      - image: mysql
        name: db-0
        resources: {}
        volumeMounts:
        - mountPath: /var/lib/mysql
          name: db
      volumes:
      - name: db
        persistentVolumeClaim:
          claimName: db
status: {}
---
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  creationTimestamp: null
  labels:
    service: backup
  name: backup
spec:
  strategy:
    type: RollingUpdate
  template:
    metadata:
      creationTimestamp: null
      labels:
        service: backup
    spec:
      containers:
      - image: backup
        name: backup-0
        resources: {}
        volumeMounts:
        - mountPath: /app/store
          name: db
          readOnly: true
          subPath: foo/bar
      volumes:
      - name: db
        persistentVolumeClaim:
          claimName: db
status: {}
---
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  creationTimestamp: null
  labels:
    service: process
  name: process
spec:
  strategy:
    type: RollingUpdate
  template:
    metadata:
      creationTimestamp: null
      labels:
        service: process
    spec:
      containers:
      - image: process
        name: process-0
        resources: {}
        volumeMounts:
        - mountPath: /app/data
          name: temp
      volumes:
      - emptyDir: {}
        name: temp
status: {}
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  annotations:
    volume.beta.kubernetes.io/storage-class: fast
  creationTimestamp: null
  name: db
spec:
  accessModes:
  - ReadWriteMany
  resources:
    requests:
      storage: 5Gi
status: {}

surajssd · 2017-04-04T10:52:18Z

Tests are pending but would appreciate feedback to see if I am going right?
ping @kadel @tnozicka

surajssd · 2017-04-10T11:09:02Z

was clicked by mistake

surajssd · 2017-04-10T11:21:47Z

@kadel For testing the parsing part/encoding_test.go I am considering following test case types:

type	pass or fail	optional comment
All fields given	✅
Required fields only given/no optional field	✅
Extra field given	❌
Wrong field types	❌
No fields given at all		✅ if all fields are optional otherwise ❌
One required field not given	❌

Do you feel anything is missing and I should think of something else?

surajssd · 2017-04-11T10:35:00Z

pkg/object/object.go

+		}
+
+		// validate mountPath
+		if err := validatePath(mount.MountPath); err != nil {


@kadel should we do this?

or just assume that user will give valid path?

If i'm not mistaken MountPath should be always absolute path. We could check at least that.

brainstorm · 2017-04-12T14:31:08Z

examples/vols/ex.yml

+    mounts:
+    - volumeName: db
+      mountPath: /app/store
+      volumeSubPath: foo/bar


Total thumbsup for this feature, I do need it for my usecase:

https://github.com/brainstorm/cgtd/commit/344fa1a15d5874c3dff17e91537864b77b5fd2b1

Would it be reasonable to include support for paths like $HOME in the spec?

Would it be reasonable to include support for paths like $HOME in the spec?

That is interesting idea. We won't add it in this initial implementation, but definitely will keep it in mind for future improvements. Will have to do some research on how this can be done in Kubernetes. (any pointers?)

I have no idea, unfortunately, quite a newbie on kubernetes but I know from related-ish projects like:

https://github.com/BD2KGenomics/toil
https://github.com/common-workflow-language/common-workflow-language

That users are gonna expect it to be supported :-S

concaf

@surajssd How about we move the type declarations to object.go?

concaf · 2017-04-12T14:32:54Z

pkg/encoding/v1/encoding.go

@@ -179,10 +179,55 @@ type ImageRef string

 // FIXME: implement ImageRef unmarshalling

+type Path string


How about we move the type declaration to object.go and do something like type Path object.Path here?

Hmm, I guess in this case its ok. As there is no object.Path its inside Mount struct.

But if we don't need custom UnmarshalYAML for Path than we don't even need Path as separate type here.
We can use just string instead of Path in Mount struct

concaf · 2017-04-12T14:34:00Z

pkg/encoding/v1/encoding.go

+	return nil
+}
+
+type Mount struct {


How about we move the type declaration to object.go

concaf · 2017-04-12T14:34:41Z

pkg/encoding/v1/encoding.go

@@ -205,10 +250,34 @@ func (c *Container) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	return nil
 }

+type EmptyDirVolume struct {


How about we move the type declaration to object.go

This needs json tags so keeping it here.
Ref: @tnozicka 's explanation

concaf · 2017-04-12T14:38:31Z

pkg/transform/kubernetes/kubernetes.go

@@ -189,14 +190,91 @@ func (t *Transformer) CreateDeployments(o *object.Service) ([]runtime.Object, er
 			})
 		}

+		// TODO: It is assumed that the check is done about the existence of volume in root level volume section
+		for _, mount := range c.Mounts {
+


nit: remove this \n

kadel

Here is my first pass on this.

Didn't have time to look at tests yet :-(

kadel · 2017-04-12T12:54:55Z

pkg/encoding/v1/encoding.go

@@ -233,16 +302,37 @@ func (s *Service) UnmarshalYAML(unmarshal func(interface{}) error) error {

 type VolumeSize string

-// FIXME: add VolumeSize parsing/validation
+func (vs *VolumeSize) UnmarshalYAML(unmarshal func(interface{}) error) error {


If no validation or additional parsing is done here, this function is not needed, is it?

I did it as it's done for ResourceName here.

UnmarshalYAML for ResourceName does validation on top of unmarshalling .
But in this case you are not doing anything special, you are just calling unmarshal function on string type. You don't need custom UnmarshalYAML function in this case

removed those types changed it to built in type string

kadel · 2017-04-12T12:55:05Z

pkg/encoding/v1/encoding.go


 type VolumeMode string

-// FIXME: add VolumeMode parsing/validation
+func (vm *VolumeMode) UnmarshalYAML(unmarshal func(interface{}) error) error {


If no validation or additional parsing is done here, this function is not needed, is it?

see the comment above!

see comment above ;-)

kadel · 2017-04-12T12:55:26Z

pkg/encoding/v1/encoding.go

@@ -179,10 +179,55 @@ type ImageRef string

 // FIXME: implement ImageRef unmarshalling

+type Path string
+
+func (p *Path) UnmarshalYAML(unmarshal func(interface{}) error) error {


If no validation or additional parsing is done here, this function is not needed, is it?

kadel · 2017-04-12T14:16:16Z

pkg/object/object.go

@@ -50,16 +71,129 @@ type OpenCompose struct {
 	Volumes  []Volume
 }

+func (s *Service) EmptyDirVolumeExists(name string) bool {


can you please add comment explaining what this function is for?

kadel · 2017-04-12T14:16:19Z

pkg/object/object.go

+	return false
+}
+
+func (o *OpenCompose) VolumeExists(name string) bool {


can you please add comment explaining what this function is for?

kadel · 2017-04-12T14:41:08Z

pkg/object/object.go

+
+// Documentation about the valid identifiers can be found at
+// https://github.com/kubernetes/community/blob/master/contributors/design-proposals/identifiers.md
+func isValidName(name string) error {


this is called isValidName but other validation function are called validate<something> ( validateVolumeMode
validatePath). Wouldn't be better to use same naming scheme for all function validation fields?

another issue is that when I put wrong name i get just this error:

ERROR: must match the regex [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)* (e.g. 'example.com')

But it doesn't say where it is. :-(
I guess this issue will be same with other validations here. :-(

this is called isValidName but other validation function are called validate<something> ( validateVolumeMode validatePath). Wouldn't be better to use same naming scheme for all function validation fields?

done

another issue is that when I put wrong name i get just this error:
ERROR: must match the regex [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)* (e.g. 'example.com')
But it doesn't say where it is. :-(
I guess this issue will be same with other validations here. :-(

done

kadel · 2017-04-12T14:43:52Z

pkg/object/object.go

+	return nil
+}
+
+func (v *Volume) Validate() error {


Is this called outside object pkg?
I think this can be private. There is no reason for someone validating just volume outside of this package.

Just thinking out loud, thought these could turn into interface or something similar?

interface? why? Don't know what you mean :-(

I actually don;t remember what was I thinking :-D

@surajssd so what do you think about making this private?
I would make all Validate function private expect func (o *OpenCompose) Validate(). If we are thinking about this being used as library, this is only Validate function that should be exposed.

kadel · 2017-04-12T15:40:25Z

pkg/object/object.go

+		// or emptydirvolumes
+		for _, container := range service.Containers {
+			for _, mount := range container.Mounts {
+				if !o.VolumeExists(mount.VolumeName) && !service.EmptyDirVolumeExists(mount.VolumeName) {


There should be one more check that validates that there are not multiple mounts for same path.

mounts: - volumeName: bar mountPath: /storage - volumeName: foo mountPath: /storage

This is not valid and should be checked.

kadel · 2017-04-12T15:48:06Z

pkg/transform/kubernetes/kubernetes.go

+			// if this mount does not exist in emptydir then this is coming from root level volumes directive
+			// if tomorrow we add support for ConfigMaps or Secrets mounted as volumes the check should be done
+			// here to see if it is not coming from configMaps or Secrets
+			if !o.EmptyDirVolumeExists(mount.VolumeName) {


Wouldn't be better to do if o.VolumeExists(mount.VolumeName) here? It might make more sense once we add secrets as volumes and It will be also a bit safer.

We don't have the object opencompose to do that call(o.VolumeExists). We have access to service type object o.

hmm, that is quite confusing if its called o :-)
OK than, lets keep it that way, but it might bring some problem in the future.

changed it to s.

kadel · 2017-04-12T15:55:26Z

pkg/transform/kubernetes/kubernetes.go

+		pvc.Spec.AccessModes = []api_v1.PersistentVolumeAccessMode{api_v1.ReadOnlyMany}
+	case "ReadWriteMany":
+		pvc.Spec.AccessModes = []api_v1.PersistentVolumeAccessMode{api_v1.ReadWriteMany}
+	}


Can we add default section that will cause error?
I know that it shouldn't happen because its validated before, but you never know ;-) That one line might save a lot of debugging in future ;-)

yeah SGTM, done.

kadel · 2017-04-24T13:16:10Z

examples/vols/ex.yml

+    - volumeName: db
+      mountPath: /var/lib/mysql
+    - volumeName: iiit
+      mountPath: /var/lib/mysql


This example is invalid!

ERROR: service "db": container#1: mount "iiit": mountPath "/var/lib/mysql": cannot have same mountPath as "db"

kadel · 2017-04-24T14:28:50Z

pkg/object/object.go

+	allMounts := make(map[string]string)
+	// validate Mounts
+	for _, mount := range c.Mounts {
+		// validate volumeName


if we have (c *Container) Validate() for container validation, (s *Service) Validate() for service validation, (v *Volume) Validate() for volume validation. Would it make sense to have (m *Mount) Validate() for validating single mount ?

Everything expect checking for colliding paths could be in that separate Validate function

The problem is that we might not be able to do the validation of colliding mountPath ? Since the variable allMounts is defined outside the for loop and used inside the loop, one workaround would be to pass it with every call but then it makes mount's validate function inconsistent than other validate functions.

What I mend is to keep validation of colliding mountPath where it is.

Just move name validation ( if err := validateName(mount.VolumeName);) and path validation (if !path.IsAbs(mount.MountPath) {) to separate (m *Mount) Validate(). Those to checks don't need information about other mounts they can be separate.

@surajssd what do you think about my suggestion?

I think having it in the function makes more sense than having half things here and others there

At least all those are checks in same place to avoid confusion

I still feel like this is more confusing. With other parts of OpenCompose object (service, volumes, ...) we have nicely separated validation function but here we are validating whole Mount object in Container object validation.

But if you think this is better than let's keep it like this. I can make peace with it. 😄

it's confusing to have some tests outside in a different place and only one in here, that is why i was planning to keep it here. If the container function grows and become very huge then we can think of separating it? WDYT?

if you think this is better, lets keep it

so is this good to go in?

kadel · 2017-04-25T12:36:28Z

do we need examples/vols/ex.yml? To be honest I don't like that file, as it is nonsense example that does nothing and its invalid :-(

surajssd · 2017-04-26T14:40:32Z

do we need examples/vols/ex.yml? To be honest I don't like that file, as it is nonsense example that does nothing and its invalid :-(

@kadel done

This PR adds support for volumes in OpenCompose. With this there is new root level directive called `volumes` and new directive in container level called `mounts` and also adds service level directive called `emptyDirVolumes`. More info about volumes is in the proposal: https://github.com/surajssd/opencompose/blob/22207ea605580d70c6718fa87368d0385bc3cbf0/docs/proposals/volumes-proposal.md Fixes redhat-developer#25

kadel

I found one small issue with this. Its described in #114 We don't have to block this PR because of that.

surajssd · 2017-05-05T18:56:49Z

Cool will merge this then

surajssd force-pushed the volumes-implementation branch from a1e11fd to 5e03a5e Compare April 4, 2017 10:46

surajssd force-pushed the volumes-implementation branch from 5e03a5e to 21b9407 Compare April 6, 2017 18:06

surajssd closed this Apr 10, 2017

surajssd reopened this Apr 10, 2017

surajssd force-pushed the volumes-implementation branch 4 times, most recently from f8070a9 to a19a891 Compare April 11, 2017 10:31

surajssd commented Apr 11, 2017

View reviewed changes

surajssd force-pushed the volumes-implementation branch 4 times, most recently from dfa2e8e to d559494 Compare April 11, 2017 18:05

brainstorm reviewed Apr 12, 2017

View reviewed changes

concaf requested changes Apr 12, 2017

View reviewed changes

kadel reviewed Apr 12, 2017

View reviewed changes

surajssd force-pushed the volumes-implementation branch 3 times, most recently from ef81fda to 6a7c60a Compare April 18, 2017 19:30

surajssd mentioned this pull request Apr 19, 2017

Validating service name in object.go #106

Open

surajssd force-pushed the volumes-implementation branch from 6a7c60a to b5ee51f Compare April 20, 2017 14:37

kadel reviewed Apr 24, 2017

View reviewed changes

surajssd force-pushed the volumes-implementation branch 3 times, most recently from 36e694e to 2a620eb Compare April 25, 2017 11:02

surajssd force-pushed the volumes-implementation branch from 2a620eb to 17a3581 Compare April 25, 2017 11:04

surajssd force-pushed the volumes-implementation branch 2 times, most recently from d0bf775 to 80f4f1c Compare April 25, 2017 13:44

surajssd force-pushed the volumes-implementation branch from 80f4f1c to 88bfdbf Compare April 26, 2017 15:49

surajssd force-pushed the volumes-implementation branch from 88bfdbf to 81ac143 Compare May 5, 2017 10:18

surajssd force-pushed the volumes-implementation branch from 81ac143 to 1369132 Compare May 5, 2017 10:21

kadel approved these changes May 5, 2017

View reviewed changes

surajssd merged commit 5b5dc72 into redhat-developer:master May 5, 2017

kadel mentioned this pull request May 31, 2017

abstract out validate() method for object.Mount #147

Merged

		@@ -179,10 +179,55 @@ type ImageRef string

		// FIXME: implement ImageRef unmarshalling

		type Path string

Add volumes implementation support #88

Add volumes implementation support #88

Conversation

surajssd commented Apr 3, 2017

surajssd commented Apr 4, 2017

surajssd commented Apr 4, 2017

surajssd commented Apr 10, 2017

surajssd commented Apr 10, 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

concaf 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

kadel 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

surajssd Apr 17, 2017 • edited Loading

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

kadel Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

kadel Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel May 3, 2017 • edited Loading

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

kadel commented Apr 25, 2017

surajssd commented Apr 26, 2017

kadel left a comment

Choose a reason for hiding this comment

surajssd commented May 5, 2017

surajssd Apr 17, 2017 •

edited

Loading

kadel Apr 24, 2017 •

edited

Loading

kadel Apr 24, 2017 •

edited

Loading

kadel May 3, 2017 •

edited

Loading