-
Notifications
You must be signed in to change notification settings - Fork 26
Add abs vm get #53
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.
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
Add abs vm get #53
Conversation
Gemfile
Outdated
@@ -7,6 +7,7 @@ gemspec | |||
gem 'rake', :require => false | |||
|
|||
group :test do | |||
gem 'faraday', '0.15.4' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to bump this over in vmfloaty.gemspec
, not here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed per slack convo, deleting reference to Faraday in spec headers.
README.md
Outdated
@@ -53,7 +53,7 @@ $ floaty --help | |||
Grabbing a token for authenticated pooler requests: | |||
|
|||
``` | |||
floaty token get --user username --url https://vmpooler.example.net/api/v1 | |||
floaty token get --user username --url https://vmpooler.example.net/api/v1 --api (vmpooler|abs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you actually add this --api
flag?
➜ vmfloaty git:(add_abs_vm_get) ✗ be bin/floaty token get --user brandon.high --url https://cinext-abs-prod.delivery.puppetlabs.net --api abs
invalid option: --api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that was a previous iteration before I realized there was a "service" feature.
beb3aa3
to
9eb7e49
Compare
FYI: This PR needs some work before it's ready to go go, I'm working on it now. |
f45677d
to
27a19d6
Compare
27a19d6
to
4bf7ee9
Compare
4bf7ee9
to
7e27542
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested the code in this PR manually, and it's ready to be tested. This adds the get token, get vms, list vms and delete vms functionality to floaty. There are a few things to note how it works:
- ABS requires the username of the person requesting vms in the payload, so I had to add username as a parameter to all the get functions for the various services.
- ABS does not return the vm immediately, it adds the request to a queue so the ABS get here polls the endpoint every 10 seconds until the request is fulfilled.
- Job ID is generated by the client, and is a 13 digit (millisecond) unix timestamp in order to "guarantee" uniqueness in case two people request around the same time.
response = conn.post "api/v2/request", reqObj.to_json | ||
|
||
i = 0 | ||
retries = 360 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what to put here, this would retry for an hour, the user can always ctrl-c.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a timeout ?
@@ -22,7 +22,7 @@ def self.list_active(verbose, url, token) | |||
status['reserved_hosts'] || [] | |||
end | |||
|
|||
def self.retrieve(verbose, os_type, token, url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ABS requires posting the username, and the params have to be the same.
77a51ed
to
6b2f2e6
Compare
There are some endpoints in ABS that don't consistently respond to /api/v2/ they only respond to This makes writing this code challenging. Merging a change on Monday so that ABS has that feature so we don't have to write exceptions here, then after some manual testing, should be good to merge this change. |
I’ll be taking a look on Monday.
…On Fri, Nov 8, 2019 at 2:27 PM Brian Cain ***@***.***> wrote:
Assigned #53 <#53> to @highb
<https://github.com/highb>.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#53>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFZQKHBS42JK4YOKVWUF6TQSXRVFANCNFSM4JDHZXEA>
.
|
6b2f2e6
to
78f42a1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it out with /api/v2
appended to the URL, we'll work out not making that part of the URL a requirement in another PR.
➜ vmfloaty git:(add_abs_vm_get) ✗ be bin/floaty get centos-7-x86_64 --service abs
Requesting VMs with job_id: 1573510037352. Will retry for up to an hour.
Waiting 10 seconds to check if ABS request has been filled. Queue Position: 85... (x1)
- adamant-bride.delivery.puppetlabs.net (centos-7-x86_64)
And getting regular pooler VMs still works:
➜ vmfloaty git:(add_abs_vm_get) be bin/floaty get centos-7-x86_64 --service vm
- deputy-dip.delivery.puppetlabs.net (centos-7-x86_64)
I think this is good to merge once the Rubocop fixes are in and anyone else who wants to test it out chimes in. |
78f42a1
to
01b5457
Compare
Will mege this in after the holiday. |
lib/vmfloaty/abs.rb
Outdated
|
||
req_hash['allocated_resources'].each do |vm_name, _i| | ||
if hosts.include? vm_name['hostname'] | ||
if all_job_resources_accounted_for(job['allocated_resources'], hosts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikkergimenez Is this job
variable defined somewhere? When I try using delete it seems to blow up here.
➜ vmfloaty git:(add_abs_vm_get) be bin/floaty delete secular-relay.delivery.puppetlabs.net --service abs --trace
bundler: failed to load command: bin/floaty (bin/floaty)
NameError: undefined local variable or method `job' for ABS:Class
/s/vmfloaty/lib/vmfloaty/abs.rb:90:in `block (2 levels) in delete'
/s/vmfloaty/lib/vmfloaty/abs.rb:88:in `each'
/s/vmfloaty/lib/vmfloaty/abs.rb:88:in `block in delete'
/s/vmfloaty/lib/vmfloaty/abs.rb:85:in `each'
/s/vmfloaty/lib/vmfloaty/abs.rb:85:in `delete'
/s/vmfloaty/lib/vmfloaty/service.rb:115:in `delete'
/s/vmfloaty/lib/vmfloaty.rb:229:in `block (2 levels) in run'
/Users/highb/.rbenv/versions/2.5.6/lib/ruby/gems/2.5.0/gems/commander-4.4.7/lib/commander/command.rb:182:in `call'
/Users/highb/.rbenv/versions/2.5.6/lib/ruby/gems/2.5.0/gems/commander-4.4.7/lib/commander/command.rb:153:in `run'
/Users/highb/.rbenv/versions/2.5.6/lib/ruby/gems/2.5.0/gems/commander-4.4.7/lib/commander/runner.rb:446:in `run_active_command'
/Users/highb/.rbenv/versions/2.5.6/lib/ruby/gems/2.5.0/gems/commander-4.4.7/lib/commander/runner.rb:68:in `run!'
/Users/highb/.rbenv/versions/2.5.6/lib/ruby/gems/2.5.0/gems/commander-4.4.7/lib/commander/delegates.rb:15:in `run!'
/s/vmfloaty/lib/vmfloaty.rb:454:in `run'
bin/floaty:8:in `<top (required)>'
conn = Http.get_conn(verbose, url) | ||
|
||
res = conn.get 'summary' | ||
JSON.parse(res.body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, this just returns OK
which isn't valid JSON.
➜ vmfloaty git:(add_abs_vm_get) be bin/floaty status --service abs
error: 765: unexpected token at 'OK'. Use --trace to view backtrace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't done status yet, but since it seems relatively easy I just pushed up a change with the status command fixed.
01b5457
to
e0384a3
Compare
e0384a3
to
4ec084b
Compare
4ec084b
to
d963e35
Compare
Status
Needs Feedback
Description
This adds the ability to request vms from ABS. Since ABS API works a bit differently from the vmpooler API, this has to work a bit differently as well, polling the API before returning the vms. This is a work in progress as the text could be a bit better and the list active vms functionality does not work.
FIXME
Related Issues
Todos
Reviewers
@highb
@briancain