Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add label 'kompose.controller.type' set service convert controller type #1001

Merged
merged 1 commit into from
May 13, 2018

Conversation

xianlubird
Copy link
Contributor

A standard wordpress compose like this

web:
  image: wordpress:4.5
  ports:
    - '80'
  environment:
    WORDPRESS_AUTH_KEY: changeme
    WORDPRESS_SECURE_AUTH_KEY: changeme
    WORDPRESS_LOGGED_IN_KEY: changeme
    WORDPRESS_NONCE_KEY: changeme
    WORDPRESS_AUTH_SALT: changeme
    WORDPRESS_SECURE_AUTH_SALT: changeme
    WORDPRESS_LOGGED_IN_SALT: changeme
    WORDPRESS_NONCE_SALT: changeme
    WORDPRESS_NONCE_AA: changeme
  restart: always
  links:
    - 'db:mysql'
db:
  image: mysql:5.7
  environment:
    MYSQL_ROOT_PASSWORD: password
  restart: always
  labels:
    project.logs: /var/log/mysql

For service db, we'd better convert it to StatefulSet. But if we run command kompose convert -f , all of them are converted to Deployment. We have to split them into two compose file.So we'd better support a label which can control what type you want to convert.

After that, a compose like this

web:
  image: wordpress:4.5
  ports:
    - '80'
  environment:
    WORDPRESS_AUTH_KEY: changeme
    WORDPRESS_SECURE_AUTH_KEY: changeme
    WORDPRESS_LOGGED_IN_KEY: changeme
    WORDPRESS_NONCE_KEY: changeme
    WORDPRESS_AUTH_SALT: changeme
    WORDPRESS_SECURE_AUTH_SALT: changeme
    WORDPRESS_LOGGED_IN_SALT: changeme
    WORDPRESS_NONCE_SALT: changeme
    WORDPRESS_NONCE_AA: changeme
  restart: always
  links:
    - 'db:mysql'
db:
  image: mysql:5.7
  environment:
    MYSQL_ROOT_PASSWORD: password
  restart: always
  labels:
    project.logs: /var/log/mysql
    kompose.controller.type: daemonset

So service web convert to Deployment, service db convert to StatefulSet, it's very useful.

What's your opinion, @cdrage @hangyan

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 7, 2018
@xianlubird xianlubird force-pushed the features/controller-label branch from 45ac711 to 806b05d Compare May 7, 2018 11:59
@xianlubird
Copy link
Contributor Author

xianlubird commented May 8, 2018

@cdrage @hangyan If you have time, pls review this. Thanks.

@cdrage
Copy link
Member

cdrage commented May 8, 2018

I fully support this. However, a few more things need to be added:

  • Documentation for StatefulSet
  • A new flag should be added (to --controller!)

This will close a bunch of issues. Specifically, the one regarding #698 !

@xianlubird xianlubird force-pushed the features/controller-label branch 2 times, most recently from 9d41e06 to 2d9bd6d Compare May 9, 2018 02:26
@xianlubird
Copy link
Contributor Author

@cdrage @hangyan

Thanks for your review.

  • Already add document for label kompose.controller.type
  • For a new flag , now we already have --controller flag and support three types, run kompose convert --help can see it
./kompose convert --help
Convert a Docker Compose file

Usage:
  kompose convert [file] [flags]

--controller string   Set the output controller ("deployment"|"daemonSet"|"replicationController")

This pr support that ,user can use kompose convert -f test.yml. If a service have label kompose.controller.type, it will be converted to the type which use specified, for other service in the same compose file, it will convert to default controller type or user specified type in --controller xxxxx.

@xianlubird xianlubird force-pushed the features/controller-label branch from 2d9bd6d to de5b565 Compare May 9, 2018 02:40
@xianlubird
Copy link
Contributor Author

@cdrage any update?

@cdrage
Copy link
Member

cdrage commented May 10, 2018

Hey @xianlubird I'll find some time this week / early next week to do another review 👍 Maybe @hangyan could do one on this PR and #994 (comment) ?

Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

I don't think you're implementing statefulset at all? You're only adding the new label type. Bit confused what you're trying to accomplish here.

@@ -301,6 +301,7 @@ The currently supported options are:
| kompose.service.expose | true / hostname |
| kompose.service.expose.tls-secret | secret name |
| kompose.volume.size | kubernetes supported volume size |
| kompose.controller.type | deployment / daemonset / replicationcontroller |
Copy link
Member

Choose a reason for hiding this comment

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

aren't you missing statefulset?

kompose.controller.type: daemonset
```

Service `web` will be converted to `Deployment` as default, service `db` will be converted to `StatefulSet` because of `kompose.controller.type` label.
Copy link
Member

Choose a reason for hiding this comment

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

this is totally wrong... if you specify kompose.controller.type daemonset, this should be converted to daemonset... if you supply statefulset, this should be converted to statefulset..

const (
DeploymentController = "deployment"
DaemonSetController = "daemonset"
ReplicationController = "replicationcontroller"
Copy link
Member

Choose a reason for hiding this comment

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

add statefulset

@xianlubird
Copy link
Contributor Author

@cdrage Yes, I make a mistake for DaemonSet and StatefulSet. This pr , I just want to add a label for that different service in one yaml file can convert to different k8s type. For this purpose, I raise this pr.

So in your opinion, I should implement StatefulSet in this PR? Or just add a label for deployment and DaemonSet and implement StatefulSet in another PR ?

many thanks.

@cdrage
Copy link
Member

cdrage commented May 10, 2018

Hey @xianlubird

For now, just implement the label, but please correct that daemonset will be converted to daemonset.

We should raise another PR in the future for adding statefulset (see issue: #698 )

@xianlubird xianlubird force-pushed the features/controller-label branch from de5b565 to 4764041 Compare May 11, 2018 02:16
@xianlubird
Copy link
Contributor Author

@cdrage
Already change document to DaemonSet.
I will raise another PR to implement StatefulSet.
Thanks for your review.

@cdrage
Copy link
Member

cdrage commented May 11, 2018

This LGTM. I'd like @kadel or @hangyan to review this too since it's fairly large.

@hangyan
Copy link
Contributor

hangyan commented May 11, 2018

@xianlubird Please add OpenShift test files

@xianlubird xianlubird force-pushed the features/controller-label branch from 4764041 to a4efdd6 Compare May 12, 2018 03:52
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 12, 2018
@xianlubird
Copy link
Contributor Author

@hangyan Already add OpenShift test case, thanks.

@xianlubird
Copy link
Contributor Author

@kadel @hangyan Any update on this.

@hangyan
Copy link
Contributor

hangyan commented May 13, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2018
@hangyan hangyan merged commit 7ba07b5 into kubernetes:master May 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants