Add orphan purging for vim_performance_states #16754

carbonin · 2018-01-05T17:07:29Z

This PR adds a new purging mode to the PurgingMixin and uses that new mode for removing orhaned rows from the vim_performance_states table.

Orphan purging is needed when we remove a row from a referenced table, but don't cascade that removal to other tables.

Vm -> VimPerformanceState is one such situation

When a vm is removed the performance states for that vm linger ... forever. This PR fixes that.

https://bugzilla.redhat.com/show_bug.cgi?id=1434918

carbonin · 2018-01-05T19:16:14Z

Leaving this as WIP until we can ensure that this is the right direction.

Specifically looking for input from @kbrock on efficiency and @Fryguy for the general layout.

I'm not sure we want to apply this purging to more classes yet as this PR is just really to fix the existing issue with vim_performance_states.

Also, I'm not sure what this would look like if we wanted to do multiple types of purging for a particular model. Maybe we could reimplement #purge_mode_and_value to nest the arrays even more? Something like:

  # In VimPerformanceState::Purging
  def purge_mode_and_value
    [
      ["orphaned", "resource"],
      ["date", 6.months.ago]
    ]
  end

  # In PurgingMixin
  def purge_timer
    purge_mode_and_value.each do |mode, args|
      purge_queue(mode, args)
    end
  end

Either way, it's probably out of the scope of this PR, but I figured it was worth thinking about now.

carbonin · 2018-01-05T19:52:03Z

I added some query tracking to the specs to ensure that we won't pull back a ton of data. This is the delete being issued:

DELETE FROM "vim_performance_states"
WHERE "vim_performance_states"."id" IN
  (
    SELECT  "vim_performance_states"."id"
    FROM "vim_performance_states"
    LEFT OUTER JOIN "vms" ON vim_performance_states.resource_id = "vms".id
    WHERE "vms"."id" IS NULL AND "vim_performance_states"."resource_type" = $1 LIMIT $2
  )

Looks good to me ... @kbrock any concerns?

carbonin · 2018-01-05T20:12:08Z

config/settings.yml

@@ -1200,6 +1203,7 @@
      :storage_file_collection_interval: 1.days
      :storage_file_collection_time_utc: 21600
      :task_timeout_check_frequency: 1.hour
+      :vim_performance_states_purge_interval: 1.day


Does this seem like a good value?

Thought we'd do something closer to 6 month. but I not know the usage of this table.

Fryguy · 2018-01-08T18:41:22Z

app/models/mixins/purging_mixin.rb

