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.

Sign up for GitHub

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

[QENG-3919] Make vmpooler checkouts be all or nothing #153

Merged
merged 20 commits into from
May 27, 2016
Merged

[QENG-3919] Make vmpooler checkouts be all or nothing #153

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e323a7f
(QENG-3919) spike for implementation of all-or-nothing checkout
rick May 20, 2016
d3c5d3b
Fix two botched variable references
rick May 20, 2016
2cd83af
Aggregate API helper methods
rick May 20, 2016
7a587b8
Add specs for failed multi-vm allocation API endpoints
rick May 20, 2016
93e218c
(QENG-3919) Add tests for multiple vm requests
rick May 23, 2016
c0cf772
(QENG-3919) Add (failing) specs for POST /vm/pool1+pool2 usages
rick May 24, 2016
0e66d35
(QENG-3919) Bring query params version in line with JSON post version
rick May 24, 2016
67748e3
(QENG-3919) extract common method from both methods of VM allocation
rick May 24, 2016
2ea47a9
(QENG-3919) Naming fix, cosmetic cleanups
rick May 24, 2016
2f9bc40
(QENG-3919) Update API docs
rick May 24, 2016
5032b51
(QENG-3919) minor readability tweak in refactored method
rick May 26, 2016
07ca820
(QENG-3919) Clean up interim comments re: status codes
rick May 26, 2016
8943ff4
(QENG-3919) Drop now-orphaned `checkout_vm` method
rick May 26, 2016
c4cac5b
(QENG-3919) Return 503 status on failed allocation
rick May 26, 2016
06ae95c
(QENG-3919) add net-ldap to Gemfile
rick May 26, 2016
61f57d5
(QENG-3919) Turns out, spush isn't a redis command
rick May 26, 2016
3e6938c
(QENG-3919) Pin the net-ldap gem to 0.11 for the jrubies, etc.
rick May 26, 2016
9123cf7
(QENG-3919) Correct an old spelling error in spec descriptions
rick May 26, 2016
9c1add1
(QENG-3919) Further tweak net-ldap version
rick May 27, 2016
f2c9720
(QENG-3919) return_single_vm -> return_vm_to_ready_state
rick May 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ $ curl -d '{"debian-7-i386":"2","debian-7-x86_64":"1"}' --url vmpooler.company.c
}
```

**NOTE: Returns either all requested VMs or no VMs.**

##### POST /vm/<pool>

Check-out a VM or VMs.
Expand Down Expand Up @@ -156,6 +158,8 @@ $ curl -d --url vmpooler.company.com/api/v1/vm/debian-7-i386+debian-7-i386+debia
}
```

**NOTE: Returns either all requested VMs or no VMs.**

##### GET /vm/<hostname>

