Consolidate multiple refresher workers for Amazon #15711

juliancheal · 2017-08-02T16:15:42Z

Attempting to consolidate multiple Amazon Refresher workers into a single refresher worker.

This involves letting MiqQueue.peek return queue messages for more than one queue per worker. And, workers that want to handle messages for more than one queue need to return a list of queue names to MiqServer when prefetching queue messages on each heartbeat.

juliancheal · 2017-08-03T18:40:52Z

| Medium | MiqQueue | MiqQueue.peek | SQL Injection | Possible SQL injection near line 276:

MiqQueue.where([("    state = 'ready'
AND (zone IS NULL OR zone = ?)\n 
AND (role IS NULL OR role IN (?))
AND (server_guid IS NULL OR server_guid = ?)
AND (deliver_on IS NULL OR deliver_on <= ?)
AND (priority <= ?)" + +self.where_queue_name(conditions[:queue_name].is_a?(Array))

https://github.com/juliancheal/manageiq/blob/d1490c51f9880dc2f418dfdcaf727e96f64296dd/app/models/miq_queue.rb#L262

juliancheal · 2017-08-03T19:10:35Z

This PR relies on ManageIQ/manageiq-providers-amazon#280

juliancheal · 2017-08-08T11:24:17Z

Getting the issue where when a queue job is created for ManageIQ::Providers::Amazon::CloudManager it's queue_name in miq_queue table is ["ems_1", "ems_3", "ems_2"] and not ems_1.

This isn't happening for Amazon::NetworkManager or ::StorageManager they are both being created in this table with the correct queue_names in this case, ems_2 and ems_3.

/cc @blomquisg, @Fryguy.

blomquisg · 2017-08-08T16:49:06Z

app/models/mixins/per_ems_worker_mixin.rb

-      return nil unless name == "ems"
-      id.to_i
+      if queue_name.kind_of?(Array)
+        queue_names = []


This should be ems_ids instead of queue_names since this block is returning a list of ems_ids

blomquisg · 2017-08-08T16:55:27Z

app/models/miq_worker.rb

        IO.for_fd(socket).close
      end
    end
  end

+  def queue_name


Maybe a quick comment for the queue_name accessor overrides. Something like "the queue_name might be an array for some workers, so serialize the queue name to JSON"

juliancheal · 2017-08-08T17:36:01Z

@miq-bot remove_label wip

blomquisg · 2017-08-08T23:04:05Z

app/models/mixins/per_ems_worker_mixin.rb

-      id.to_i
+      if queue_name.kind_of?(Array)
+        ems_ids = []
+        ems_ids.each do |ems_id|


Should be queue_name instead of ems_ids here ... I couldn't figure out why this was always returning an empty array.

Also, this isn't converting the id to an int.

Though, leaning towards smaller methods, maybe something like this is better anyway:

def parse_ems_id(queue_name) return nil if queue_name.blank? name, id = queue_name.split("_") return nil unless name == "ems" id.to_i end def ems_id_from_queue_name(queue_name) queue_name.kind_of?(Array) ? queue_name.collect { |q| parse_ems_id(q) }.flatten : parse_ems_id(queue_name) end

This way, the parsing of the ems_id is done in one place one way, not in two places in the if and else block, slightly differently.

This feels redundant (having two separate calls to parse_ems_id), but we need to be able to return arrays when appropriate and single values when appropriate.

Doh what a silly mistake!

You really like your foo.collect methods :)

blomquisg · 2017-08-08T23:27:11Z

With the change I suggested, I now see these workers:

worker_type	queue_name	status
ManageIQ::Providers::Amazon::CloudManager::EventCatcher	ems_1	started
ManageIQ::Providers::Amazon::CloudManager::RefreshWorker	["ems_1", "ems_3", "ems_2"]	started
MiqEventHandler	ems	started
MiqGenericWorker	generic	started
MiqScheduleWorker		started

And I saw this in the log:

[----] I, [2017-08-08T19:16:40.283480 #32273:2af621ed5110]  INFO -- : MIQ(ManageIQ::Providers::Amazon::CloudManager::Refresher#refresh) Refreshing all targets...
.....
[----] I, [2017-08-08T19:16:55.814041 #32273:2af621ed5110]  INFO -- : MIQ(ManageIQ::Providers::Amazon::CloudManager::Refresher#refresh) Refreshing all targets...Complete
.....
[----] I, [2017-08-08T19:16:55.837777 #32273:2af621ed5110]  INFO -- : MIQ(ManageIQ::Providers::Amazon::StorageManager::Ebs::Refresher#refresh) Refreshing all targets...
.....
[----] I, [2017-08-08T19:16:57.545894 #32273:2af621ed5110]  INFO -- : MIQ(ManageIQ::Providers::Amazon::StorageManager::Ebs::Refresher#refresh) Refreshing all targets...Complete
.....
[----] I, [2017-08-08T19:16:57.561174 #32273:2af621ed5110]  INFO -- : MIQ(ManageIQ::Providers::Amazon::NetworkManager::Refresher#refresh) Refreshing all targets...
.....
[----] I, [2017-08-08T19:17:01.645353 #32273:2af621ed5110]  INFO -- : MIQ(ManageIQ::Providers::Amazon::NetworkManager::Refresher#refresh) Refreshing all targets...Complete

Showing all refreshes being handle by the same worker PID!

Ladas

The SQL injection warning is a bit weird. Otherwise this starts to look great, awesome job @blomquisg & @juliancheal :-)

Ladas · 2017-08-16T07:31:06Z

@juliancheal @blomquisg so, what is missing here?

The SQL injection warning seems like a false positive, the ? should sanitize all values passed. We can maybe try "{string1}/n{string2}" instead of string1 + string2? Or any other idea to make it go away? :-)

miq-bot · 2017-08-17T22:02:23Z

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

Also added a comment on to why we're overriding queue_name.

To be re-copped another day.

juliancheal · 2017-08-21T15:04:01Z

@agrare all unrelated cops removed 😢

agrare · 2017-08-21T15:36:34Z

@juliancheal thanks!

juliancheal · 2017-08-21T15:37:11Z

Looks like Breakman is unhappy again, it seemed ok earlier. I'll try @Ladas' fix.

juliancheal · 2017-08-21T16:01:49Z

I can't workout how to resolve this Brakeman issue. It's the same code in self.get as in self.peak I think.

https://github.com/ManageIQ/manageiq/pull/15711/files#diff-d8c450b508868941c2b0c5884e718324R207

&

https://github.com/ManageIQ/manageiq/pull/15711/files#diff-d8c450b508868941c2b0c5884e718324R262

Ladas · 2017-08-21T16:41:02Z

@juliancheal ok, a stupid solution: can you try to define MIQ_QUEUE_GET_ARRAY and the same for peek, to see if it passes as 1 query and the issue is the concatenation?

Then the best solution would be to rewrite it to arel query I suppose, it's horrible to pass array like this. Or, we can put there a TODO to write AREL and unify those queries. :-D

juliancheal · 2017-08-21T16:44:03Z

@Ladas I'm not sure I understand this bit:

@juliancheal ok, a stupid solution can you try to define MIQ_QUEUE_GET_ARRAY and the same for peek, to see if it passes as 1 query and the issue is the concatenation?

Totally would rather rewrite this to use Arel query, @blomquisg and I discussed that, but thought best not to bite off more than we can chew in this PR :)

Then the best solution would be to rewrite it to arel query I suppose, it's horribly to pass array like this. Or, we can put there a TODO to write AREL and unify those queries. :-D

Ladas · 2017-08-21T16:48:43Z

@juliancheal to write 2 full queries MIQ_QUEUE_GET and MIQ_QUEUE_GET_ARRAY, 1 having miq_queue = ? and another miq_queue IN (?), the same for the other. It's super ugly, but it will help us rule out that the query is wrong and there is a false positive because we are using + to concat 2 queries.

This isn't a tidy replacement, but will be replaced by an Arel query at some point.

juliancheal · 2017-08-22T13:33:55Z

@Ladas, @Fryguy, @agrare, @blomquisg so it seems that concatenating the sql query was causing Brakeman to freakout. I've replaced it with two queries and that still seems to work, and also makes Brakeman happy.

I think this PR is good to go. What do y'all think?

Ladas · 2017-08-22T13:56:55Z

app/models/miq_queue.rb

    AND (role IS NULL OR role IN (?))
    AND (server_guid IS NULL OR server_guid = ?)
    AND (deliver_on IS NULL OR deliver_on <= ?)
    AND (priority <= ?)
+    AND queue_name in (?)


@kbrock is there a perf issue with always using IN? I think PG was smart enough to optimize that if it's e.g queue_name IN ('generic')

durandom · 2017-08-24T13:50:44Z

app/models/manageiq/providers/base_manager.rb

@@ -4,6 +4,10 @@ class BaseManager < ExtManagementSystem

    include Inflector::Methods

+    has_many :child_managers,


Does it matter where the asscociation is defined?
I need the :child_managers here too, but for orchestrate_destroy, which is implemented in ExtManagementSystem

Ooh fancy. Having child_managers on EMS seems to make more sense.

Yeah I agree child_managers at the ems level is better

Should I remove this then? And then just wait until @durandom's PR is merged?

@juliancheal or you can put in a PR to just add :child_managers

#15889 child manager PR.

So all managers can now find their children.

juliancheal · 2017-08-24T15:39:01Z

Of course this is now going to break until #15889 is merged. 🍰

miq-bot · 2017-08-24T15:43:13Z

Checked commits juliancheal/manageiq@80e7848~...67905a3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 7 offenses detected

app/models/miq_queue.rb

❗ - Line 111, Col 31 - Style/GlobalVars - Do not introduce global variables.
❗ - Line 113, Col 19 - Style/ExtraSpacing - Unnecessary spacing detected.
❗ - Line 113, Col 29 - Style/SpaceAroundOperators - Operator = should be surrounded by a single space.
❗ - Line 253, Col 5 - Style/CommentAnnotation - Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem.
❗ - Line 268, Col 26 - Style/MutableConstant - Freeze mutable objects assigned to constants.
❗ - Line 392, Col 16 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

app/models/miq_worker.rb

❗ - Line 350, Col 5 - Style/RedundantBegin - Redundant begin block detected.

agrare · 2017-08-24T16:13:10Z

@juliancheal dependent PR merged

juliancheal · 2017-08-24T16:15:59Z

@agrare you are a gentleman and a scholar.

agrare · 2017-08-24T16:16:55Z

https://media.giphy.com/media/jt9FFt6f2Q0Te/giphy.gif

agrare · 2017-08-29T13:11:46Z

app/models/mixins/per_ems_worker_mixin.rb

      return nil if queue_name.blank?
      name, id = queue_name.split("_")
      return nil unless name == "ems"
      id.to_i
    end

+    def ems_id_from_queue_name(queue_name)
+      queue_name.kind_of?(Array) ? queue_name.collect { |q| parse_ems_id(q) }.flatten : parse_ems_id(queue_name)


@juliancheal might Array(queue_name).collect ... make this simpler? Would just be one case then

So you mean, if queue_name isn't an Array turn in into one anyway?

Yeah actually now that I think of it I like this better since 90% of the time it'll be not an array :)

right, we will need only IN query for it, but I think we can do that. Seems like PG is smart enough to convert IN with 1 element array into = (look at the index cond below, same applies for seq. scan)

(1.0ms) EXPLAIN ANALYZE SELECT "container_images".* FROM "container_images" WHERE (id IN (1710695)) => [["Index Scan using container_images_pkey on container_images (cost=0.43..8.45 rows=1 width=685) (actual time=0.026..0.027 rows=1 loops=1)"], [" Index Cond: (id = 1710695)"], ["Planning time: 0.220 ms"], ["Execution time: 0.076 ms"]] 2.3.0 :009 > ActiveRecord::Base.connection.query("EXPLAIN ANALYZE #{ContainerImage.where('id=1710695').to_sql}") (1.0ms) EXPLAIN ANALYZE SELECT "container_images".* FROM "container_images" WHERE (id=1710695) => [["Index Scan using container_images_pkey on container_images (cost=0.43..8.45 rows=1 width=685) (actual time=0.026..0.028 rows=1 loops=1)"], [" Index Cond: (id = 1710695)"], ["Planning time: 0.147 ms"], ["Execution time: 0.076 ms"]]

agrare · 2017-08-29T13:21:36Z

This looks good to me, @juliancheal is there anything else that this depends on or is it okay to merge?

juliancheal · 2017-08-29T13:24:42Z

This one needs merging, ManageIQ/manageiq-providers-amazon#280 but I don't know which one first tbh.

agrare

LGTM

miq-bot added the wip label Aug 2, 2017

chessbyte assigned blomquisg Aug 2, 2017

chessbyte added providers/amazon providers/inventory refactoring labels Aug 2, 2017

chessbyte requested a review from Fryguy August 2, 2017 16:37

blomquisg force-pushed the consolidate_amazon_workers branch from 28f115f to 6786c74 Compare August 3, 2017 17:20

blomquisg assigned Fryguy and agrare and unassigned blomquisg and Fryguy Aug 3, 2017

blomquisg changed the title ~~[WIP] This is experimental.~~ [WIP] Consolidate multiple refresher workers Aug 3, 2017

juliancheal mentioned this pull request Aug 3, 2017

Overriding queue name to generate an array of queue names ManageIQ/manageiq-providers-amazon#280

Merged

juliancheal force-pushed the consolidate_amazon_workers branch 3 times, most recently from 972caea to 6a71980 Compare August 8, 2017 11:21

blomquisg reviewed Aug 8, 2017

View reviewed changes

juliancheal changed the title ~~[WIP] Consolidate multiple refresher workers~~ Consolidate multiple refresher workers for Amazon Aug 8, 2017

miq-bot removed the wip label Aug 8, 2017

blomquisg reviewed Aug 8, 2017

View reviewed changes

Ladas reviewed Aug 15, 2017

View reviewed changes

miq-bot added the unmergeable label Aug 17, 2017

Julian Cheal added 2 commits August 21, 2017 15:47

Updated ems_id_from_queue_name with better variable naming

bc0bf25

Also added a comment on to why we're overriding queue_name.

Fixing merge conflict mistake.

1d07c43

juliancheal force-pushed the consolidate_amazon_workers branch from d4a0be6 to 1d07c43 Compare August 21, 2017 14:48

Removed unrelated Rubocop fixes.

af22185

To be re-copped another day.

Fixing Brakeman sql injection warning.

3c32915

This isn't a tidy replacement, but will be replaced by an Arel query at some point.

Ladas reviewed Aug 22, 2017

View reviewed changes

durandom reviewed Aug 24, 2017

View reviewed changes

juliancheal mentioned this pull request Aug 24, 2017

Adding Child Managers to EMS #15889

Merged

Removed child managers as it's been moved up to EMS

67905a3

So all managers can now find their children.

agrare reviewed Aug 29, 2017

View reviewed changes

agrare approved these changes Aug 29, 2017

View reviewed changes

agrare merged commit bf8f9a8 into ManageIQ:master Aug 29, 2017

agrare added this to the Sprint 68 Ending Sep 4, 2017 milestone Aug 29, 2017

carbonin mentioned this pull request Oct 6, 2017

[WIP] Do not discard worker options - fixes fork removal PR #16140

Closed

Fryguy added the providers/cloud label Jan 24, 2020

		@@ -4,6 +4,10 @@ class BaseManager < ExtManagementSystem

		include Inflector::Methods

		has_many :child_managers,

Consolidate multiple refresher workers for Amazon #15711

Consolidate multiple refresher workers for Amazon #15711

Conversation

juliancheal commented Aug 2, 2017 • edited by blomquisg Loading

juliancheal commented Aug 3, 2017 • edited Loading

juliancheal commented Aug 3, 2017

juliancheal commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliancheal commented Aug 8, 2017

blomquisg Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blomquisg commented Aug 8, 2017

Ladas left a comment

Choose a reason for hiding this comment

Ladas commented Aug 16, 2017

miq-bot commented Aug 17, 2017

juliancheal commented Aug 21, 2017

agrare commented Aug 21, 2017

juliancheal commented Aug 21, 2017

juliancheal commented Aug 21, 2017

Ladas commented Aug 21, 2017 • edited Loading

juliancheal commented Aug 21, 2017

Ladas commented Aug 21, 2017 • edited Loading

juliancheal commented Aug 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliancheal Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

juliancheal commented Aug 24, 2017

miq-bot commented Aug 24, 2017

agrare commented Aug 24, 2017

juliancheal commented Aug 24, 2017

agrare commented Aug 24, 2017

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

agrare commented Aug 29, 2017

juliancheal commented Aug 29, 2017

agrare left a comment

Choose a reason for hiding this comment

juliancheal commented Aug 2, 2017 •

edited by blomquisg

Loading

juliancheal commented Aug 3, 2017 •

edited

Loading

blomquisg Aug 8, 2017 •

edited

Loading

Ladas commented Aug 21, 2017 •

edited

Loading

Ladas commented Aug 21, 2017 •

edited

Loading

juliancheal commented Aug 22, 2017 •

edited

Loading

juliancheal Aug 24, 2017 •

edited

Loading