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

Conversation

@carbonin
Copy link
Member

carbonin commented Mar 10, 2017

These objects will be used for dynamically creating things like job templates during playbook runs from automate.

Requires #14377 for storing the ansible side ids with the provider.

https://www.pivotaltracker.com/story/show/140098929

@carbonin

This comment has been minimized.

Copy link
Member Author

carbonin commented Mar 10, 2017

All, marked as WIP until you all sign off on where this kind of stuff belongs. I tested out the logic on an appliance so I really just need opinions on where this should live. (Of course if the logic is wrong, please tell me that too 😜 )

@blomquisg from a provider side view, is this where this kind of thing belongs? I don't know what the responsibilities of the provider/manager typically are so I added it here just because we already had a subclass for embedded ansible for the provider class.

@Fryguy I'm not sure I would want to add the other seeding stuff here. Maybe something in the worker for that as it's more system/filesystem related?

@gmcculloug Is this about what you guys need to create the job templates for the service runs? The provider instance will be accessible to you guys when you need this info I hope?

@carbonin

This comment has been minimized.

Copy link
Member Author

carbonin commented Mar 10, 2017

Ping @jrafanie
This is the bit that will break if Apache isn't up and running when the worker starts. We may want to figure that out before merging this ...

app/models/manageiq/providers/ansible_tower/shared/provider.rb Outdated
end