Query a checked-out VM.
Expand Down
1 change: 1 addition & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @shermdog (hard-wired to make travis happy on jruby, etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not hardwire as we do have some customers using this tool.


# Test deps
group :test do
Expand Down
150 changes: 74 additions & 76 deletions lib/vmpooler/api/v1.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,38 +45,75 @@ def alias_deref(hash)
newhash
end

def checkout_vm(template, result)
vm = backend.spop('vmpooler__ready__' + template)
def fetch_single_vm(template)
backend.spop('vmpooler__ready__' + template)
end

def return_single_vm(template, vm)
Copy link
Contributor

Choose a reason for hiding this comment

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

This confused me at first - could we rename it to something like re_ready_single_vm or something along those lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing!

backend.sadd('vmpooler__ready__' + template, vm)
end

unless vm.nil?
backend.sadd('vmpooler__running__' + template, vm)
backend.hset('vmpooler__active__' + template, vm, Time.now)
backend.hset('vmpooler__vm__' + vm, 'checkout', Time.now)
def account_for_starting_vm(template, vm)
backend.sadd('vmpooler__running__' + template, vm)
backend.hset('vmpooler__active__' + template, vm, Time.now)
backend.hset('vmpooler__vm__' + vm, 'checkout', Time.now)

if Vmpooler::API.settings.config[:auth] and has_token?
validate_token(backend)
if Vmpooler::API.settings.config[:auth] and has_token?
validate_token(backend)

backend.hset('vmpooler__vm__' + vm, 'token:token', request.env['HTTP_X_AUTH_TOKEN'])
backend.hset('vmpooler__vm__' + vm, 'token:user',
backend.hget('vmpooler__token__' + request.env['HTTP_X_AUTH_TOKEN'], 'user')
)
backend.hset('vmpooler__vm__' + vm, 'token:token', request.env['HTTP_X_AUTH_TOKEN'])
backend.hset('vmpooler__vm__' + vm, 'token:user',
backend.hget('vmpooler__token__' + request.env['HTTP_X_AUTH_TOKEN'], 'user')
)

if config['vm_lifetime_auth'].to_i > 0
backend.hset('vmpooler__vm__' + vm, 'lifetime', config['vm_lifetime_auth'].to_i)
end
if config['vm_lifetime_auth'].to_i > 0
backend.hset('vmpooler__vm__' + vm, 'lifetime', config['vm_lifetime_auth'].to_i)
end
end
end

result[template] ||= {}
def update_result_hosts(result, template, vm)
result[template] ||= {}
if result[template]['hostname']
result[template]['hostname'] = Array(result[template]['hostname'])
result[template]['hostname'].push(vm)
else
result[template]['hostname'] = vm
end
end

if result[template]['hostname']
result[template]['hostname'] = [result[template]['hostname']] unless result[template]['hostname'].is_a?(Array)
result[template]['hostname'].push(vm)
else
result[template]['hostname'] = vm
def atomically_allocate_vms(payload)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the case of single-vm requests we "checkout vm" but in the case of multiple-vm requests we "atomically allocate vms"...should we try to align the semantics between these two methods to make it clear that they serve similar purposes? (the former being an operation that returns a scalar whereas the latter returns a collection) At the very least it seems like we should be able to say that the scalar operation is trivially atomic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see if there's a straightforward way to unify these two code paths. There are definitely a couple/few places where there are array vs. scalar semantics that are a bit ugly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out, checkout_vm is no longer even used any more. 🔥 💀

return false unless payload and !payload.empty?

result = { 'ok' => false }
failed = false
vms = []

payload.each do |template, count|
count.to_i.times do |_i|
vm = fetch_single_vm(template)
if !vm
failed = true
break
else
vms << [ template, vm ]
end
end
end

if failed
vms.each do |(template, vm)|
return_single_vm(template, vm)
status 503
end
else
status 503
result['ok'] = false
vms.each do |(template, vm)|
account_for_starting_vm(template, vm)
update_result_hosts(result, template, vm)
end

result['ok'] = true
result['domain'] = config['domain'] if config['domain']
end

result
Expand Down Expand Up @@ -334,80 +371,41 @@ def checkout_vm(template, result)
end

post "#{api_prefix}/vm/?" do
jdata = alias_deref(JSON.parse(request.body.read))
content_type :json

result = { 'ok' => false }

jdata = alias_deref(JSON.parse(request.body.read))

if not jdata.nil? and not jdata.empty?
available = 1
if jdata and !jdata.empty?
result = atomically_allocate_vms(jdata)
else
status 404
end

jdata.each do |key, val|
if backend.scard('vmpooler__ready__' + key).to_i < val.to_i
available = 0
end
end

if (available == 1)
result['ok'] = true
JSON.pretty_generate(result)
end

jdata.each do |key, val|
val.to_i.times do |_i|
result = checkout_vm(key, result)
end
end
end
def extract_templates_from_query_params(params)
payload = {}

if result['ok'] && config['domain']
result['domain'] = config['domain']
params.split('+').each do |template|
payload[template] ||= 0
payload[template] += 1
end

JSON.pretty_generate(result)
payload
end

post "#{api_prefix}/vm/:template/?" do
payload = alias_deref(extract_templates_from_query_params(params[:template]))
content_type :json

result = { 'ok' => false }
payload = {}

params[:template].split('+').each do |template|
payload[template] ||= 0
payload[template] = payload[template] + 1
end

payload = alias_deref(payload)

if not payload.nil? and not payload.empty?
available = 1
if payload and !payload.empty?
result = atomically_allocate_vms(payload)
else
status 404
end

payload.each do |key, val|
if backend.scard('vmpooler__ready__' + key).to_i < val.to_i
available = 0
end
end

if (available == 1)
result['ok'] = true

payload.each do |key, val|
val.to_i.times do |_i|
result = checkout_vm(key, result)
end
end
end

if result['ok'] && config['domain']
result['domain'] = config['domain']
end

JSON.pretty_generate(result)
end

Expand Down
Loading