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.

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

Add features based on supported_api_versions to RedHat provider #11418

Merged
merged 6 commits into from
Oct 13, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,23 +1,35 @@
module ManageIQ::Providers::Redhat::InfraManager::ApiIntegration
extend ActiveSupport::Concern

require 'ovirtsdk4'

included do
process_api_features_support
end

def supported_features
@supported_features ||= supported_api_versions.collect { |version| self.class.api_features[version] }.flatten.uniq
end

def connect(options = {})
raise "no credentials defined" if self.missing_credentials?(options[:auth_type])

version = options[:version] || 3
unless options[:skip_supported_api_validation] || supports_the_api_version?(version)
raise "version #{version} of the api is not supported by the provider"
end
# If there is API path stored in the endpoints table and use it:
path = default_endpoint.path
path = options[:path] || default_endpoint.path
_log.info("Using stored API path '#{path}'.") unless path.blank?

server = options[:ip] || address
port = options[:port] || self.port
username = options[:user] || authentication_userid(options[:auth_type])
password = options[:pass] || authentication_password(options[:auth_type])
service = options[:service] || "Service"
version = options[:version] || 3

# Create the underlying connection according to the version of the oVirt API requested by
# the caller:
connect_method = version == 4 ? :raw_connect_v4 : :raw_connect_v3
connect_method = "raw_connect_v#{version}".to_sym
connection = self.class.public_send(connect_method, server, port, path, username, password, service)

# Copy the API path to the endpoints table:
Expand All @@ -30,6 +42,30 @@ def supports_port?
true
end

def supported_api_versions
supported_api_versions_from_cache
end

def supported_api_versions_from_cache
Cacher.new(cache_key).fetch_fresh(last_refresh_date) { supported_api_verions_from_sdk }
end

def cache_key
"REDHAT_EMS_CACHE_KEY_#{id}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use OVIRT instead of REDHAT ?

Copy link
Author

Choose a reason for hiding this comment

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

We can, but in the source code all the code related to oVirt is under a folder and name space named "redhat" that is why I used REDHAT. Considering this do you still think it should be OVIRT?

end

def supported_api_verions_from_sdk
username = authentication_userid(:basic)
password = authentication_password(:basic)
probe_args = { :hostname => hostname, :port => port, :username => username, :password => password }
probe_results = OvirtSDK4::Probe.probe(probe_args)
Copy link
Member

Choose a reason for hiding this comment

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

So, this makes a live connection to the ovirt server, right?
What happens if the server is unreachable?

I'm also not sure about this, because we create a matrix of supported features #11281

Copy link
Member

Choose a reason for hiding this comment

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

hmm, but its only done at runtime, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it only happens at runtime.

probe_results.map(&:version) if probe_results
end

def supports_the_api_version?(version)
supported_api_versions.include?(version)
end

def supported_auth_types
%w(default metrics)
end
Expand Down Expand Up @@ -142,9 +178,9 @@ def verify_credentials(auth_type = nil, options = {})

def history_database_name
@history_database_name ||= begin
version = version_3_0? ? '3_0' : '>3_0'
self.class.history_database_name_for(version)
end
version = version_3_0? ? '3_0' : '>3_0'
self.class.history_database_name_for(version)
end
Copy link
Member

Choose a reason for hiding this comment

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

[Even though I align as you have here, too], there's no reason to touch this code and change the blame on these lines for the sake of Git history. ¯_(ツ)_/¯

end

def version_3_0?
Expand All @@ -170,16 +206,39 @@ def with_disk_attachments_service(vm)
end

class_methods do
def api3_supported_features
[]
end

def api4_supported_features
[:snapshots]
end

def api_features
{ 3 => api3_supported_features, 4 => api4_supported_features }
end

def process_api_features_support
all_features = api_features.values.flatten.uniq
all_features.each do |feature|
Copy link
Member

Choose a reason for hiding this comment

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

TBH I would like to make this explicit.
I know that it is some sort of duplication, but it makes it way easier to grep and search for supports :feature
Unless you say there will be a ton of features.

So my suggestion would be to remove this and api?features and just have 2 blocks in the included block above.

Copy link
Author

Choose a reason for hiding this comment

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

I asked and it seems we will have more features and more features.

supports feature do
unless supported_features.include?(feature)
unsupported_reason_add(feature, _("This feature is not supported by the api version of the provider"))
end
end
end
end

# Connect to the engine using version 4 of the API and the `ovirt-engine-sdk` gem.
def raw_connect_v4(server, port, path, username, password, service)
def raw_connect_v4(server, port, path, username, password, service, scheme = 'https')
require 'ovirtsdk4'

# Get the timeout from the configuration:
timeout, = ems_timeouts(:ems_redhat, service)

