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.

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

(POOLER-123) Implement a max TTL #349

Merged
merged 4 commits into from
Dec 5, 2019
Merged

(POOLER-123) Implement a max TTL #349

merged 4 commits into from
Dec 5, 2019

Conversation

sbeaulie
Copy link
Contributor

@sbeaulie sbeaulie commented Dec 2, 2019

Before this change, we could checkout a vm and set the lifetime to a
very high number which would esssentially keep the vm running forever.
Now implementing a config setting max_lifetime_upper_limit which enforces
a maximum lifetime in hours both for initial checkout and extending a
running vm

@sbeaulie sbeaulie requested a review from a team as a code owner December 2, 2019 19:39
@sbeaulie sbeaulie force-pushed the POOLER-123 branch 2 times, most recently from 9122c2d to a8ca47a Compare December 2, 2019 19:50
@highb
Copy link
Contributor

highb commented Dec 2, 2019

Hmmm... seems that when I spin this up with max_lifetime_upper_limit: 9000 set in the config I can no longer change the lifetime at all? Here's how it works normally against prod:

➜  vmpooler git:(POOLER-123) floaty get centos-7-64 --service vm
- diffuse-pizza.delivery.puppetlabs.net (centos-7-64)
➜  vmpooler git:(POOLER-123) floaty modify diffuse-pizza.delivery.puppetlabs.net --service vm --lifetime 10

Successfully modified VM diffuse-pizza.delivery.puppetlabs.net.
Use `floaty list --active` to see the results.
➜  vmpooler git:(POOLER-123) floaty list --active --service vm

Your VMs on vmpooler.delivery.puppetlabs.net:
- diffuse-pizza.delivery.puppetlabs.net (centos-7-x86_64, 0.01/10 hours)

And here's with the changes running locally:

➜  vmpooler git:(POOLER-123) floaty list --active --service vmlocal
Your VMs on localhost:
- amateur-voyage.delivery.puppetlabs.net (centos-7-x86_64, 0.08/2 hours)
➜  vmpooler git:(POOLER-123) floaty modify amateur-voyage.delivery.puppetlabs.net --lifetime 8999 --trace

Successfully modified VM amateur-voyage.delivery.puppetlabs.net.
Use `floaty list --active` to see the results.
➜  vmpooler git:(POOLER-123) floaty list --active --service vmlocal
Your VMs on localhost:
- amateur-voyage.delivery.puppetlabs.net (centos-7-x86_64, 0.09/2 hours)

@sbeaulie
Copy link
Contributor Author

sbeaulie commented Dec 3, 2019

I should check what api command floaty uses? Did you put max_lifetime_upper_limit as a child of the config: key, and not top scope?

@highb
Copy link
Contributor

highb commented Dec 3, 2019 via email

@highb
Copy link
Contributor

highb commented Dec 3, 2019

As for floaty, I think it just PUTS to the vm endpoint https://github.com/briancain/vmfloaty/blob/master/lib/vmfloaty/pooler.rb#L54

  def self.modify(verbose, url, hostname, token, modify_hash)
    raise TokenError, 'Token provided was nil. Request cannot be made to modify vm' if token.nil?

    modify_hash.keys.each do |key|
      raise ModifyError, "Configured service type does not support modification of #{key}." unless %i[tags lifetime disk].include? key
    end
...
  response = conn.put do |req|
      req.url "vm/#{hostname}"
      req.body = modify_hash.to_json
    end

@@ -892,6 +892,22 @@ def invalid_pool(payload)
when 'lifetime'
need_token! if Vmpooler::API.settings.config[:auth]

# in hours, defaults to one week
max_lifetime_upper_limit = config[:max_lifetime_upper_limit]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I found the issue. This key is a string, not a symbol. When I switched that out, it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. That was an issue with testing this as well, since we set the parameters and I wasn't sure if it was a symbol or a string. Thanks!

Before this change, we could checkout a vm and set the lifetime to a
very high number which would esssentially keep the vm running forever.
Now implementing a config setting max_lifetime_upper_limit which enforces
a maximum lifetime in hours both for initial checkout and extending a
running vm
Prior to this commit the PUT vm endpoint didn't give any useful
information about why a user's request failed.

This commit updates PUT to output a more helpful set of error messages
in the `failure` key that gets returned in the JSON response.
@highb
Copy link
Contributor

highb commented Dec 4, 2019

I've switched out the symbol for a string in the config hash reference and added a little better error messaging so a user knows what actually went wrong. Also, vmfloaty didn't correctly handle when the PUT came back with ok: false so I've put up a PR to fix that up a bit. puppetlabs/vmfloaty#55

@@ -22,6 +22,8 @@ services:
- redislocal
redislocal:
image: redis
# Uncomment this if you don't want the redis data to persist
#command: "redis-server --save '' --appendonly no"
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh and this little line was helpful for me to ensure that redis wasn't persisting between testing runs, as it was getting a strange no space left on device error. My computer has plenty of room, so I'm not sure where that is coming from all of a sudden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check the docker settings for how much space is allowed for all containers? I had to bump mine up a few times.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I didn't even know that was a setting. Thanks!

Brandon High added 2 commits December 3, 2019 17:48
This commit switches the max_lifetime_upper_limit key from being a
symbol to being a string, which is what the config hash seems to contain.
This commit is just a handy little command override to the redis
container to prevent persistence.
@sbeaulie
Copy link
Contributor Author

sbeaulie commented Dec 4, 2019

Thanks for the commits @highb

high five

Copy link
Contributor

@kevpl kevpl left a comment

Choose a reason for hiding this comment

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

lgtm, I'm especially glad to see we'll be including the message with an error return.

this will still have to be released, I'm not planning on doing this on merge.

@kevpl kevpl merged commit ae10bd4 into master Dec 5, 2019
@rooneyshuman rooneyshuman deleted the POOLER-123 branch June 5, 2020 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants