Adds a new mechanism to load providers from any gem or file path automatically #263

logicminds · 2018-06-12T00:25:37Z

This extends #181 by making it easier to load providers.

I am just throwing this up here for initial review. There are no unit tests at this point but there are also other things that need to be added before this can be merged.

What this code does is find all gems with providers in 'lib/vmpooler/providers' directories from all gems and loads any file found in that directory. This allows anyone to simply install a new provider gem and start using right away. I would prefer that we only load the provider that is needed instead of everything. However, more work will need to be done. This will make it dead simple to start using external providers since all this is required is too have it installed. This allows anyone to create a provider and keep separate from main gem.

Currently this loads all providers whenever the vmpooler file is loaded. I don't think this is optimal and we should probably only load the provider being used or set in the config file.

# load all providers including third party plugins
require 'vmpooler/providers'
Vmpooler::Providers.load_all_providers

requires #262 (mainly a gemspec file is required) I had to add a temporary gemspec to make this work.

You can test this code out by doing the following:

bundle install
bundle exec pry
1] pry(main)> require 'vmpooler'
Loading file /Users/user1/vmpooler/lib/vmpooler/providers/base.rb
Loading file /Users/user1/vmpooler/lib/vmpooler/providers/dummy.rb
Loading file /Users/user1/vmpooler/lib/vmpooler/providers/vsphere.rb

logicminds · 2018-06-12T00:27:35Z

@glennsarti

glennsarti · 2018-06-12T01:00:31Z

@logicminds Heya Corey... You just want me to review?

logicminds · 2018-06-12T01:12:50Z

Thoughts on using this approach? I use this type of design for retrospec, puppet-debugger and a few other places.

Questions:

Why is a provider also titled pool_manager?
Does lib/pool_manager have anything to do with lib/providers?
Should we load the providers from the pool_manager.rb?
Can the config have more than one provider at a time? (Is this a future feature?)
Where and should we load/use the providers? Seems like we could be lazier when loading things. Possibly lib/pool_manager?

glennsarti · 2018-06-12T01:23:19Z

That approach looks nice

Why is a provider also titled pool_manager?

Not sure I know where in the code that is.

Does lib/pool_manager have anything to do with lib/providers?

The manager uses the providers. Separation of concerns.

The Manager manages providers
The Providers manages actual VMs

Should we load the providers from the pool_manager.rb?

