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.

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

[WIP] Only collect rollup associations #22609

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jul 10, 2023

VimPerformanceState are used to represent an object in the past.

These records are used for metrics rollups, metric calculations, and chargeback calculations.
Do note, that chargeback calculations do contain some form of a rollup.

We only rollup metrics by the PERF_ROLLUP_CHILDREN association.

Collecting all these other grow our VimPerformanceState table and take up processing time
for collecting vps and downloading vps.

Goal v2

The caching and others assume that the name of the association is the kind of association. so vms is stored in 'Model#vms'. But as seen, sometimes we take vms from all_vms_and_templates and others.

The goal is to define in the model the associations.
From here, hopefully we can prune out any associations that don't make sense. (which will give us a big reduction in speed of creating a VimPerformanceState and the amount of data stored in the database.)

@kbrock kbrock requested a review from agrare as a code owner July 10, 2023 17:41
@kbrock kbrock changed the title Only collect rollup associations [WIP] Only collect rollup associations Jul 11, 2023
@kbrock
Copy link
Member Author

kbrock commented Jul 11, 2023

WIP: code is done. need to research whether we use any of these other associations

@kbrock
Copy link
Member Author

kbrock commented Jul 15, 2023

update:

  • fixed clear cache part of find_states, which needed to take into account that ASSOCIATIONS is gutted

@kbrock kbrock force-pushed the vps_associations branch 3 times, most recently from fd43d93 to 14a8029 Compare July 19, 2023 03:10
@kbrock kbrock requested a review from Fryguy as a code owner July 19, 2023 03:10
@kbrock
Copy link
Member Author

kbrock commented Jul 21, 2023

update:

  • renamed vm_and_templates to vms_and_templates
  • renamed container to containers
  • now clearing associations by model association name (vs vps association name before)

Comment on lines 66 to 103
if defined?(self.class::PERF_ROLLUPS)
self.class::PERF_ROLLUPS
else
self.class::PERF_ROLLUP_CHILDREN.each_with_object({}) { |x, h| h[x] = x }
end
Copy link
Member

Choose a reason for hiding this comment

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

This makes me think that instead of introducing a new constant that partially duplicates PERF_ROLLUP_CHILDREN that we should just outright change PERF_ROLLUP_CHILDREN.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that they serve 2 different purposes, so perhaps the other option is a better name than PERF_ROLLUPS; perhaps VIM_PERFORMANCE_STATE_ROLLUP_CHILDREN?

@kbrock
Copy link
Member Author

kbrock commented Aug 1, 2023

outstanding:

  • determine how chargebacks uses VPS. We can see the specs are creating these records, how did they fetch the states and how do they access VPS.
  • image tag names are a good example of something that needs help

thoughts:

  • premise is they are going through vim performance tag values
  • how do image tags tie in

@miq-bot
Copy link
Member

miq-bot commented Aug 11, 2023

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Nov 13, 2023

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@kbrock
Copy link
Member Author

kbrock commented Nov 13, 2023

update:

  • rebase and fixed merge

comments:

  • like idea of changing PERF_ROLLUPS into key/value instead of 2 (duplicate) variables
  • unhappy with current state of vps lookups and caching. the vms column remapping wasn't handled correctly. probably one of the drivers for this PR in the first place
  • most of these associations look like they will never be used. Not positive that I fully understand how the values are used so reluctant to put my foot down saying this is the only access path.

@miq-bot miq-bot added the stale label Feb 19, 2024
@miq-bot
Copy link
Member

miq-bot commented Feb 19, 2024

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 stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

consolidate logic for association names
Kubernetes currently defines this, but making it generic for all child container managers
before, the rollups were limited
now, unsure

not sure if clearing associations code is still valid
@miq-bot
Copy link
Member

miq-bot commented Mar 11, 2024

Checked commits kbrock/manageiq@172dcbe~...5b45f67 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
17 files checked, 6 offenses detected

app/models/metric/ci_mixin/state_finders.rb

@kbrock
Copy link
Member Author

kbrock commented Mar 11, 2024

update:

in a WIP state. getting ready to green-red-green that the rollups are correct.

all the warnings highlight temporary code that want to remove.

@miq-bot
Copy link
Member

miq-bot commented Jun 17, 2024

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 stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

1 similar comment
@miq-bot
Copy link
Member

miq-bot commented Sep 23, 2024

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 stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants