fix driver:local in prefixing volumes with current dir name #557

procrypt · 2017-04-19T08:17:12Z

If we have a docker-file with root level volumes and we do a kompose up using that docker-file, libcompose will add an additional _ followed by the current directory name.

docker-compose.yml used

version: '2'
services:
  mariadb:
    image: 'bitnami/mariadb:latest'
    volumes:
      - 'mariadb-data:/bitnami/mariadb'
    environment:
      - MARIADB_USER=bn_wordpress
      - MARIADB_DATABASE=bitnami_wordpress
      - ALLOW_EMPTY_PASSWORD=yes
  wordpress:
    image: 'bitnami/wordpress:latest'
    ports:
      - '80:80'
      - '443:443'
    volumes:
      - 'wordpress-data:/bitnami/wordpress'
      - 'apache-data:/bitnami/apache'
      - 'php-data:/bitnami/php'
    depends_on:
      - mariadb
    environment:
      - MARIADB_HOST=mariadb
      - MARIADB_PORT=3306
      - WORDPRESS_DATABASE_USER=bn_wordpress
      - WORDPRESS_DATABASE_NAME=bitnami_wordpress
      - ALLOW_EMPTY_PASSWORD=yes
volumes:
  mariadb-data:
    driver: local
  wordpress-data:
    driver: local
  apache-data:
    driver: local
  php-data:
    driver: local

- apiVersion: v1
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: kompose_apache-data
    name: kompose_apache-data
  spec:
    accessModes:
    - ReadWriteOnce
    resources:
      requests:
        storage: 100Mi
  status: {}
- apiVersion: v1
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: kompose_php-data
    name: kompose_php-data
  spec:
    accessModes:
    - ReadWriteOnce
    resources:
      requests:
        storage: 100Mi
  status: {}
kind: List
metadata: {}

$ kompose -f docker-compose.yml up
WARN Unsupported root level volumes key - ignoring 
WARN Unsupported depends_on key - ignoring        
INFO We are going to create Kubernetes Deployments, Services and PersistentVolumeClaims for your Dockerized application. If you need different kind of resources, use the 'kompose convert' and 'kubectl create -f' commands instead. 
 
INFO Deploying application in "default" namespace 
INFO Successfully created Service: mariadb        
INFO Successfully created Service: wordpress      
FATA Error while deploying application: Deployment.extensions "mariadb" is invalid: [spec.template.spec.volumes[0].name: Invalid value: "komposefiles_mariadb-data": must match the regex [a-z0-9]([-a-z0-9]*[a-z0-9])? (e.g. 'my-name' or '123-abc'), spec.template.spec.containers[0].volumeMounts[0].name: Not found: "komposefiles_mariadb-data"]

Since kubernetes doesn't allow _ in the objects created so kompose up will fail to deploy it.

As a solution we replace _ to - and we can then deploy it successfully.

$ kompose -f docker-compose.yml up
WARN Unsupported root level volumes key - ignoring 
WARN Unsupported depends_on key - ignoring        
INFO We are going to create Kubernetes Deployments, Services and PersistentVolumeClaims for your Dockerized application. If you need different kind of resources, use the 'kompose convert' and 'kubectl create -f' commands instead. 
 
INFO Deploying application in "default" namespace 
INFO Successfully created Service: mariadb        
INFO Successfully created Service: wordpress      
INFO Successfully created Deployment: mariadb     
INFO Successfully created PersistentVolumeClaim: komposefiles-mariadb-data 
INFO Successfully created Deployment: wordpress   
INFO Successfully created PersistentVolumeClaim: komposefiles-wordpress-data 
INFO Successfully created PersistentVolumeClaim: komposefiles-apache-data 
INFO Successfully created PersistentVolumeClaim: komposefiles-php-data 

Your application has been deployed to Kubernetes. You can run 'kubectl get deployment,svc,pods,pvc' for details.

