Use BinaryBlob to store ManagerRefresh::Target payload #16413

agrare · 2017-11-07T20:24:30Z

In order to parse pod events for pods which might already be deleted we
need to pass the payload for the pod with the refresh target. This can
however lead to very large queue items as the full payload for a pod can
be quite large.

This moves the payload to a binary blob when the target is created with
a payload, and loads the binary blob then deletes it when the target is
created with a payload_id.

Related: ManageIQ/manageiq-providers-kubernetes#164

agrare · 2017-11-07T20:24:59Z

cc @Fryguy @Ladas

chessbyte · 2017-11-07T21:55:19Z

app/models/manager_refresh/target.rb

+ payload_blob = BinaryBlob.find(payload_id)
+ @options[:payload] = payload_blob.binary
+ payload_blob.destroy
+ end


maybe something like this would be clearer?

if options[:payload] @options[:payload_id] = BinaryBlob.create!(:binary => options[:payload]).id elsif options[:payload_id] payload_blob = BinaryBlob.find(options[:payload_id]) @options[:payload] = payload_blob.binary payload_blob.destroy end

I would prefer the actual database access to be moved out of here.

But the if / elsif removes my concern that we are creating the Blob and then deleting it a few instructions later

So there is another problem and the reason why I asked about the 'purging'. To maintain the current workflow of: if there is refresh and the process dies, the restarted process will pick up the same refresh again. That means we can't destroy the payload as part of input. We need to refresh first, then delete the payload in post refresh or some scheduled purger looking at orphaned payloads.

kbrock · 2017-11-07T22:23:55Z

app/models/manager_refresh/target.rb

+
+ payload_id = options[:payload_id]
+ if payload_id
+ payload_blob = BinaryBlob.find(payload_id)


Can we move this to a separate call?

May need to change change k8s InventoryCollectorMixin#parse_target_from_notice
and TargetCollection#add_target

plus there are many uses of Target#load

kbrock · 2017-11-08T15:03:10Z

ok.
How do we clean out the payload?
Is that 100% on the caller now?

agrare · 2017-11-08T15:04:10Z

@kbrock yes now it is on the caller to call target.payload = nil to clear the binary blob

cben · 2017-11-08T15:35:50Z

Is there any purging on BinaryBlob table, in case we leak them?

agrare · 2017-11-08T15:36:23Z

Is there any purging on BinaryBlob table, in case we leak them?

Not that I'm aware of we'll have to add it

Ladas · 2017-11-08T15:43:17Z

@agrare seems like the added specs are failing :-)

agrare · 2017-11-08T16:01:03Z

@Ladas working on it :)

Ladas

Ok, so we will probably need purging, in a case that this fails during post refresh.

But for now, it looks good 👍 (after CI passes)

Ladas · 2017-11-08T12:21:18Z

app/models/manager_refresh/target.rb

+ payload_blob = BinaryBlob.find(payload_id)
+ @options[:payload] = payload_blob.binary
+ payload_blob.destroy
+ end


So there is another problem and the reason why I asked about the 'purging'. To maintain the current workflow of: if there is refresh and the process dies, the restarted process will pick up the same refresh again. That means we can't destroy the payload as part of input. We need to refresh first, then delete the payload in post refresh or some scheduled purger looking at orphaned payloads.

cben · 2017-11-08T16:50:59Z

how does consuming work off MiqQueue normally work? at what point is the row claimed/marked done/deleted? any chance to atomically delete blobs together with it?

Ladas · 2017-11-08T17:05:05Z

@cben @agrare We could wrap MiqQueue item deletion with BinaryBlob deletion into a transaction? Not sure where is the MiqQueue item actually deleted, it must be after post refresh?

agrare · 2017-11-08T18:02:27Z

Okay tests updated for the new interface

Ladas

Awesome

agrare · 2017-11-08T19:19:46Z

The queue item is typically destroyed here destroy_potentially_stale_record after it is delivered. We also destroy queue items here and here.

Fryguy · 2017-11-20T19:46:15Z

app/models/miq_queue.rb

@@ -19,6 +19,7 @@
 #
 class MiqQueue < ApplicationRecord
 belongs_to :handler, :polymorphic => true
+ has_many :binary_blobs, :as => :resource, :dependent => :nullify


The dependent bit is going to add a query to every delete...we may just want to put nothing here.

Fryguy · 2017-11-20T19:46:47Z

Can we extract purging into a separate PR? I think that one is less controversial.

agrare · 2017-11-22T13:57:32Z

Can we extract purging into a separate PR? I think that one is less controversial.

Yes, #16524

Ladas

Looks great now

agrare · 2017-12-13T16:56:21Z

Seems to be a weird github bug, can't reply to the comment directly

The dependent bit is going to add a query to every delete...we may just want to put nothing here.

@Fryguy We would have to change the purging scope which currently looks for where(:resource => nil) any ideas?

The best I can come up with is BinaryBlob.where(:resource_type => 'MiqQueue').where.not('resource_id in (select id from miq_queue)') which is a little gross :/

