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

Add ContainerImage raise event post processing job #77

Merged
merged 3 commits into from
Aug 24, 2017
Merged

Add ContainerImage raise event post processing job #77

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
800619e
Add ContainerImage raise event post processing job
Ladas Jul 28, 2017
1376fca
The priority of raising event needs to be the same as of call_automate
Ladas Jul 31, 2017
4eadeb0
Use pluralized raise_creation_events method name
Ladas Jul 31, 2017
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
22 changes: 22 additions & 0 deletions app/models/manageiq/providers/kubernetes/container_manager/refresher_mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,28 @@ def fetch_entities(client, entities)
end
end
end

def manager_refresh_post_processing(_ems, _target, inventory_collections)
Copy link

@moolitayer moolitayer Aug 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not a show stopper, but the name is confusing.
This happens after save to the database right? (since we have the ids already)
I read processing as post parsing(manager_refresh_post would make more sense to me)

I think @cben had a previous commented about something related

Copy link
Contributor Author

@Ladas Ladas Aug 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

"This happens after save to the database right? (since we have the ids already)" yes

right, I am fine with any sane name :-) we will just need to change it also in the core MIQ

Copy link

Choose a reason for hiding this comment

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

OK no point in waiting for the rename, please rename this when you can.

indexed = inventory_collections.index_by(&:name)
container_images_post_processing(indexed[:container_images])
end

def container_images_post_processing(container_images)
# We want this post processing job only for batches, for the rest it's after_create hook on the Model
return unless container_images.saver_strategy == :batch
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer looser coupling (but not a blocker).

  • Can we add a method to InventoryCollection to ask it "did AR hooks run?" instead of "is your strategy one where we happen to know hooks don't run?"

I have several gripes with the after_create hook, I'm wondering if we could switch to post-refresh in all modes (or at least all graph refresh modes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cben hm, right now the non batch strategies always use Active Record objects, rails with hooks, validations, etc.

Maybe we could leverage the container_images.use_ar_object? and implement non AR mode for the default saver.

Yeah, it would be nice to get rid of the after_create entirely, but it will be harder

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 agree it'd be nicer to use this in all cases, not just batch saving, and get rid of the after_create

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for usage in all cases, we would need to implement the Old refresh timestamp based post refresh and then remove it from the Model after_create


# TODO extract the batch size to Settings
Copy link

Choose a reason for hiding this comment

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

👍

batch_size = 100
container_images.created_records.each_slice(batch_size) do |batch|
Copy link

Choose a reason for hiding this comment

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

@Ladas in what order are we getting created records? is it guaranteed by inventory collection's interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hum, the order should be the same as we saved them, which should be the same as we parsed them

container_images_ids = batch.collect { |x| x[:id] }
MiqQueue.submit_job(
:class_name => "ContainerImage",
:method_name => 'raise_creation_event',
:args => [container_images_ids],
:priority => MiqQueue::MIN_PRIORITY
Copy link
Contributor

Choose a reason for hiding this comment

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

What priorities are image scans queued with currently? Can this lead to starvation?

Copy link
Contributor Author

@Ladas Ladas Jul 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, if there is not enough generic workers, the queue can grow, we've seen that. @kbrock have we seen low priority jobs starving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the call_automate job scheduled inside has :priority => MiqQueue::HIGH_PRIORITY, so the same should probably be here

)
end
end
end
end
end
Expand Down