Add policy check to Vm retirement. #19064

lfu · 2019-07-26T14:06:08Z

Vm retirement is handled as a request per #16933.

Depends on ManageIQ/manageiq-content#552.
https://bugzilla.redhat.com/show_bug.cgi?id=1702018

@miq-bot add_label bug, retirement, hammer/yes, ivanchuk/yes, changelog/yes
@miq-bot assign @tinaafitz

mkanoor · 2019-08-09T16:28:45Z

spec/models/vm/retirement_management_spec.rb

+    it "policy passes" do
+      expect(VmRetireRequest).to receive(:make_request)
+
+      allow(MiqAeEngine).to receive_messages(:deliver => ['ok', 'sucess', MiqAeEngine::MiqAeWorkspaceRuntime.new])


@lfu can you use

let(:ws) { MiqAeEngine::MiqAeWorkspaceRuntime.new }

and use it in the 2 tests

@lfu sucess should be success

mkanoor · 2019-08-09T16:29:59Z

spec/models/vm/retirement_management_spec.rb

+      event = {:attributes => {"full_data" => {:policy => {:prevented => true}}}}
+      ws = MiqAeEngine::MiqAeWorkspaceRuntime.new
+      allow(ws).to receive(:get_obj_from_path).with("/").and_return(:event_stream => event)
+      allow(MiqAeEngine).to receive_messages(:deliver => ['ok', 'sucess', ws])


@lfu sucess should be success

mkanoor · 2019-08-09T16:30:22Z

spec/models/vm/retirement_management_spec.rb

        vm_with_owner.update_attributes(:retires_on => 90.days.ago, :retirement_warn => 60, :retirement_last_warn => nil)
        expect(vm_with_owner.retirement_last_warn).to be_nil
+
+        allow(MiqAeEngine).to receive_messages(:deliver => ['ok', 'sucess', MiqAeEngine::MiqAeWorkspaceRuntime.new])


@lfu sucess?

mkanoor · 2019-08-13T13:34:36Z

@lfu
Looks good waiting for the conflicts to be resolved.

tinaafitz · 2019-08-13T17:09:39Z

@d-m-u Please review.

tinaafitz · 2019-08-15T14:43:56Z

@gmcculloug Please review.

d-m-u

yeah, looks okay to me

tinaafitz · 2019-08-17T12:38:39Z

Hi @gmcculloug Please review. This is needed for the 5.10 build.

gmcculloug · 2019-08-18T20:28:35Z

app/models/vm_or_template.rb

+
+      target.check_policy_prevent('request_vm_retire', "retire_request_after_policy_check", src_id, requester.userid, :initiated_by => initiated_by)
+    end
+  end


The find_by lookup could end up being expensive since it would hit the db for every Vm id passed in.

If we do not need the "missing VM" logging it could be simplified to:

def self.make_retire_request(*src_ids, requester, initiated_by: 'user') where(:id => src_ids).each do |target| target.check_policy_prevent('request_vm_retire', "retire_request_after_policy_check", src_id, requester.userid, :initiated_by => initiated_by) end end

Otherwise, you can still use the ActiveRecord_Relation returned by where to determine the missing IDs as well as loop over the valid VMs:

def self.make_retire_request(*src_ids, requester, initiated_by: 'user') vms = where(:id => src_ids) missing_ids = (src_ids - vms.pluck(:id)) _log.error("Retirement of [Vm] IDs: [#{missing_ids.join(', ')}] skipped - target(s) does not exist") vms.each do |target| target.check_policy_prevent('request_vm_retire', "retire_request_after_policy_check", src_id, requester.userid, :initiated_by => initiated_by) end end

gmcculloug · 2019-08-19T12:42:52Z

app/models/vm_or_template.rb

+  end
+
+  def retire_request_after_policy_check(*src_ids, userid, initiated_by: 'user')
+    klass = VmRetireRequest


Is there a reason to put the class into a variable if it never modified?

Can we just call VmRetireRequest.request_types.first and VmRetireRequest.make_request directly in this method?

gmcculloug · 2019-08-19T14:54:37Z

@lfu Please review test failures.

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

miq-bot · 2019-08-19T16:17:19Z

Checked commit lfu@710fecf with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

lpichler · 2019-08-21T10:07:12Z

Method make_retire_request overrides method with same name in RetirementMixin and it causes error in API repo

@lfu Do you have idea how to fix it ?
thanks!!

it is about 2 specs in context in api repo:

./spec/requests/vms_spec.rb:1098

and the method make_retire_request is called here in api

lfu · 2019-08-21T13:28:48Z

@tinaafitz This PR requires API code change.

d-m-u · 2019-08-23T13:26:00Z

Yeah, I'm just here to say that Libor's right and the API spec failure's due to this PR

lpichler · 2019-08-23T14:52:51Z

thanks @d-m-u
fyi: we are working on fix with @lfu and work is in progress

lfu · 2019-08-27T14:48:43Z

Not sure why these label were not applied.

@miq-bot add_label hammer/yes, ivanchuk/yes, changelog/yes

simaishi · 2019-12-13T16:24:16Z

@lfu There are multiple conflicts in the spec file for backporting to hammer branch. Can you please create a separate PR for hammer?

simaishi · 2019-12-16T17:41:58Z

@lfu This PR conflicts backporting to invahcuk branch as well. If the conflict is same between hammer/ivanchuk, just 1 PR is needed (against ivanchuk) and that can be marked as hammer/yes.
If they're different, please create another PR for ivanchuk.

