(QENG-5305) Improve vmpooler host selection by mattkirby · Pull Request #242 · puppetlabs/vmpooler

mattkirby · 2017-10-14T01:12:36Z

This change updates how vmpooler creates VMs, migrates VMs, and significantly reduces strain placed upon vsphere appliances by eliminating duplicate host state inspection.

Clone operations target the resource pool for a cluster instead of a specific host. Host selection is left for vsphere to determine.
Host selection used for migrating VMs is updated to eliminate duplicate host inspection by maintaining a global list of available target hosts within a cluster and updating it when it becomes stale.
VMs are evaluated for migration against the list instead of occupying a slot for migration to be evaluated.
A optional capability to create pool folders is added (this can be moved to a separate PR if desired)
Host object retrieval is updated to use the searchIndex instead of iterating through a list of cluster objects until the matching name is found.
A safeguard against no quickstats being found is added by ensuring that CPU utilization is not 0.

TODO:

Finish adding tests for new functionality in vsphere.rb

sbeaulie · 2017-10-17T23:12:24Z

-          finish = migrate_vm_and_record_timing(vm_name, pool_name, parent_host_name, host_name, provider)
-          $logger.log('s', "[>] [#{pool_name}] '#{vm_name}' migrated from #{parent_host_name} to #{host_name} in #{finish} seconds")
+          $redis.sadd('vmpooler__migration', vm_name)
+          target_host_name = select_next_host(provider_name, vm['datacenter'], vm['cluster'], vm['architecture'])


what if select_next_host returns 'nil'?

It should cause a failure to be logged reporting that vm migration failed with an error raised from lib/vmpooler/providers/vsphere.rb:90 when migrate_vm is called. Perhaps it's worth emitting a more clear error that there was no target host provided.

I think it will call the next line migrate_vm_and_record_timing() with target_host_name = nil which seems to go all the way down to the vcenter API to try and get the host by dns name?
We should fail earlier than that.

This will now fail if the datacenter, or cluster are missing, and if the target is empty, before trying to run the methods with bad data.

sbeaulie · 2017-10-17T23:18:09Z

+
+    def select_next_host(provider_name, datacenter, cluster, architecture)
+      provider_hosts = $provider_hosts
+      host = provider_hosts[provider_name][datacenter][cluster]['architectures'][architecture][0]


I think if provider_hosts[provider_name][datacenter][cluster]['architectures'][architecture] is a string then [0] could return the first character which is not what you probably expect here?

That's true. It's populated by select_least_used_hosts from lib/vmpooler/providers/vsphere.rb:607 which returns an array. Do you think I should be more explicit than this at some point to ensure an array is returned?

Ok I got it, thanks

sbeaulie · 2017-10-17T23:24:43Z

+          end
+
+          hosts_with_arch_versions.each do |host|
+            architectures[host[2]] << [host[0], host[1], host[2]]


I find it hard to know what is in each index host[0], host[1], host[2]

Yeah, it probably makes sense to put it in a hash and give these values some titles.

This now uses a hash which should make clear what's being placed into this array.

glennsarti

There is vpshere specific concepts in the pool manager code (datacenter, cluster etc.). This should really all be in the vsphere provider.

glennsarti · 2017-10-18T16:04:27Z

      $threads = {}
+
+      # Host tracking object
+      $provider_hosts = {}


nit: REALLY REALLY try to avoid adding more global variables. Instance level should be enough.

@glennsarti are you sure? I specifically require that this variable be accessible across all vmpooler provider threads.

mattkirby · 2017-10-18T16:15:13Z

@glennsarti how do you think this can be solved in the vsphere code when it needs to constrain and share data across pool manager operations?

glennsarti · 2017-10-18T16:22:35Z

Sure

Only one instance of a provider exists for one or more pools so it's able to maintain the state inside the provider
All operations have to go through the provider so they can be gated through there

The only reason I can think of to keep state in the pool manager is to allow across-provider communication. Which is not (should not) be required for vsphere host selection.

mattkirby · 2017-10-25T00:37:57Z

Ok. I have reworked this to do away with global variables and now use an instance variable in the provider to track host state on a per-provider basis. Tests pass, but I need to complete adding coverage for changes in vsphere and the base provider, but wanted to get this put here so I could get further feedback and input if there are still some clear changes needed. I still intend to address the other feedback.

mattkirby · 2017-11-03T19:57:22Z

I've pushed an update here that moves migrate_vm to the vsphere provider. This should be a lot cleaner. I've addressed the confusing array population that @sbeaulie brought up earlier, and global variables are no longer used here in favor of instance variables on the provider, which works great. A mutex is added to ensure that all writing to provider_hosts in the provider is safe across all threads.

I added one more change to this, which allows a user to specify whether vmpooler should manage host selection at clone time, or if it should be deferred to vsphere. If vmpooler is managing host selection it uses the same method now used for migrations.

I'm still working through a handful of remaining tests on the vsphere provider, but this is all functional now.

mattkirby · 2017-11-03T20:02:32Z

