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

Create initial tower objects when we start the worker #14283

Merged
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions app/models/embedded_ansible_worker.rb
@@ -1,5 +1,6 @@
class EmbeddedAnsibleWorker < MiqWorker
require_nested :Runner
include_concern 'ObjectManagement'

self.required_roles = ['embedded_ansible']

Expand Down
56 changes: 56 additions & 0 deletions app/models/embedded_ansible_worker/object_management.rb
@@ -0,0 +1,56 @@
module EmbeddedAnsibleWorker::ObjectManagement
extend ActiveSupport::Concern

def ensure_initial_objects(provider, connection)
ensure_organization(provider, connection)
ensure_credential(provider, connection)
ensure_inventory(provider, connection)
ensure_host(provider, connection)
end

def remove_demo_data(connection)
connection.api.credentials.all(:name => "Demo Credential").each(&:destroy!)
connection.api.inventories.all(:name => "Demo Inventory").each(&:destroy!)
connection.api.job_templates.all(:name => "Demo Job Template").each(&:destroy!)
connection.api.projects.all(:name => "Demo Project").each(&:destroy!)
connection.api.organizations.all(:name => "Default").each(&:destroy!)
end

def ensure_organization(provider, connection)
return if provider.default_organization

provider.default_organization = connection.api.organizations.create!(
Copy link
Member

Choose a reason for hiding this comment

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

Will this even work? default_orgainzation= creates a CustomAttribute on the Provider right? But you're passing in a AnsibleTowerClient::Organization instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

:name => I18n.t("product.name"),
:description => "#{I18n.t("product.name")} Default Organization"
).id
end

def ensure_credential(provider, connection)
return if provider.default_credential

provider.default_credential = connection.api.credentials.create!(
:name => "#{I18n.t("product.name")} Default Credential",
:kind => "ssh",
:organization => provider.default_organization
).id
end

def ensure_inventory(provider, connection)
return if provider.default_inventory

provider.default_inventory = connection.api.inventories.create!(
:name => "#{I18n.t("product.name")} Default Inventory",
:organization => provider.default_organization
).id
end

def ensure_host(provider, connection)
return if provider.default_host

provider.default_host = connection.api.hosts.create!(
Copy link
Contributor

Choose a reason for hiding this comment

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

@carbonin make sure the created localhost has variable ansible_connection: local

Copy link
Member Author

Choose a reason for hiding this comment

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

@bzwei Is that in the variables yaml string?

I'm guessing that would be :variables => {'ansible_connection' => "local"}.to_yaml for the API?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should be addressed now.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

:name => "localhost",
:inventory => provider.default_inventory,
:variables => {'ansible_connection' => "local"}.to_yaml
).id
end
end
4 changes: 4 additions & 0 deletions app/models/embedded_ansible_worker/runner.rb
Expand Up @@ -49,6 +49,10 @@ def update_embedded_ansible_provider

provider.save!

api_connection = EmbeddedAnsible.api_connection
worker.remove_demo_data(api_connection)
worker.ensure_initial_objects(provider, api_connection)

admin_auth = MiqDatabase.first.ansible_admin_authentication

provider.update_authentication(:default => {:userid => admin_auth.userid, :password => admin_auth.password})
Expand Down
29 changes: 19 additions & 10 deletions lib/embedded_ansible.rb
Expand Up @@ -12,6 +12,7 @@ class EmbeddedAnsible
START_EXCLUDE_TAGS = "packages,migrations,firewall".freeze
HTTP_PORT = 54_321
HTTPS_PORT = 54_322
WAIT_FOR_ANSIBLE_SLEEP = 1.second

def self.available?
path = ENV["APPLIANCE_ANSIBLE_DIRECTORY"] || APPLIANCE_ANSIBLE_DIRECTORY
Expand Down Expand Up @@ -50,6 +51,15 @@ def self.configure
def self.start
configure_secret_key
run_setup_script(START_EXCLUDE_TAGS)

