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

Changes to catch the retirement requester in automate. #17033

Merged
merged 1 commit into from
Feb 23, 2018
Merged

Changes to catch the retirement requester in automate. #17033

merged 1 commit into from
Feb 23, 2018

Conversation

lfu
Copy link
Member

@lfu lfu commented Feb 21, 2018

The retirement_initiator is 'system' instead of 'user' as it is when it is initiated from the UI.

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

@miq-bot assign @gmcculloug
@miq-bot add_label bug, gaprindashvili/yes

cc @tinaafitz @mkanoor

@lfu lfu mentioned this pull request Feb 21, 2018
Merged
mkanoor
mkanoor reviewed Feb 21, 2018
View reviewed changes
app/models/mixins/retirement_mixin.rb
@@ -15,7 +15,10 @@ def retire(ids, options = {})
object = find_by(:id => id)
object.retire(options) if object.respond_to?(:retire)
end
MiqQueue.put(:class_name => 'RetirementManager', :method_name => 'check')
q_options = {:class_name => 'RetirementManager', :method_name => 'check'}
user = User.current_user
Copy link
Contributor

Choose a reason for hiding this comment

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

@lfu How do we ensure that the User.current_user is set? Should we be raising an error if its not set

Copy link
Member Author

Choose a reason for hiding this comment

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

@mkanoor We only take and store the user info into the queue only when the user is set. The retirement requester would default to system if no user is specified.

Copy link
Member

Choose a reason for hiding this comment

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

The system account would be used when retirement is initiated from the scheduler.

mkanoor
mkanoor reviewed Feb 21, 2018
View reviewed changes
spec/models/vm/retirement_management_spec.rb Outdated
@@ -143,6 +143,18 @@
@vm.raise_retirement_event(event_name)
end

it "#raise_retirement_event with current user" do
User.current_user = FactoryGirl.create(:user_with_group, :userid => 'freddy')
Copy link
Contributor

Choose a reason for hiding this comment

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

@lfu Instead of directly setting the User.current user you should use the User.with_user e.g. https://github.com/ManageIQ/manageiq/blob/master/spec/models/miq_event_spec.rb#L112

@lfu lfu force-pushed the retirement_requester_1547527 branch from 63266f0 to bc7e5a0 Compare February 22, 2018 14:30
@tinaafitz
Copy link
Member

@lfu Looks good.

gmcculloug
gmcculloug reviewed Feb 23, 2018
View reviewed changes
app/models/mixins/retirement_mixin.rb
@@ -178,6 +181,7 @@ def retired_event_name
end

def raise_retirement_event(event_name, requester = nil)
requester ||= User.current_user.try(:userid)
Copy link
Member

Choose a reason for hiding this comment

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

The raise_retirement_event method gets call from two places: retirement_check and retire_now.

The retirement_check method is called from the scheduler and I do not believe it gets called anywhere else, so I would expect User.current_user to be nil.

The retire_now method can get called directly by a user and on line 128 sets update_attributes(:retirement_requester => requester). With your current code change the User.current_user would be passed to the event, but not set for the retirement_requester on the object, so this is only a partial fix.

Moving requester ||= User.current_user.try(:userid) into retire_now before the update_attributes call would resolve this.

Copy link
Member Author

Choose a reason for hiding this comment

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

retirement_check gets called when running Set Retirement Date.

When retire_now gets called from UI or API, requester is already set with this change.

@lfu lfu force-pushed the retirement_requester_1547527 branch from bc7e5a0 to 1c1ec01 Compare February 23, 2018 16:09
@lfu
Copy link
Member Author

lfu commented Feb 23, 2018

There is still one place where the userid is set to system with this PR as shown in the audit log messages.

[----] I, [2018-02-23T11:47:50.423611 #36362:3fc0cfa54cfc]  INFO -- : <AuditSuccess> MIQ(RetirementMixin.raise_audit_event) userid: [system] - Vm: [2_vcr_icehouse], Retires On: [02/23/18 16:47 UTC], has been retired
[----] I, [2018-02-23T11:47:50.423717 #36362:3fc0cfa54cfc]  INFO -- : Called audit event for: Vm: [2_vcr_icehouse], Retires On: [02/23/18 16:47 UTC], has been retired

That happens when retirement is done, finish_retirement is called from automate. At that point, automate still has the correct userid but that info does not get passed back to backend model method vm.finish_retirement.

@gmcculloug
Copy link
Member

@lfu Wouldn't it be safe to assume that finish_retirement is happening under the context of the retirement_requester? Automate would not have to pass this information along.

@lfu lfu force-pushed the retirement_requester_1547527 branch from 1c1ec01 to a51fa32 Compare February 23, 2018 19:06
@gmcculloug
Copy link
Member

@lfu Please review test failures which are based on passing :userid=>"system".

@lfu lfu force-pushed the retirement_requester_1547527 branch from a51fa32 to 037c427 Compare February 23, 2018 20:36
@miq-bot
Copy link
Member

miq-bot commented Feb 23, 2018

Checked commit lfu@037c427 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@gmcculloug gmcculloug merged commit 7856a59 into ManageIQ:master Feb 23, 2018
@gmcculloug gmcculloug added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 23, 2018
simaishi pushed a commit that referenced this pull request Mar 7, 2018
@gmcculloug @simaishi
Merge pull request #17033 from lfu/retirement_requester_1547527
9042c3e 
Changes to catch the retirement requester in automate.
(cherry picked from commit 7856a59)

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

simaishi commented Mar 7, 2018

Gaprindashvili backport details:

$ git log -1
commit 9042c3eab43bdd1724431cb22c71b4c651d54846
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Fri Feb 23 16:25:49 2018 -0500

    Merge pull request #17033 from lfu/retirement_requester_1547527
    
    Changes to catch the retirement requester in automate.
    (cherry picked from commit 7856a59838008f3d651a81f994c193a792d2c670)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552800

@lfu lfu deleted the retirement_requester_1547527 branch September 29, 2018 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkanoor mkanoor mkanoor left review comments

@gmcculloug gmcculloug gmcculloug left review comments

Assignees

@gmcculloug gmcculloug

Labels
bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

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