Add purge timer for archived entities #14322

zeari · 2017-03-14T13:14:28Z

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1428595
Periodically purge all archived container entities that were deleted more than 6 months ago.
This change currently applies only to containers but it may be used generically for any other areas of the project similarly to ArchivedMixin

@simon3z @moolitayer PTAL

@miq-bot add_label providers/containers, wip

Edit: I tested this out locally in the console with:

MiqQueue.put_unless_exists(:class_name => "Container", :method_name => "purge_timer", :zone => MiqServer.my_zone, :priority => MiqScheduleWorker::Runner::SCHEDULE_MEDIUM_PRIORITY)

Fryguy · 2017-03-14T14:49:36Z

I expected to see the PurgingMixin somewhere, but I don't see it. How does purge_scope get used?

Fryguy · 2017-03-14T14:50:33Z

app/models/container/purging.rb

+  module Purging
+    include ArchivedPurgingMixin
+  end
+end


I don't think you need these layers of indirection. I think you can just include ArchivedPurgingMixin directly into the model.

Is this working out of the box? If so, that is cool.

Thought you'd have to define scope or something.

Fryguy · 2017-03-14T14:51:09Z

app/models/mixins/archived_purging_mixin.rb

+
+  def self.included(base)
+    base.extend(ClassMethods)
+  end


This is unnecessary with ActiveSupport::Concern because it does this for you.

jrafanie · 2017-03-14T16:00:35Z

@kbrock can you also take a look? 🙇

zeari · 2017-03-30T13:19:50Z

config/settings.yml

@@ -1363,6 +1367,7 @@
        :poll_method: :normal
        :queue_timeout: 120.minutes
    :schedule_worker:
+      :archived_entities_purge_interval: 1.day


@simon3z @moolitayer Are these the settings we want for container entities?

:archived_ is not descriptive here. probably want to use container_

zeari · 2017-03-30T13:31:57Z

I expected to see the PurgingMixin somewhere, but I don't see it. How does purge_scope get used?

@Fryguy i didnt understand purging correctly. I made some changes and mostly borrowed examples from app/models/event_stream/purging.rb.

zeari · 2017-03-30T14:38:39Z

Added tests

zeari · 2017-03-30T14:51:14Z

@miq-bot remove_label wip

kbrock

We tend to have a Purging module for each class.
But this seems to do well.

Was hoping ArchivedMixin would have defined purge and purge_queue method for us.

kbrock · 2017-03-31T15:21:28Z

app/models/miq_schedule_worker/jobs.rb

@@ -121,6 +121,14 @@ def miq_report_result_purge_timer
    queue_work(:class_name => "MiqReportResult", :method_name => "purge_timer", :zone => nil)
  end

+  def archived_entities_purge_timer


@jrafanie up until now, Job inserts a single work record and the model's Purge coordinates multiple records.

But this is nice that it reduces the number of settings timers

@durandom something to add to the pluggable backlog ... how can we allow providers (and separate provider repos) to engage with different timers?

kbrock · 2017-03-31T15:23:25Z

app/models/mixins/archived_purging_mixin.rb

+        :class_name  => name,
+        :method_name => "purge",
+        :queue_name  => "ems",
+        :args        => [ts]


:args is not unique, so we need to get it out of here.

MiqQueue.create_with(:args => [ts]).put_unless_exists( :class_name => name, :method_name => "purge", :queue_name => "ems" )

kbrock · 2017-03-31T15:25:50Z

app/models/mixins/archived_purging_mixin.rb

+
+    alias_method :purge_timer, :purge_queue
+
+    def purge_timer


Either use alias_method OR define purge_timer

if you go with alias_method, add a ts ||= purge_date

kbrock · 2017-03-31T15:27:01Z

app/models/mixins/archived_purging_mixin.rb

+      window ||= purge_window_size
+
+      total = where(arel_table[:deleted_on].lteq(older_than)).delete_in_batches(window, limit) do |count, _total|
+        _log.info("Purging #{count} events.")


