Tower CUD to invoke targeted refresh #14954

jameswnl · 2017-05-01T19:21:46Z

For bug https://bugzilla.redhat.com/show_bug.cgi?id=1445995

After adding/editing a repo, user would like to see the playbooks asap. This PR is to trigger Tower's internal repo sync and we'll then inventory the updated repo and its playbooks.

@miq-bot add_label bug, providers/ansible_tower, wip

jameswnl · 2017-05-01T19:33:05Z

@miq-bot remove_label wip

blomquisg · 2017-05-03T18:16:45Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/tower_api.rb

@@ -8,7 +8,9 @@ def create_in_provider(manager_id, params)
 tower_object = provider_collection(manager).create!(params)

 refresh(manager)
- find_by!(:manager_id => manager.id, :manager_ref => tower_object.id)
+ target = find_by!(:manager_id => manager.id, :manager_ref => tower_object.id)
+ refresh(target)


There has to be a better way to do this than refreshing twice.

@agrare @Fryguy any ideas on this? Can we leverage any of the work that the oVirt team did for event-based refreshes?

Event-base has to perform a 2nd refresh as well -- except if we don't need to return the db record here.

I must be missing something but why would we need to refresh the target again if we just refreshed the whole manager?

The target refresh of a project would trigger Tower side repo sync first. Ems refresh will not.

Agreed, once you've refreshed the manager, the target must exist.

I'll try to fill in the details from what I know.

"We" want to make sure that the Tower Repos have done a git pull before we refresh them. This allows the inventory collection to get the most up-to-date playbooks from the repos.

However, this is only implemented for targeted refreshes. This means that while doing a targeted refresh of a Tower Repo, the refresh process will ask the repo to update (git pull), and waits until it knows that's done. Then, it continue with the targeted refresh.

This is not done for full refreshes. I guess, because it would mean that the full refresh would have to ask all of the repos to git pull and maybe it's too hard correlating all those events back together to mean "all repos are updated, continue full refresh".

To me this looks like a combination of refresh_new_target (the oVirt event-based refresh stuff I talked about earlier) and native operation workflow.

Talked to @agrare a little offline and sounds like we're in agreement.

The trick here is that this change is blocking the final Fine build scheduled for next week. And, I'm not sure there's going to be time to change this all around to try to use any of that.

Yeah if this is what we have to do to fix a blocker I'm 👍 with it

Longer term I agree refresh_new_target and native op workflow can help here; I think we should also look at the inconsistent behavior between manager/full refresh and targeted refresh.

I think we should also look at the inconsistent behavior between manager/full refresh and targeted refresh.

Yeah, big 👍 here.

See my latest patch. It is now 1 refresh.

jameswnl · 2017-05-03T19:22:17Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/tower_api.rb

@@ -6,7 +6,9 @@ def create_in_provider(manager_id, params)
 params = provider_params(params) if respond_to?(:provider_params)
 manager = ExtManagementSystem.find(manager_id)
 tower_object = provider_collection(manager).create!(params)
-
+ if respond_to?(:refresh_in_provider)


here we already have the tower object and so we can try to trigger the sync.

I like this better ... The full refresh can stay for now until we have @agrare's refresh_new_target in place.

And, this avoids doing the double refresh just to make sure we get the updated Tower git repo.

Nice @jameswnl this looks much better :)

dont want to be the spoilsport, but refresh_new_target is for the old refresh and this is using the graph refresh.
But good news follows: afaik the graph refresh supports that out of the box. ManagerRefresh::Target
@Ladas can you point to a small code sample in amazon, where you create objects from hashes?

https://github.com/Ladas/manageiq-providers-amazon/blob/cd02dc09e805424ef90cf30f082f710000c09319/spec/models/manageiq/providers/amazon/cloud_manager/vcr_specs/targeted_refresh_spec.rb#L273-L273

I think we can integrate that to the refresh_new_target and switch based on configuration.

blomquisg · 2017-05-03T19:48:26Z

app/models/manageiq/providers/ansible_tower/shared/automation_manager/tower_api.rb

@@ -81,7 +82,7 @@ def update_in_provider_queue(params)

 def delete_in_provider
 with_provider_object(&:destroy!)
- self.class.send('refresh', manager)
+ self.class.send('refresh', self)


I think the delete should still be a full refresh, right? I don't know if deletes can be targeted since it might have to be removed from other relationships.

that's true!

blomquisg · 2017-05-03T20:01:02Z

...s/manageiq/providers/ansible_tower/shared/inventory/collector/configuration_script_source.rb

@@ -4,7 +4,7 @@ def connection
 end

 def projects
- target.refresh_in_provider
+ target.class.refresh_in_provider(project)


I hate being a jerk about this, but I think the instance method should remain, even if there needs to be a class method available for calling refresh_in_provider on objects not yet in the MIQ DB.

I would see it as:

module Shared::ConfigurationScriptSource module ClassMethods def refresh_in_provider(project) # ... full impl end end def refresh_in_provider with_provider_object { |project| self.class.refresh_in_provider(project) } end end

Then, callers don't need to do all the class dereferencing on instances and then have to know that they need to get the underlying provider project reference to pass into the class method.

And, then this line would just revert to the original source.

This is nice. Done now!

durandom · 2017-05-03T20:27:59Z

"We" want to make sure that the Tower Repos have done a git pull before we refresh them. This allows the inventory collection to get the most up-to-date playbooks from the repos.

However, this is only implemented for targeted refreshes. This means that while doing a targeted refresh of a Tower Repo, the refresh process will ask the repo to update (git pull), and waits until it knows that's done. Then, it continue with the targeted refresh.