5.times do
return if alive?

_log.info("Waiting for EmbeddedAnsible to respond")
sleep WAIT_FOR_ANSIBLE_SLEEP
end
Copy link
Member

Choose a reason for hiding this comment

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

I like this better. Now the caller can be guaranteed to be alive or get an exception. 👍


raise "EmbeddedAnsible service is not responding after setup"
end

def self.stop
Expand All @@ -64,6 +74,15 @@ def self.services
AwesomeSpawn.run!("source /etc/sysconfig/ansible-tower; echo $TOWER_SERVICES").output.split
end

def self.api_connection
admin_auth = miq_database.ansible_admin_authentication
AnsibleTowerClient::Connection.new(
:base_url => URI::HTTP.build(:host => "localhost", :path => "/api/v1", :port => HTTP_PORT).to_s,
:username => admin_auth.userid,
:password => admin_auth.password
)
end

def self.run_setup_script(exclude_tags)
json_extra_vars = {
:minimum_var_space => 0,
Expand Down Expand Up @@ -171,14 +190,4 @@ def self.database_connection
ActiveRecord::Base.connection
end
private_class_method :database_connection

def self.api_connection
admin_auth = miq_database.ansible_admin_authentication
AnsibleTowerClient::Connection.new(
:base_url => URI::HTTP.build(:host => "localhost", :path => "/api/v1", :port => HTTP_PORT).to_s,
:username => admin_auth.userid,
:password => admin_auth.password
)
end
private_class_method :api_connection
end
23 changes: 22 additions & 1 deletion spec/lib/embedded_ansible_spec.rb
Expand Up @@ -275,12 +275,16 @@
end

describe ".start" do
it "runs the setup script with the correct args" do
before do
miq_database.set_ansible_admin_authentication(:password => "adminpassword")
miq_database.set_ansible_rabbitmq_authentication(:userid => "rabbituser", :password => "rabbitpassword")
miq_database.set_ansible_database_authentication(:userid => "databaseuser", :password => "databasepassword")

expect(described_class).to receive(:configure_secret_key)
stub_const("EmbeddedAnsible::WAIT_FOR_ANSIBLE_SLEEP", 0)
end

it "runs the setup script with the correct args" do
expect(AwesomeSpawn).to receive(:run!) do |script_path, options|
params = options[:params]
inventory_file_contents = File.read(params[:inventory=])
Expand All @@ -296,9 +300,26 @@
expect(inventory_file_contents).to include("pg_username='databaseuser'")
expect(inventory_file_contents).to include("pg_password='databasepassword'")
end
expect(described_class).to receive(:alive?).and_return(true)

described_class.start
end

it "waits for Ansible to respond" do
expect(AwesomeSpawn).to receive(:run!)

expect(described_class).to receive(:alive?).exactly(3).times.and_return(false, false, true)

described_class.start
end

it "raises if Ansible doesn't respond" do
expect(AwesomeSpawn).to receive(:run!)

expect(described_class).to receive(:alive?).exactly(5).times.and_return(false)

expect { described_class.start }.to raise_error(RuntimeError)
end
end

describe ".generate_database_authentication (private)" do
Expand Down
36 changes: 34 additions & 2 deletions spec/models/embedded_ansible_worker/runner_spec.rb
Expand Up @@ -10,7 +10,9 @@
let(:runner) {
worker
allow_any_instance_of(described_class).to receive(:worker_initialization)
described_class.new(:guid => worker_guid)
r = described_class.new(:guid => worker_guid)
allow(r).to receive(:worker).and_return(worker)
Copy link
Member

Choose a reason for hiding this comment

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

This is only necessary because of the stub above. Instead can we just tap the .new and call #find_worker_record?

#initialize calls: https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_worker/runner.rb#L51
#worker_initialization calls: https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_worker/runner.rb#L58
#starting_worker_record calls: https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_worker/runner.rb#L170
#find_worker_record sets @worker: https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_worker/runner.rb#L165

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I'll look into this.

Copy link
Member

Choose a reason for hiding this comment

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

😢 I have to finish fixing this. So much code now depends on all of this junk running when you call .new though.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I tried to do a bit to get rid of this and it turns out we have to jump through hoops because sync_config changes the server's zone to the one called "default" which ends up being nil unless we Zone.seed ...

Even then there is no way to deal with how the runner looks up the worker, I would still need to stub the .worker method (attribute) to make sure we don't actually call #remove_demo_data and #ensure_initial_objects.

So then the last option would be to get rid of the allow_any_instance_of and also stub the #worker method ... That makes the test that asserts that we change the zone of the provider fail because somehow we are getting this "default" zone in there.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the answer here will be to just remove the zone/role stuff from settings. Then we don't have to deal with multiple sources of information for this type of thing.

Also, I don't really want to do that in this PR. I'll work on that as a follow up and fix this up there.

r
}

it "#do_before_work_loop exits on exceptions" do
Expand All @@ -21,13 +23,20 @@
end

context "#update_embedded_ansible_provider" do
let(:api_connection) { double("AnsibleAPIConnection") }
before do
EvmSpecHelper.local_guid_miq_server_zone
MiqDatabase.seed
MiqDatabase.first.set_ansible_admin_authentication(:password => "secret")

allow(EmbeddedAnsible).to receive(:api_connection).and_return(api_connection)
end

it "creates initial" do
expect(worker).to receive(:remove_demo_data).with(api_connection)
expect(worker).to receive(:ensure_initial_objects)
.with(instance_of(ManageIQ::Providers::EmbeddedAnsible::Provider), api_connection)

runner.update_embedded_ansible_provider

provider = ManageIQ::Providers::EmbeddedAnsible::Provider.first
Expand All @@ -39,6 +48,10 @@
end

it "updates existing" do
expect(worker).to receive(:remove_demo_data).twice.with(api_connection)
expect(worker).to receive(:ensure_initial_objects).twice
.with(instance_of(ManageIQ::Providers::EmbeddedAnsible::Provider), api_connection)

runner.update_embedded_ansible_provider
new_zone = FactoryGirl.create(:zone)
miq_server.update(:hostname => "boringserver", :zone => new_zone)
Expand All @@ -51,6 +64,25 @@
expect(provider.default_endpoint.url).to eq("https://boringserver/ansibleapi/v1")
end
end

context "#setup_ansible" do
it "configures EmbeddedAnsible if it is not configured" do
expect(EmbeddedAnsible).to receive(:start)

expect(EmbeddedAnsible).to receive(:configured?).and_return(false)
expect(EmbeddedAnsible).to receive(:configure)

runner.setup_ansible
end

it "doesn't call configure if EmbeddedAnsible is already configured" do
expect(EmbeddedAnsible).to receive(:start)

expect(EmbeddedAnsible).to receive(:configured?).and_return(true)
expect(EmbeddedAnsible).not_to receive(:configure)

runner.setup_ansible
end
end
end
end

138 changes: 138 additions & 0 deletions spec/models/embedded_ansible_worker_spec.rb
@@ -0,0 +1,138 @@
describe EmbeddedAnsibleWorker do
subject { FactoryGirl.create(:embedded_ansible_worker) }

context "ObjectManagement concern" do
let(:provider) { FactoryGirl.create(:provider_embedded_ansible) }
let(:api_connection) { double("AnsibleAPIConnection", :api => tower_api) }
let(:tower_api) do
methods = {
:organizations => org_collection,
:credentials => cred_collection,
:inventories => inv_collection,
:hosts => host_collection,
:job_templates => job_templ_collection,
:projects => proj_collection

}
double("TowerAPI", methods)
end

let(:org_collection) { double("AnsibleOrgCollection", :all => [org_resource]) }
let(:cred_collection) { double("AnsibleCredCollection", :all => [cred_resource]) }
let(:inv_collection) { double("AnsibleInvCollection", :all => [inv_resource]) }
let(:host_collection) { double("AnsibleHostCollection", :all => [host_resource]) }
let(:job_templ_collection) { double("AnsibleJobTemplCollection", :all => [job_templ_resource]) }
let(:proj_collection) { double("AnsibleProjCollection", :all => [proj_resource]) }

let(:org_resource) { double("AnsibleOrgResource", :id => 12) }
let(:cred_resource) { double("AnsibleCredResource", :id => 13) }
let(:inv_resource) { double("AnsibleInvResource", :id => 14) }
let(:host_resource) { double("AnsibleHostResource", :id => 15) }
let(:job_templ_resource) { double("AnsibleJobTemplResource", :id => 16) }
let(:proj_resource) { double("AnsibleProjResource", :id => 17) }

describe "#ensure_initial_objects" do
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of the describe with only one it inside, but don't change just because of that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, I agree for contexts, but I always use a describe for the method under test regardless of how many tests I'm writing.

it "creates the expected objects" do
expect(org_collection).to receive(:create!).and_return(org_resource)
expect(cred_collection).to receive(:create!).and_return(cred_resource)
expect(inv_collection).to receive(:create!).and_return(inv_resource)
expect(host_collection).to receive(:create!).and_return(host_resource)

subject.ensure_initial_objects(provider, api_connection)
end
end

describe "#remove_demo_data" do
it "removes the existing data" do
expect(org_resource).to receive(:destroy!)
expect(cred_resource).to receive(:destroy!)
expect(inv_resource).to receive(:destroy!)
expect(job_templ_resource).to receive(:destroy!)
expect(proj_resource).to receive(:destroy!)

subject.remove_demo_data(api_connection)
end
end

describe "#ensure_organization" do
it "sets the provider default organization" do
expect(org_collection).to receive(:create!).with(
:name => "ManageIQ",
:description => "ManageIQ Default Organization"
).and_return(org_resource)

subject.ensure_organization(provider, api_connection)
expect(provider.default_organization).to eq(12)
end

it "doesn't recreate the organization if one is already set" do
provider.default_organization = 1
expect(org_collection).not_to receive(:create!)

subject.ensure_organization(provider, api_connection)
end
end

describe "#ensure_credential" do
it "sets the provider default credential" do
provider.default_organization = 123
expect(cred_collection).to receive(:create!).with(
:name => "ManageIQ Default Credential",
:kind => "ssh",
:organization => 123
).and_return(cred_resource)

subject.ensure_credential(provider, api_connection)
expect(provider.default_credential).to eq(13)
end

it "doesn't recreate the credential if one is already set" do
provider.default_credential = 2
expect(cred_collection).not_to receive(:create!)

subject.ensure_credential(provider, api_connection)
end
end

describe "#ensure_inventory" do
it "sets the provider default inventory" do
provider.default_organization = 123
expect(inv_collection).to receive(:create!).with(
:name => "ManageIQ Default Inventory",
:organization => 123
).and_return(inv_resource)

subject.ensure_inventory(provider, api_connection)
expect(provider.default_inventory).to eq(14)
end

it "doesn't recreate the inventory if one is already set" do
provider.default_inventory = 3
expect(inv_collection).not_to receive(:create!)

subject.ensure_inventory(provider, api_connection)
end
end

describe "#ensure_host" do
it "sets the provider default host" do
provider.default_inventory = 234
expect(host_collection).to receive(:create!).with(
:name => "localhost",
:inventory => 234,
:variables => "---\nansible_connection: local\n"
).and_return(host_resource)

subject.ensure_host(provider, api_connection)
expect(provider.default_host).to eq(15)
end

it "doesn't recreate the host if one is already set" do
provider.default_host = 1
expect(host_collection).not_to receive(:create!)

subject.ensure_host(provider, api_connection)
end
end
end
end