this is not deleting events.

update the name here and 3 lines down

kbrock · 2017-03-31T15:41:24Z

spec/models/mixins/archived_purging_mixin_spec.rb

+describe ArchivedPurgingMixin do
+  let(:example_class) { Container }
+
+  context "::Purging" do


nit: Remove this line since we do not have a module named ::Purging

kbrock · 2017-03-31T15:44:00Z

app/models/container.rb

@@ -2,6 +2,7 @@ class Container < ApplicationRecord
  include SupportsFeatureMixin
  include NewWithTypeStiMixin
  include ArchivedMixin


remove ArchivedMixin (since that is included by ArchivedMixin) - (5 changes)

@kbrock Not sure what you mean here. Do you mean to unify ArchivedMIxin and ArchivedPurgingMixin? I think that would work well

@zeari well, merging them is one option.

But I was saying that since ArchivedPurgingMixin includes ArchivedMixin, there is no reason to keep the include ArchivedMixin in these files.

since ArchivedPurgingMixin includes ArchivedMixin

@kbrock It doesnt though(unless im missing something)

@zeari Aah, I was referring the https://github.com/ManageIQ/manageiq/pull/14322/files#diff-bfb9598d272bb5d013105604b37dcabaR3

Ok, now that I look at the code, please remove it from ArchivedPurgingMixin. Do you think a better name would be PurgingContinerMixin?

kbrock · 2017-03-31T15:56:50Z

spec/models/mixins/archived_purging_mixin_spec.rb

+        expect(q.first).to have_attributes(
+                             :class_name  => example_class.name,
+                             :method_name => "purge",
+                             :args        => [purge_time]


bonus: this test ensures that the create_with(:args => ...) works well too.

zeari · 2017-03-31T16:48:05Z

app/models/container_image.rb

@@ -27,9 +28,6 @@ class ContainerImage < ApplicationRecord
  serialize :exposed_ports, Hash
  serialize :environment_variables, Hash

-  # Needed for scanning & tagging action
-  delegate :my_zone, :to => :ext_management_system
-


ContainerImage didnt use ArchivedMixin but definitely should

kbrock · 2017-03-31T17:24:28Z

@zeari
Ok, so this seems like it is "container" related not "archived" related.
Let's change the names accordingly.

Every model used to define different purging methods but we were able to consolidate most of those methods into PurgingMixin. Think we're down to just 2 patterns.
So, if you feel you are duplicating a lot of code from the ::Purging classes, consider moving a few of the methods into the mixin.

zeari · 2017-04-02T09:29:44Z

@zeari
Ok, so this seems like it is "container" related not "archived" related.
Let's change the names accordingly.

@kbrock
I named it ArchivedPurgingMixin with this comment by @Fryguy in mind: #13686 (comment)

I don't like that this is container specific, in particular the name of the mixin implies that it can only be used for containers, which isn't necessarily true.

I thought it would apply here too. How about unifying this new purging mixin with the pre-exisisting ArchivedMixin? btw we can also change archived_entities_purge_timer to go over all models that include the mixin instead of listing each of them by name

kbrock · 2017-04-05T19:13:12Z

The variable is for running containers. Would like the variable renamed. "Purging anything that has an ems archived" seems a little vague.

A majority of the mixin seems like basic pruning functionality. Can we purge, purge_timer, and purge_queue into the parent class.

Can you use MiqReportResult::Purging as a basic implementation?

def purge_timer
  purge_queue(*purge_mode_and_value)
end

I'm pretty sure we only have 2 different versions of this method.
Ping me if you want me to create a PR for this

kbrock · 2017-04-07T13:22:55Z

app/models/mixins/archived_purging_mixin.rb

+      )
+    end
+
+    def purge_timer


see other note, think purge_timer and purge_queue and purge can be removed.

def purge_scope(older_than) where(arel_table[:deleted_on].lteq(older_than)) end

kbrock · 2017-04-07T13:24:55Z

