fixes #13618 - cache expensive vmware api calls #4238

timogoebel · 2017-01-27T14:20:29Z

In my dev environment the compute_resource_selected call got approx. 10 times faster. I'm really thrilled!

The caching can be enabled for every compute resource. The code is reusable to plugins or other compute resources can reuse the caching mechanism.
Cache invalidation was not straight forward to implement because I wanted to support memcached so I had to use a "scope" when creating the cache keys. The scope is changed when the cache is invalidated.

Some "benchmarks":

Before:
Completed 200 OK in 16121ms (Views: 15963.0ms | ActiveRecord: 6.4ms)

After:
Completed 200 OK in 1710ms (Views: 1583.8ms | ActiveRecord: 5.1ms)

mention-bot · 2017-01-27T14:20:31Z

@timogoebel, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brandonweeks, @mmoll and @shlomizadok to be potential reviewers.

tbrisker

Looks very good in general, a few comments inline about the code. Couldn't test as I don't have a vmware resource.

tbrisker · 2017-01-29T08:43:02Z

app/services/compute_resource_cache.rb

+
+  def initialize(compute_resource, opts = {})
+    self.compute_resource = compute_resource
+    self.cache_duration = opts.fetch(:cache_duration, 180.minutes)


You can use a kwarg here

would it make sense to include some sort of a hourly task that keeps refreshing the cache offline?

Yes, I do want to add a hourly rake task for this that can be called by cron. I'd prefer to do this in a separate PR, though.

tbrisker · 2017-01-29T08:47:33Z

app/controllers/api/v2/compute_resources_controller.rb

      private

      def action_permission
        case params[:action]
          when 'available_images', 'available_clusters', 'available_flavors', 'available_folders', 'available_networks', 'available_resource_pools', 'available_security_groups', 'available_storage_domains', 'available_zones', 'associate', 'available_storage_pods'
            :view
+          when 'invalidate_cache'
+            :edit


I think it makes sense to allow viewers to also invalidate the cache (for example if they notice the data seems outdated), this doesn't actually do anything to the compute resource other then reload the data.

tbrisker · 2017-01-29T08:48:03Z

app/controllers/compute_resources_controller.rb

@@ -123,7 +137,7 @@ def resource_pools

  def action_permission
    case params[:action]
-      when 'associate'
+      when 'associate', 'invalidate_cache'


tbrisker · 2017-01-29T08:51:09Z

app/models/concerns/compute_resource_caching.rb

+  private
+
+  def cache
+    @cache || ComputeResourceCache.new(self)


I think this needs to be ||= for the new cache to be saved

tbrisker · 2017-01-29T09:01:41Z

app/views/compute_resources/form/_vmware.html.erb

@@ -7,4 +7,6 @@
 <%= checkbox_f f, :set_console_password, :checked => f.object.set_console_password?,
                  :label => _("Console passwords"),
                  :help_inline => _("Set a randomly generated password on the display connection") %>
+<%= checkbox_f f, :caching_enabled, :label => _("Enable caching"),


This parameter should also be added to the api

tbrisker · 2017-01-29T09:02:16Z

db/migrate/20170127094357_add_caching_enabled_to_compute_resource.rb

@@ -0,0 +1,5 @@
+class AddCachingEnabledToComputeResource < ActiveRecord::Migration
+  def change
+    add_column :compute_resources, :caching_enabled, :boolean


This should have a default false I believe

do you see a need to default to false? I would rather opt in by default, no one wants the slower option unless they have a problem?

ohadlevy · 2017-01-29T09:35:15Z

app/views/compute_resources/show.html.erb

@@ -4,6 +4,7 @@
 <% title_actions display_link_if_authorized(_("Associate VMs"),
                                            hash_for_associate_compute_resource_path(:compute_resource_id => @compute_resource).merge(:auth_object => @compute_resource, :permission => 'edit_compute_resources'),
                                            :title => _("Associate VMs to Foreman hosts"), :method => :put, :class=>"btn btn-default"),
+(display_link_if_authorized(_('Clear Cache'), hash_for_invalidate_cache_compute_resource_path(@compute_resource).merge(:auth_object => @compute_resource), :method => :put, :class => "btn btn-default") if @compute_resource.respond_to?(:invalidate_cache) && @compute_resource.caching_enabled),


nitpick: I would call it reload vs clear cache.

How about "refresh cache"?

ohadlevy · 2017-01-29T09:37:08Z

app/views/compute_resources/show/_vmware.html.erb

@@ -6,3 +6,7 @@
  <td><%= _("Console passwords") %></td>
  <td><%= @compute_resource.set_console_password? ? _("Enabled") : _("Disabled") %></td>
 </tr>
+<tr>
+  <td><%= _("Caching") %></td>
+  <td><%= @compute_resource.caching_enabled ? _("Enabled") : _("Disabled") %></td>


consider using an icon ? from http://www.patternfly.org/styles/icons/#_ maybe pficon-ok and pficon-warning-triangle-o?

