Config + hook approach for restarting docker #54

coolbrg · 2016-02-17T05:30:03Z

Fix: #12 and #21.

With this approach Vagrantfile will look like

 Vagrant.configure(2) do |config|
  config.vm.box = "projectatomic/adb"
  config.vm.network "private_network", type: "dhcp"
  config.servicemanager.providers = 'docker'
end

bexelbie · 2016-02-17T09:23:23Z

README.md

+
+      config.vm.box = "projectatomic/adb"
+
+      config.vm.network "private_network", type: "dhcp"


This example should probably be the only line cited.

The Vagrantfile should contain comments with the list and format.

bexelbie · 2016-02-17T09:26:34Z

Can we also get a changelog modification? Can you build a test ADB and gem that we can send out for use as a "nightly"?

LalatenduMohanty · 2016-02-19T05:33:54Z

We should squash the commits in to one.

bexelbie · 2016-02-23T11:16:08Z

Vagrantfile

@@ -1,9 +1,20 @@
 # -*- mode: ruby -*-
 # vi: set ft=ruby :

+unless Vagrant.has_plugin?("vagrant-service-manager")
+  raise <<-MSG


Can you examine this code relative to this: projectatomic/adb-atomic-developer-bundle#245

Can we do a cleaner error and exit?

I'd still like to see this as a print to STDERR and exit combination.

coolbrg · 2016-02-23T12:44:38Z

@LalatenduMohanty done with sqashing commit.

coolbrg · 2016-02-23T12:45:41Z

@bexelbie,

Latest changes includes provider centric service operation.

coolbrg · 2016-02-23T12:48:11Z

lib/vagrant-service-manager/config.rb

+      def validate_providers
+        errors = []
+
+        if !Set[@providers.split(',').map(&:strip)].subset?(Set[PROVIDERS])


If specified provider(s) in configuration is not listed in PROVIDERS then it errors out.

hferentschik · 2016-02-24T11:43:01Z

Vagrantfile

  config.vm.box = "projectatomic/adb"

  config.vm.network "private_network", type: "dhcp"

+  # enable multiple providers as comma separated list. Eg: 'docker, openshift'
+  config.servicemanager.providers = 'docker'


why the need for explicitly enabling this? Does this also mean that the docker certificates are only generated if the docker provider is set? They should really be generated regardless.

The aim is zero conf Vagrantfile. I think this config should not be required.

Does this also mean that the docker certificates are only generated if the docker provider is set? They should really be generated regardless.

I agree.

The aim is zero conf Vagrantfile. I think this config should not be required.

For docker provider yes, for other providers we may need config in Vagrantfile.

For docker provider yes, for other providers we may need config in Vagrantfile.

Ok, seems reasonable. We seem to agree the proper configuration of Docker is a given. There is no need to explicitly configure it. What other providers are there? Kubernetes vs OpenShift? Is really OpenShift not a super-set of Kubernetes here? Also OpenShift is the default. It could be generated unless something else is explicitly configured. Again, the aim is zero config Vagrantfile. Let's discuss the need for configuring other providers, but Docker should just work.

What other providers are there?

Apart from kube and openshift, Mesos-Marathon

Does this also mean that the docker certificates are only generated if the docker provider is set? They should really be generated regardless.

Need to check if the approach we are following (config + hook) can trigger task of configuring-docker without any mention of config in Vagrantfile explicitly.

Does this also mean that the docker certificates are only generated if the docker provider is set? They should really be generated regardless.

@navidshaikh , @hferentschik
It is working. 😄 .
Just omit config line from Vagrantfile. This line is doing the trick for it.

@budhrg

It is working. 😄 .
Just omit config line from Vagrantfile. This line is doing the trick for it.

Excellent!

bexelbie · 2016-02-24T13:28:51Z

Should execute_docker_info in lib/vagrant-service-manager/command.rb actually be in lib/vagrant-service-manager/providers/docker.rb ?

bexelbie · 2016-02-24T13:29:21Z

lib/vagrant-service-manager/plugin.rb

@@ -1,3 +1,5 @@
+require_relative 'providers/docker'


Can we require_relative 'providers/*' to eliminate the need to edit this file when adding new services?

Also, should this be called services/ ?

@bexelbie , @navidshaikh :
Would it better if we use consistent naming? I would like to agree with the term service.
This line will be require_relative 'services/docker' or
Dir.glob('lib/*.rb') { |f| require_relative f }(best I could) for automatically adding new services.

Also, Vagrantfile line as:
config.servicemanager.services = 'openshift'(docker default hence this line will not be needed).

And class name Provider to Service.

+1 service(s) - it really opens up the options for this plugin and makes the language so much better

+1 Service is the better term. I think we need to aim for a consistent terminology not only in this plugin, but across the whole ADB/CDK. "The CDK offers a OpenShift service" or "To enable the Kubernetes service ...", etc

bexelbie · 2016-02-24T13:32:09Z

Can we somehow validate the commands in vagrant-service-manager/command.rb by using what is provided in the ruby file in providers?