The jruby tests pass, the others are failing. If I'm reading travis correctly it looks as though setup to run the tests is what's failing. Tests do pass locally for me on ruby 2.3.5p376.

glennsarti · 2017-11-03T22:05:00Z

-
    def migrate_vm(vm_name, pool_name, provider)
      Thread.new do
        begin


The provider really shouldn't need to contact redis for this. In this case the provider is just removing it from the queue. So the $redis.srem('vmpooler__migrating__' + pool_name, vm_name) should be called here PRIOR to the provider.migrate_vm call

I took your suggestion and placed this prior to calling provider.migrate_vm.

glennsarti · 2017-11-03T22:06:33Z

        rescue => err
          $logger.log('s', "[x] [#{pool_name}] '#{vm_name}' migration failed with an error: #{err}")
-          remove_vmpooler_migration_vm(pool_name, vm_name)
+          provider.remove_vmpooler_migration_vm(pool_name, vm_name, $redis)


This new method remove_vmpooler_migration_vm doesn't exist on the base and on inspection probably shouldn't be in a provider anyway. It's just removing the VM from the task queue.

By doing my previous suggestion, it's not needed as the VM is removed from the queue prior to be actioned in the provider.

glennsarti · 2017-11-03T22:07:43Z

      end
    end

-    def migration_limit(migration_limit)


As the migration limit is now a per vSphere provider setting the documentation should be updated too.

It probably should be a per vSphere provider setting, but it's still tracking migrations in redis and getting the setting from the global configuration section instead of checking the provider for this setting. I could add this change here. I was considering tackling that and the task_limit concept in a future change.

Could be as simple as add in the docs migration limit (Only affects vSphere Provider)

Oh I see what you're saying, not that it's a per provider setting, but rather that it is only a vsphere provider setting.

yeah... The docs suggest its global but no longer.

I added a mention in the example configuration file. I couldn't find another reference to migration_limit, please let me know if I've missed another location this should go.

glennsarti · 2017-11-03T22:09:07Z

        attr_reader :metrics
        # Provider options passed in during initialization
        attr_reader :provider_options
+        # Hash for tracking hosts for deployment


These settings seem vsphere specific. Perhaps instead of modifying base, which affects all providers, move them there

glennsarti · 2017-11-03T22:09:24Z

          @logger = logger
          @metrics = metrics
          @provider_name = name
+          @provider_hosts = {}


As above. vsphere provider specific.

glennsarti · 2017-11-03T22:11:10Z

+        # inputs
+        #   [String] pool_name      : Name of the pool
+        #   [String] vm_name        : Name of the VM to migrate
+        #   [Class] redis           : Redis object


Apart from Redis, how is this method signature different from migrate_vm_to_host?. Is it just simply calling

migrate_vm_to_host(pool_name, vm_name, nil) ?

Redis is already a global variable, so no ed to pass it through. That should probably be a separate PR

The main difference to me, beyond the redis part which as you point out shouldn't be there, is that there's no longer a need to provide the destination host name, so I figured keeping migrate_vm instead of migrate_vm_to_host made sense. I can change it if it makes more sense.

glennsarti · 2017-11-03T22:17:52Z

-      migration_limit if migration_limit >= 1
-    end
-
    def migrate_vm(vm_name, pool_name, provider)


There is currently no way in Pool Manager to actually add a VM to the migration queue. So this PR will not have any affect yet. Is this by design?

No, it's an oversight. It's fixed now.

mattkirby · 2017-11-06T23:10:40Z

I've updated this to include a fix for a behavior witnessed when running this over the weekend. Specifically, if no hosts are available for deployment check_time_finished is still updated in order to allow for things waiting on host selection to proceed. Previously, they would be stuck waiting and gum up pool_manager. Now, they will fail fast and report an error that makes clear why the clone or migration failed. Additionally, settings for utilization maximum and max age are exposed and documented.

This commit adds a global provider_hosts concept in order to allow checking cluster utilization once per interval for a given cluster and retain the results, reusing them for an interval, and tracking the least used set of hosts. Without this change each migration and clone operation inspect host utilization and state for each host in the cluster, which is computationally expensive for vsphere.

This commit adds the capability to create folders within an existing target folder. Without this change folders to support platforms targets need to be created manually.

This commit updates create_vm to target a cluster instead of an individual host for clone operations. Without this change cluster host utilization needs to be inspected for everyone clone operation.

This commit updates cluster host resource inspection to stop weighting memory as a part of the results. Additionally, instead of returning a single least used host a percentage of the eligible hosts with below average utilization are selected. Without this change host migration logic can cause cluster resource imbalances due to a single host being targeted for many migrations before the impacts of the migrations cause numbers to adjust.

…ction

This commit moves the migrate_vm logic to the vsphere provider. Without this change migrate_vm has lots of vsphere specific logic in pool_manager migrate_vm method.

This commit updates the providers to move provider_hosts under the vsphere provider, which is the only place it's applicable. Methods where redis is passed through are updated to remove this pass through and use the globally available redis object, where applicable. Remove_vmpooler_migration_vm method is not needed and is removed.

