-
Notifications
You must be signed in to change notification settings - Fork 33
Extending event catcher and adding targeted refresh for availabilities #27
Conversation
…ails * Event catcher is extended to retrieve availability metrics from Hawkular for all deployments in inventory. Availabilities are "parsed" and compared against the status stored in MiQ database. Deployments whose status has changed are enqueued to be updated in the refresh worker. * A targeted refresh is added. This targeted refresh only updates the status field of deployment entities when an availability update is received by the refresh worker. No other field is updated, nor entity creation or deletion is performed. That work is left to the full graph refresh. These changes is work towards covering ManageIQ#5
* Event catcher is extended to fetch availability metrics of each inventoried server in MiQ. Also its server state is fetched from Hawkular's inventory. A targeted refresh is queued for servers that have changed their availability or status. * Targeted refresh "availability updated" is also extended to support updating server entities with the fetched new status.
1181377
to
24f7c82
Compare
@miq-bot add_label events |
end | ||
|
||
def id | ||
"Collection: #{@targets.map(&:manager_ref)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the id be such a long string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need to be long. But, as name
, it's only used when writing logs here. I found no other usage of it. I'm trying to be verbose for logging.
end | ||
|
||
def name | ||
"Collection of availabilities to update on inventory entities" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the name displayed somewhere? if yes, it needs translation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only usage is for writing it in logs.
fetch_deployment_availabilities | ||
end | ||
|
||
def fetch_server_availabilities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like there's an opportunity here for code reuse.
because this method and the one below look exactly the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I'll try to think a way to make it reusable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't think a way to refactor it :(
194780f
to
e5054c2
Compare
I think this is ready. Only reviews left. So, I will do: |
@Jiri-Kremser please review. thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, my only concern is whether it was tested with the domain mode and deployments on server groups. From the code I think it should work, but it's hard to say.
|
||
def fetch_server_availabilities(parser) | ||
# For servers, it's also needed to refresh server state from inventory. | ||
$mw_log.debug("#{log_prefix} Retrieving server state from Hawkular inventory") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: perhaps, s/state/states/g
would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now changed :)
end | ||
|
||
def server_updates | ||
@target.select { |item| item.association == :middleware_servers } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you test your code for the WF running in domain mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I tested domain mode. There is no need to make any distinction.
Components being tested are: * Persister and parser of targeted refresh. Collector is implicitly tested with parser. * New fuctions of event catcher's stream. Some existing test are slightly complemented. Some existing tests were adjusted due to small refactors for code reusability and avoid repeating stuff.
e5054c2
to
7e68ff0
Compare
Checked commits israel-hdez/manageiq-providers-hawkular@6cce4f8~...7e68ff0 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@Jiri-Kremser there's no milestone set for this PR |
With these changes, status of deployments and servers will be updated very frequently avoiding any manual refresh.