Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Queue the VM scan command after vm_scan_start event is handled by automate. #15228

Merged
merged 2 commits into from
May 30, 2017
Merged

Queue the VM scan command after vm_scan_start event is handled by automate. #15228

merged 2 commits into from
May 30, 2017

Conversation

lfu
Copy link
Member

@lfu lfu commented May 25, 2017

All events are handled by automate role server. And VM scan is handled only by SmartState role server.
Queueing the VM scan signal for next step allows the separation of these two server roles.

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

@miq-bot assign @gmcculloug
@miq-bot add_label bugs, euwe/yes, fine/yes, smart state

@miq-bot
Copy link
Member

miq-bot commented May 25, 2017

@lfu Cannot apply the following label because they are not recognized: bugs

@lfu
Copy link
Member Author

lfu commented May 25, 2017

@miq-bot add_label bug

@miq-bot miq-bot added the bug label May 25, 2017
@lfu lfu force-pushed the vm_scan_with_automate_1454936 branch from ac170ee to bbf41a5 Compare May 25, 2017 21:53
gmcculloug
gmcculloug reviewed May 25, 2017
View reviewed changes
app/models/vm_scan.rb Outdated
:instance_id => id,
:method_name => "signal",
:args => [:abort, message, "error"],
:zone => from_zone, # continue in the same zone where the scan event is created
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update the comment to say: # Continue in the same zone were the scan job was initiated.

@lfu lfu force-pushed the vm_scan_with_automate_1454936 branch from bbf41a5 to ed22062 Compare May 25, 2017 22:07
gmcculloug
gmcculloug reviewed May 30, 2017
View reviewed changes
app/models/vm_scan.rb Outdated
unless status == 'ok'
_log.error("Status = #{status}, message = #{message}")
signal(:abort, message, "error")
MiqQueue.put_unless_exists(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbrock @mkanoor Should we avoid using MiqQueue.put_unless_exists?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lfu Is there a reason to queue this one? It is early enough in the VMscan process that there should not be any requirement to talk to the provider. Could this remain as the signal(:abort, message, "error") call?

@mkanoor
Copy link
Contributor

mkanoor commented May 30, 2017

@gmcculloug Can we justify the use case. Are we expecting duplicate entries being created in a short time frame. Could this just be MiqQueue.put

@lfu
Copy link
Member Author

lfu commented May 30, 2017

User might run the task again if it is backed up for some reason. There is no way from UI side to prevent running multiple scanning for the same target.

@gmcculloug
Copy link
Member

@mkanoor @lfu The VM scan method is already checking if there is a scan request that has not run yet https://github.com/ManageIQ/manageiq/blob/master/app/models/vm_or_template/scanning.rb#L7.

This logic is all happening after we have initiated the scan so I do see multiple scans running at the same time being an issue. Both of the MiqQueue.put calls are already for a specific instance of VmScan. My thinking is this could be just MiqQueue.put.

@mkanoor
Copy link
Contributor

mkanoor commented May 30, 2017

@lfu @gmcculloug
Now that we have notification can that be used to inform the user that the scan has been scheduled, running, ended (with success/failure)

@gmcculloug
Copy link
Member

Yes, but I do not think that applies to this PR as we still need to handle the states of the scan job.

@lfu lfu force-pushed the vm_scan_with_automate_1454936 branch from ed22062 to 1c18b89 Compare May 30, 2017 21:03
gmcculloug
gmcculloug reviewed May 30, 2017
View reviewed changes
spec/models/vm_scan_spec.rb Outdated
end

it "does not send signal :abort if passed status is 'ok' " do
expect(MiqQueue).not_to receive(:put_unless_exists).with(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test needs to be updated as it is still based on MiqQueue.

lfu added 2 commits May 30, 2017 17:51
@lfu
Queue the VM scan command after vm_scan_start event is handled by aut…
234af0a 
…omate.

All events are handled by automate role server. And VM scan is handled only by SmartState role server.
Queueing the VM scan signal for next step allows the separation of these two server roles.

https://bugzilla.redhat.com/show_bug.cgi?id=1454936
@lfu
Fix rubocop warnings.
e86b4a0 
https://bugzilla.redhat.com/show_bug.cgi?id=1454936
@lfu lfu force-pushed the vm_scan_with_automate_1454936 branch from 1c18b89 to e86b4a0 Compare May 30, 2017 21:51
@miq-bot
Copy link
Member

miq-bot commented May 30, 2017

Checked commits lfu/manageiq@234af0a~...e86b4a0 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@gmcculloug gmcculloug merged commit 7a7350e into ManageIQ:master May 30, 2017
@gmcculloug gmcculloug added this to the Sprint 62 Ending Jun 5, 2017 milestone May 30, 2017
simaishi pushed a commit that referenced this pull request Jun 1, 2017
@gmcculloug @simaishi
Merge pull request #15228 from lfu/vm_scan_with_automate_1454936
99cebfa 
Queue the VM scan command after vm_scan_start event is handled by automate.
(cherry picked from commit 7a7350e)

https://bugzilla.redhat.com/show_bug.cgi?id=1457899
@simaishi
Copy link
Contributor

simaishi commented Jun 1, 2017

Euwe backport details:

$ git log -1
commit 99cebfa0e4fbf3a2086d86b05902162076fd89d4
Author: Greg McCullough <[email protected]>
Date:   Tue May 30 18:51:26 2017 -0400

    Merge pull request #15228 from lfu/vm_scan_with_automate_1454936
    
    Queue the VM scan command after vm_scan_start event is handled by automate.
    (cherry picked from commit 7a7350e514d0a06c4ef555486486bbf75aceb74d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1457899

simaishi pushed a commit that referenced this pull request Jun 9, 2017
@gmcculloug @simaishi
Merge pull request #15228 from lfu/vm_scan_with_automate_1454936
0ec4e76 
Queue the VM scan command after vm_scan_start event is handled by automate.
(cherry picked from commit 7a7350e)

https://bugzilla.redhat.com/show_bug.cgi?id=1460339
@simaishi
Copy link
Contributor

simaishi commented Jun 9, 2017

Fine backport details:

$ git log -1
commit 0ec4e762ea05fd491376ae4ea8cad5690a81a44f
Author: Greg McCullough <[email protected]>
Date:   Tue May 30 18:51:26 2017 -0400

    Merge pull request #15228 from lfu/vm_scan_with_automate_1454936
    
    Queue the VM scan command after vm_scan_start event is handled by automate.
    (cherry picked from commit 7a7350e514d0a06c4ef555486486bbf75aceb74d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460339

@lfu lfu deleted the vm_scan_with_automate_1454936 branch October 16, 2017 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug approved these changes

Assignees

@gmcculloug gmcculloug

Labels
bug core/smart state euwe/backported fine/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lfu @miq-bot @mkanoor @gmcculloug @simaishi