Sorry, @zeari - I did not forget about you here.

This got me looking at all the various purging examples out there. Put in #14676 to make the implementations more consistent.

There are 3 examples of purging that I like a lot:

Ones that want :remaining for purging DriftState::Purging and MiqReportResult::Purging.
simpler ones PolicyEvent::Purging

Think the simpler one meets your needs better. It should simplify your implementation greatly.

I think of PurgingMixin as the general implementation, and this module should have the name Container in it. For all other implementations, we implemented one purge module per model. I understand the desire for the generalization here. So while out of the norm, I'm open to it.

jrafanie · 2017-04-07T21:52:42Z

@kbrock should this wait until the normalization is merged in #14676?

jrafanie · 2017-04-07T21:53:50Z

@kbrock Or, do you want to get this PR merged and add a commit in #14676 to normalize this for containers?

I'm guessing you're probably best to make the change since you've been refactoring them all.

zeari · 2017-04-12T13:07:59Z

Added modules and specs for each entity

@kbrock @simon3z Please review

miq-bot · 2017-04-12T13:08:28Z

Checked commit zeari@88dd01c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
18 files checked, 0 offenses detected
Everything looks good. 👍

kbrock

Looking good.

Straight forward and easy to figure out in the future

Let's hope it will keep these tables from growing too large.

zeari · 2017-04-13T15:24:55Z

ping @simon3z can you approve the changes?

simon3z

LGTM 👍

simon3z · 2017-04-27T12:16:55Z

@miq-bot add_label fine/yes euwe/yes

miq-bot · 2017-04-27T12:17:06Z

@simon3z Cannot apply the following label because they are not recognized: fine/yes euwe/yes

simon3z · 2017-04-27T12:17:15Z

@miq-bot add_label fine/yes, euwe/yes

Add purge timer for archived entities (cherry picked from commit b8b9c81) https://bugzilla.redhat.com/show_bug.cgi?id=1448148

simaishi · 2017-05-04T15:52:48Z

Euwe backport details:

$ git log -1
commit 3cc76af9a8a66a1777120527858f0d166b43c867
Author: Keenan Brock <[email protected]>
Date:   Tue Apr 18 11:44:10 2017 -0400

    Merge pull request #14322 from zeari/purge_containers
    
    Add purge timer for archived entities
    (cherry picked from commit b8b9c81719111cdbc2f27e8fee74e572bbb9c128)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1448148

simaishi · 2017-05-04T16:43:40Z

@zeari @kbrock It looks like this PR depends on #14676. Travis is now failing on Euwe after backporting this PR. Not sure if it's ok to backport #14676 (it's currently not marked as euwe/yes) to Euwe branch, or if you'd rather create a separate Euwe PR to address the Travis failure.

Add purge timer for archived entities (cherry picked from commit b8b9c81) https://bugzilla.redhat.com/show_bug.cgi?id=1458333

simaishi · 2017-06-02T15:26:53Z

Fine backport details:

$ git log -1
commit 45b315a1eee6980730cac7b24f35a165837aa3ce
Author: Keenan Brock <[email protected]>
Date:   Tue Apr 18 11:44:10 2017 -0400

    Merge pull request #14322 from zeari/purge_containers
    
    Add purge timer for archived entities
    (cherry picked from commit b8b9c81719111cdbc2f27e8fee74e572bbb9c128)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1458333

miq-bot added providers/containers wip labels Mar 14, 2017

Fryguy assigned Fryguy and kbrock and unassigned Fryguy Mar 14, 2017

Fryguy reviewed Mar 14, 2017

View reviewed changes

zeari force-pushed the purge_containers branch 2 times, most recently from 34a3755 to f476c39 Compare March 30, 2017 13:18

zeari commented Mar 30, 2017

View reviewed changes

zeari force-pushed the purge_containers branch from f476c39 to a5b93cb Compare March 30, 2017 13:49

zeari force-pushed the purge_containers branch from a5b93cb to 71adb8c Compare March 30, 2017 14:48