# Create the connection:
OvirtSDK4::Connection.new(
:url => "https://#{server}:#{port}#{path}",
:url => "#{scheme}://#{server}:#{port}#{path}",
:username => username,
:password => password,
:timeout => timeout,
Expand Down Expand Up @@ -233,4 +292,30 @@ def extract_ems_ref_id(href)
href && href.split("/").last
end
end

class Cacher
Copy link
Member

Choose a reason for hiding this comment

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

@chrisarcand can you have a look at this? I wonder if a) we should/do use the Rails.cache and b) if this does not rebuild functionality thats probably already in Rails.cache

Copy link
Member

Choose a reason for hiding this comment

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

@borod108 I'm not sure why you're creating your own cache object only to store it in the application cache (Rails.cache). Any specific reasoning?

Perhaps class Cacher < ActiveSupport::Cache::Store and subclass any custom logic you need. There are a variety of other build cache mechanisms to choose from, as well.

Copy link
Author

Choose a reason for hiding this comment

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

I had two reasons for doing this:

  1. I did not see Rails.cache used anywhere in the project, and although I asked blomquisg and it seems ok I thought I better wrap it in something so that if I will have to use some other method I will just change this without changing the logic and tests.
  2. I do not know how to do "fetch" that receives a condition to check if it need to rebuild the cache or just return the value (except for time based one which will not do here).
    Any way I will see how to subclass the Store class. Tnx for the idea!

Copy link
Member

Choose a reason for hiding this comment

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

thought I better wrap it in something so that if I will have to use some other method I will just change this without changing the logic and tests.

Yup, a great thought 👍

I do not know how to do "fetch" that receives a condition to check if it need to rebuild the cache or just return the value

It'll look something sort of like this:

class Cacher < ActiveSupport::Cache::Store
  def read(name, options = nil)
    # clear is the underlying method for clearing the cache, but #rebuild_cache cache if needed, whatever
    clear if stale_cache?(options[:last_refresh_time])
    super
  end
end

attr_reader :key

def initialize(key)
@key = key
end

def fetch_fresh(last_refresh_time)
force = stale_cache?(last_refresh_time)
res = Rails.cache.fetch(key, force: force) { build_entry { yield } }
res[:value]
end

private

def build_entry
{:created_at => Time.now.utc, :value => yield}
end

def stale_cache?(last_refresh_time)
current_val = Rails.cache.read(key)
return true unless current_val && current_val[:created_at] && last_refresh_time
last_refresh_time > current_val[:created_at]
end
end
end
8 changes: 5 additions & 3 deletions app/models/mixins/supports_feature_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,9 @@ module SupportsFeatureMixin
extend ActiveSupport::Concern

QUERYABLE_FEATURES = {
:associate_floating_ip => 'Associate a Floating IP',
:control => 'Basic control operations', # FIXME: this is just a internal helper and should be refactored
:associate_floating_ip => 'Associate a Floating IP',
# FIXME: this is just a internal helper and should be refactored
:control => 'Basic control operations',
:cloud_tenant_mapping => 'CloudTenant mapping',
:backup_create => 'CloudVolume backup creation',
:backup_restore => 'CloudVolume backup restore',
Expand All @@ -81,7 +82,8 @@ module SupportsFeatureMixin
:resize => 'Resizing',
:retire => 'Retirement',
:smartstate_analysis => 'Smartstate Analaysis',
:terminate => 'Terminate a VM'
:snapshots => 'Snapshots',
:terminate => 'Terminate a VM',
}.freeze

# Whenever this mixin is included we define all features as unsupported by default.
Expand Down
2 changes: 2 additions & 0 deletions spec/controllers/ems_infra_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,8 @@
allow(controller).to receive(:check_privileges).and_return(true)
allow(controller).to receive(:assert_privileges).and_return(true)
login_as FactoryGirl.create(:user, :features => "ems_infra_new")
allow_any_instance_of(ManageIQ::Providers::Redhat::InfraManager)
.to receive(:supported_api_versions).and_return([3, 4])
end

render_views
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@

allow(rhevm_vm).to receive_messages(:nics => [rhevm_nic1, rhevm_nic2])
allow(Ovirt::Cluster).to receive_messages(:find_by_href => rhevm_cluster)
allow_any_instance_of(ManageIQ::Providers::Redhat::InfraManager).to receive(:supported_api_versions)
.and_return([3, 4])
end

context "#configure_network_adapters" do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
guid, server, zone = EvmSpecHelper.create_guid_miq_server_zone
@ems = FactoryGirl.create(:ems_redhat, :zone => zone, :hostname => "192.168.252.231", :ipaddress => "192.168.252.231", :port => 8443)
@ems.update_authentication(:default => {:userid => "evm@manageiq.com", :password => "password"})
allow(@ems).to receive(:supported_api_versions).and_return([3])
end

it ".ems_type" do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
guid, server, zone = EvmSpecHelper.create_guid_miq_server_zone
@ems = FactoryGirl.create(:ems_redhat, :zone => zone, :hostname => "192.168.252.230", :ipaddress => "192.168.252.230", :port => 443)
@ems.update_authentication(:default => {:userid => "evm@manageiq.com", :password => "password"})
allow(@ems).to receive(:supported_api_versions).and_return([3])
end