I actually do prefer the text vs. the icons to be consistent in the UI. We can change this to icons in a separate PR. pficon-warning-triangle-o is too much warning. If a user disables the caching, it won't break anything.

ohadlevy · 2017-01-29T09:37:50Z

config/routes.rb

@@ -399,6 +399,7 @@
          get 'resource_pools'
          post 'ping'
          put 'associate'
+          put 'invalidate_cache'


nitpick: i would prefer reload vs invalidate (sounds scary as we all know cache invalidation is a hard problem ;))

ohadlevy · 2017-01-29T09:39:45Z

I would like to see a refresh / reload button on the host edit page, i think this is where you would really need up to date / latest info as you dont have the option (dc, cluster etc).

timogoebel · 2017-01-29T17:12:27Z

I would like to see a refresh / reload button on the host edit page, i think this is where you would really need up to date / latest info as you dont have the option (dc, cluster etc).

@ohadlevy : Would it be ok for you to add this in a separate PR? I think, this will make coding and reviewing easier.

tbrisker

A couple more comments inline. As for the naming of the action, "reload" seems to be a bit misleading, as it isn't actually reloaded, rather "cleared" or "flushed" IIUC.

tbrisker · 2017-01-30T11:58:41Z

app/services/compute_resource_cache.rb

+
+  delegate :logger, :to => ::Rails
+
+  def initialize(compute_resource:, cache_duration: nil)


I'm afraid required kwargs is supported only in ruby 2.1, so we can't use that yet. I'd suggest making the first parameter a regular argument and just the cache duration a kwarg. Then you can set the default to 180.minutes in the kwarg rather then below, like so:

def initialize(compute_resource, cache_duration: 180.minutes)

tbrisker · 2017-01-30T12:03:22Z

db/migrate/20170127094357_add_caching_enabled_to_compute_resource.rb

@@ -0,0 +1,5 @@
+class AddCachingEnabledToComputeResource < ActiveRecord::Migration
+  def change
+    add_column :compute_resources, :caching_enabled, :boolean, default: true


Won't this make compute resources that don't support caching appear to have caching enabled?

@tbrisker : Yes, it will. But it's not shown in the UI or API. So it figured, it doesn't really matter. Do you see that differently?

ok, makes sense.

tbrisker · 2017-01-30T13:05:58Z

app/services/compute_resource_cache.rb

+
+  def initialize(compute_resource, cache_duration: nil)
+    self.compute_resource = compute_resource
+    self.cache_duration = cache_duration || 180.minutes


Instead of defaulting the kwarg to nil then defaulting here to 180.minutes, you can do that in the kwarg itself

timogoebel · 2017-01-30T13:07:01Z

@tbrisker , @ohadlevy : Addressed all of your comments. Thanks a lot for these.

As for the naming of the action, "reload" seems to be a bit misleading, as it isn't actually reloaded, rather "cleared" or "flushed" IIUC.

@tbrisker : Would refresh cache be fine for your opinion? I had clear cache before, but @ohadlevy suggested to use something different.

I currently see these follow up tasks:

Add a rake task to refresh the cache.
Add a cache refresh button to the select boxed in the host form that flushes the cache and reloads the data.

tbrisker · 2017-01-30T14:06:32Z

@timogoebel I guess refresh is better, @ohadlevy what do you think? Still one minor comment on the kwargs, other than that LGTM but I would like someone with access to VMWare to test it as well before merging.

timogoebel · 2017-01-30T14:10:45Z

@shlomizadok : Would you mind testing this for me? I think a VM show and create should suffice.

timogoebel · 2017-01-30T15:18:38Z

Caching is now disabled if the compute resource has not been saved yet, fixed the kwarg default and removed caching from dc method as method is quite fast.

timogoebel · 2017-01-30T16:27:56Z

Rebased and renamed reload_cache to refresh_cache.

timogoebel · 2017-01-31T08:04:50Z

Fixed an issue with caching of dc.clusters.

@TheKangaroo: Would you mind testing this PR and let us know if you see any issues? Thanks.

tbrisker · 2017-01-31T08:35:31Z

app/models/compute_resources/foreman/model/vmware.rb

    end

    def datastores(opts = {})
      if opts[:storage_domain]
-        name_sort(dc.datastores.get(opts[:storage_domain]))
+        cache.cache(:"datastores-#{opts[:storage_domain]}") do
+          name_sort(dc.datastores.get(opts[:storage_domain]))


not directly required, but this could benefit from a kwarg for storage domain

tbrisker · 2017-01-31T08:40:31Z

Looks good to me pending testing, test failure is unrelated.

TheKangaroo · 2017-01-31T08:45:30Z

Hi @tbrisker, I tested it in our environment.
I couldn't find any issues and the runtime improvements in creating hosts is huge :)
I really like your PR @timogoebel

shlomizadok · 2017-01-31T08:46:32Z

@tbrisker - testing this now. give me 2 minutes to ack :)

shlomizadok · 2017-01-31T09:01:53Z

ACK 👍

