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

Change container_quota_items float columns to decimals #151

Merged
merged 1 commit into from
Jan 18, 2018

Conversation

cben
Copy link
Contributor

@cben cben commented Jan 10, 2018

@Fryguy @agrare @Ladas @moolitayer @zeari @bazulay
I'm told this might still be possible for gaprindashvili?!

Fixes ManageIQ/manageiq-providers-kubernetes#208
Kubernetes quotas are mostly integers but cpu-related quotas are
in "millicores", multiples of 0.001.
With decimal columns, we'll be able to round-trip quotas exactly to DB
and back to Ruby, so refresh can determine whether they changed or not.
(requires couple more code changes, but this fixes the deep root problem)

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

Let's not merge until I have all pieces together, but this does allow the exact fraction to come back to Ruby.

TODO: tests demonstrating what happens to existing values.

TODO: benchmark on lots of data.

Alternative NOT TAKEN: fixed-point integer column (5 would mean 5milli) ?

Some of you said that'd simpler and faster.
We wouldn't want all quotas scaled (memory in millibytes, number of pods in millipods...) but only scale CPU in millicores.

  • However it'd need data migration. Hmm, I suppose if we can do this in gaprindashvili, then data migration is possible too?
  • I'm concerned at this point we'd be likely to introduce unit mismatch bugs.

@miq-bot miq-bot added the wip label Jan 10, 2018
@oourfali
Copy link

@chessbyte you approved this schema change, right?

@Fryguy
Copy link
Member

Fryguy commented Jan 17, 2018

I believe he approved to go into gaprindashvili but not the PR itself (wasn't sure what you meant by "approved")

@Fryguy
Copy link
Member

Fryguy commented Jan 17, 2018

Code-wise LGTM 👍

@Fryguy
Copy link
Member

Fryguy commented Jan 17, 2018

However it'd need data migration. Hmm, I suppose if we can do this in gaprindashvili, then data migration is possible too?

Yes, if this is backported to gap we would also need to bring back the corresponding data migration. Once we cut gaprindashvili, then we can't backport migrations anymore.

@cben
Copy link
Contributor Author

cben commented Jan 17, 2018

Oops, I omitted in PR description what "would be simpler & faster" and "would need data migration".
Edited now. This was referring to alternative of switching from float to integer columns, requiring cpu quotas to be scaled by fixed factor of 1000.

I don't like that, not at this point, concerned we'd miss off-by-1000x bugs in other places... Decimal is overkill but conceptually clean Right way to do it. "Last-moment feature, correct, fast — pick at most 2"...

@Fryguy
Copy link
Member

Fryguy commented Jan 17, 2018

Ah ok, so then this is ready to go?

@cben
Copy link
Contributor Author

cben commented Jan 18, 2018

Let me test this without the other PRs. I think existing refresh that doesn't archive yet would work with decimal just as well but need to confirm.

Fixes ManageIQ/manageiq-providers-kubernetes#208
Kubernetes quotas are mostly integers but cpu-related quotas are
in "millicores", multiples of 0.001.
With decimal columns, we'll be able to round-trip quotas exactly to DB
and back to Ruby, so refresh can determine whether they changed or not.

https://bugzilla.redhat.com/show_bug.cgi?id=1504560
@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2018

Checked commit cben@06227a5 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

cben added a commit to cben/manageiq that referenced this pull request Jan 18, 2018
@cben
Copy link
Contributor Author

cben commented Jan 18, 2018

Rebased and updated timestamp in file name to stay in-order.

I believe this is safe to merge without any code changes.

@cben
Copy link
Contributor Author

cben commented Jan 18, 2018

  • ooh, also need to test UI.

    • quota_desired.to_s still works, quota_desired_display still works (omits ".0" for integral values) and has tests.
    • Project details view — good: project details quotas
    • Project dashboard view — good:
      ("0 cores" looks like a problem EDIT: I had 0 pods running, updated screenshot)
      project dashboard quotas
      • hover tooltip good (on the cpu 0.2 cores bar says "98% available")
  • and reports — good:

    (All Reports -> Configuration Management -> Containes -> Projects by Quota Items)
    manageiq report - projects by quota items

@cben cben changed the title [WIP] Change container_quota_items float columns to decimals Change container_quota_items float columns to decimals Jan 18, 2018
@cben
Copy link
Contributor Author

cben commented Jan 18, 2018

@Fryguy all tested.

@miq-bot miq-bot removed the wip label Jan 18, 2018
@Fryguy Fryguy merged commit 909736e into ManageIQ:master Jan 18, 2018
@Fryguy Fryguy added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 18, 2018
@Fryguy Fryguy added the blocker label Jan 18, 2018
@Fryguy Fryguy self-assigned this Jan 18, 2018
simaishi pushed a commit that referenced this pull request Jan 18, 2018
Change container_quota_items float columns to decimals
(cherry picked from commit 909736e)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 90a252db428966120151c8be57a3745bda44ff76
Author: Jason Frey <[email protected]>
Date:   Thu Jan 18 15:31:15 2018 -0500

    Merge pull request #151 from cben/quota-decimal
    
    Change container_quota_items float columns to decimals
    (cherry picked from commit 909736e0d08de464d352980306a451af5e4d4228)

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.

Float troubles in ContainerQuotaItem archiving
5 participants