simaishi · 2019-12-16T21:41:44Z

Backported to hammer via #19643
Backported to ivanchuk via #19647

miq-bot added bug lifecycle/retirement labels Jul 26, 2019

miq-bot assigned tinaafitz Jul 26, 2019

lfu mentioned this pull request Jul 26, 2019

Vm retirement is processed by VmRetireRequest state machine. ManageIQ/manageiq-content#552

Merged

lfu force-pushed the prevent_retirement_1702018 branch 2 times, most recently from 9ed931d to 0b8c5a2 Compare July 29, 2019 15:16

lfu force-pushed the prevent_retirement_1702018 branch 2 times, most recently from 0718c2a to bb7af00 Compare August 9, 2019 12:58

mkanoor reviewed Aug 9, 2019

View reviewed changes

lfu force-pushed the prevent_retirement_1702018 branch from bb7af00 to 511dfa1 Compare August 9, 2019 17:39

miq-bot added the unmergeable label Aug 12, 2019

lfu force-pushed the prevent_retirement_1702018 branch from 511dfa1 to e823e50 Compare August 13, 2019 14:09

mkanoor approved these changes Aug 13, 2019

View reviewed changes

miq-bot removed the unmergeable label Aug 13, 2019

tinaafitz approved these changes Aug 13, 2019

View reviewed changes

d-m-u approved these changes Aug 15, 2019

View reviewed changes

gmcculloug reviewed Aug 18, 2019

View reviewed changes

gmcculloug reviewed Aug 19, 2019

View reviewed changes

lfu force-pushed the prevent_retirement_1702018 branch 2 times, most recently from d5630d1 to 5976230 Compare August 19, 2019 14:25

Add policy check to Vm retirement.

710fecf

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

lfu force-pushed the prevent_retirement_1702018 branch from 5976230 to 710fecf Compare August 19, 2019 16:14

gmcculloug merged commit 351f8f3 into ManageIQ:master Aug 19, 2019

gmcculloug added this to the Sprint 118 Ending Aug 19, 2019 milestone Aug 19, 2019

lpichler mentioned this pull request Aug 26, 2019

(CI Failure Fix ) Put make_retire_request on queue in VmsController ManageIQ/manageiq-api#662

Merged

miq-bot added changelog/yes hammer/yes labels Aug 27, 2019

d-m-u mentioned this pull request Oct 21, 2019

Generate retire requests from the base class name #19398

Merged

lfu deleted the prevent_retirement_1702018 branch November 4, 2019 15:25

simaishi added the hammer/conflict label Dec 13, 2019

lfu mentioned this pull request Dec 13, 2019

[HAMMER] Add policy check to Vm retirement. #19643

Merged

simaishi added the ivanchuk/conflict label Dec 16, 2019

lfu mentioned this pull request Dec 16, 2019

[IVANCHUK] Add policy check to Vm retirement. #19647

Merged

simaishi added hammer/backported ivanchuk/backported and removed hammer/conflict labels Dec 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add policy check to Vm retirement. #19064

Add policy check to Vm retirement. #19064

lfu commented Jul 26, 2019 •

edited

Loading

mkanoor Aug 9, 2019

mkanoor Aug 9, 2019

mkanoor Aug 9, 2019

mkanoor Aug 9, 2019

mkanoor commented Aug 13, 2019

tinaafitz commented Aug 13, 2019

tinaafitz commented Aug 15, 2019

d-m-u left a comment

tinaafitz commented Aug 17, 2019

gmcculloug Aug 18, 2019

gmcculloug Aug 19, 2019

gmcculloug commented Aug 19, 2019

miq-bot commented Aug 19, 2019

lpichler commented Aug 21, 2019 •

edited

Loading

lfu commented Aug 21, 2019

d-m-u commented Aug 23, 2019

lpichler commented Aug 23, 2019

lfu commented Aug 27, 2019

simaishi commented Dec 13, 2019

simaishi commented Dec 16, 2019

simaishi commented Dec 16, 2019

Add policy check to Vm retirement. #19064

Add policy check to Vm retirement. #19064

Conversation

lfu commented Jul 26, 2019 • edited Loading

mkanoor Aug 9, 2019

Choose a reason for hiding this comment

mkanoor Aug 9, 2019

Choose a reason for hiding this comment

mkanoor Aug 9, 2019

Choose a reason for hiding this comment

mkanoor Aug 9, 2019

Choose a reason for hiding this comment

mkanoor commented Aug 13, 2019

tinaafitz commented Aug 13, 2019

tinaafitz commented Aug 15, 2019

d-m-u left a comment

Choose a reason for hiding this comment

tinaafitz commented Aug 17, 2019

gmcculloug Aug 18, 2019

Choose a reason for hiding this comment

gmcculloug Aug 19, 2019

Choose a reason for hiding this comment

gmcculloug commented Aug 19, 2019

miq-bot commented Aug 19, 2019

lpichler commented Aug 21, 2019 • edited Loading

lfu commented Aug 21, 2019

d-m-u commented Aug 23, 2019

lpichler commented Aug 23, 2019

lfu commented Aug 27, 2019

simaishi commented Dec 13, 2019

simaishi commented Dec 16, 2019

simaishi commented Dec 16, 2019

lfu commented Jul 26, 2019 •

edited

Loading

lpichler commented Aug 21, 2019 •

edited

Loading