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

Add aggregate_memory to container project #18159

Merged

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Nov 5, 2018

Currently container project is missing aggregate_memory method.

BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1618536

@yaacov
Copy link
Member Author

yaacov commented Nov 5, 2018

@agrare @cben please review

@agrare agrare self-assigned this Nov 5, 2018
@agrare
Copy link
Member

agrare commented Nov 5, 2018

@yaacov if you include the AggregationMixin you shouldn't need to call aggregate_memory off of ext_management_system

@yaacov yaacov force-pushed the wip-add-aggregate_memory-to-container-project branch from 1f25ed6 to 9920193 Compare November 5, 2018 15:37
@yaacov
Copy link
Member Author

yaacov commented Nov 5, 2018

@agrare please re-review

@yaacov yaacov force-pushed the wip-add-aggregate_memory-to-container-project branch 4 times, most recently from 70e499f to dadc7b5 Compare November 5, 2018 16:54
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

Can you also add an aggregate_cpu_speed and aggregate_cpu_total_cores methods? Then you shouldn't need to define the alias on all_host_ids.

app/models/container_project.rb Outdated Show resolved Hide resolved
@yaacov yaacov force-pushed the wip-add-aggregate_memory-to-container-project branch from dadc7b5 to a97887c Compare November 5, 2018 19:53
@JPrause
Copy link
Member

JPrause commented Nov 5, 2018

@miq-bot add_label blocker

@JPrause
Copy link
Member

JPrause commented Nov 5, 2018

@yaacov if this will be able to be backported, can you add the hammer/yes flag.

@miq-bot miq-bot added the blocker label Nov 5, 2018
@miq-bot
Copy link
Member

miq-bot commented Nov 5, 2018

Checked commit yaacov@a97887c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@agrare
Copy link
Member

agrare commented Nov 5, 2018

Ran a quick test with the expected changes applied:

VimPerformanceState.where(:resource_type => "ContainerProject").map { |state| [state.resource.name, state.state_data[:total_mem]] }
=> [["bookinfo", 128656], ["customer-logging", 128656], ["inventory-dev", 128656], ["default", 128656], ["istio-system", 128656], ["joev-miq-11", 128656]]

So the total_mem is no longer nil 👍

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

LGTM, future enhancement would be nice to get away from the hard-coded :host in aggregate_hardware in the mixin and define what assoc to use for the aggregation. For now this looks good though.

@yaacov
Copy link
Member Author

yaacov commented Nov 5, 2018

@yaacov if this will be able to be backported, can you add the hammer/yes flag.

@JPrause I do not know this part of the code :-( @agrare, WDYT ?

@agrare
Copy link
Member

agrare commented Nov 5, 2018

@JPrause yes it can be backported

@agrare agrare merged commit edd65ff into ManageIQ:master Nov 5, 2018
@agrare agrare added this to the Sprint 98 Ending Nov 5, 2018 milestone Nov 5, 2018
@simaishi
Copy link
Contributor

simaishi commented Nov 5, 2018

@agrare how about gaprindashvili?

@agrare
Copy link
Member

agrare commented Nov 5, 2018

Should be fine for gaprindashvili

simaishi pushed a commit that referenced this pull request Nov 5, 2018
…tainer-project

Add aggregate_memory to container project

(cherry picked from commit edd65ff)

https://bugzilla.redhat.com/show_bug.cgi?id=1618536
@simaishi
Copy link
Contributor

simaishi commented Nov 5, 2018

Hammer backport details:

$ git log -1
commit 58788a9dcf7205712837e301bfc57073af45e911
Author: Adam Grare <[email protected]>
Date:   Mon Nov 5 15:18:47 2018 -0500

    Merge pull request #18159 from yaacov/wip-add-aggregate_memory-to-container-project
    
    Add aggregate_memory to container project
    
    (cherry picked from commit edd65ff422b8179622b284a549bfb4afe92168ce)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1618536

simaishi pushed a commit that referenced this pull request Nov 6, 2018
…tainer-project

Add aggregate_memory to container project

(cherry picked from commit edd65ff)

https://bugzilla.redhat.com/show_bug.cgi?id=1647056
@simaishi
Copy link
Contributor

simaishi commented Nov 6, 2018

Gaprindashvili backport details:

$ git log -1
commit 4136e963cdb36ff49c46429acc0509d801b82f54
Author: Adam Grare <[email protected]>
Date:   Mon Nov 5 15:18:47 2018 -0500

    Merge pull request #18159 from yaacov/wip-add-aggregate_memory-to-container-project
    
    Add aggregate_memory to container project
    
    (cherry picked from commit edd65ff422b8179622b284a549bfb4afe92168ce)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1647056

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.

5 participants