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

Don't create the embedded ansible default project #19056

Merged
merged 1 commit into from
Jul 25, 2019

Conversation

carbonin
Copy link
Member

We don't currently provide any playbooks so we don't need this.
We do provide roles so we still need to gather up the content
provided by the plugins when the embedded_ansible role is enabled
on an appliance. So we still call
Ansible::Content.consolidate_plugin_playbooks here.

Making the roles accessible to other playbook runs is done by altering
the roles search path here:
https://github.com/ManageIQ/manageiq-appliance/blob/9f0e0ed0381785387207343867173888e0b60788/LINK/root/.ansible.cfg#L3

Creating this project will cause an issue when we need to run the
embedded ansible role on multiple appliances. Each appliance would
have created the source SCM locally and the git shas would not match
between the different appliances which would cause issues with the
GitRepository model we use for managing ConfigurationScriptSources.

In the future if we do choose to provide playbooks out of the box
from plugins we can re-evaluate how to deal with this scenario at
that time.

We don't currently provide any playbooks so we don't need this.
We do provide roles so we still need to gather up the content
provided by the plugins when the embedded_ansible role is enabled
on an appliance. So we still call
Ansible::Content.consolidate_plugin_playbooks here.

Making the roles accessible to other playbook runs is done by altering
the roles search path here:
https://github.com/ManageIQ/manageiq-appliance/blob/9f0e0ed0381785387207343867173888e0b60788/LINK/root/.ansible.cfg#L3

Creating this project will cause an issue when we need to run the
embedded ansible role on multiple appliances. Each appliance would
have created the source SCM locally and the git shas would not match
between the different appliances which would cause issues with the
GitRepository model we use for managing ConfigurationScriptSources.

In the future if we do choose to provide playbooks out of the box
from plugins we can re-evaluate how to deal with this scenario at
that time.
@carbonin
Copy link
Member Author

Not sure what the thoughts are here on a data migration. I think we might run into a related issue for upgrades because the previous location of this project was not the same one we have here.

Previously we used /var/lib/awx_consolidated_source and for the last few weeks we've been using /var/www/miq/vmdb/content/ansible_consolidated/ so I could conceive of a data migration to remove any repo with one of those file urls ...

NickLaMuro
NickLaMuro previously approved these changes Jul 24, 2019
Copy link
Member

@NickLaMuro NickLaMuro left a comment

Choose a reason for hiding this comment

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

Heh, find it a bit funny that this chunk of code got added and removed within the same release, but I think the reasoning behind removing it makes sense.

👍

@miq-bot
Copy link
Member

miq-bot commented Jul 24, 2019

Checked commit carbonin@195a04f with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@NickLaMuro
Copy link
Member

Okay, after just seeing this comment:

#19056 (comment)

I am going to remove my approval as I am less certain of the overall effects of making this change when doing an upgrade.

@NickLaMuro NickLaMuro dismissed their stale review July 24, 2019 22:08

Don't have enough grasp on the usage of manageiq-content to be objective here.

@carbonin
Copy link
Member Author

To be clear. We might need the migration either way to make sure we don't have a repo hanging around with the old path. So that might be a good place to get rid of repos with either of these paths.

@Fryguy
Copy link
Member

Fryguy commented Jul 25, 2019

I agree with your analysis here. If we don't need the default project it's better to drop it, but I definitely think we need a data migration to remove it from an existing embedded ansible provider.

cc @agrare @gmcculloug @gtanzillo @jrafanie This removes the ability to provide playbooks from a plugin repo, but it wasn't being used and we can add it back in later in a better way. At present only the roles for manageiq's API access were in the content repo, and we haven't change the part where those roles are available. I just want to check, but were provider playbooks expected to be accessible from EmbeddedAnsible (i.e. via services or automate)?

@agrare
Copy link
Member

agrare commented Jul 25, 2019

I just want to check, but were provider playbooks expected to be accessible from EmbeddedAnsible (i.e. via services or automate)?

AFAIK The playbooks in the provider repos e.g. manageiq-providers-nuage/content/ansible_runner/create-subnet.yml are meant to be self-contained and only intended to be called by ansible-runner from a provider operation, not from embedded_ansible.

If you wanted to create a nuage subnet from a service or automate it is expected that you would queue a ...::CloudSubnet.create_cloud_subnet call which would end up calling the playbook, not call the playbook directly.

@carbonin
Copy link
Member Author

Yeah, @agrare there are two different ways for plugins to provide ansible content. One is as you describe, the other got shoved into a "project" for the AWX-based embedded ansible.

@carbonin
Copy link
Member Author

ansible_content vs ansible_runner_content here:

def ansible_content
@ansible_content ||= begin
require_relative 'plugins/ansible_content'
flat_map do |engine|
content_directories(engine, "ansible").map { |dir| AnsibleContent.new(dir) }
end
end
end
def ansible_runner_content
@ansible_runner_content ||= begin
map do |engine|
content_dir = engine.root.join("content", "ansible_runner")
next unless File.exist?(content_dir.join("requirements.yml"))
[engine, content_dir]
end.compact
end
end

@agrare
Copy link
Member

agrare commented Jul 25, 2019

@carbonin correct, that was my long winded way of saying we dont' need to worry about the provider playbooks in this context.

@Fryguy Fryguy merged commit 564a6be into ManageIQ:master Jul 25, 2019
@Fryguy Fryguy added this to the Sprint 117 Ending Aug 5, 2019 milestone Jul 25, 2019
simaishi pushed a commit that referenced this pull request Jul 25, 2019
Don't create the embedded ansible default project

(cherry picked from commit 564a6be)
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 85a9399af44b40826ffeca44190044f6763dbc3d
Author: Jason Frey <jfrey@redhat.com>
Date:   Thu Jul 25 14:13:45 2019 -0400

    Merge pull request #19056 from carbonin/dont_create_a_default_project
    
    Don't create the embedded ansible default project
    
    (cherry picked from commit 564a6beb0e2a71f3ccc83f0c5306677185af948f)

carbonin added a commit to carbonin/manageiq-schema that referenced this pull request Jul 26, 2019
carbonin added a commit to carbonin/manageiq-schema that referenced this pull request Aug 2, 2019
carbonin added a commit to carbonin/manageiq-schema that referenced this pull request Aug 2, 2019
carbonin added a commit to carbonin/manageiq-schema that referenced this pull request Aug 2, 2019
@carbonin carbonin deleted the dont_create_a_default_project branch August 16, 2019 15:53
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.

6 participants