[QENG-3919] Make vmpooler checkouts be all or nothing #153
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Do we consider this an API v2 type thing? |
|
@shermdog my understanding is that this is addressing a bug in the v1 API so I don't think so. |
|
Does this deprecate the use of fetching vms through query params like what was mentioned in the ticket, or no? I don't see any spec tests around that, just fetching vms through a request body. |
|
Good eye. I think we should probably bolster tests around that query params On Mon, May 23, 2016 at 4:56 PM Brian Cain notifications@github.com wrote:
|
|
Note to self: also, double-check the docs to make sure they're clear about the intended behavior. |
This exposes the old (bad) behavior on this other code path. Will fix this up next.
Not clear to me why these had to be implemented so differently.
I mean, I presume all these commits are going to get squashed away on merge anyway.
We consider it a bug that the actual behavior was not this behavior, but the documentation was also silent on this point.
|
Would actually be good to get another look at this, as I've added tests for and fixed the second code path, refactored some things and tweaked the docs. |
| account_for_starting_vm(template, vm) | ||
| update_result_hosts(result, template, vm) | ||
| else | ||
| status 503 |
There was a problem hiding this comment.
In the specs I see a comment that says "which HTTP status code?" on tests that seem (by the name) to be expecting failure and I also see http = 200 in the expect_json call. Should that be http = 503?
There was a problem hiding this comment.
I added those comments and will clean them up (thanks for the reminder) ... It looks like vmpooler has always returned a 200 status, even when it cannot allocate vm's, using return['ok'] as the indicator of whether the allocation was ok. The 503 status is for cases where no templates were specified.
I don't 100% agree with the way it's using status + result['ok'], but I'm going to leave it alone, otherwise this would be a breaking change for folks.
There was a problem hiding this comment.
Dropped the comments in question after another round of verifying behavior.
We kept this up-to-date while we were upgrading and refactoring, but, turns out, this method is no longer called anywhere. 💀 🔥
|
Going to go over the return status stuff one more time. I think I dropped the '503' on the floor in this change and want to make sure I get these right. |
Making sure we go back to the original functionality, which was: - status 200 when vms successfully allocated - status 404 when a pool name is unknown - status 404 when no pool name is specified - status 503 when vm allocation failed
Maybe we shouldn't foil-ball gems onto servers.
And hence we see once again the weakness of mockist tests.
|
Trivia fun facts:
Testing results on vmpooler-dev: |
|
@rick it seems like those development setup gotchas should be documented somewhere, should we include that in this PR or file a ticket to write up a more thorough guide to developing on vmpooler? |
|
@waynr yeah, no doubt. There's a part of it (maybe specific examples) that are pretty specific to our internal network ("here, do this to grab a db dump and throw it in your local db, and then make a token and do this"), so perhaps for some of our internal wiki docs we get really specific, and for vmpooler in general we document the more general process. |
|
As an aside, after testing this in dev I think this is good to 🚢, but one last 👀 wouldn't hurt. |
|
(this is also a great candidate for github's new squash merge) 🎉 |
| @@ -6,6 +6,7 @@ gem 'rake', '>= 10.4' | |||
| gem 'rbvmomi', '>= 1.8' | |||
| gem 'redis', '>= 3.2' | |||
| gem 'sinatra', '>= 1.4' | |||
| gem 'net-ldap', '= 0.11' | |||
There was a problem hiding this comment.
cc @shermdog (hard-wired to make travis happy on jruby, etc.)
There was a problem hiding this comment.
Let's not hardwire as we do have some customers using this tool.
|
Looks good to me! 👍 |
| backend.spop('vmpooler__ready__' + template) | ||
| end | ||
|
|
||
| def return_single_vm(template, vm) |
There was a problem hiding this comment.
This confused me at first - could we rename it to something like re_ready_single_vm or something along those lines?
shermdog
added a commit
that referenced
this pull request
Jul 25, 2016
* [QENG-3919] Make vmpooler checkouts be all or nothing (#153) * (QENG-3919) spike for implementation of all-or-nothing checkout * Fix two botched variable references * Aggregate API helper methods * Add specs for failed multi-vm allocation API endpoints * (QENG-3919) Add tests for multiple vm requests * (QENG-3919) Add (failing) specs for POST /vm/pool1+pool2 usages This exposes the old (bad) behavior on this other code path. Will fix this up next. * (QENG-3919) Bring query params version in line with JSON post version Not clear to me why these had to be implemented so differently. * (QENG-3919) extract common method from both methods of VM allocation * (QENG-3919) Naming fix, cosmetic cleanups I mean, I presume all these commits are going to get squashed away on merge anyway. * (QENG-3919) Update API docs We consider it a bug that the actual behavior was not this behavior, but the documentation was also silent on this point. * (QENG-3919) minor readability tweak in refactored method * (QENG-3919) Clean up interim comments re: status codes * (QENG-3919) Drop now-orphaned `checkout_vm` method We kept this up-to-date while we were upgrading and refactoring, but, turns out, this method is no longer called anywhere. 💀 🔥 * (QENG-3919) Return 503 status on failed allocation Making sure we go back to the original functionality, which was: - status 200 when vms successfully allocated - status 404 when a pool name is unknown - status 404 when no pool name is specified - status 503 when vm allocation failed * (QENG-3919) add net-ldap to Gemfile Maybe we shouldn't foil-ball gems onto servers. * (QENG-3919) Turns out, spush isn't a redis command And hence we see once again the weakness of mockist tests. * (QENG-3919) Pin the net-ldap gem to 0.11 for the jrubies, etc. * (QENG-3919) Correct an old spelling error in spec descriptions * (QENG-3919) Further tweak net-ldap version * (QENG-3919) return_single_vm -> return_vm_to_ready_state cc @shermdog * (RE-7014) Add support for statsd 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 * (RE-7014) Add tracking of vm gets via statsd Add the tracking of successful, failed, invalid, and empty pool vm gets. It is possible we may want to tweak this, but have validated with spec tests and pcaps. ``` vmpooler-tmp-dev.ready.debian-7-x86_64:1|c vmpooler-tmp-dev.running.debian-7-x86_64:1|c vmpooler-tmp-dev.checkout.invalid:1|c vmpooler-tmp-dev.checkout.success.debian-7-x86_64:1|c vmpooler-tmp-dev.checkout.empty:1|c vmpooler-tmp-dev.running.debian-7-x86_64:1|c vmpooler-tmp-dev.clone.debian-7-x86_64:12.10|ms vmpooler-tmp-dev.ready.debian-7-x86_64:1|c ``` * (RE-7014) statsd nitpicks and additional rspec Cleaned up some code review nitpicks and added pool_manager_spec for empty pool. * (RE-7014) update statsd to use gauge for running/ready Previously was using increment which was incorrect for that particular application. * Revert "Merge pull request #155 from shermdog/RE-7014-cinext" This reverts commit cc03a86, reversing changes made to 5aaab7c. * (QENG-4070) Consistently return 503 if valid pool is empty 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.. * (maint) Add json pessimistic pin json 2.0.x was released on July 1 and is not compatible with ruby < 2.0. Since we still support that version, add a pessimistic pin, which is what we were using prior to July 1. * [QENG-4070] Make json version conditional on RUBY_VERSION * Drop extraneous mocks from updated test * Revert "Revert "Merge pull request #155 from shermdog/RE-7014-cinext"" This reverts commit 0fd6fff. * Fix some spec errors These were caused in part by dropping changes from the original PR when we dropped the v1_spec.rb master test file (in favor of the updated and separated versions). * [QENG-4075] Fix bug with template name on allocation failure We're returning [nil,nil] in this case, meaning that name will not be set. This means we'll get an error trying to concatenate the stats string. Use the requested template name here instead. * [QENG-4075] Refactor statsd methods / classes 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. * [QENG-4075] Drop now-superfluous :statsd config defaulting * [QENG-4075] Unify graphite and statsd for the pool manager 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. * [QENG-4075] Clean up pool manager, specs Prior to this, `pool_manager.rb` allowed the `metrics` argument to be optional, but at this point it will be an instance of `Vmpooler::Statsd`, 'Vmpooler::Graphite', or `Vmpooler::DummyStatsd`, so making this non-optional. Cleaned up that file's tests, cosmetically, as well as recognizing that the behavioral difference between graphite and statsd does not depend on the pool manager. * [QENG-4075] update example vmpooler.yaml file This documents the changes to :server being mandatory for all metrics endpoints, as well as the graphite endpoint supporting an optional :port configuration value. * [QENG-4075] Rename usages of statsd -> metrics Really, let's just support a generic metrics interface. * (maint) move statsd-ruby require into Vmpooler::Statsd class We've managed to move mentions of this out of the calling code, so let's move the require. * (maint) metrics.log -> metrics.timing We missed this during the refactoring. Bringing this up to date. * [QENG-4075] Allow specifying 'graphs:' for dashboard Prior to this the dashboard front-end would use the configuration settings for `graphite[:server]`/`graphite[:prefix]` to locate a graphite server to use for rendering graphs. Now that we have multiple possible metrics backends, the front-end graph host for the dashboard could be entirely different from the back-end metrics server that we publish to (if any). This decouples those settings: - use `graphs[:server]` / `graphs[:prefix]` for the graphite-compatible web front-end to use for dashboard display graphs - fall back to `graphite[:server]`/`graphite[:prefix]` if `graphs` is not specified, in order to support legacy `vmpooler.yaml` configurations. Note that since `statsd` takes precedence over `graphite`, it's possible to specify both `statsd` (for publishing) and `graphite` (for reading). We still prefer `graphs` over `graphite`. Updated the example `vmpooler.yaml` config file. * (maint) fix variable reference in new_metrics This was referencing config directly, when what we want is for a hash to be passed in (derived from config). * (maint) Fix typo in updated graph link call * (maint) default :graphs prefix to 'vmpooler' * (maint) Fix parse error in vmpooler script The things you find through manual QA 🧌 * (maint) use strings instead of symbols in config Nested hash data comes back with string keys, not symbols. Be consistent. * [QENG-4075] Factor out Vmpooler::DummyStatsd This makes it visible to lib/vmpooler.rb, as well as putting this dummy metrics endpoint in its own file for easier discovery. * (maint) clean up statsd inclusion and require lines The library is actually required as 'statsd' and not 'ruby-statsd', best I can tell. * (maint) construct ::Statsd instead of Statsd Because it's ambiguous in this scope, and, well, it doesn't actually work in production. * [QENG-4075] Also track completely invalid requests When we don't even get a pool name we still want metrics to be recorded.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this, vmpooler could partially fill requests for multiple VMs. Here we move to an algorithm that returns the entire set of requested VMs or no VMs.
/cc @puppetlabs/cinext