This commit updates host selection to write check_time_finished whether host retrieval was successful or not. Without this change when host selection fails threads waiting for host selection to complete will stuck waiting because check_time_finished doesn't update. Additionally, because it would leave checking => true it would not attempt to inspect and run host selection again. Host selection for clones and migrations now make clear that no hosts are available and fail logging a message. Without this change both migrations and clones would fail with cryptic error messages logged indicating clone and migrations failed. Additionally, this change makes max_age configurable so a user can specify that host selection should happen more or less frequently, as required for migrations or clone operations when host selection is enabled.

This change documents new vshpere specific parameters introduced related to host selection and folder creation. Without this change these paremeters are not documented.

This commit removes an unnecessary rescue that results in duplicate clone error messages. Without this change clone failures due to unavailable host resources are logged twice. A log message is added to specify the host the VM is running on when migration_limit is not set and migration is disabled. Lastly, when a migration fails it reports the host the VM is running on in addition to the reason for the failed migration.

glennsarti

Ship it

shrug · 2018-01-02T23:14:54Z

@mattkirby Ready to merge this?

mattkirby · 2018-01-02T23:29:34Z

Just about. I've got one more set of tests I'm going to push shortly after which this should be ready to go. That should cover the bits I've added that haven't had tests added yet.

This commit adds tests for the remaining new vsphere functionality.

mattkirby · 2018-01-03T23:46:29Z

I've finished tests and this should be ready to go.

mattkirby changed the title ~~Improve vmpooler host selection~~ (QENG-5305) Improve vmpooler host selection Oct 17, 2017

mattkirby force-pushed the improved_host_selection_cleanup branch from f768ec3 to 0d8c131 Compare October 17, 2017 19:40

mattkirby requested a review from glennsarti October 17, 2017 19:47

underscorgan requested review from demophoon and shrug October 17, 2017 22:05

sbeaulie reviewed Oct 17, 2017

View reviewed changes

glennsarti suggested changes Oct 18, 2017

View reviewed changes

mattkirby force-pushed the improved_host_selection_cleanup branch 2 times, most recently from 407af83 to 5252075 Compare November 3, 2017 19:54

mattkirby force-pushed the improved_host_selection_cleanup branch from 5252075 to 2d343f0 Compare November 3, 2017 20:23

glennsarti suggested changes Nov 3, 2017

View reviewed changes

mattkirby force-pushed the improved_host_selection_cleanup branch 2 times, most recently from c81c83c to 1aed90b Compare November 8, 2017 20:59

mattkirby added 9 commits November 28, 2017 13:14

Add capability to create folders

4d7e9c7

This commit adds the capability to create folders within an existing target folder. Without this change folders to support platforms targets need to be created manually.

Only specify cluster for clone operations

cf2f9fa

This commit updates create_vm to target a cluster instead of an individual host for clone operations. Without this change cluster host utilization needs to be inspected for everyone clone operation.

Update pool_manager and vsphere tests to support changes in host sele…

2e3e83a

…ction

Move migrate_vm logic to vsphere provider

dc8067a

This commit moves the migrate_vm logic to the vsphere provider. Without this change migrate_vm has lots of vsphere specific logic in pool_manager migrate_vm method.

Remove references to redis in migrate_vm

5a19ef5

Remove attr_reader for provider_hosts

f47b804

mattkirby added 6 commits November 28, 2017 13:14

Specify that migration_limit is vsphere only

35817fa

Document parameters added in vmpooler.yaml.example

6bf2eb7

This change documents new vshpere specific parameters introduced related to host selection and folder creation. Without this change these paremeters are not documented.

Ensure vm is removed from migrations

f75d4f6

Simplify run_select_hosts

dd12125

mattkirby force-pushed the improved_host_selection_cleanup branch from 96f68ec to dd12125 Compare November 28, 2017 21:14

reduce loop delay when waiting for host selection

1a01855

glennsarti approved these changes Dec 14, 2017

View reviewed changes

Add tests for new vsphere functionality

b5fd0f7

This commit adds tests for the remaining new vsphere functionality.

mattkirby mentioned this pull request Jan 8, 2018

vsphere_helper.rb: find_least_used_host should warn if no suitable hosts are found #164

Closed

sbeaulie approved these changes Jan 8, 2018

View reviewed changes

mattkirby merged commit 6ab2e2f into puppetlabs:master Jan 10, 2018

mattkirby deleted the improved_host_selection_cleanup branch January 10, 2018 20:03

Uh oh!

Conversation

mattkirby commented Oct 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glennsarti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkirby commented Oct 18, 2017

Uh oh!

glennsarti commented Oct 18, 2017

Uh oh!

mattkirby commented Oct 25, 2017

Uh oh!

mattkirby commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattkirby commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkirby Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattkirby commented Nov 6, 2017

Uh oh!

glennsarti left a comment

Choose a reason for hiding this comment

mattkirby commented Oct 14, 2017 •

edited

Loading

mattkirby commented Nov 3, 2017 •

edited

Loading

mattkirby commented Nov 3, 2017 •

edited

Loading

mattkirby Nov 3, 2017 •

edited

Loading