This is not done for full refreshes. I guess, because it would mean that the full refresh would have to ask all of the repos to git pull and maybe it's too hard correlating all those events back together to mean "all repos are updated, continue full refresh".

I think I outlined this here https://www.pivotaltracker.com/story/show/144430761 quite good.
We dont want to do a git pull on the repo on every refresh, because thats not what users expect, I guess. There is a difference between refresh inventory and update configuration script source

agrare · 2017-05-03T20:32:48Z

@durandom yup that's exactly it :)

Also sorry for the close/reopen...its right next to the comment button haha

blomquisg · 2017-05-03T20:33:32Z

agrare closed this just now

Dang @agrare! Throwin' down the gauntlet!

miq-bot · 2017-05-03T20:37:37Z

Checked commit jameswnl@e623e7f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

jameswnl · 2017-05-04T14:05:29Z

@miq-bot add_label fine/yes

simaishi · 2017-05-04T14:10:48Z

Fine backport details:

$ git log -1
commit 4cd3f1426f21580cfb6a37b9435efb4773de9848
Author: Greg Blomquist <blomquisg@gmail.com>
Date:   Thu May 4 09:38:41 2017 -0400

    Merge pull request #14954 from jameswnl/project-target-refresh
    
    Tower CUD to invoke targeted refresh
    (cherry picked from commit edd5c1b26c9072807cc04f63219306dda856b44e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1448098

Tower CUD to invoke targeted refresh (cherry picked from commit edd5c1b) https://bugzilla.redhat.com/show_bug.cgi?id=1448098

miq-bot added bug providers/ansible_tower wip labels May 1, 2017

jameswnl changed the title ~~[WIP] Tower CUD to invoke targeted refresh~~ Tower CUD to invoke targeted refresh May 1, 2017

miq-bot removed the wip label May 1, 2017

jameswnl force-pushed the project-target-refresh branch 3 times, most recently from 0377e18 to 8f2e0cd Compare May 2, 2017 19:23

chessbyte assigned blomquisg May 3, 2017

jameswnl force-pushed the project-target-refresh branch 3 times, most recently from 78dfd3f to eaa0422 Compare May 3, 2017 17:40

blomquisg reviewed May 3, 2017

View reviewed changes

jameswnl commented May 3, 2017

View reviewed changes

jameswnl force-pushed the project-target-refresh branch from 5437da9 to d584c35 Compare May 3, 2017 19:23

blomquisg reviewed May 3, 2017

View reviewed changes

jameswnl force-pushed the project-target-refresh branch from d584c35 to bd16336 Compare May 3, 2017 20:00

blomquisg reviewed May 3, 2017

View reviewed changes

jameswnl force-pushed the project-target-refresh branch from bd16336 to c051b3a Compare May 3, 2017 20:19

blomquisg approved these changes May 3, 2017

View reviewed changes

agrare closed this May 3, 2017

agrare reopened this May 3, 2017

Tower CUD to invoke targeted refresh

e623e7f

jameswnl force-pushed the project-target-refresh branch from c051b3a to e623e7f Compare May 3, 2017 20:34

blomquisg merged commit edd5c1b into ManageIQ:master May 4, 2017

blomquisg added this to the Sprint 60 Ending May 8, 2017 milestone May 4, 2017

jameswnl deleted the project-target-refresh branch May 4, 2017 13:42

simaishi added the blocker label May 4, 2017

miq-bot added the fine/yes label May 4, 2017

simaishi added fine/backported and removed fine/yes labels May 4, 2017

simaishi pushed a commit that referenced this pull request May 4, 2017

Merge pull request #14954 from jameswnl/project-target-refresh

4cd3f14

Tower CUD to invoke targeted refresh (cherry picked from commit edd5c1b) https://bugzilla.redhat.com/show_bug.cgi?id=1448098

jameswnl mentioned this pull request May 8, 2017

Tower CUD check and run refresh_in_provider followed by refreshing manager #15025

Merged

Fryguy added the providers/automation label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tower CUD to invoke targeted refresh #14954

Tower CUD to invoke targeted refresh #14954

jameswnl commented May 1, 2017 •

edited

Loading

jameswnl commented May 1, 2017

blomquisg May 3, 2017

jameswnl May 3, 2017

agrare May 3, 2017

jameswnl May 3, 2017

Fryguy May 3, 2017

blomquisg May 3, 2017

blomquisg May 3, 2017

agrare May 3, 2017

blomquisg May 3, 2017

jameswnl May 3, 2017

jameswnl May 3, 2017

blomquisg May 3, 2017

agrare May 3, 2017

durandom May 3, 2017

Ladas May 4, 2017

blomquisg May 3, 2017

jameswnl May 3, 2017

jameswnl May 3, 2017

blomquisg May 3, 2017

jameswnl May 3, 2017

durandom commented May 3, 2017

agrare commented May 3, 2017

blomquisg commented May 3, 2017

miq-bot commented May 3, 2017

jameswnl commented May 4, 2017

simaishi commented May 4, 2017

Tower CUD to invoke targeted refresh #14954

Tower CUD to invoke targeted refresh #14954

Conversation

jameswnl commented May 1, 2017 • edited Loading

jameswnl commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom commented May 3, 2017

agrare commented May 3, 2017

blomquisg commented May 3, 2017

miq-bot commented May 3, 2017

jameswnl commented May 4, 2017

simaishi commented May 4, 2017

jameswnl commented May 1, 2017 •

edited

Loading