zeari changed the title ~~[WIP] Add purge timer for archived entities~~ Add purge timer for archived entities Mar 30, 2017

miq-bot removed the wip label Mar 30, 2017

kbrock reviewed Mar 31, 2017

View reviewed changes

zeari force-pushed the purge_containers branch 2 times, most recently from 47318e0 to 92f3e88 Compare March 31, 2017 16:46

zeari commented Mar 31, 2017

View reviewed changes

kbrock reviewed Apr 7, 2017

View reviewed changes

zeari force-pushed the purge_containers branch from b3f0b33 to 6e40f76 Compare April 12, 2017 12:13

zeari mentioned this pull request Apr 12, 2017

MiqQueue consistency with purging #14676

Merged

Add purge timer for archived entities

88dd01c

zeari force-pushed the purge_containers branch from 6e40f76 to 88dd01c Compare April 12, 2017 13:05

kbrock approved these changes Apr 13, 2017

View reviewed changes

simon3z approved these changes Apr 14, 2017

View reviewed changes

kbrock merged commit b8b9c81 into ManageIQ:master Apr 18, 2017

kbrock added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 18, 2017

durandom mentioned this pull request Apr 25, 2017

Pluggable Providers TODO #14840

Closed

38 tasks

chessbyte added the enhancement label Apr 26, 2017

miq-bot added euwe/yes labels Apr 27, 2017

simaishi pushed a commit that referenced this pull request May 4, 2017

Merge pull request #14322 from zeari/purge_containers

3cc76af

Add purge timer for archived entities (cherry picked from commit b8b9c81) https://bugzilla.redhat.com/show_bug.cgi?id=1448148

simaishi added euwe/backported and removed euwe/yes labels May 4, 2017

kbrock mentioned this pull request May 4, 2017

[EUWE] partial Add purge timer for archived entities #15008

Merged

simaishi pushed a commit that referenced this pull request Jun 2, 2017

Merge pull request #14322 from zeari/purge_containers

45b315a

Add purge timer for archived entities (cherry picked from commit b8b9c81) https://bugzilla.redhat.com/show_bug.cgi?id=1458333

simaishi added fine/backported and removed fine/yes labels Jun 2, 2017

zeari mentioned this pull request Jun 5, 2017

Purging Archived Container Records #11518

Closed

cben mentioned this pull request Mar 18, 2018

Purging of ContainerQuota & ContainerQuotaItem #17167

Merged

Add purge timer for archived entities #14322

Add purge timer for archived entities #14322

Conversation

zeari commented Mar 14, 2017 • edited Loading

Fryguy commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeari commented Mar 30, 2017 • edited Loading

zeari commented Mar 30, 2017

zeari commented Mar 30, 2017

kbrock 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

Choose a reason for hiding this comment

kbrock commented Mar 31, 2017

zeari commented Apr 2, 2017 • edited Loading

kbrock commented Apr 5, 2017 • edited Loading

kbrock Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

kbrock commented Apr 7, 2017 • edited Loading

jrafanie commented Apr 7, 2017

jrafanie commented Apr 7, 2017

zeari commented Apr 12, 2017

miq-bot commented Apr 12, 2017

kbrock left a comment

Choose a reason for hiding this comment

zeari commented Apr 13, 2017

simon3z left a comment

Choose a reason for hiding this comment

simon3z commented Apr 27, 2017

miq-bot commented Apr 27, 2017

simon3z commented Apr 27, 2017

simaishi commented May 4, 2017

simaishi commented May 4, 2017

simaishi commented Jun 2, 2017

zeari commented Mar 14, 2017 •

edited

Loading

zeari commented Mar 30, 2017 •

edited

Loading

zeari commented Apr 2, 2017 •

edited

Loading

kbrock commented Apr 5, 2017 •

edited

Loading

kbrock Apr 7, 2017 •

edited

Loading

kbrock commented Apr 7, 2017 •

edited

Loading