coolbrg · 2016-02-24T14:34:49Z

Should execute_docker_info in lib/vagrant-service-manager/command.rb actually be in lib/vagrant-service-manager/providers/docker.rb ?

@bexelbie , @LalatenduMohanty , @navidshaikh : Let's discuss such changes as part of #49 . There are other more things I need to discuss as part of refactoring.

bexelbie · 2016-02-25T15:35:14Z

@budhrg Can you think about this idea, even if it comes in a separate patch.

If projectatomic/adb-utils#45 merges, it would be nice to be able to do this in the vagrant file

config.vm.service-manager.openshift.image_name = "openshift/origin"
config.vm.service-manager.openshift.image_tag = "latest"
config.vm.service-manager.openshift.docker_registry = "docker.io"

And have those values available for a later provisioner shell call.

bonus points if the presence of any those caused to assume this line:

config.vm.service-manager.provision = 'openshift'

hferentschik · 2016-02-25T19:07:19Z

Awesome. Now we start talking. This is exactly what I had in mind. The service-manager basically offering nice and simple extension/config options to handle some of these default things. Much better than fiddling around with sed and co in some shell provisioning script. It would be awesome, if we could get this all sorted for GA. Also needs of course be well documented.

coolbrg · 2016-02-26T04:50:26Z

@bexelbie , @hferentschik :

config.vm.service-manager.openshift.image_name = "openshift/origin"
config.vm.service-manager.openshift.image_tag = "latest"
config.vm.service-manager.openshift.docker_registry = "docker.io"
....... or more such lines possibly ....

How about this?

config.servicemanager.provision(or service) = 'openshift'   # enable openshift
config.servicemanager.service.openshift = { image: "openshift/origin", tag: "latest", docker_registry: "docker.io", (other values too...)}
# above values will be available to later provisioners

Above lines are just imaginary which I think possible and would be reference for my working.
Please correct me if I interpreted it differently.

bexelbie · 2016-02-26T05:21:44Z

@budhrg I'm not a vagrant expert but it feels like making the user write json in the Vagrantfile is weird and an anti-pattern. The other feels cleaner to me. Why are you suggesting this?

coolbrg · 2016-02-26T06:12:50Z

@bexelbie Oh ok. I thought it would be only one line than having multiple lines.
Also agree that multi-line approach would be far more comfortable for users than json which would be syntax error prone.
Thanks for clarification.

hferentschik · 2016-02-26T09:24:45Z

Also agree that multi-line approach would be far more comfortable for users than json

+1 We basically offer one option for everything we think is relevant and should be configurable.

coolbrg · 2016-02-26T09:28:47Z

@bexelbie

Can we somehow validate the commands in vagrant-service-manager/command.rb by using what is provided in the ruby file in providers?

Need more info about it. Could you elaborate it little bit?

LalatenduMohanty · 2016-02-26T09:38:33Z

Let's discuss such changes as part of #49 . There are other more things I need to discuss as part of refactoring.

Agree. We might want to do that in a separate PR

LalatenduMohanty · 2016-02-26T09:41:27Z

@budhrg I'm not a vagrant expert but it feels like making the user write json in the Vagrantfile is weird and an anti-pattern.

+1. We dont need json kind of structure

bexelbie · 2016-02-26T10:01:30Z

@budhrg What I mean here is that it would be nice if the service could define what commands it could interact with. So for example if the foo service doesn't have an env verb then 'env foo' fails but we don't have to create a command table and order to know this. I suspect the word introspection somehow comes into play here.

coolbrg · 2016-02-26T10:14:29Z

+1 for the idea 😄 .
@bexelbie - This could be possible with Ruby Meta-programming(introspection) as what I could infer from your comment. Let's discuss it in the issue #79(Service driven command structure) as it has to be implemented well as it includes metaprogramming.*

Power comes with great responsibility

coolbrg · 2016-02-26T12:54:57Z

@bexelbie , @LalatenduMohanty , @navidshaikh - Done with suggested changes from my side.

bexelbie · 2016-02-26T14:21:33Z

README.md


        vagrant plugin install vagrant-service-manager

-2. Get the [Vagrantfile](Vagrantfile) and start the ADB using `vagrant up`. [More](https://github.com/projectatomic/adb-atomic-developer-bundle/blob/master/docs/installing.rst) documentation on setting up ADB.
+2. Get the [Vagrantfile](Vagrantfile).


This needs to be a reference to the ADB repo. The Vagrantfile in this repo is for testing and should possibly be deleted or augmented.

I suggest:

Get a Vagrantfile for your box. Users of the Atomic Developer Bundle (ADB) should download a Vagrantfile from the repository.

bexelbie · 2016-02-26T14:52:26Z

I added some line notes.

navidshaikh · 2016-02-29T12:18:09Z

If we are looking for finding out the box OS, in case of non supported boxes, we should just return silently without any trace on the normal execution flow of vagrant up.

This should help user have the plugin installed and not having any interception while working with non-supported boxes.

coolbrg · 2016-02-29T12:50:06Z

@LalatenduMohanty , @navidshaikh , @bexelbie updated with changes. Now only adb and cdk boxes will run this plugin.

Also tested with "cdk" with box rhel-cdk-kubernetes-7.2-14.

navidshaikh · 2016-02-29T14:10:46Z

lib/vagrant-service-manager/services/docker.rb

+            @machine.communicate.execute(command) do |type, data|
+              if type == :stderr
+                @ui.error(data)
+                exit 126


navidshaikh · 2016-02-29T14:12:30Z

@LalatenduMohanty : Once you have tested the PR against non supported box, can you give feedback ?

navidshaikh · 2016-03-01T09:20:09Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Changelog

+## Unreleased
+- Fix #12 and #21: Restart docker service on 'vagrant up' @budhrg


Kindly put this changelog on v0.0.3 release

navidshaikh · 2016-03-01T09:22:10Z

I tested this PR against projectatomic/adb and fedora/23-cloud-base for ensuring it identifies between supported and non supported boxes, works fine for me.

navidshaikh · 2016-03-01T09:34:14Z

LGTM!

* Updated README and CHANGELOG * Checks dependency for vagrant-service-manager plugin in Vagrantfile * Cleaner error and exit if plugin not found * Added guest capability to know the box version(adb/cdk)

bexelbie · 2016-03-01T16:08:25Z

lib/vagrant-service-manager/services/docker.rb

+        end
+
+        def call(env)
+          if SUPPORTED_BOXES.include? @machine.guest.capability(:flavor)


Why is this inside of the docker hook and not at a higher level? This structure means that every time we add a new service we have to write this same code again.

I am going to LGTM this, but I am opening an issue to get this fixed as we need to get openshift, etc. added.

Agreed @bexelbie . Will work on it.

bexelbie · 2016-03-01T16:09:32Z

LGTM with new issue coming

Config + hook approach for restarting docker

coolbrg mentioned this pull request Feb 17, 2016

Config + hook to restart docker on 'vagrant up' #51

Closed

bexelbie reviewed Feb 17, 2016
View reviewed changes

bexelbie reviewed Feb 23, 2016
View reviewed changes

coolbrg reviewed Feb 23, 2016
View reviewed changes

coolbrg mentioned this pull request Feb 23, 2016

Code refactoring #49

Closed

hferentschik reviewed Feb 24, 2016
View reviewed changes

bexelbie reviewed Feb 24, 2016
View reviewed changes

coolbrg mentioned this pull request Feb 26, 2016

Service driven command structure #79

Closed

bexelbie reviewed Feb 26, 2016
View reviewed changes

navidshaikh reviewed Feb 29, 2016
View reviewed changes

navidshaikh reviewed Mar 1, 2016
View reviewed changes

navidshaikh mentioned this pull request Mar 1, 2016

vagrant-service-manager plugin version 0.0.3 release #74

Closed

Restart docker service on 'vagrant up'

3e78c39

* Updated README and CHANGELOG * Checks dependency for vagrant-service-manager plugin in Vagrantfile * Cleaner error and exit if plugin not found * Added guest capability to know the box version(adb/cdk)

bexelbie reviewed Mar 1, 2016
View reviewed changes

bexelbie added a commit that referenced this pull request Mar 1, 2016

Merge pull request #54 from budhrg/config-hook

1e15105

Config + hook approach for restarting docker

bexelbie merged commit 1e15105 into projectatomic:master Mar 1, 2016


		config.vm.box = "projectatomic/adb"

		config.vm.network "private_network", type: "dhcp"

Config + hook approach for restarting docker #54

Config + hook approach for restarting docker #54

Conversation

coolbrg commented Feb 17, 2016

Choose a reason for hiding this comment

bexelbie commented Feb 17, 2016

LalatenduMohanty commented Feb 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coolbrg commented Feb 23, 2016

coolbrg commented Feb 23, 2016

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

bexelbie commented Feb 24, 2016

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

bexelbie commented Feb 24, 2016

coolbrg commented Feb 24, 2016

bexelbie commented Feb 25, 2016

hferentschik commented Feb 25, 2016

coolbrg commented Feb 26, 2016

bexelbie commented Feb 26, 2016

coolbrg commented Feb 26, 2016

hferentschik commented Feb 26, 2016

coolbrg commented Feb 26, 2016

LalatenduMohanty commented Feb 26, 2016

LalatenduMohanty commented Feb 26, 2016

bexelbie commented Feb 26, 2016

coolbrg commented Feb 26, 2016

coolbrg commented Feb 26, 2016

Choose a reason for hiding this comment

bexelbie commented Feb 26, 2016

navidshaikh commented Feb 29, 2016

coolbrg commented Feb 29, 2016

Choose a reason for hiding this comment

navidshaikh commented Feb 29, 2016

Choose a reason for hiding this comment

navidshaikh commented Mar 1, 2016

navidshaikh commented Mar 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bexelbie commented Mar 1, 2016