tbrisker · 2017-01-31T09:05:03Z

Thanks @timogoebel !

timogoebel · 2017-01-31T09:27:42Z

Created two follow-up tickets:

Cache-Warming rake task:
http://projects.theforeman.org/issues/18314

Refresh from host page:
http://projects.theforeman.org/issues/18315

After 5 failed attempts from the same ip, login will be blocked for 5 minutes.

After 30 failed attempts from the same ip, login will be blocked for 5 minutes from that ip.

theforeman-bot added Needs testing Not yet reviewed labels Jan 27, 2017

tbrisker suggested changes Jan 29, 2017

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jan 29, 2017

ohadlevy reviewed Jan 29, 2017

View reviewed changes

timogoebel force-pushed the 13618-vmware-caching branch from 33c8ab4 to cd93cf5 Compare January 29, 2017 17:35

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jan 29, 2017

timogoebel force-pushed the 13618-vmware-caching branch 2 times, most recently from b0d2e9d to 6e18e82 Compare January 30, 2017 11:39

tbrisker reviewed Jan 30, 2017

View reviewed changes

timogoebel force-pushed the 13618-vmware-caching branch from 6e18e82 to 6955436 Compare January 30, 2017 12:23

tbrisker reviewed Jan 30, 2017

View reviewed changes

tbrisker added Waiting on contributor and removed Needs re-review Needs testing labels Jan 30, 2017

timogoebel force-pushed the 13618-vmware-caching branch from 6955436 to d67c313 Compare January 30, 2017 15:17

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jan 30, 2017

timogoebel force-pushed the 13618-vmware-caching branch 2 times, most recently from 0520693 to 4fc0c9f Compare January 30, 2017 16:27

timogoebel force-pushed the 13618-vmware-caching branch 2 times, most recently from d73543a to e8621dc Compare January 31, 2017 07:49

fixes #13618 - cache expensive vmware api calls

b0947a4

timogoebel force-pushed the 13618-vmware-caching branch from e8621dc to b0947a4 Compare January 31, 2017 07:50

tbrisker reviewed Jan 31, 2017

View reviewed changes

tbrisker approved these changes Jan 31, 2017

View reviewed changes

tbrisker removed the Needs re-review label Jan 31, 2017

tbrisker merged commit 9caab04 into theforeman:develop Jan 31, 2017

tbrisker added a commit to tbrisker/foreman that referenced this pull request Dec 5, 2017

Fixes theforeman#4238 - Prevent login brute forcing

dfea1cc

After 5 failed attempts from the same ip, login will be blocked for 5 minutes.

tbrisker added a commit to tbrisker/foreman that referenced this pull request Dec 5, 2017

Fixes theforeman#4238 - Prevent login brute forcing

fee60b2

After 30 failed attempts from the same ip, login will be blocked for 5 minutes from that ip.

tbrisker added a commit to tbrisker/foreman that referenced this pull request Dec 5, 2017

Fixes theforeman#4238 - Prevent login brute forcing

9b0f0a1

After 30 failed attempts from the same ip, login will be blocked for 5 minutes from that ip.

tbrisker added a commit to tbrisker/foreman that referenced this pull request Dec 10, 2017

Fixes theforeman#4238 - Prevent login brute forcing

9936ab5

After 30 failed attempts from the same ip, login will be blocked for 5 minutes from that ip.

lzap pushed a commit that referenced this pull request Dec 11, 2017

Fixes #4238 - Prevent login brute forcing

1ece1d3

After 30 failed attempts from the same ip, login will be blocked for 5 minutes from that ip.

mmack pushed a commit to mmack/foreman that referenced this pull request Dec 13, 2017

Fixes theforeman#4238 - Prevent login brute forcing

f4d023c

After 30 failed attempts from the same ip, login will be blocked for 5 minutes from that ip.


		delegate :logger, :to => ::Rails

		def initialize(compute_resource:, cache_duration: nil)

fixes #13618 - cache expensive vmware api calls #4238

fixes #13618 - cache expensive vmware api calls #4238

Conversation

timogoebel commented Jan 27, 2017 • edited Loading

mention-bot commented Jan 27, 2017

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timogoebel Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadlevy commented Jan 29, 2017

timogoebel commented Jan 29, 2017

tbrisker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timogoebel commented Jan 30, 2017 • edited Loading

tbrisker commented Jan 30, 2017

timogoebel commented Jan 30, 2017

timogoebel commented Jan 30, 2017

timogoebel commented Jan 30, 2017

timogoebel commented Jan 31, 2017

Choose a reason for hiding this comment

tbrisker commented Jan 31, 2017

TheKangaroo commented Jan 31, 2017

shlomizadok commented Jan 31, 2017

shlomizadok commented Jan 31, 2017

tbrisker commented Jan 31, 2017

timogoebel commented Jan 31, 2017

timogoebel commented Jan 27, 2017 •

edited

Loading

timogoebel Jan 29, 2017 •

edited

Loading

timogoebel commented Jan 30, 2017 •

edited

Loading