Added support for tmpfs #484

surajnarwade · 2017-03-10T10:38:53Z

fixes #436

surajnarwade · 2017-03-10T12:15:13Z

cc @cdrage @kadel @surajssd @containscafeine

cdrage · 2017-03-10T15:04:12Z

Missing tests! Will do a review now however.

cdrage · 2017-03-10T15:08:07Z

@surajnarwade

This doesn't work..

redis:                                                                                                                                                                                                                                                                            
  image: redis:3.0                                                                                                                                                                                                                                                                
  ports:                                                                                                                                                                                                                                                                          
    - "6379"                                                                                                                                                                                                                                                                      
  tmpfs: /run

github.com/kubernetes-incubator/kompose  pr_484 ✗                                                                                                                                                                                                                       4h29m ◒  
▶ ./kompose convert --stdout
ERRO Could not parse config for project kompose : Unsupported config option for redis service: 'tmpfs' 
FATA Failed to load compose file: Unsupported config option for redis service: 'tmpfs'

cdrage · 2017-03-10T15:08:35Z

pkg/transformer/kubernetes/k8sutils.go

@@ -315,6 +315,19 @@ func (k *Kubernetes) UpdateKubernetesObjects(name string, service kobject.Servic

 	// Configure the container volumes.
 	volumesMount, volumes, pvc := k.ConfigVolumes(name, service)
+	// Configure Tmpfs
+	if len(service.TmpFs) > 0 {


Shouldn't this check if service.TmpFs != nil?

I think this is better than just !=nil

if you test only for !=nil than it would be false if service.Tmpfs is empty array.

https://play.golang.org/p/9W5KUrr9jY

cdrage · 2017-03-10T15:09:42Z

pkg/kobject/kobject.go

@@ -83,6 +83,7 @@ type ServiceConfig struct {
 	Stdin         bool                `compose:"stdin_open" bundle:""`
 	Tty           bool                `compose:"tty" bundle:""`
 	MemLimit      yaml.MemStringorInt `compose:"mem_limit" bundle:""`
+	TmpFs         []string            `compose:"tmpfs" bundle:""`


There may be some issues with this, see https://github.com/docker/libcompose/blob/f12d14623f73a0ca0d7fc37253a99165ae48d39f/config/types.go

It can either be a slice of a string, see: https://docs.docker.com/compose/compose-file/compose-file-v2/#tmpfs

Nevermind! I confirmed that both work regardless of string slice or normal string.

it is ok

[]string is hidden under yaml.Stringorslice ;-)

#types_yaml.go type Stringorslice strslice.StrSlice #strslice.go type StrSlice []string

cdrage · 2017-03-10T15:09:52Z

pkg/loader/compose/compose.go

@@ -369,7 +369,7 @@ func (c *Compose) LoadFile(files []string) kobject.KomposeObject {
 		serviceConfig.Stdin = composeServiceConfig.StdinOpen
 		serviceConfig.Tty = composeServiceConfig.Tty
 		serviceConfig.MemLimit = composeServiceConfig.MemLimit
-
+		serviceConfig.TmpFs = composeServiceConfig.Tmpfs


cdrage · 2017-03-10T15:10:53Z

pkg/transformer/kubernetes/kubernetes.go

+func (k *Kubernetes) ConfigTmpfs(name string, service kobject.ServiceConfig) ([]api.VolumeMount, []api.Volume) {
+	volumeMounts := []api.VolumeMount{}
+	volumes := []api.Volume{}
+	var count int


no need for count

cdrage · 2017-03-10T15:11:24Z

pkg/transformer/kubernetes/kubernetes.go

+	for _, volume := range service.TmpFs {
+
+		volumeName = fmt.Sprintf("%s-tmpfs%d", name, count)
+		count++


no need for count, you can use the range loop for the index value..

for index, volume := range service.TmpFs {

cdrage · 2017-03-10T15:13:39Z

pkg/transformer/kubernetes/kubernetes.go

+		}
+		volumeMounts = append(volumeMounts, volmount)
+
+		var volsource *api.VolumeSource


no need, use implicit, much cleaner

cdrage · 2017-03-10T15:13:49Z

pkg/transformer/kubernetes/kubernetes.go

+
+		var volsource *api.VolumeSource
+		//create tmpfs specific empty volumes
+		volsource = k.ConfigEmptyVolumeSource("tmpfs")


volSource := ...

cdrage · 2017-03-10T15:14:14Z

pkg/transformer/kubernetes/kubernetes.go

+		// create a new volume object using the volsource and add to list
+		vol := api.Volume{
+			Name:         volumeName,
+			VolumeSource: *volsource,


volSource for value

no it has to passed like as it is mentioned

@surajnarwade what I mean is that it's suppose to be Go conventions for naming, so volSource instead of volsource

@cdrage oh got it

cdrage · 2017-03-10T15:14:35Z

pkg/transformer/kubernetes/kubernetes.go

@@ -368,7 +400,7 @@ func (k *Kubernetes) ConfigVolumes(name string, service kobject.ServiceConfig) (
 		// For PVC we will also create a PVC object and add to list
 		var volsource *api.VolumeSource
 		if useEmptyVolumes {
-			volsource = k.ConfigEmptyVolumeSource()
+			volsource = k.ConfigEmptyVolumeSource("volume")


again, can be implied, using volSource := ...

we can't implied here, because volsource is used in if and else and outside of that too

cdrage · 2017-03-10T15:15:37Z

pkg/transformer/kubernetes/kubernetes.go

@@ -389,10 +421,19 @@ func (k *Kubernetes) ConfigVolumes(name string, service kobject.ServiceConfig) (
 }

 // ConfigEmptyVolumeSource is helper function to create an EmptyDir api.VolumeSource
-func (k *Kubernetes) ConfigEmptyVolumeSource() *api.VolumeSource {
+func (k *Kubernetes) ConfigEmptyVolumeSource(key string) *api.VolumeSource {
+	if key == "tmpfs" {


add comments

cdrage · 2017-03-10T15:17:23Z

Hiya @surajnarwade

So a few things!

Great code, but there's a lot of missing comments, explaining what's happening in a few sentences would really help with some parts of the code.

The commit description is lacking, please type up what you do, why it's necessary and link the issue (like you already did), you can modify it with git commit --amend

cdrage · 2017-03-10T15:19:15Z

A good example for a descriptive git message is @containscafeine 's one he used on a recent PR:

This commit refactors the code to remove more or less
all occurences of logrus.Fatalf() from the code under
pkg/ except for app.go where all the errors are being
handled currently.

This is being done since random logrus.Fatalf() calls
all around the code was making handling the errors,
unit testing and troubleshooting a bit more painful.

logrus.Fatalf() calls are either replaced by
return errors.New("new error")
or
return errors.Wrap(err, "annonate error")
calls, and the function signatures are accordingly
changed to accomodate the new return values.

The unit tests which previously used to check
if logrus.Fatalf() calls worked fine have also
been fixed to only check for errors now.

Fixes #416

kadel · 2017-03-10T15:22:45Z

This doesn't work..

redis:
image: redis:3.0
ports:
- "6379"
tmpfs: /run
github.com/kubernetes-incubator/kompose pr_484 ✗ 4h29m ◒
▶ ./kompose convert --stdout
ERRO Could not parse config for project kompose : Unsupported config option for redis service: 'tmpfs'
FATA Failed to load compose file: Unsupported config option for redis service: 'tmpfs'

tmpfs is not in docker-compose from v2 and up ;-)

cdrage · 2017-03-10T15:32:16Z

There we go, my mistake!

github.com/kubernetes-incubator/kompose  pr_484 ✗                                                                                                                                                                                                                      4h53m ◒  ⍉
▶ cat docker-compose.yml 
version: '2'

services:

  redis:
    image: redis:3.0
    tmpfs: /foobar

github.com/kubernetes-incubator/kompose  pr_484 ✗                                                                                                                                                                                                                       4h54m ◒  
▶ ./kompose convert --stdout
apiVersion: v1
items:
- apiVersion: v1
  kind: Service
  metadata:
    creationTimestamp: null
    labels:
      service: redis
    name: redis
  spec:
    clusterIP: None
    ports:
    - name: headless
      port: 55555
      targetPort: 0
    selector:
      service: redis
  status:
    loadBalancer: {}
- apiVersion: extensions/v1beta1
  kind: Deployment
  metadata:
    creationTimestamp: null
    name: redis
  spec:
    replicas: 1
    strategy: {}
    template:
      metadata:
        creationTimestamp: null
        labels:
          service: redis
      spec:
        containers:
        - image: redis:3.0
          name: redis
          resources: {}
          volumeMounts:
          - mountPath: /foobar
            name: redis-tmpfs0
        restartPolicy: Always
        volumes:
        - emptyDir:
            medium: Memory
          name: redis-tmpfs0
  status: {}
kind: List
metadata: {}

cdrage · 2017-03-10T15:33:40Z

And I can confirm it works with a list:

github.com/kubernetes-incubator/kompose  pr_484 ✗                                                                                                                                                                                                                      4h54m ◒  ⍉
▶ cat docker-compose.yml 
version: '2'

services:

  redis:
    image: redis:3.0
    tmpfs: 
      - /foobar
      - /foobar2

github.com/kubernetes-incubator/kompose  pr_484 ✗                                                                                                                                                                                                                       4h54m ◒  
▶ ./kompose convert --stdout
apiVersion: v1
items:
- apiVersion: v1
  kind: Service
  metadata:
    creationTimestamp: null
    labels:
      service: redis
    name: redis
  spec:
    clusterIP: None
    ports:
    - name: headless
      port: 55555
      targetPort: 0
    selector:
      service: redis
  status:
    loadBalancer: {}
- apiVersion: extensions/v1beta1
  kind: Deployment
  metadata:
    creationTimestamp: null
    name: redis
  spec:
    replicas: 1
    strategy: {}
    template:
      metadata:
        creationTimestamp: null
        labels:
          service: redis
      spec:
        containers:
        - image: redis:3.0
          name: redis
          resources: {}
          volumeMounts:
          - mountPath: /foobar
            name: redis-tmpfs0
          - mountPath: /foobar2
            name: redis-tmpfs1
        restartPolicy: Always
        volumes:
        - emptyDir:
            medium: Memory
          name: redis-tmpfs0
        - emptyDir:
            medium: Memory
          name: redis-tmpfs1
  status: {}
kind: List
metadata: {}

surajnarwade · 2017-03-14T10:00:22Z

cc @kadel @cdrage

cdrage · 2017-03-14T20:10:50Z

Saw the new comments added to the code, thanks!

This LGTM.

kadel · 2017-03-15T12:53:41Z

pkg/transformer/kubernetes/kubernetes_test.go

@@ -511,3 +512,14 @@ func TestInitPodSpec(t *testing.T) {
 		t.Fatalf("Pod object not found")
 	}
 }
+
+func TestKubernetes_ConfigTmpfs(t *testing.T) {


One last change here ;-)
Can you please rename this? Its in Kubernetes package so it doesn't have to to have Kubernetes in name and _ is not GOlangy :-)

just TestConfigTmpfs says everything about what it is doing

Sure, I will make it GOlangy ;-)

surajnarwade · 2017-03-15T16:29:32Z

@kadel , done with correction.

cdrage · 2017-03-15T17:13:50Z

@surajnarwade

Almost there! Just need to resolve the conflicts from me merging in #416

After that, I think it's ready to be merged! Cause as of right now, the code LGTM 👍

kadel

This should be also last thing from me ;-)

kadel · 2017-03-16T16:31:31Z

pkg/transformer/kubernetes/kubernetes_test.go

+	k := Kubernetes{}
+	resultVolumeMount, resultVolume := k.ConfigTmpfs(name, newServiceConfig())
+
+	if resultVolumeMount[0].Name != "foo-tmpfs0" && resultVolume[0].EmptyDir.Medium != "Memory" {


One last thing. Should it be || instead of &&?

What if only one of them is wrong?

fixes kubernetes#436 This commit will add support for tmpfs, configEmptyVolumeSource function is being modified as it have to work in two ways now. (For emptyvols and tmpfs) Added unit test for tmpfs too.

surajnarwade · 2017-03-16T16:47:57Z

@kadel , last thing updated ;-)

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

surajnarwade force-pushed the add_tmpfs_support branch 2 times, most recently from 5b08fe1 to db527f1 Compare March 10, 2017 12:01

cdrage requested changes Mar 10, 2017

View reviewed changes

surajnarwade force-pushed the add_tmpfs_support branch from db527f1 to be6ce08 Compare March 13, 2017 15:48

cdrage approved these changes Mar 14, 2017

View reviewed changes

kadel reviewed Mar 15, 2017

View reviewed changes

surajnarwade force-pushed the add_tmpfs_support branch from be6ce08 to 99d612e Compare March 15, 2017 14:19

surajnarwade force-pushed the add_tmpfs_support branch from 99d612e to eb4f7df Compare March 16, 2017 08:40

kadel requested changes Mar 16, 2017

View reviewed changes

Added support for tmpfs

4941334

fixes kubernetes#436 This commit will add support for tmpfs, configEmptyVolumeSource function is being modified as it have to work in two ways now. (For emptyvols and tmpfs) Added unit test for tmpfs too.

surajnarwade force-pushed the add_tmpfs_support branch from eb4f7df to 4941334 Compare March 16, 2017 16:45

kadel approved these changes Mar 16, 2017

View reviewed changes

kadel merged commit 752b203 into kubernetes:master Mar 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for tmpfs #484

Added support for tmpfs #484

surajnarwade commented Mar 10, 2017

surajnarwade commented Mar 10, 2017

cdrage commented Mar 10, 2017

cdrage commented Mar 10, 2017

cdrage Mar 10, 2017

kadel Mar 10, 2017

cdrage Mar 10, 2017

cdrage Mar 10, 2017

kadel Mar 10, 2017

cdrage Mar 10, 2017

cdrage Mar 10, 2017

cdrage Mar 10, 2017

cdrage Mar 10, 2017

cdrage Mar 10, 2017

cdrage Mar 10, 2017

surajnarwade Mar 12, 2017

cdrage Mar 13, 2017

surajnarwade Mar 13, 2017

cdrage Mar 10, 2017

surajnarwade Mar 11, 2017

cdrage Mar 10, 2017

cdrage commented Mar 10, 2017

cdrage commented Mar 10, 2017

kadel commented Mar 10, 2017

cdrage commented Mar 10, 2017

cdrage commented Mar 10, 2017

surajnarwade commented Mar 14, 2017

cdrage commented Mar 14, 2017

kadel Mar 15, 2017

surajnarwade Mar 15, 2017 •

edited

Loading

surajnarwade commented Mar 15, 2017

cdrage commented Mar 15, 2017

kadel left a comment •

edited

Loading

kadel Mar 16, 2017 •

edited

Loading

surajnarwade commented Mar 16, 2017

Added support for tmpfs #484

Added support for tmpfs #484

Conversation

surajnarwade commented Mar 10, 2017

surajnarwade commented Mar 10, 2017

cdrage commented Mar 10, 2017

cdrage commented Mar 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

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

cdrage commented Mar 10, 2017

cdrage commented Mar 10, 2017

kadel commented Mar 10, 2017

cdrage commented Mar 10, 2017

cdrage commented Mar 10, 2017

surajnarwade commented Mar 14, 2017

cdrage commented Mar 14, 2017

Choose a reason for hiding this comment

surajnarwade Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

surajnarwade commented Mar 15, 2017

cdrage commented Mar 15, 2017

kadel left a comment • edited Loading

Choose a reason for hiding this comment

kadel Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

surajnarwade commented Mar 16, 2017

surajnarwade Mar 15, 2017 •

edited

Loading

kadel left a comment •

edited

Loading

kadel Mar 16, 2017 •

edited

Loading