Prior to this, the `pool_manager.rb` library could take handles for both
graphite and statsd endpoints (which were considered mutually exclusive),
and then would use one. There was a bevy of conditional logic around sending
metrics to the graphite/statsd handles (and actually at least one bug of
omission).
Here we refactor more, building on earlier work:
- Our graphite class comes into line with the API of our Statsd and DummyStatsd classes
- In `pool_manager.rb` we now accept a single "metrics" handle, and we drop all the conditional logic around statsd vs. graphite
- We move the inconsistent error handling out of the calling classes and into our metrics classes, actually logging to `$stderr` when we can't publish metrics
- We unify the setup code to use `config` to determine whether statsd, graphite, or a dummy metrics handle should be used, and make that happen.
- Cleaned up some tests. We could probably stand to do a bit more work in this area.
Prior to this we could easily run into situations where `statds_prefix` would
be `nil` (and possibly the `statsd` handle itself). There was some significant
complexity and brittleness in how statsd was set up.
Refactored so that:
- `statsd_prefix` is no longer exposed to any callers of statsd methods
- there is now a `Vmpooler::DummyStatsd` class which can be returned when we are not actually going to publish stats, but would like to keep the calling interface consistent
- setup of the statsd handle is via just passing in `config[:statsd]`, if `nil`, this will result in a dummy handle being return
- defaulting of `server` values was fixed -- this did not actually work in the previous implementation. `config[:statsd][:server]` is now required.
- tests use a `DummyStatsd` instance instead of an rspec double.
- calls to `statsd.increment` were taking incorrect arguments (some our fault, some part of the prior implementation), and were not collecting data on which pools were "invalid" or "empty". Fixed this and are now explicitly tracking the invalid/empty pool names.
There were several problems with how the pooler checked out vms with
respect to empty pools, invalid pools, and aliases:
- If the vmpooler config did not contain any aliases and the caller
requested a vm from an empty pool or a non-existent one, the vmpooler
would error with:
NoMethodError - undefined method `[]' for nil:NilClass
If the config contained a non-nil alias section, then:
- If the caller requested a vm from an empty pool and either the vm
didn't have an alias or the aliased pool was empty or non-existent, then
the request for that vm would be silently ignored. The vmpooler would
return 200 if the caller asked for multiple vms and the vmpooler was
able to checkout at least one vm. Otherwise it would return 404.
- Similarly, if the caller requested a vm from a non-existent pool, then
the request was silently ignored.
This commit adds a `pool_names` Set to the config containing all valid
pool names including aliases. This is used to determine whether a
requested template name is valid or not. This is necessary because redis
does not distinguish between empty and non-existent sets, e.g. the
following returns false in both cases:
backend.exists('vmpooler__ready__' + key)
If the caller requests a vm (single or multiple), and any vm references
an invalid pool name, we immediately return 404. Otherwise, we know the
request is for valid pool names, since the vmpooler requires a restart
to change pool names and counts.
We then attempt to acquire each vm, trying to match on pool name or
failing back to aliased pool name, as was the previous behavior.
The resulting behavior is:
- If the caller asks for at least one vm from an unknown pool, then
don't try to checkout any vms and respond with 404.
- If the caller asks for a vm, and at least one pool is empty, then
respond with 503, returning checked out vms back to the pool.
- Otherwise return 200 with the list of checked out vms.
This commit also makes `alias` optional again.
This commit also re-enables tests that were merged in from master, but
originally commented out due to the bugs described above..
They way we were using graphite was incorrect for the type of data we were sending it. statsd is the appropriate mechanism for our needs.
statsd and graphite are mutually exclusive and configuring statsd will take precendence over Graphite. Example of configuration in vmpooler.yaml.example
The following pool configuration would allow a pool to be aliased in POST
requests as 'centos-6-x86_64', 'centos-6-amd64', or 'centos-6-64':
````yaml
- name: 'centos-6-x86_64'
alias: [ 'centos-6-amd64', 'centos-6-64' ]
template: 'templates/centos-6-x86_64'
folder: 'vmpooler/centos-6-x86_64'
datastore: 'instance1'
size: 5
````
The 'alias' configuration can be either a string or an array.
Note that even when requesting an alias, the pool's 'name' is returned in
the JSON response:
````
$ curl -d '{"centos-6-64":"1"}' --url vmpooler/api/v1/vm
````
````json
{
"ok": true,
"centos-6-x86_64": {
"hostname": "cuna2qeahwlzji7"
},
"domain": "company.com"
}
````
Prior to this commit, several pieces of vmpooler performed configuration
and initialization steps within 'initialize'. This made it difficult to
pass in mock objects for testing purposes.
This commit performs a single configuration and passes the results to
the various pieces of vmpooler.
