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.

Sign up for GitHub

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

Jump to bottom

Allow to run post processing job for ManagerRefresh (Graph Refresh) #15678

Merged
merged 1 commit into from
Aug 1, 2017
Merged

Allow to run post processing job for ManagerRefresh (Graph Refresh) #15678

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion app/models/ems_refresh/refreshers/ems_refresher_mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,11 @@ def preprocess_targets
end

def refresh_targets_for_ems(ems, targets)
# handle a 3-part inventory refresh process
# handle a 4-part inventory refresh process
# 1. collect inventory
# 2. parse inventory
# 3. save inventory
# 4. post process inventory (only when using InventoryCollections)
log_header = format_ems_for_logging(ems)

targets_with_inventory, _ = Benchmark.realtime_block(:collect_inventory_for_targets) do
Expand All @@ -90,9 +91,20 @@ def refresh_targets_for_ems(ems, targets)

Benchmark.realtime_block(:save_inventory) { save_inventory(ems, target, hashes) }
_log.info "#{log_header} Refreshing target #{target.class} [#{target.name}] id [#{target.id}]...Complete"

if hashes.kind_of?(Array)
Copy link
Member

Choose a reason for hiding this comment

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

I really dislike checking if hashes is an array to imply manager_refresh, we already do that here save_ems_inventory and I don't want to add another.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it's a funky condition, but we don't really have better. We could extract that condition somewhere though.

Copy link
Member

@agrare agrare Aug 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I was wondering if we could move the existing post_refresh here, but this is per target and that is global so not a great fit.
Responded to the wrong comment

Copy link
Member

Choose a reason for hiding this comment

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

I think we should at least be sending a ManagerRefresh::Inventory::Persister instead of an Array here so it is more obvious what case we are dealing with, but that can be a follow-up refactoring PR lets just not lose track of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, but the problem is that we throw away the Persister too now. All we send is a list of ICs. Might be good to refactor it all to pass Persister though.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I was thinking if instead of sending the inventory collections here we could just send the persister but I'll need to play around with it a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that would make sense

_log.info "#{log_header} ManagerRefresh Post Processing #{target.class} [#{target.name}] id [#{target.id}]..."
# We have array of InventoryCollection, we want to use that data for post refresh
Benchmark.realtime_block(:manager_refresh_post_processing) { manager_refresh_post_processing(ems, target, hashes) }
_log.info "#{log_header} ManagerRefresh Post Processing #{target.class} [#{target.name}] id [#{target.id}]...Complete"
end
end
end

def manager_refresh_post_processing(_ems, _target, _inventory_collections)
Copy link
Member

Choose a reason for hiding this comment

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

Can we re-use the existing post_refresh method? If you need the inv collections maybe we could move that into the refresh_targets_for_ems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I see we can't, the current post refresh is called to late, so at a point where all InventoryCollections are thrown away. And we cannot simply store them, since it goes over many targets, so there is possibility of having more Persiters there

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I was wondering if we could move the existing post_refresh here, but this is per target and that is global so not a great fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the old refresh is kind of disconnected from the processing, so only thing we can do (and are doing) are timestamp based queries to figure out what have changed

# Implement post refresh actions in a specific refresher
end

def collect_inventory_for_targets(ems, targets)
# TODO: implement this method in all refreshers and remove from here
# legacy refreshers collect inventory during the parse phase so the
Expand Down