Fixes #20622 - puppet import does not refresh all statuses #4749
Conversation
|
Issues: #20622 |
|
@jlsherrill how about that? In Katello we can now override the method same way to only refresh relevant status(es). |
| logger.info("Imported report for #{name} in #{(Time.now.utc - start_time).round(2)} seconds") | ||
| host.refresh_statuses | ||
| imported_time = Time.now | ||
| host.refresh_statuses(statuses_for_refresh) |
There was a problem hiding this comment.
This line is the only change in this method, the rest are changes in logging so it produces saner output.
| @@ -88,6 +89,10 @@ def report_status | |||
| raise NotImplementedError | |||
| end | |||
|
|
|||
| def statuses_for_refresh | |||
| HostStatus.status_registry | |||
There was a problem hiding this comment.
I wonder, is there a way to infer which status changed from the importer class rather then defaulting to refreshing all of them and having to edit all of the importers manually?
There was a problem hiding this comment.
If there is, I'd rather prefer separate PR to be honest :-)
| def refresh_statuses | ||
| HostStatus.status_registry.each do |status_class| | ||
| def refresh_statuses(which = HostStatus.status_registry) | ||
| which.each do |status_class| |
There was a problem hiding this comment.
Is there any situation where one report import needs to update multiple statuses?
There was a problem hiding this comment.
I think the idea came from build substatus where we wanted to flip the state with any other state update. It seems to be displayed correctly but it wouldn't be reflected in DB if it was not updated on this refresh_statuses call, so we wouldn't be able to search for host which token expired.
now when we have active job, we could have a job to verify this state in Time.now + $provisioning_interval
lzap
force-pushed
the
importer-status-20622
branch
from
08402b4
to
a09cf7b
Compare
|
Not really sure why all PG tests failed, rebased. This is ready for review. |
|
@tbrisker mind taking a closer look? |
|
Thanks @lzap ! |
|
Wait @tbrisker which are you referring to? Can't find any. |
|
I think this would benefit from the change https://github.com/theforeman/foreman_openscap/blob/b328b6d6e012cc49c87f034fa2db4da349d6d7be/app/controllers/api/v2/compliance/arf_reports_controller.rb#L53 updating http://projects.theforeman.org/projects/foreman/wiki/How_to_Create_a_Plugin#Extending-host-model would be also good |
|
Updated docs, filed: http://projects.theforeman.org/issues/21910 |
It appears that puppet reports cause all statuses to be updated:
https://github.com/theforeman/foreman/blob/develop/app/services/report_importer.rb#L42
On a heavily loaded system this could mean this is being called 100s of times per second. This is a fairly slow operation to be doing when not necessary.
Puppet reports should only refresh statuses that are relevant to them.