@@ -55,7 +55,7 @@ def purge_queue(mode, value = nil)
      MiqQueue.submit_job(
        :class_name  => name,
        :method_name => "purge_by_#{mode}",
-        :args        => [value]
+        :args        => value.kind_of?(Array) ? value : [value]


Does Array.wrap do this?

Fryguy · 2018-01-08T18:42:02Z

app/models/mixins/purging_mixin.rb

@@ -98,8 +98,40 @@ def purge_by_scope(older_than = nil, window = nil, &block)
      _log.info("Purging #{table_name.humanize}...Complete - Deleted #{total} records")
    end

+    def purge_by_orphaned(fk_name, polymorphic, join_table = "", window = purge_window_size)
+      _log.info("Purging orphans in #{table_name.humanize}...")
+      total = polymorphic ? purge_polymorphic_orphans(fk_name, window) : purge_orhans(fk_name, join_table, window)


Typo purge_orhans... Need a test to go through this.

Fryguy · 2018-01-08T18:45:19Z

app/models/mixins/purging_mixin.rb

+      total
+    end
+
+    def polymorphic_classes(type_column)


Can we make this variable resource_type_column? Only because type is it's own thing, and it's confusing to read.

Or polymorphic_type_column ?

Fryguy · 2018-01-08T18:47:33Z

app/models/mixins/purging_mixin.rb

+        scope = joins("LEFT OUTER JOIN #{resource_table} ON #{table_name}.resource_id = #{resource_table}.id")
+                .where(resource_table => {:id => nil})
+                .where("#{table_name}.#{connection.quote_column_name(type_column)} = #{connection.quote(klass.name)}")
+        total += purge_in_batches(scope, window)


This might have to support passing a block in the future so you can deal with tools that need to show progress, but I'm fine with passing on that for now.

Fryguy · 2018-01-08T18:49:09Z

app/models/mixins/purging_mixin.rb

+    end
+
+    def purge_orphans(fk_name, join_table, window)
+      raise "Purging non-polymorphic refrences requires a join table name" if join_table.empty?


Typo refrences

Fryguy · 2018-01-08T18:50:55Z

config/settings.yml

@@ -1043,6 +1043,9 @@
  :active_task_timeout: 6.hours
 :ui:
  :mark_translated_strings: false
+:vim_performance_state:


state or states? I;m thinking states.

Fryguy · 2018-01-08T18:52:23Z

As discussed let's remove the non-polymorphic case and just put a comment in the method that it only supports polymorphic for now, since we don't have any use cases for orphaned and non-polymorphic (and thus can't test it either).

kbrock

This looks nice.

ASIDE (and unrelated to this PR):
Wish we didn't have to copy and paste so much for the purging.
Seems we should just have 1 purge task.

kbrock · 2018-01-11T22:03:42Z

config/settings.yml

@@ -1200,6 +1203,7 @@
      :storage_file_collection_interval: 1.days
      :storage_file_collection_time_utc: 21600
      :task_timeout_check_frequency: 1.hour
+      :vim_performance_states_purge_interval: 1.day


Thought we'd do something closer to 6 month. but I not know the usage of this table.

kbrock · 2018-01-11T22:24:24Z

app/models/mixins/purging_mixin.rb

+      polymorphic_classes(polymorphic_type_column).each do |klass|
+        resource_table = connection.quote_table_name(klass.table_name)
+
+        scope = joins("LEFT OUTER JOIN #{resource_table} ON #{table_name}.resource_id = #{resource_table}.id")


nit: think resource_id may be "#{fk_name}_id?

But to be honest, I almost feel like passing in fk_name is overkill.

I just saw the "#{fk_name}_type" and thought the same pattern should be used for the _id portion. (otherwise if they pass in something other than "resource" I think this will fail.

nit: think resource_id may be "#{fk_name}_id? ... I think this will fail.

Yup, good catch. Will change this.

kbrock · 2018-01-11T22:25:56Z

app/models/mixins/purging_mixin.rb

+
+        scope = joins("LEFT OUTER JOIN #{resource_table} ON #{table_name}.resource_id = #{resource_table}.id")
+                .where(resource_table => {:id => nil})
+                .where("#{table_name}.#{connection.quote_column_name(polymorphic_type_column)} = #{connection.quote(klass.name)}")


can you just do?

where(polymorphic_type_column => klass.name)

Brakeman complained about that 😟

kbrock · 2018-01-11T22:27:03Z

app/models/mixins/purging_mixin.rb

+    end
+
+    def polymorphic_classes(polymorphic_type_column)
+      select(polymorphic_type_column).distinct.pluck(polymorphic_type_column).map(&:constantize)


Think select is not necessary with the pluck.

This method allows for purging by broken polymorphic references rather than just a timestamp.

This uses the new "orhaned" purging mode to remove rows from vim_performance_states which have dangling pointers Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1434918

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1434918

miq-bot · 2018-01-15T18:33:59Z

Checked commits carbonin/manageiq@e189955~...1c3b2c9 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 1 offense detected

app/models/vim_performance_state/purging.rb

❗ - Line 8, Col 9 - Style/PercentLiteralDelimiters - %w-literals should be delimited by [ and ].

agrare

Love the new purge_by_orphaned method 👍

agrare · 2018-01-18T14:16:45Z

@gtanzillo what do you think?

carbonin added core enhancement labels Jan 5, 2018

carbonin requested review from kbrock and Fryguy January 5, 2018 17:07

carbonin changed the title ~~Add orphan purging for vim_performance_states~~ [WIP] Add orphan purging for vim_performance_states Jan 5, 2018

carbonin added the wip label Jan 5, 2018

carbonin force-pushed the add_orphan_purging branch from 85f480c to b21431b Compare January 5, 2018 19:28

carbonin commented Jan 5, 2018

View reviewed changes

carbonin force-pushed the add_orphan_purging branch from b4b6c73 to ef35a34 Compare January 5, 2018 22:19

Fryguy reviewed Jan 8, 2018

View reviewed changes

carbonin force-pushed the add_orphan_purging branch from ef35a34 to b1f6b72 Compare January 8, 2018 19:34

carbonin changed the title ~~[WIP] Add orphan purging for vim_performance_states~~ Add orphan purging for vim_performance_states Jan 8, 2018

carbonin removed the wip label Jan 8, 2018

carbonin assigned gtanzillo Jan 8, 2018

carbonin force-pushed the add_orphan_purging branch from b1f6b72 to eddc4c0 Compare January 8, 2018 19:43

carbonin mentioned this pull request Jan 10, 2018

Use BinaryBlob to store ManagerRefresh::Target payload #16413

Closed

kbrock approved these changes Jan 11, 2018

View reviewed changes

carbonin added 4 commits January 15, 2018 13:26

Add .purge_by_orphaned method to PurgingMixin

e189955

This method allows for purging by broken polymorphic references rather than just a timestamp.

Purge orphans from VimPerformanceState

84ca04c

This uses the new "orhaned" purging mode to remove rows from vim_performance_states which have dangling pointers Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1434918

Add purge schedule for VimPerformanceState

ea5394d

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1434918

Appease brakeman with many wheres

1c3b2c9

carbonin force-pushed the add_orphan_purging branch from 3ec1365 to 1c3b2c9 Compare January 15, 2018 18:29

agrare approved these changes Jan 18, 2018

View reviewed changes

gtanzillo approved these changes Jan 18, 2018

View reviewed changes

gtanzillo added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 18, 2018

gtanzillo merged commit b343ea0 into ManageIQ:master Jan 18, 2018

This was referenced Feb 23, 2018

Adds purging for notifications #17046

Merged

[WIP] Add purging for notifications using expires_in #17073

Closed

NickLaMuro mentioned this pull request Apr 12, 2018

[FINE] Adds purging for notifications #17294

Merged

kbrock mentioned this pull request Jan 30, 2019

Purge dated VimPerformanceStates #11868

Closed

carbonin deleted the add_orphan_purging branch August 16, 2019 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add orphan purging for vim_performance_states #16754

Add orphan purging for vim_performance_states #16754

carbonin commented Jan 5, 2018

carbonin commented Jan 5, 2018 •

edited

Loading

carbonin commented Jan 5, 2018

carbonin Jan 5, 2018

kbrock Jan 11, 2018

Fryguy Jan 8, 2018

Fryguy Jan 8, 2018

Fryguy Jan 8, 2018

Fryguy Jan 8, 2018

Fryguy Jan 8, 2018

Fryguy Jan 8, 2018

Fryguy Jan 8, 2018

Fryguy commented Jan 8, 2018

kbrock left a comment

kbrock Jan 11, 2018

kbrock Jan 11, 2018 •

edited

Loading

carbonin Jan 15, 2018

kbrock Jan 11, 2018

carbonin Jan 15, 2018

kbrock Jan 11, 2018

miq-bot commented Jan 15, 2018

agrare left a comment

agrare commented Jan 18, 2018

Add orphan purging for vim_performance_states #16754

Add orphan purging for vim_performance_states #16754

Conversation

carbonin commented Jan 5, 2018

carbonin commented Jan 5, 2018 • edited Loading

carbonin commented Jan 5, 2018

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

Fryguy commented Jan 8, 2018

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Jan 11, 2018 • edited Loading

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

miq-bot commented Jan 15, 2018

agrare left a comment

Choose a reason for hiding this comment

agrare commented Jan 18, 2018

carbonin commented Jan 5, 2018 •

edited

Loading

kbrock Jan 11, 2018 •

edited

Loading