it ".ems_type" do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
:ems_id => @ems.id,
:uid_ems => "00000002-0002-0002-0002-0000000001e9_respool")
@cluster.with_relationship_type("ems_metadata") { @cluster.add_child @rp }
allow(@ems).to receive(:supported_api_versions).and_return([3, 4])
end

it "should refresh a vm" do
Expand Down
124 changes: 123 additions & 1 deletion spec/models/manageiq/providers/redhat/infra_manager_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

it "rhevm_metrics_connect_options fetches configuration and allows overrides" do
expect(ems.rhevm_metrics_connect_options[:host]).to eq("some.thing.tld")
expect(ems.rhevm_metrics_connect_options({:hostname => "different.tld"})[:host])
expect(ems.rhevm_metrics_connect_options(:hostname => "different.tld")[:host])
.to eq("different.tld")
end
end
Expand Down Expand Up @@ -78,4 +78,126 @@
expect(described_class.extract_ems_ref_id("/ovirt-engine/api/vms/123")).to eq("123")
end
end

context "api versions" do
require 'ovirtsdk4'
let(:ems) { FactoryGirl.create(:ems_redhat_with_authentication) }
subject(:supported_api_versions) { ems.supported_api_versions }
context "#supported_api_versions" do
before(:each) do
Rails.cache.delete(ems.cache_key)
Rails.cache.write(ems.cache_key, cached_api_versions)
end

context "when no cached supported_api_versions" do
let(:cached_api_versions) { nil }
it 'calls the OvirtSDK4::Probe.probe' do
expect(OvirtSDK4::Probe).to receive(:probe).and_return([])
supported_api_versions
end

it 'properly parses ProbeResults' do
allow(OvirtSDK4::Probe).to receive(:probe)
.and_return([OvirtSDK4::ProbeResult.new(:version => '3'),
OvirtSDK4::ProbeResult.new(:version => '4')])
expect(supported_api_versions).to match_array(%w(3 4))
end
end

context "when cache of supported_api_versions is stale" do
let(:cached_api_versions) do
{
:created_at => Time.now.utc,
:value => [3]
}
end

before(:each) do
allow(ems).to receive(:last_refresh_date)
.and_return(cached_api_versions[:created_at] + 1.day)
end

it 'calls the OvirtSDK4::Probe.probe' do
expect(OvirtSDK4::Probe).to receive(:probe).and_return([])
supported_api_versions
end
end

context "when cache of supported_api_versions available" do
let(:cached_api_versions) do
{
:created_at => Time.now.utc,
:value => [3]
}
end

before(:each) do
allow(ems).to receive(:last_refresh_date)
.and_return(cached_api_versions[:created_at] - 1.day)
end

it 'returns from cache' do
expect(supported_api_versions).to match_array([3])
end
end
end

describe "#supports_the_api_version?" do
it "returns the supported api versions" do
allow(ems).to receive(:supported_api_versions).and_return([3])
expect(ems.supports_the_api_version?(3)).to eq(true)
expect(ems.supports_the_api_version?(6)).to eq(false)
end
end
end

context "supported features" do
let(:ems) { FactoryGirl.create(:ems_redhat) }
let(:supported_api_versions) { [3, 4] }
context "#process_api_features_support" do
before(:each) do
allow(SupportsFeatureMixin).to receive(:guard_queryable_feature).and_return(true)
Copy link
Member

Choose a reason for hiding this comment

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

hmm, why do you mock this?

Copy link
Member

Choose a reason for hiding this comment

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

ah, ok, because you are testing against arbirtrary features.

See my comment above. I dont really like this generic feature handling. If you just add the two supports blocks for your two features you can get along without this whole spec context.

allow(described_class).to receive(:api_features)
.and_return(3 => %w(feature1 feature3), 4 => %w(feature2 feature3))
described_class.process_api_features_support
allow(ems).to receive(:supported_api_versions).and_return(supported_api_versions)
end

context "no versions supported" do
let(:supported_api_versions) { [] }
it 'supports the right features' do
expect(ems.supports_feature1?).to be_falsey
expect(ems.supports_feature2?).to be_falsey
expect(ems.supports_feature3?).to be_falsey
end
end

context "version 3 supported" do
let(:supported_api_versions) { [3] }
it 'supports the right features' do
expect(ems.supports_feature1?).to be_truthy
expect(ems.supports_feature2?).to be_falsey
expect(ems.supports_feature3?).to be_truthy
end
end

context "version 4 supported" do
let(:supported_api_versions) { [4] }
it 'supports the right features' do
expect(ems.supports_feature1?).to be_falsey
expect(ems.supports_feature2?).to be_truthy
expect(ems.supports_feature3?).to be_truthy
end
end

context "all versions supported" do
let(:supported_api_versions) { [3, 4] }
it 'supports the right features' do
expect(ems.supports_feature1?).to be_truthy
expect(ems.supports_feature2?).to be_truthy
expect(ems.supports_feature3?).to be_truthy
end
end
end
end
end