- apiVersion: v1
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: kompose-apache-data
    name: kompose-apache-data
  spec:
    accessModes:
    - ReadWriteOnce
    resources:
      requests:
        storage: 100Mi
  status: {}
- apiVersion: v1
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: kompose-php-data
    name: kompose-php-data
  spec:
    accessModes:
    - ReadWriteOnce
    resources:
      requests:
        storage: 100Mi
  status: {}
kind: List
metadata: {}

CC: @kadel @cdrage
Fixes #550

dkarlovi · 2017-04-19T08:31:37Z

pkg/transformer/kubernetes/kubernetes.go

+	}
+	for _, label := range pvc.Labels {
+		if strings.Contains(label, "_") {
+			pvc.Labels = transformer.ConfigLabels(strings.Replace(label, "_", "-", -1))


A better idea here might be not to hardcode underscore-to-dash transition but instead to allow for an array of tuples (blacklisted char, replacement char) which will enable easier fix for this problem with other characters (if they show up) in the future.

dkarlovi · 2017-04-19T17:56:15Z

@procrypt this doesn't seem to work, here's the full test (using Compose file from #554 (comment)). Note that version says 0.4.0, but the SHA is correct.

[dkarlovi@fredo kompose_underscore-test]$ kompose version
0.4.0 (87fa7cb)
[dkarlovi@fredo kompose_underscore-test]$ pwd
/home/dkarlovi/Development/R&D/DevOps/Kubernetes/kompose_underscore-test
[dkarlovi@fredo kompose_underscore-test]$ kompose -f underscore-pvc.yml convert --stdout | grep PersistentVolumeClaim -A 5
WARN Unsupported root level volumes key - ignoring
WARN Unsupported depends_on key - ignoring
WARN Unsupported hostname key - ignoring
WARN Kubernetes provider doesn't support build key - ignoring
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: komposeunderscoretest_broker
    name: komposeunderscoretest_broker
--
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: komposeunderscoretest_cache
    name: komposeunderscoretest_cache
--
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: komposeunderscoretest_db
    name: komposeunderscoretest_db

dkarlovi · 2017-04-19T18:01:35Z

I've figured it out: if you leave the root-level volumes: block, it doesn't work. If you remove it, it does work.

kompose says it's ignoring it, but that doesn't seem to be the case.

Also, it works in HEAD too, without this PR.

procrypt · 2017-04-20T04:48:08Z

@dkarlovi Did you build kompose correctly after fetching my branch. It seems to work fine when I run kompose convert. Please take a look at the output below, I'm using the docker-compose file provided by you.

$ kompose convert -f dev.yml --stdout  | grep PersistentVolumeClaim -A 5
WARN Unsupported root level volumes key - ignoring 
WARN Unsupported depends_on key - ignoring        
WARN Unsupported hostname key - ignoring          
WARN Kubernetes provider doesn't support build key - ignoring 
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: komposefiles-broker
    name: komposefiles-broker
--
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: komposefiles-cache
    name: komposefiles-cache
--
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: komposefiles-db
    name: komposefiles-db

kompose version 
0.5.0 (HEAD)

Please make sure you build kompose correctly and try it again.

surajnarwade · 2017-04-20T05:02:38Z

@procrypt @dkarlovi , its working for me,

$ kompose version
0.5.0 (HEAD)

output is:

$ kompose convert -f docker-compose.yml --stdout  | grep PersistentVolumeClaim -A 5
WARN Unsupported root level volumes key - ignoring 
WARN Unsupported depends_on key - ignoring        
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: procrypt-mariadb-data
    name: procrypt-mariadb-data
--
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: procrypt-wordpress-data
    name: procrypt-wordpress-data
--
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: procrypt-apache-data
    name: procrypt-apache-data
--
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: procrypt-php-data
    name: procrypt-php-data

Make sure you are building from this PR only:

 git fetch upstream pull/557/head:driver_local

dkarlovi · 2017-04-20T06:18:02Z

@surajnarwade @procrypt if it's working for you two, let's just assume I've PEBKACed because of my inexperience with Go and that it works for me too, I just can't reproduce it. :)

I've followed the instructions to check out the PR branch locally, did a build and my kompose version still doesn't say HEAD, it states the exact commit SHA1.

Here's what I did:

[dkarlovi@fredo kompose]$ git remote
origin
[dkarlovi@fredo kompose]$ git fetch origin pull/557/head:driver_local
From https://github.com/kubernetes-incubator/kompose
 * [new ref]         refs/pull/557/head -> driver_local
[dkarlovi@fredo kompose]$ git checkout driver_local
Switched to branch 'driver_local'
[dkarlovi@fredo kompose]$ git branch -vv
* driver_local 12808ac vendor_update
  master       46426fb [origin/master] Merge pull request #558 from cdrage/add-dependencies-to-makefile
[dkarlovi@fredo kompose]$ make
go build -ldflags="-w -X github.com/kubernetes-incubator/kompose/cmd.GITCOMMIT=12808ac" -o kompose main.go
[dkarlovi@fredo kompose]$ cd ../kompose_underscore-test/
[dkarlovi@fredo kompose_underscore-test]$ ../kompose/kompose version
0.5.0 (12808ac)
[dkarlovi@fredo kompose_underscore-test]$ ../kompose/kompose convert -f underscore-pvc.yml --stdout | grep PersistentVolumeClaim -A 5
WARN Unsupported root level volumes key - ignoring 
WARN Unsupported depends_on key - ignoring        
WARN Unsupported hostname key - ignoring          
WARN Kubernetes provider doesn't support build key - ignoring 
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: komposeunderscoretest_broker
    name: komposeunderscoretest_broker
--
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: komposeunderscoretest_cache
    name: komposeunderscoretest_cache
--
  kind: PersistentVolumeClaim
  metadata:
    creationTimestamp: null
    labels:
      io.kompose.service: komposeunderscoretest_db
    name: komposeunderscoretest_db

surajssd · 2017-04-21T10:04:53Z

@procrypt why this can't be fixed as it was done in #509 ?

I think here we need to normalize only in https://github.com/kubernetes-incubator/kompose/blob/master/pkg/loader/compose/compose.go#L341 ?

surajssd · 2017-04-21T10:06:03Z

So with this we take care of it at source so we don;t have problems later since the data is scattered in all over the place.

procrypt · 2017-04-21T11:55:13Z

@surajssd Thanks I have updated the PR.

ngtuna

code lgtm. Can you explain the main update in glide @procrypt ?

surajssd · 2017-04-24T12:49:18Z

@procrypt tests, functional ones.

cdrage · 2017-04-24T16:32:20Z

Hey @procrypt ! Almost there. Could you update your commit messages with what you wrote here? Otherwise, code LGTM.

kadel

code lgtm. Can you explain the main update in glide @procrypt ?

Same question here. What was reason for updating those vendored libraries?
Is it required for this to work?

ngtuna · 2017-04-25T04:02:56Z

@procrypt @kadel Can you help to make sure this PR being merged in the upcoming release this week? We have some Bitnami compose files need this fix on this iteration (end by next Thursday). I tested it. The only concern from me is the vendor update.

procrypt · 2017-04-25T05:26:37Z

@cdrage @kadel @ngtuna I don't think vendor update is required so I removed it.

surajnarwade · 2017-04-25T08:07:16Z

@procrypt LGTM :)

cdrage · 2017-04-26T13:50:04Z

Ideally some tests should be added, but I don't see any issues arising because of this.

LGTM.

@kadel Wanna have a quick-lookover this before we merge this in before today's release?

procrypt · 2017-04-26T14:37:23Z

@cdrage Tests added 👍

If we have a docker-file with root level volumes and we do a kompose up using that docker-file, libcompose will add an additional _ followed by the current directory name. Since kubernetes doesn't allow _ in the objects created so kompose up will fail to deploy it. As a solution we replace _ to - and we can then deploy it successfully.

ngtuna · 2017-04-27T08:25:52Z

I am gonna merge this as all reviewers have approved.

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

dkarlovi reviewed Apr 19, 2017

View reviewed changes

procrypt mentioned this pull request Apr 19, 2017

driver:local in prefixing volumes with current dir name #550

Closed

procrypt force-pushed the driver_local branch from 12808ac to 171b5e3 Compare April 21, 2017 11:51

ngtuna reviewed Apr 24, 2017

View reviewed changes

ngtuna approved these changes Apr 24, 2017

View reviewed changes

cdrage approved these changes Apr 24, 2017

View reviewed changes

kadel requested changes Apr 24, 2017

View reviewed changes

procrypt force-pushed the driver_local branch from c0b968f to 00a5350 Compare April 25, 2017 05:25

procrypt force-pushed the driver_local branch from 00a5350 to fd25848 Compare April 25, 2017 05:54

surajnarwade approved these changes Apr 25, 2017

View reviewed changes

procrypt force-pushed the driver_local branch from fd25848 to 7eccb37 Compare April 26, 2017 14:44

cdrage mentioned this pull request Apr 26, 2017

0.6.0 Release #574

Closed

kadel approved these changes Apr 26, 2017

View reviewed changes

ngtuna merged commit 65c9cf9 into kubernetes:master Apr 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix driver:local in prefixing volumes with current dir name #557

fix driver:local in prefixing volumes with current dir name #557

procrypt commented Apr 19, 2017

dkarlovi Apr 19, 2017

dkarlovi commented Apr 19, 2017

dkarlovi commented Apr 19, 2017 •

edited

Loading

procrypt commented Apr 20, 2017 •

edited

Loading

surajnarwade commented Apr 20, 2017

dkarlovi commented Apr 20, 2017

surajssd commented Apr 21, 2017

surajssd commented Apr 21, 2017 •

edited

Loading

procrypt commented Apr 21, 2017

ngtuna left a comment

surajssd commented Apr 24, 2017

cdrage commented Apr 24, 2017

kadel left a comment

ngtuna commented Apr 25, 2017

procrypt commented Apr 25, 2017

surajnarwade commented Apr 25, 2017

cdrage commented Apr 26, 2017

procrypt commented Apr 26, 2017 •

edited

Loading

ngtuna commented Apr 27, 2017

fix driver:local in prefixing volumes with current dir name #557

fix driver:local in prefixing volumes with current dir name #557

Conversation

procrypt commented Apr 19, 2017

dkarlovi Apr 19, 2017

Choose a reason for hiding this comment

dkarlovi commented Apr 19, 2017

dkarlovi commented Apr 19, 2017 • edited Loading

procrypt commented Apr 20, 2017 • edited Loading

surajnarwade commented Apr 20, 2017

dkarlovi commented Apr 20, 2017

surajssd commented Apr 21, 2017

surajssd commented Apr 21, 2017 • edited Loading

procrypt commented Apr 21, 2017

ngtuna left a comment

Choose a reason for hiding this comment

surajssd commented Apr 24, 2017

cdrage commented Apr 24, 2017

kadel left a comment

Choose a reason for hiding this comment

ngtuna commented Apr 25, 2017

procrypt commented Apr 25, 2017

surajnarwade commented Apr 25, 2017

cdrage commented Apr 26, 2017

procrypt commented Apr 26, 2017 • edited Loading

ngtuna commented Apr 27, 2017

dkarlovi commented Apr 19, 2017 •

edited

Loading

procrypt commented Apr 20, 2017 •

edited

Loading

surajssd commented Apr 21, 2017 •

edited

Loading

procrypt commented Apr 26, 2017 •

edited

Loading