-
Notifications
You must be signed in to change notification settings - Fork 900
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 awx as an embedded ansible plugin #16205
Add awx as an embedded ansible plugin #16205
Conversation
ec9dab6
to
e45b418
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! 👍
require 'docker' | ||
Docker.validate_version! | ||
rescue RuntimeError | ||
false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should log something here in case the error is not what we are expecting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to avoid logging here because this could get called whenever we do EmbeddedAnsible.new
which could be quite a lot in production.
I will look into if we can rescue a more specific error though.
This pull request is not mergeable. Please rebase and repush. |
e45b418
to
285ceda
Compare
end | ||
|
||
def awx_task_image_name | ||
"ansible/awx_task:latest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking for suggestions around how to make this configurable.
Should I go for creating a new key in Settings
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for Settings
285ceda
to
f04352a
Compare
lib/embedded_ansible.rb
Outdated
@@ -43,9 +43,45 @@ def api_connection_raw(host, port) | |||
) | |||
end | |||
|
|||
def find_or_create_secret_key | |||
miq_database.ansible_secret_key || miq_database.ansible_secret_key = SecureRandom.hex(16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be saved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I persuade you to miq_database.ansible_secret_key ||= SecureRandom.hex(16)
?
lib/embedded_ansible.rb
Outdated
end | ||
|
||
def database_configuration | ||
@db_config ||= Rails.configuration.database_configuration[Rails.env] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer ActiveRecord::Base.configurations
. See #15269 for more info
def start | ||
run_rabbitmq_container | ||
run_memcached_container | ||
sleep(15) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there something better that we could do? I assume we're waiting for rabbitmq and memcached to start, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the other containers have any of their own checks for these services?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually yanked this directly from the playbook in ansible/awx
Because these are not "services" it seems harder to check their status.
Maybe we can pull the ncat
checks into ManageIQ/manageiq in some way and reuse those here and in the pod in a follow up?
spec/lib/embedded_ansible_spec.rb
Outdated
expect(connection).to receive(:select_value).with("CREATE DATABASE awx OWNER \"awx\" ENCODING 'utf8'") | ||
|
||
auth = subject.send(:find_or_create_database_authentication) | ||
expect(auth.userid).to eq("awx") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use have_attributes
to reduce the number of expectations.
end | ||
|
||
URI::HTTPS.build(:host => host, :path => path).to_s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but you can also code this up such that each path in the conditional creates a hash, with :scheme
as one of the keys, then you can DRY up the URI, build, to_s bits down here into a single line.
auth | ||
end | ||
|
||
def generate_password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't tell if they aren't already, but these should probably be private methods from here down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, they're private starting at line 34
Looks good! Sorry about the delayed review. |
e42f543
to
13891f9
Compare
Leaving this as WIP until ManageIQ/manageiq-appliance-build#250 is merged which adds docker to the appliance. |
Requires ManageIQ/manageiq-appliance#160 |
lib/embedded_ansible.rb
Outdated
end | ||
|
||
def find_or_create_admin_authentication | ||
miq_database.ansible_admin_authentication || miq_database.set_ansible_admin_authentication(:password => generate_password) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these also be moved to miq_database.ansible_admin_authentication=
then collapsed to ||=
like 52bab34#r155362349 ?
This will make it easier to move this method into a shared location
74396f2
to
574a6c4
Compare
This allows the "fetch from the database or generate and save" behavior to be shared across different embedded ansible platforms
This will run the containers which make up AWX (https://github.com/ansible/awx) and configure our app to use that for the embedded ansible feature. This class uses the docker-api gem to communicate with the locally running docker daemon to pull and launch the containers. We use port 54321 as the host port so that this can be used seamlessly in place of ApplianceEmbeddedAnsible when ansible tower is not installed locally
When we have "localhost" in our database configuration, we have to change that to the local machine's IP on the docker NIC
This also adds stubs for all of the subclass availability in each of the specs to avoid sporadic test failures depending on the order the subclasses are evaluated for availability.
This sorts the subclasses and instantiates the first available one
This error will be raised when the containers are just started. Every API end point during the initial migration will return an html page rather than a json payload. This accounts for that specific situation by assuming if we don't get a valid json response the service is not ready to serve requests
This really just assumes that a dev environment isn't multi-appliance and isn't fronted by our httpd configuration. This means that we always go to localhost, use http over https and hardcode the port and path.
574a6c4
to
b015723
Compare
Checked commits carbonin/manageiq@a754fa8~...b015723 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0 app/models/embedded_ansible_worker/runner.rb
|
end | ||
end | ||
|
||
private_constant :DockerDaemon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
|
||
def docker_bridge_gateway | ||
br = Docker::Network.get("bridge") | ||
br.info["IPAM"]["Config"].first["Gateway"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetch_path
FTW
…ackend Add awx as an embedded ansible plugin (cherry picked from commit 5d27b97)
Gaprindashvili backport details:
|
This fixes #15975
This PR builds on #16168 by adding a new EmbeddedAnsible plugin which handles running AWX.
AWX is the upstream of Ansible Tower and is distributed in a collection of containers rather than rpms.
The
DockerEmbeddedAnsible
class manages these containers using the docker-api gem.When the role is enabled on an upstream appliance or on a development machine, we will pull the latest images, and run the containers using the same credential management system as we would in the appliance case.
The class also handles setting up the container environment required. This is mostly a ruby version of the playbook role that the AWX project uses to install the containers without all the image build and PG bits.