They are used when pool_manager starts up (https://github.com/puppetlabs/vmpooler/blob/master/lib/vmpooler/pool_manager.rb#L788) so yes, you could argue that.

Can the config have more than one provider at a time? (Is this a future feature?)

Yes (not future, but present). And you can have multiple instances of the same provider .e.g Two VSphere installations

Where and should we load/use the providers? Seems like we could be lazier when loading things.

All of the providers are loaded the first time the pool manager runs so laziness won't help too much

logicminds · 2018-07-15T23:28:25Z

I refactored this to only load the providers found in the "config file". This should prevent unwanted things from being loaded.

I also added a short document for creating a provider gem. It is a good start but still needs more work to understand which methods should be implemented in the provider code.

logicminds · 2018-07-15T23:43:49Z

lib/vmpooler/pool_manager.rb

      @vm_mutex = {}
+
+      # load specified providers from config file
+      load_used_providers


All the providers referenced in the config are now loaded here. Previously these were loaded during the loading of the vmpooler file.

logicminds · 2018-07-15T23:45:17Z

@glennsarti This is ready to use now unless anyone has some concerns.

glennsarti · 2018-07-16T00:25:56Z

Looks like a single travis failure.

logicminds · 2018-07-16T16:07:31Z

I updated the dockerfile so I could find this failure. I never found the failure but looks like my change fixed the tests anyway.

mattkirby · 2018-07-16T18:59:50Z

It looks like this may conflict with some changes that #281 is making as well. Specifically, all load path assumptions are being removed so vmpooler can be packaged as a gem. I think it's reasonable to consider being able to dynamically load additional providers, however if they are to be loaded as a separate gem then I think there should be no requirement of them being in the providers directory since when the gem is published there will be no need to run vmpooler from source any longer.

I think it could be worth considering integrating the provider with the vmpooler codebase so it can be loaded into the gem. Alternately, this change could be updated to pull in a gem that is configured and installed.

The Dockerfile is changing significantly in #281 . I think the first failure may have been erroneous. Can you remove the Dockerfile changes since they appear otherwise unrelated?

logicminds · 2018-07-16T19:42:01Z

@mattkirby

 however if they are to be loaded as a separate gem then I think there should be no requirement of them being in the providers directory since when the gem is published there will be no need to run vmpooler from source any longer

In case it was not clear. The providers themselves don't need to be in this repository. But any provider plugin will need to have the same lib/vmpooler/providers directory layout in order for this mechanism to discover it. Otherwise, there is no good way to determine if a plugin is for vmpooler. File based detection is the most efficient way AFAIK. Additionally, if the plugin is supposed to have the namespace of Vmpooler::Providers then it is good ruby practice to put the file in a similar named directory lib/vmpooler/providers/. See #284 for why I don't like the current layout though.

I removed my dockerfile mods.

mattkirby · 2018-07-16T19:43:04Z

@logicminds thanks for the clarification, I think I understand now.

logicminds · 2018-07-16T19:47:03Z

I'll create a example provider repo just in case and attach to my PROVIDER doc.

logicminds · 2018-07-16T20:16:01Z

Here is a example gem provider. https://github.com/logicminds/vmpooler-vsphere-provider

You could also use this as a foundation for the vsphere provider and then remove that provider from this repo.

mattkirby · 2018-07-16T21:01:44Z

lib/vmpooler.rb

  require 'set'

-  %w[api graphite logger pool_manager statsd dummy_statsd generic_connection_pool providers].each do |lib|
+


Nitpick: extra lines added here.

logicminds · 2018-07-17T00:08:16Z

Need to drop support for ruby 2.2 in travis as it is failing my tests. Gemspec says ruby >= 2.3.

@mattkirby do you want me to remove 2.2 from travis file?

mattkirby · 2018-07-17T00:13:12Z

I put up #286 to remove that. I forgot to get that added to the branch that just landed, sorry.

logicminds · 2018-07-17T02:36:58Z

I'll rebase once 285, 286 and 287 are merged.

logicminds · 2018-07-21T20:25:21Z

@mattkirby Is there anything I need to do here?

glennsarti · 2018-07-23T01:50:16Z

PROVIDER_API.md

+2. the main provider code file should be named the same at the name of the provider. ie. (vpshere == lib/vmpoolers/providers/vsphere.rb)
+3. The gem must be installed on the same machine as vmpooler
+4. The provider name must be referenced in the vmpooler config file in order for it to be loaded.
+5. Your gem name or repository name should contain vmppoler-<name>-provider so the community can easily search provider plugins


nit: Typo vmppoler1

glennsarti · 2018-07-23T01:53:27Z

spec/unit/pool_manager_spec.rb

+    EOT
+    )
+    }
+    it do


nit: Would prefer a descriptive name for test

+1 to descriptive name for test, otherwise looks ready to roll.

glennsarti

Apart from some minor edits, I say get this merged ASAP. This is an awesome contributionm with docs no-less!!

mattkirby · 2018-07-16T21:11:31Z

lib/vmpooler/providers.rb

+    #
+    # @return [Array[String]] - a array of provider files
+    # @param name [String] - the name of the provider to load
+    def load_from_gems(name = nil)


I would prefer to see something that loads code from a gem explicitly when the provider is defined in the configuration. For example require 'foo' when 'foo' is configured as a provider for a pool. It seems messy to start having to explore file paths.

This already exists. But the code is in the pool_manager.rb file instead because I need to separate the concerns. The code below is what currently resides in this MR for pool_manager.

