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

Use the Ansible service in containers rather than starting it locally #15423

Merged
merged 6 commits into from
Jun 27, 2017

Conversation

carbonin
Copy link
Member

This PR makes the EmbeddedAnsible class and the EmbeddedAnsibleWorker::Runner container-aware. This means that their behavior will change when they detect that we are running in a container rather than in an appliance.

In an appliance we start and configure embedded ansible by installing it on one of our servers and then manage the services running local to that appliance. In the container (OpenShift) case we will have a separate container dedicated to running the Ansible service which we will configure against when our server is told to start the "embedded_ansible" role.

This allows everything else in our application to use the embedded ansible provider exactly the same as they had before.

@carbonin
Copy link
Member Author

Marking this as WIP until I get the ansible container up and running in the manageiq-pods repo.

@carbonin carbonin added the wip label Jun 21, 2017
@carbonin carbonin changed the title Use the Ansible service in containers rather than starting it locally [WIP] Use the Ansible service in containers rather than starting it locally Jun 21, 2017
@carbonin carbonin force-pushed the use_ansible_service_in_containers branch from b8f6b91 to 949556f Compare June 21, 2017 20:45
:base_url => URI::HTTP.build(:host => "localhost", :path => "/api/v1", :port => HTTP_PORT).to_s,
:username => admin_auth.userid,
:password => admin_auth.password
:base_url => URI::HTTP.build(:host => host, :path => "/api/v1", :port => port).to_s,
Copy link
Member

Choose a reason for hiding this comment

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

I know this isn't changed in this PR but here and the place we do the URI...build isn't checking if the host is nil. I'm not sure what we can do there but I believe this will blow up building the URI if it's nil.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a better choice than blowing up?

:base_url => URI::HTTP.build(:host => host, :path => "/api/v1", :port => port).to_s,
:username => admin_auth.userid,
:password => admin_auth.password,
:verify_ssl => 0
Copy link
Member

Choose a reason for hiding this comment

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

🙈

Copy link
Member

Choose a reason for hiding this comment

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

It's internal API that's not exposed externally, so not a problem

def self.container_start
miq_database.set_ansible_admin_authentication(:password => ENV["ANSIBLE_ADMIN_PASSWORD"])

loop do
Copy link
Member

Choose a reason for hiding this comment

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

ooh, infinite loop?

Copy link
Member

Choose a reason for hiding this comment

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

any reason it's not 5.times... like the other places?

Copy link
Member Author

Choose a reason for hiding this comment

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

@carbonin carbonin changed the title [WIP] Use the Ansible service in containers rather than starting it locally Use the Ansible service in containers rather than starting it locally Jun 22, 2017
@carbonin carbonin removed the wip label Jun 22, 2017
@@ -133,6 +165,29 @@
EvmSpecHelper.create_guid_miq_server_zone
end

describe ".api_connection" do
around do |example|
old_env = ENV["ANSIBLE_SERVICE_NAME"]
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason that this ENV variable should exist in the test environment? I would expect it to look more like:

around do |example|
  ENV["ANSIBLE_SERVICE_NAME"] = "ansible-service"
  example.run
  ENV.delete("ANSIBLE_SERVICE_NAME")
end

miq_database.set_ansible_admin_authentication(:password => ENV["ANSIBLE_ADMIN_PASSWORD"])

loop do
return if alive?
Copy link
Member

Choose a reason for hiding this comment

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

You may want to use break rather than return since return doesn't just stop the loop, it also returns out of the method. I know it doesn't matter right now, but could be confusing later if you add more to the method after the loop.

end

around do |example|
old_env = ENV["ANSIBLE_SERVICE_NAME"]
Copy link
Member

Choose a reason for hiding this comment

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

I don't expect this to exist

When we know we are running in a container (read: OpenShift) we
also know that embedded ansible will be provided as a separate
service. So rather than pointing at one of our servers as the
provider URL, use the service name.
We don't need to concern ourselves with trying to manage the
ansible services when we are in a container because we will be
running them as a separate container and contacting the API through
a service rather than locally.
This also sets verify_ssl to 0 for this connection.
This is okay because in the appliance case we are communicating locally
and in the container case we are communicating within the openshift
project.
If we are in a container we don't attempt to start any of the
services locally, but we set the password appropriatly and
wait for the service to be available
This shouldn't really be set anywhere we are running the specs
@carbonin carbonin force-pushed the use_ansible_service_in_containers branch from df7943f to dee942b Compare June 26, 2017 19:07
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@miq-bot
Copy link
Member

miq-bot commented Jun 26, 2017

Checked commits carbonin/manageiq@36ea0ea~...dee942b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. 🍪

@Fryguy Fryguy merged commit f3ae24e into ManageIQ:master Jun 27, 2017
@Fryguy Fryguy added this to the Sprint 64 Ending Jul 3, 2017 milestone Jun 27, 2017
@carbonin carbonin deleted the use_ansible_service_in_containers branch October 13, 2017 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants