From 6661b89dd6367aa781b56bac83e07338404e57bb Mon Sep 17 00:00:00 2001 From: Drew Bomhof Date: Fri, 16 Dec 2016 17:42:29 -0500 Subject: [PATCH] Allow a service power state to correctly handle nil actions In the case where the :start_action or :stop_action was not set on a service we need to assume the user would want a :start_action to equal 'Power On' and subsequently a nil :stop_action would equal a 'Power Off' This change only affects services where none of the start or stop actions were set This change also correctly handles passing back the power state and power status for atomic services If a user sets any of the service actions then we assume they meant to leave some of them as 'Do Nothing' https://bugzilla.redhat.com/show_bug.cgi?id=1394202 --- app/models/service.rb | 34 ++++++++++++++--- spec/models/service_spec.rb | 74 ++++++++++++++++++++++++++++++++----- 2 files changed, 92 insertions(+), 16 deletions(-) diff --git a/app/models/service.rb b/app/models/service.rb index 0c0f43ef08c..2086a937177 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -50,6 +50,8 @@ class Service < ApplicationRecord delegate :custom_actions, :custom_action_buttons, :to => :service_template, :allow_nil => true delegate :provision_dialog, :to => :miq_request, :allow_nil => true delegate :user, :to => :miq_request, :allow_nil => true + delegate :atomic?, :to => :service_template + delegate :composite?, :to => :service_template include ServiceMixin include OwnershipMixin @@ -201,17 +203,37 @@ def modify_power_state_delay(opts) end def power_states_match?(action) - if power_states.uniq == map_power_states(action) - options[:power_status] = "#{action}_complete" - update_attributes(:options => options) - return true + if composite? && (power_states.uniq == map_composite_power_states(action)) + return update_power_status(action) + elsif atomic? && (power_states[0] == POWER_STATE_MAP[action]) + return update_power_status(action) end false end - def map_power_states(action) + def map_composite_power_states(action) action_name = "#{action}_action" - service_resources.map(&action_name.to_sym).uniq.map { |x| Service::ACTION_RESPONSE[x] }.map { |x| Service::POWER_STATE_MAP[x] } + service_actions = service_resources.map(&action_name.to_sym).uniq + + # We need to account for all nil :start_action or :stop_action attributes + # When all :start_actions are nil then return 'Power On' for the :start_action + # When all :stop_actions are nil then return 'Power Off' for the :stop_action + if service_actions.compact.empty? + action_index = Service::ACTION_RESPONSE.values.index(action) + mod_resources = service_actions.each_with_index do |sa, i| + sa.nil? ? service_actions[i] = Service::ACTION_RESPONSE.to_a[action_index][0] : sa + end + else + mod_resources = service_actions + end + + # Map out the final power state we should have for the passed in action + mod_resources.map { |x| Service::ACTION_RESPONSE[x] }.map { |x| Service::POWER_STATE_MAP[x] } + end + + def update_power_status(action) + options[:power_status] = "#{action}_complete" + update_attributes(:options => options) end def update_progress(hash = {}) diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index d3c64733f96..2d8aeaeebb7 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -79,25 +79,27 @@ allow(MiqServer).to receive(:my_server).and_return(@zone1.miq_servers.first) @vm = FactoryGirl.create(:vm_vmware) @vm_1 = FactoryGirl.create(:vm_vmware) + @vm_2 = FactoryGirl.create(:vm_vmware) @service = FactoryGirl.create(:service) @service_c1 = FactoryGirl.create(:service, :service => @service) + @service_c2 = FactoryGirl.create(:service, :service => @service_c1) @service << @vm @service_c1 << @vm_1 + @service_c2 << @vm_1 + @service_c2 << @vm_2 + @service.service_resources.first.start_action = "Power On" + @service.service_resources.first.stop_action = "Power Off" @service.save @service_c1.save + @service_c2.save end it "#power_states" do - expect(@service.power_states).to eq %w(on on) + expect(@service.power_states).to eq %w(on on on on) end it "#update_progress" do - expect(@service.power_state).to be_nil - @service.update_progress(:power_state => "on") - expect(@service.power_state).to be_nil - @service.update_progress(:power_state => "off") - expect(@service.power_state).to be_nil @service.update_progress(:power_status => "stopping") expect(@service.power_status).to eq "stopping" expect { |b| @service.update_progress(:power_state => "timeout", &b) }.to yield_with_args(:reset => true) @@ -112,7 +114,13 @@ end context "#calculate_power_state" do + let(:service_template) { instance_double('ServiceTemplate', :service_type => 'compositie') } + it "delays if power states don't match" do + expect(@service).to receive(:composite?).once.and_return(true) + expect(@service).to receive(:atomic?).once.and_return(false) + @vm.send(:power_state=, 'off') + @vm.save @service.calculate_power_state(:start) expect(@service.options[:delayed]).to eq 1 end @@ -127,17 +135,63 @@ context "#power_states_match?" do it "returns the uniq value for the 'on' power state" do - expect(@service).to receive(:map_power_states).with(:start).and_return(["on"]) + allow(@service).to receive(:composite?).and_return(true) + expect(@service).to receive(:map_composite_power_states).with(:start).and_return(["on"]) + expect(@service).to receive(:update_power_status).with(:start).and_return(true) expect(@service.power_states_match?(:start)).to be_truthy end it "returns the uniq value for the 'off' power state" do - expect(@service).to receive(:map_power_states).with(:stop).and_return(["off"]) + allow(@service).to receive(:composite?).and_return(true) + expect(@service).to receive(:map_composite_power_states).with(:stop).and_return(["off"]) + expect(@service).to receive(:update_power_status).with(:stop).and_return(true) + expect(@service).to receive(:power_states).and_return(["off"]) + expect(@service.power_states_match?(:stop)).to be_truthy + end + + it "returns the uniq value for the 'on' power state with an atomic service" do + allow(@service).to receive(:composite?).and_return(false) + allow(@service).to receive(:atomic?).and_return(true) + + expect(@service).to receive(:update_power_status).with(:start).and_return(true) + expect(@service.power_states_match?(:start)).to be_truthy + end + + it "returns the uniq value for the 'off' power state with an atomic service" do + allow(@service).to receive(:composite?).and_return(false) + allow(@service).to receive(:atomic?).and_return(true) + + expect(@service).to receive(:update_power_status).with(:stop).and_return(true) expect(@service).to receive(:power_states).and_return(["off"]) expect(@service.power_states_match?(:stop)).to be_truthy end end + context "#map_power_states" do + it "returns the power value when start_action is set" do + expect(@service.service_resources.first.start_action).to eq "Power On" + expect(@service.map_composite_power_states(:start)).to eq ["on"] + end + + it "returns the power value when stop_action is set" do + expect(@service.service_resources.first.stop_action).to eq "Power Off" + expect(@service.map_composite_power_states(:stop)).to eq ["off"] + end + + it "assumes the start_action and returns a value if none of the start_actions are set" do + expect(@service_c2.service_resources.first.id).to_not eq @service_c2.service_resources.last.id + expect(@service_c2.service_resources.first.start_action).to be_nil + expect(@service_c2.service_resources.last.start_action).to be_nil + expect(@service_c2.map_composite_power_states(:start)).to eq ["on"] + end + + it "assumes the stop_action and returns a value if none of the stop_actions are set" do + expect(@service_c2.service_resources.first.stop_action).to be_nil + expect(@service_c2.service_resources.last.stop_action).to be_nil + expect(@service_c2.map_composite_power_states(:stop)).to eq ["off"] + end + end + context "#modify_power_state_delay" do it "sets delayed to nil on a reset request" do options = {:reset => true} @@ -162,8 +216,8 @@ end it "#all_vms" do - expect(@service_c1.all_vms).to match_array [@vm_1] - expect(@service.all_vms).to match_array [@vm, @vm_1] + expect(@service_c1.all_vms).to match_array [@vm_1, @vm_1, @vm_2] + expect(@service.all_vms).to match_array [@vm, @vm_1, @vm_1, @vm_2] end it "#direct_service" do