# load only providers used in config file def load_used_providers Vmpooler::Providers.load_by_name(used_providers) end # @return [Array] - a list of used providers from the config file, defaults to the default providers # ie. ["vsphere", "dummy"] def used_providers pools = config[:pools] || [] @used_providers ||= (pools.map { |pool| pool[:provider] || pool['provider'] }.compact + default_providers ).uniq end # @param [Array] - returns a list of providers that should always be loaded # note: vsphere is the default if user does not specify although this should not be # if vsphere is to no longer be loaded by default please remove def default_providers @default_providers ||= %w( vsphere dummy ) end

Thanks @logicminds . I'm good with merging this, just have one last nitpick on the test name that is untitled and otherwise this looks ready to roll.

Pretty sure I addressed that already.

it '#correct class' do expect(providers).to be_a Vmpooler::Providers end

In the providers spec I see your example, but the one I was referring to is the one with an in-line comment here https://github.com/puppetlabs/vmpooler/pull/263/files#diff-04094802b5bab73dcebfdd6e91f2b0a1R62

* Adds ability to load only providers used in config file

logicminds force-pushed the providers branch from d8f10a9 to 2fcb0cb Compare July 15, 2018 23:24

logicminds force-pushed the providers branch 3 times, most recently from d0f3680 to 151b13f Compare July 15, 2018 23:42

logicminds commented Jul 15, 2018

View reviewed changes

logicminds force-pushed the providers branch from 151b13f to fabe7da Compare July 16, 2018 15:48

logicminds force-pushed the providers branch from fabe7da to c36649f Compare July 16, 2018 19:26

logicminds force-pushed the providers branch from c36649f to abe30b0 Compare July 16, 2018 19:42

logicminds force-pushed the providers branch from abe30b0 to 181cbe5 Compare July 16, 2018 20:11

mattkirby reviewed Jul 16, 2018

View reviewed changes

logicminds force-pushed the providers branch 3 times, most recently from 7e249b5 to d000857 Compare July 16, 2018 23:58

logicminds force-pushed the providers branch from d000857 to d4a5098 Compare July 19, 2018 16:59

logicminds force-pushed the providers branch 2 times, most recently from 34c35ad to 507dc58 Compare July 20, 2018 22:28

glennsarti reviewed Jul 23, 2018

View reviewed changes

glennsarti approved these changes Jul 23, 2018

View reviewed changes

logicminds force-pushed the providers branch from 507dc58 to 39ffab0 Compare July 23, 2018 07:03

mattkirby approved these changes Jul 23, 2018

View reviewed changes

Adds a new mechanism to load providers from any gem or file path.

66324f9

* Adds ability to load only providers used in config file

logicminds force-pushed the providers branch from 39ffab0 to 66324f9 Compare July 24, 2018 04:41

Merge branch 'master' into providers

c9c97e1

mattkirby merged commit 2daa524 into puppetlabs:master Jul 24, 2018

		require 'set'

		%w[api graphite logger pool_manager statsd dummy_statsd generic_connection_pool providers].each do \|lib\|

Adds a new mechanism to load providers from any gem or file path automatically #263

Adds a new mechanism to load providers from any gem or file path automatically #263

Uh oh!

Conversation

logicminds commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

logicminds commented Jun 12, 2018

Uh oh!

glennsarti commented Jun 12, 2018

Uh oh!

logicminds commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennsarti commented Jun 12, 2018

Uh oh!

logicminds commented Jul 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logicminds commented Jul 15, 2018

Uh oh!

glennsarti commented Jul 16, 2018

Uh oh!

logicminds commented Jul 16, 2018

Uh oh!

mattkirby commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

logicminds commented Jul 16, 2018

Uh oh!

mattkirby commented Jul 16, 2018

Uh oh!

logicminds commented Jul 16, 2018

Uh oh!

logicminds commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logicminds commented Jul 17, 2018

Uh oh!

mattkirby commented Jul 17, 2018

Uh oh!

logicminds commented Jul 17, 2018

Uh oh!

logicminds commented Jul 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glennsarti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

logicminds commented Jun 12, 2018 •

edited

Loading

logicminds commented Jun 12, 2018 •

edited

Loading

mattkirby commented Jul 16, 2018 •

edited

Loading

logicminds commented Jul 16, 2018 •

edited

Loading