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

Cast virtual attribute 'Hardware#ram_size_in_bytes' to bigint #15554

Merged
merged 2 commits into from
Jul 13, 2017

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Jul 12, 2017

There is PG::NumericValueOutOfRange thrown in report on ram_size_in_bytes

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

@miq-bot add-label bug, fine/yes

\cc @gtanzillo

virtual_attribute :ram_size_in_bytes, :integer, :arel => (lambda do |t|
t.grouping(Arel::Nodes::Multiplication.new(
Arel::Nodes::NamedFunction.new("CAST", [t[:memory_mb].as("bigint")]), 1.megabyte))
end)
Copy link
Member

Choose a reason for hiding this comment

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

Arel... 😱 .... @kbrock @NickLaMuro Can you review this? I have no idea.

@yrudman Can you add a test that recreates the problem? I don't understand how it overflows ram_size_in_bytes for a single vm in a report. I was under the impression it was a problem aggregating many hardwares for many vm/instances that caused the overflow.

Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable to me, as long as the casting is passed on in the multiplication (I think it is, but unsure). I agree with adding a test to confirm this functionality fails in the old code, and works with the new.

This may be my fault from some of the aggregation mixin performance improvements I did a while back, so if it was, I apologize.

Copy link
Contributor Author

@yrudman yrudman Jul 12, 2017

Choose a reason for hiding this comment

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

I was testing before/after manually.
Not sure how to write rspec, virtual_column_sql_value(Hardware, "ram_size_in_bytes") is working fine. Also, ActiveRecord::Base.connection.execute("SELECT hardwares.memory_mb * 1048576 FROM hardwares") failing on console :

screen shot 2017-07-12 at 4 03 20 pm

but works fine from rspec

Copy link
Member

Choose a reason for hiding this comment

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

@yrudman I would say write a test that creates a hardware record with the same value as what you have in the screenshot you just share, and then try calling the ram_size_in_bytes method on that record before and after this change. Should break in master and then be fixed with your change.

So, you should just be able to use the console you have open already, and run ActiveRecord::Base.connection.select_values("SELECT MAX(hardwares.memory_mb) FROM hardwares") to get your out of range value to plug into a spec. The spec should be something simple like this:

describe "#ram_size_in_bytes" do
  it "should not cause out of range errors" do
    FactoryGirl.create(:hardware, :memory_mb => <<VALUE_YOU_GOT_FROM_THE_CONSOLE_ABOVE>>)
    expect{ Hardware.select(:ram_size_in_bytes).all }.to_not raise_error(ActiveRecord::StatementInvalid)
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As an alternative solution I have migration PR (not merged) to change datatype of memory_mb to bigint and on my test db that column was bigint. So, I was struggling with providing test which is failing on master ....

@yrudman yrudman force-pushed the cust-ram_size_in_bytes-to-bigint branch 2 times, most recently from a145bf0 to c98213d Compare July 12, 2017 20:17
@yrudman yrudman force-pushed the cust-ram_size_in_bytes-to-bigint branch from c98213d to 92301f3 Compare July 13, 2017 12:17
@yrudman yrudman force-pushed the cust-ram_size_in_bytes-to-bigint branch from 92301f3 to 068eb1b Compare July 13, 2017 12:47
@miq-bot
Copy link
Member

miq-bot commented Jul 13, 2017

Checked commits yrudman/manageiq@5231493~...068eb1b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

Nice! 👍

@gtanzillo gtanzillo added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 13, 2017
@gtanzillo gtanzillo merged commit a1b007b into ManageIQ:master Jul 13, 2017
@yrudman yrudman deleted the cust-ram_size_in_bytes-to-bigint branch July 13, 2017 15:03
simaishi pushed a commit that referenced this pull request Aug 4, 2017
Cast virtual attribute 'Hardware#ram_size_in_bytes' to bigint
(cherry picked from commit a1b007b)

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

simaishi commented Aug 4, 2017

Fine backport details:

$ git log -1
commit fec5287534126e44555e4096997e0745e798b416
Author: Gregg Tanzillo <[email protected]>
Date:   Thu Jul 13 09:27:04 2017 -0400

    Merge pull request #15554 from yrudman/cust-ram_size_in_bytes-to-bigint
    
    Cast virtual attribute 'Hardware#ram_size_in_bytes' to bigint
    (cherry picked from commit a1b007b9c76f62a5d152720039b782358fdd7629)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1478565

d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
…-to-bigint

Cast virtual attribute 'Hardware#ram_size_in_bytes' to bigint
(cherry picked from commit a1b007b)

https://bugzilla.redhat.com/show_bug.cgi?id=1478565
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.

7 participants