def default_organization(connection = connect)
@@default_org ||= connection.api.organizations.all(

This comment has been minimized.

Copy link
@Fryguy

Fryguy Mar 10, 2017

Member

Not sure why you are using a class variable. In fact, given that those are inherited, I think that's generally bad right?

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 13, 2017

Author Member

Well, I don't want it to be an instance variable because I want to cache more aggressively than that. I don't have much experience with class instance variables, but it feels like I would need to write explicit accessors to use them from an instance context, right? Plus it's all a bit more complicated because this is a mixin ...

I'm not crazy about any of the options, what do you think?

@Fryguy

This comment has been minimized.

Copy link
Member

Fryguy commented Mar 10, 2017

Not a fan of the term "Sandbox" in these objects. Sandbox implies things you can play with and wreck. This is more of a "ManageIQ default"...closer to seeding.

@Fryguy

This comment has been minimized.

Copy link
Member

Fryguy commented Mar 10, 2017

When we discussed yesterday I thought this would live as part of the embeddedansible worker during its do_before_work_loop phase. Did that not work as expected?

@carbonin

This comment has been minimized.

Copy link
Member Author

carbonin commented Mar 13, 2017

@Fryguy From a timing perspective, yes, it will be called from around there, but I'm not sold on where to put the code.

I felt like it would be a bit strange for the automate methods to call out to a worker class or instance to get the default values so I put everything on the provider.

Like I said, I think the rest of the plugin import stuff should belong with the worker, but I'm a bit split on this part.

@gmcculloug

This comment has been minimized.

Copy link
Member

gmcculloug commented Mar 15, 2017

@bzwei @bdunne Please review

@carbonin carbonin force-pushed the carbonin:add_embedded_tower_initial_objects branch Mar 15, 2017
app/models/manageiq/providers/ansible_tower/shared/provider.rb Outdated

def default_organization(connection = connect)
@@default_org ||= connection.api.organizations.all(
:name => product_name

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 15, 2017

Author Member

Is this a reliable way to identify these objects? If the product_name changes we will not be able to find our defaults.

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 15, 2017

Author Member

Maybe this is a case for adding some kind of "system" identifier @Fryguy ? Then we can just look them up that way after creating them.

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 15, 2017

Author Member

On second (third?) thought a flag won't really help, not for the organization at least, because we don't collect inventory for the organization.

@carbonin

This comment has been minimized.

Copy link
Member Author

carbonin commented Mar 15, 2017

This is the bit that will break if Apache isn't up and running when the worker starts. We may want to figure that out before merging this ...

Turns out it wasn't that complicated #14353 😅

app/models/manageiq/providers/ansible_tower/shared/provider.rb Outdated
@@ -58,8 +58,85 @@ def url=(new_url)
default_endpoint.url = uri.to_s
end

def create_default_data

This comment has been minimized.

Copy link
@Fryguy

Fryguy Mar 16, 2017

Member

This should not be in shared I don't think

@carbonin

This comment has been minimized.

Copy link
Member Author

carbonin commented Mar 16, 2017

Okay, spoke with @Fryguy and we're taking a bit of a new approach.

We're going to store these in custom attributes related to the provider. That way we don't have to rely on our modeling and potentially conflict with the refresher by adding fields (like a system flag or something) to the AR instances and we also don't have to constantly connect to the provider and match names to find things.

The custom attribute setter/getters will live in a concern and they will be used by the EmbeddedAnsibleWorker (for setting) and automate will use the getters when they need to create stuff referencing these objects.

We also won't need a migration, so that's good 😄

@carbonin carbonin force-pushed the carbonin:add_embedded_tower_initial_objects branch 2 times, most recently Mar 16, 2017
@carbonin

This comment has been minimized.

Copy link
Member Author

carbonin commented Mar 17, 2017

I split out the provider side stuff into a separate PR here #14377

I'm going to make this one specific to the EmbeddedAnsibleWorker stuff.

@carbonin carbonin force-pushed the carbonin:add_embedded_tower_initial_objects branch Mar 17, 2017
@carbonin carbonin requested review from jrafanie and removed request for blomquisg and gmcculloug Mar 17, 2017
@carbonin carbonin assigned gtanzillo and unassigned blomquisg Mar 17, 2017
def ensure_host(provider, connection)
return if provider.default_host

provider.default_host = connection.api.hosts.create!(

This comment has been minimized.

Copy link
@bzwei

bzwei Mar 17, 2017

Contributor

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

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 17, 2017

Author Member

@bzwei Is that in the variables yaml string?

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

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 17, 2017

Author Member

I think this should be addressed now.

This comment has been minimized.

Copy link
@bzwei

bzwei Mar 17, 2017

Contributor

👍

@carbonin carbonin force-pushed the carbonin:add_embedded_tower_initial_objects branch Mar 17, 2017
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Mar 17, 2017

This pull request is not mergeable. Please rebase and repush.

@miq-bot miq-bot added the unmergeable label Mar 17, 2017
@carbonin carbonin force-pushed the carbonin:add_embedded_tower_initial_objects branch Mar 17, 2017
@miq-bot miq-bot removed the unmergeable label Mar 17, 2017
@carbonin carbonin force-pushed the carbonin:add_embedded_tower_initial_objects branch Mar 17, 2017
def ensure_organization(provider, connection)
return if provider.default_organization

provider.default_organization = connection.api.organizations.create!(

This comment has been minimized.

Copy link
@bdunne

bdunne Mar 17, 2017

Member

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

app/models/embedded_ansible_worker/runner.rb Outdated
@@ -36,6 +38,17 @@ def setup_ansible
_log.info("calling EmbeddedAnsible.start")
EmbeddedAnsible.start
_log.info("calling EmbeddedAnsible.start finished")

5.times do

This comment has been minimized.

Copy link
@bdunne

bdunne Mar 17, 2017

Member

Is 5 seconds long enough for the server to start up and respond?

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 20, 2017

Author Member

EmbeddedAnsible.start is synchronous so this felt unnecessary to me, but this solved the 502 errors I was getting when testing on an appliance.

We can also make the WAIT_FOR_ANSIBLE_SLEEP a worker setting, but I like to lean towards fewer knobs where I can 😄

This comment has been minimized.

Copy link
@jrafanie

jrafanie Mar 20, 2017

Member

hmmm, it returned to you but it wasn't ready yet? Can we try to hit it using curl or something?

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 20, 2017

Author Member

Can we try to hit it using curl or something?

That's essentially what EmbeddedAnsible.alive? does.

This comment has been minimized.

Copy link
@jrafanie

jrafanie Mar 20, 2017

Member

Does it make sense to have start do the sleep until alive? so start doesn't return when it's not yet alive?

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 20, 2017

Author Member

Yeah, we could do that @jrafanie 👍

app/models/embedded_ansible_worker/runner.rb Outdated

5.times do
if EmbeddedAnsible.alive?
break

This comment has been minimized.

Copy link
@bdunne

bdunne Mar 17, 2017

Member

Would it be simpler if this acted like guard clause?

5.times do
  break if EmbeddedAnsible.alive?

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

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 20, 2017

Author Member

Yup thanks.

@@ -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)

This comment has been minimized.

Copy link
@bdunne

bdunne Mar 17, 2017

Member

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

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 20, 2017

Author Member

👍 I'll look into this.

This comment has been minimized.

Copy link
@jrafanie

jrafanie Mar 20, 2017

Member

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

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 20, 2017

Author Member

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.

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 20, 2017

Author Member

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.

spec/models/embedded_ansible_worker_spec.rb Outdated
context "ObjectManagement concern" do
let(:provider) { FactoryGirl.create(:provider_embedded_ansible) }
let(:api_connection) do
conn = double("AnsibleAPIConnection")

This comment has been minimized.

Copy link
@bdunne

bdunne Mar 17, 2017

Member

It's kind of strange to stub #api to return the connection object itself. This could be:

let(:api_connection) { double("AnsibleAPIConnection", :api => tower_api) }
let(:tower_api)      { double("TowerAPI") }

This comment has been minimized.

Copy link
@bdunne

bdunne Mar 17, 2017

Member

Actually, now that I read further...

let(:tower_api)      { double("TowerAPI", :organizations => org_collection, .....) }
let(:org_collection)       { double("AnsibleOrgCollection", :all => [org_resource]) }

Then you can get rid of the before block with all of the allows.

spec/models/manageiq/providers/embedded_ansible/provider_spec.rb Outdated

context "DefaultAnsibleObjects concern" do
before do
subject.save

This comment has been minimized.

Copy link
@bdunne

bdunne Mar 17, 2017

Member

Why not FactoryGirl.create rather than adding the subject.save in the before block?

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 20, 2017

Author Member

Ah, I think this was a rebase issue. I started this branch before #14383 was merged so instead of changing the subject for all the tests, I saved the record in just my context, during the rebase I think this line got left here.

I'll refactor in a new commit so that the individual specs define the subject rather than the shared example.

Thanks for catching this.

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 20, 2017

Author Member

Also, I'll make this change in #14377

@carbonin carbonin force-pushed the carbonin:add_embedded_tower_initial_objects branch 4 times, most recently Mar 20, 2017
carbonin added 4 commits Mar 16, 2017
…bleWorker::Runner

This creates a concern around creating and removing objects
in the embedded ansible instance.

In particular we remove all of the demo data that is initialized
for us during setup and we create just the objects that we need
and save the ids to the provider.
Without this we were we were getting "502 Bad Gateway" errors when
trying to remove the demo data through API calls to the embedded
ansible instance.  This was because even though the setup playbook
finished, the server would take just a bit longer to start to serve
requests.
@carbonin carbonin force-pushed the carbonin:add_embedded_tower_initial_objects branch to 5d2d5df Mar 20, 2017
@carbonin carbonin changed the title [WIP] Create initial tower objects when we start the worker Create initial tower objects when we start the worker Mar 20, 2017
@carbonin carbonin removed the wip label Mar 20, 2017
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Mar 20, 2017

Checked commits carbonin/manageiq@3484703~...5d2d5df with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
7 files checked, 0 offenses detected
Everything looks good. ⭐️

@carbonin

This comment has been minimized.

Copy link
Member Author

carbonin commented Mar 21, 2017

@bdunne @jrafanie @Fryguy #14377 is merged so I think this one is ready for review.

@gmcculloug

This comment has been minimized.

Copy link
Member

gmcculloug commented Mar 21, 2017

@bdunne @jrafanie @Fryguy Any comments? Would like to get this merged if it is ready as there are followup changes required to reference these new elements.

Specifically I am aware of this one: https://github.com/ManageIQ/manageiq/blob/master/app/models/service_template_ansible_playbook.rb#L94
cc @bzwei


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

This comment has been minimized.

Copy link
@jrafanie

jrafanie Mar 21, 2017

Member

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

@bdunne
bdunne approved these changes Mar 21, 2017
Copy link
Member

bdunne left a comment

👍 LGTM

let(:job_templ_resource) { double("AnsibleJobTemplResource", :id => 16) }
let(:proj_resource) { double("AnsibleProjResource", :id => 17) }

describe "#ensure_initial_objects" do

This comment has been minimized.

Copy link
@bdunne

bdunne Mar 21, 2017

Member

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

This comment has been minimized.

Copy link
@carbonin

carbonin Mar 21, 2017

Author Member

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

@gmcculloug gmcculloug merged commit 5e631c1 into ManageIQ:master Mar 22, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.0007%) to 43.416%
Details
@carbonin carbonin deleted the carbonin:add_embedded_tower_initial_objects branch May 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.