Ladas · 2018-01-02T11:21:19Z

app/models/binary_blob/purging.rb

@@ -13,7 +13,7 @@ def purge_window_size
 end

 def purge_scope(_older_than = nil)
- where(:resource => nil)
+ where(:resource_type => 'MiqQueue').where.not('resource_id in (select id from miq_queue)')


hm, this would explode, given select id from miq_queue would unpack the whole miq queue (that can go to milions)

refresh_queue_items = MiqQueue.where(:method_name => 'refresh') # rails is smart enough to add .select(:id) where(:resource_type => 'MiqQueue').where.not(:resource_id => refresh_queue_items)

I think :method_name => 'refresh' might be enough to fetch existing refresh MiqQueue items, or maybe more from https://github.com/Ladas/manageiq/blob/acf2ad8e29b9e70858fc99e22614d2112312066c/app/models/ems_refresh.rb#L162 ?

Is purging Zone agnostic? Or we should limit it by zone, if each zone has its own purger?

query looks fast enough in hundred k world

2.4.2 :019 > BinaryBlob.where(:resource_type => 'MiqQueue').where.not(:resource_id => refresh_queue_items).count (92.6ms) SELECT COUNT(*) FROM "binary_blobs" WHERE "binary_blobs"."resource_type" = $1 AND ("binary_blobs"."resource_id" NOT IN (SELECT "miq_queue"."id" FROM "miq_queue" WHERE "miq_queue"."method_name" = $2)) [["resource_type", "MiqQueue"], ["method_name", "refresh"]] Query Trace > lib/extensions/ar_virtual.rb:741:in `calculate' => 351020

Do we only care about purging blobs related to MiqQueue? If not, you can use the polymorphic orphan purging I added in #16754

It uses a join so it should be a bit more efficient than the subquery you have here. Also it won't pull back the ids to the server at all.

@carbonin 👍 looks much better, I'll switch to that when yours is merged

kbrock · 2018-02-26T21:37:27Z

in a perfect world, knowing a blob is used would be hidden - so when we use a different queuing mechanism, it can be used without too much of a change.

Having said that, this change should be a good performance improvement.
can we ship it?

agrare · 2018-02-28T19:40:12Z

@Fryguy what do you think about this? This will get the ManagerRefresh::Target objects out of the queue args for amazon/openstack and help us do the watcher for kubernetes

miq-bot · 2018-03-16T18:12:02Z

This pull request is not mergeable. Please rebase and repush.

Tie MiqQueue to BinaryBlob

Fill resource id only to new targets, to avoid growing query.

miq-bot · 2018-03-16T19:16:46Z

Checked commits agrare/manageiq@fe72473~...41f3e1d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 10 offenses detected

app/models/ems_refresh.rb

❗ - Line 213, Col 12 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.

app/models/miq_queue.rb

❗ - Line 23, Col 3 - Rails/HasManyOrHasOneDependent - Specify a :dependent option.
❗ - Line 23, Col 3 - Rails/InverseOf - Specify an :inverse_of option.

spec/models/manager_refresh/target_spec.rb

❗ - Line 136, Col 7 - Naming/VariableNumber - Use normalcase for variable numbers.
❗ - Line 210, Col 9 - Naming/VariableNumber - Use normalcase for variable numbers.
❗ - Line 222, Col 9 - Naming/VariableNumber - Use normalcase for variable numbers.
❗ - Line 237, Col 9 - Naming/VariableNumber - Use normalcase for variable numbers.
❗ - Line 254, Col 9 - Naming/VariableNumber - Use normalcase for variable numbers.
❗ - Line 271, Col 9 - Naming/VariableNumber - Use normalcase for variable numbers.
❗ - Line 62, Col 7 - Naming/VariableNumber - Use normalcase for variable numbers.

gtanzillo · 2018-05-02T18:33:45Z

@Fryguy Ok to merge this?

agrare added performance providers/inventory labels Nov 7, 2017

agrare assigned Fryguy Nov 7, 2017

agrare requested a review from Ladas November 7, 2017 20:25

agrare force-pushed the manager_refresh_target_payload_binary_blob branch 3 times, most recently from cf0b1e9 to 164b808 Compare November 7, 2017 21:17

chessbyte reviewed Nov 7, 2017

View reviewed changes

kbrock reviewed Nov 7, 2017

View reviewed changes

agrare force-pushed the manager_refresh_target_payload_binary_blob branch 2 times, most recently from e171d87 to f7b8a9b Compare November 8, 2017 14:49

agrare mentioned this pull request Nov 8, 2017

Set the target payload for watch inventory collector ManageIQ/manageiq-providers-kubernetes#164

Closed

Ladas approved these changes Nov 8, 2017

View reviewed changes

agrare force-pushed the manager_refresh_target_payload_binary_blob branch 2 times, most recently from 4e81f9f to 5fcfb49 Compare November 8, 2017 17:56

agrare force-pushed the manager_refresh_target_payload_binary_blob branch from 5fcfb49 to 326371c Compare November 8, 2017 18:04

Ladas approved these changes Nov 8, 2017

View reviewed changes

agrare force-pushed the manager_refresh_target_payload_binary_blob branch 2 times, most recently from 83816cb to 0078066 Compare November 8, 2017 19:27

Ladas mentioned this pull request Nov 20, 2017

Max queued targets threshold #16502

Closed

1 task

Fryguy reviewed Nov 20, 2017

View reviewed changes

agrare force-pushed the manager_refresh_target_payload_binary_blob branch from f0d971c to e8a467c Compare November 22, 2017 13:56

Ladas approved these changes Dec 4, 2017

View reviewed changes

agrare force-pushed the manager_refresh_target_payload_binary_blob branch from e8a467c to 6449ac2 Compare December 20, 2017 14:04

Ladas reviewed Jan 2, 2018

View reviewed changes

agrare force-pushed the manager_refresh_target_payload_binary_blob branch from 6449ac2 to 0855779 Compare January 18, 2018 19:05

agrare force-pushed the manager_refresh_target_payload_binary_blob branch from 0855779 to 006e4b0 Compare February 28, 2018 19:38

miq-bot added the unmergeable label Mar 16, 2018

agrare and others added 8 commits March 16, 2018 15:14

Use BinaryBlob to store ManagerRefresh::Target payload

fe72473

Tie MiqQueue to BinaryBlob

0ae031a

Tie MiqQueue to BinaryBlob

Handle mixed target types updating binary blobs

478b16a

Nullify the miq_queue -> binary_blob relationship

b20afaf

Param doc for payload_id to ManagerRefresh::Target

e1447f0

Fill resource id only to new targets

9bf586b

Fill resource id only to new targets, to avoid growing query.

Do not nullify binary_blob on miq_queue destroy

96abeec

Use new purge_by_orphaned method for binary_blobs

41f3e1d

agrare force-pushed the manager_refresh_target_payload_binary_blob branch from 006e4b0 to 41f3e1d Compare March 16, 2018 19:15

miq-bot removed the unmergeable label Mar 16, 2018

gtanzillo approved these changes May 2, 2018

View reviewed changes

agrare closed this Jul 1, 2018

agrare deleted the manager_refresh_target_payload_binary_blob branch July 16, 2018 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use BinaryBlob to store ManagerRefresh::Target payload #16413

Use BinaryBlob to store ManagerRefresh::Target payload #16413

agrare commented Nov 7, 2017 •

edited

Loading

agrare commented Nov 7, 2017

chessbyte Nov 7, 2017

kbrock Nov 7, 2017

Ladas Nov 8, 2017

kbrock Nov 7, 2017 •

edited

Loading

kbrock commented Nov 8, 2017

agrare commented Nov 8, 2017

cben commented Nov 8, 2017 via email

agrare commented Nov 8, 2017

Ladas commented Nov 8, 2017

agrare commented Nov 8, 2017

Ladas left a comment •

edited

Loading

Ladas Nov 8, 2017

cben commented Nov 8, 2017 via email

Ladas commented Nov 8, 2017

agrare commented Nov 8, 2017

Ladas left a comment

agrare commented Nov 8, 2017 •

edited

Loading

Fryguy Nov 20, 2017

Fryguy commented Nov 20, 2017

agrare commented Nov 22, 2017

Ladas left a comment

agrare commented Dec 13, 2017 •

edited

Loading

Ladas Jan 2, 2018

Ladas Jan 2, 2018

carbonin Jan 10, 2018

agrare Jan 15, 2018

kbrock commented Feb 26, 2018

agrare commented Feb 28, 2018

miq-bot commented Mar 16, 2018

miq-bot commented Mar 16, 2018

gtanzillo commented May 2, 2018

Use BinaryBlob to store ManagerRefresh::Target payload #16413

Use BinaryBlob to store ManagerRefresh::Target payload #16413

Conversation

agrare commented Nov 7, 2017 • edited Loading

agrare commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

kbrock commented Nov 8, 2017

agrare commented Nov 8, 2017

cben commented Nov 8, 2017 via email

agrare commented Nov 8, 2017

Ladas commented Nov 8, 2017

agrare commented Nov 8, 2017

Ladas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben commented Nov 8, 2017 via email

Ladas commented Nov 8, 2017

agrare commented Nov 8, 2017

Ladas left a comment

Choose a reason for hiding this comment

agrare commented Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Fryguy commented Nov 20, 2017

agrare commented Nov 22, 2017

Ladas left a comment

Choose a reason for hiding this comment

agrare commented Dec 13, 2017 • 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

kbrock commented Feb 26, 2018

agrare commented Feb 28, 2018

miq-bot commented Mar 16, 2018

miq-bot commented Mar 16, 2018

gtanzillo commented May 2, 2018

agrare commented Nov 7, 2017 •

edited

Loading

kbrock Nov 7, 2017 •

edited

Loading

Ladas left a comment •

edited

Loading

agrare commented Nov 8, 2017 •

edited

Loading

agrare commented Dec 13, 2017 •

edited

Loading