Improve performance capture ems targets #494
Conversation
| with_archived(ems.all_container_nodes) + | ||
| with_archived(ems.all_container_groups) + | ||
| with_archived(ems.all_containers) + | ||
| with_archived(ems.container_images) |
There was a problem hiding this comment.
NOTE there are no container_images that support capture, but they are rolled up.
Do we need to return them from capture_ems_targets for rollups to work? ems.container_images.supporting(:capture) will return [] always.
There was a problem hiding this comment.
I still need to look into this question
There was a problem hiding this comment.
capture_ems_target are only called for capturing.
the images are captured and then that kicks off perf_capture_rollup which then rollsup images into containers via ContainerImage::PERF_ROLLUP_CHILDREN
So I don't see how we can roll those up without a container image going through metrics collection.
the VimPerformanceState does collect the images association, but I think that is unrelated
There was a problem hiding this comment.
@agrare ok. This is a simple answer: If no data will come back, then there is no reason to query them.
Was that the only thing that was holding this up?
Aah, just read again.
Hmm... yea. back to "no idea"
| end | ||
| end | ||
|
|
||
| trait :with_invalid_auth do |
There was a problem hiding this comment.
these 2 traits are order dependent.
Not sure if you document or something
For Kubernetes type providers if the ems has metrics credentials then all instances of container_nodes, container_groups, etc... supports capture. We don't have to check `.supporting(:capture)` if we are in a kubernetes provider, but this isn't generally true so only implement this behavior in k8s.
agrare
force-pushed
the
improve_performance_capture_ems_targets
branch
from
a639cde to
1d48e80
Compare
|
Checked commit agrare@1d48e80 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint app/models/manageiq/providers/kubernetes/container_manager/metrics_capture.rb
|
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
agrare
changed the title
| @@ -57,7 +57,12 @@ def capture_ems_targets(_options = {}) | |||
| return [] | |||
| end | |||
|
|
|||
| super | |||
| MiqPreloader.preload([ems], :container_images => :tags, :container_nodes => :tags, :container_groups => [:tags, :containers => :tags]) | |||
There was a problem hiding this comment.
:tags are included so we can ask each of the objects if they have capture enabled. (answered through tags)
If we are punting on that behavior, I think we can drop => :tags
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
|
@agrare what do you need from me before I merge it? |
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
2 similar comments
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
|
This pull request has been automatically marked as stale because it has not been updated for at least 3 months. If these changes are still valid, please remove the |
For Kubernetes type providers if the ems has metrics credentials then all instances of container_nodes, container_groups, etc... supports capture.
We don't have to check
.supporting(:capture)if we are in a kubernetes provider, but this isn't generally true so only implement this behavior in k8s.Depends on: