New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queue reporting work for any zone #18041

Merged
merged 3 commits into from Oct 1, 2018

Conversation

@jrafanie
Member

jrafanie commented Oct 1, 2018

Because reporting is a regional role, meaning, it's possible to do the same reporting work from different zones in the same region, we need to queue reporting work for the nil ("any") zone. By allowing queue put to choose the current server's zone, it was possible for UI appliances without the reporting role to create reporting queue messages that would never get processed.

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

Queue zone is handled in 'put' for regional roles
Notifier is a regional role, so there's no need to set the zone for it
here.  It's done lower in the stack, when 'put' calls 'determine_queue_zone'.

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

@jrafanie jrafanie requested a review from bdunne Oct 1, 2018

@jrafanie

This comment has been minimized.

Member

jrafanie commented Oct 1, 2018

This seems like a good candidate to backport to g and h branches since it's not that risky.

@miq-bot add_label gaprindashvili/yes
@miq-bot add_label hammer/yes

@@ -164,8 +164,8 @@ def queue_generate_table(options = {})
if sync
_async_generate_table(task.id, options)
else
MiqQueue.put(
:queue_name => "reporting",

This comment has been minimized.

@jrafanie

jrafanie Oct 1, 2018

Member

This was most likely the cause of the reporting bug where reporting work would be enqueued for a zone without a reporting role. The absence of the :role => "reporting" and :zone => nil causes this message to be queued for :zone => Zone.my_zone (the current server's zone)

@@ -792,9 +792,8 @@ def queue_report_result(options, res_opts)
_log.info("Adding generate report task to the message queue...")
task = MiqTask.create(:name => "Generate Report: '#{name}'", :userid => options[:userid])
MiqQueue.put(
:queue_name => "reporting",
:role => "reporting",

This comment has been minimized.

@jrafanie

jrafanie Oct 1, 2018

Member

Note, this was probably not causing the issue seen above because it specified the role as reporting, which should set the zone to nil. To simplify things, I moved this code to use the submit_job method to be more consistent.

@@ -108,6 +108,7 @@ def queue_generate_content_for_users_or_group(*args)
MiqQueue.create_with(callback).put_unless_exists(
:queue_name => "reporting",
:role => "reporting",
:zone => nil, # any zone

This comment has been minimized.

@jrafanie

jrafanie Oct 1, 2018

Member

Note, I didn't change this to use submit_job because that method doesn't support put_unless_exists. Therefore, I have to set the zone to nil.

@jrafanie jrafanie requested a review from kbrock Oct 1, 2018

it "#queue_generate_table" do
report.queue_generate_table(:userid => "admin")
task = MiqTask.first
expect(task.name).to eq("Generate Report: '#{report.name}'")

This comment has been minimized.

@bdunne

bdunne Oct 1, 2018

Member

expect(task).to have_attributes(:name => ...)

it "#queue_report_result" do
task_id = report.queue_report_result({:userid => "admin"}, {})
task = MiqTask.find(task_id)
expect(task.name).to eq("Generate Report: '#{report.name}'")

This comment has been minimized.

@bdunne

bdunne Oct 1, 2018

Member

same here?

MiqQueue.put(
:queue_name => "reporting",
MiqQueue.submit_job(
:service => "reporting",

This comment has been minimized.

@jrafanie

jrafanie Oct 1, 2018

Member

To be clear, submit_job will set the role as reporting and because of this, determine_queue_zone, from put, will set the zone to nil for "any" zone.

)
message = MiqQueue.where(:method_name => "_async_generate_table")
message = message.first

This comment has been minimized.

@bdunne

bdunne Oct 1, 2018

Member

find_by?

)
message = MiqQueue.where(:method_name => "build_report_result")
message = message.first

This comment has been minimized.

@bdunne

bdunne Oct 1, 2018

Member

same here

jrafanie added some commits Sep 28, 2018

Leverage submit_job for role based queueing
https://bugzilla.redhat.com/show_bug.cgi?id=1629945

Because the queue put was specifying a queue_name without a role OR
zone, the zone was being set to MiqServer.my_zone.  By leveraging the
higher level method, submit_job, specifying the service of "reporting"
sets the role and because reporting is a regional role, there's no need
to specify the zone, it will default to the "nil" (any) zone.
@jrafanie

This comment has been minimized.

Member

jrafanie commented Oct 1, 2018

Ok, I think I got all the things cleaned up

@bdunne

bdunne approved these changes Oct 1, 2018

@miq-bot

This comment has been minimized.

Member

miq-bot commented Oct 1, 2018

Checked commits jrafanie/manageiq@68dad68~...c5c004c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 0 offenses detected
Everything looks fine. 🍪

@bdunne bdunne merged commit 65ef49c into ManageIQ:master Oct 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bdunne bdunne added this to the Sprint 96 Ending Oct 8, 2018 milestone Oct 1, 2018

@bdunne bdunne self-assigned this Oct 1, 2018

@jrafanie jrafanie deleted the jrafanie:queue_reporting_work_for_any_zone branch Oct 1, 2018

simaishi added a commit that referenced this pull request Oct 2, 2018

@simaishi

This comment has been minimized.

Contributor

simaishi commented Oct 2, 2018

Hammer backport details:

$ git log -1
commit 9d680ed61217a82f61fe964357ad65aac7ea8d25
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Mon Oct 1 14:34:36 2018 -0400

    Merge pull request #18041 from jrafanie/queue_reporting_work_for_any_zone
    
    Queue reporting work for any zone
    
    (cherry picked from commit 65ef49c7d2bc27a56406d18bf9e40d1696871e94)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1629945

@simaishi simaishi added the blocker label Nov 5, 2018

simaishi added a commit that referenced this pull request Nov 5, 2018

@simaishi

This comment has been minimized.

Contributor

simaishi commented Nov 5, 2018

Gaprindashvili backport details:

$ git log -1
commit 652732ba513d7c6810f610e50292119eb959133a
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Mon Oct 1 14:34:36 2018 -0400

    Merge pull request #18041 from jrafanie/queue_reporting_work_for_any_zone
    
    Queue reporting work for any zone
    
    (cherry picked from commit 65ef49c7d2bc27a56406d18bf9e40d1696871e94)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1635255

lfu added a commit to lfu/manageiq-automation_engine that referenced this pull request Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment