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

Keep container quota history by archiving #16722

Merged
merged 1 commit into from
Mar 13, 2018
Merged

Conversation

cben
Copy link
Contributor

@cben cben commented Dec 31, 2017

Container projects have quotas (optionally scoped), and quota items for specific resources. Some users desire "chargeback by allocation" where the quota is what tenants pay for (whether they used it or not).
We're already collecting quotas, but we need to also retain full quota history — this PR implements this by (mis)using archiving.

See comments in models for design assumptions/choices.

https://bugzilla.redhat.com/show_bug.cgi?id=1504560
@miq-bot add-label enhancement, gaprindashvili/yes

Docs on quotas: https://kubernetes.io/docs/concepts/policy/resource-quotas/, https://docs.openshift.com/container-platform/3.9/admin_guide/quota.html

@@ -16,6 +16,24 @@ def active?
deleted_on.nil?
end

module ClassMethods
def were_active_at_time(time)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were_active_at_time and walk_history don't belong in this PR, I'm just sketching what kind of history access we might want in the future...
I'll remove them when PR is ready (so brakeman travis failure on this will go away)

@cben
Copy link
Contributor Author

cben commented Jan 2, 2018

@zeari @yaacov @moolitayer This now seems to work in interactive testing, please review.
I have to add automated tests of course.

if attribute == "timestamp"
type = model_class.type_for_attribute(attribute)
type.cast(record_key(record, attribute)).utc.iso8601.to_s
type.cast(value).utc.iso8601.to_s
elsif %w(quota_desired quota_enforced quota_observed).include?(attribute)
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, so this starts to look like I should refactor it :-) though a generic solution will be slower

also I am trying to experiment with just removing all of this comparing in ruby and just do things via upsert (so the comparing is done on PG side, which has it's own (fast) type casting)

@cben
Copy link
Contributor Author

cben commented Feb 14, 2018

UPDATE: what I describe here was split off to #17020.

I just pushed commit beggining of refactor I'm trying — add `@deserializable_keys` similar to `@serializable_keys`. What do you think of the direction? I suspect anything serializable should be in deserialiazable set too, i.e. the 2 sets will be same, but I want isolated change for easier backporting (I know code here has already changed; I'll split this to separate PR).

But it's nagging me that such normalization would only happen at save time. I'm tempted to type.cast() at InventoryObject building time (or collection.push time) . Potential benefits:

  1. may be needed for [lazy_]find to work robustly.
  2. would tolerate parser emiting wrong types, eg. 42 vs 42.0 vs '42'.

P.S. I'm stuck on this too long, would like to chat/pair with you...

@cben
Copy link
Contributor Author

cben commented Feb 14, 2018

Finally found a way to test this such that it fails if I don't normalize DB representation vs Ruby representation. I made parser emit instances of this:

class BadDecimal < BigDecimal
  def to_s(*a)
    "000" + super
  end
end

well, that may be too wrong — if casting data from DB creates BigDecimal it may not match a BadDecimal instance (while in reality once you have BigDecimals from both sides, it's fine). But I'll try to write a core test involving some intentional mismatch like that...

@cben
Copy link
Contributor Author

cben commented Feb 19, 2018

These are ready, only [WIP] due to dependencies. @zeari @yaacov please review.

@cben cben force-pushed the quota-history branch 2 times, most recently from 77e1543 to 9727b9b Compare February 19, 2018 16:15
@cben cben changed the title [WIP] Keep container quota history by archiving Keep container quota history by archiving Feb 22, 2018
@miq-bot miq-bot removed the wip label Feb 22, 2018
@cben cben closed this Mar 5, 2018
@cben cben reopened this Mar 5, 2018
@cben
Copy link
Contributor Author

cben commented Mar 5, 2018

@zeari @yaacov @agrare deps merged, please review this & ManageIQ/manageiq-providers-kubernetes#198
(this PR passes core tests alone but I think would break k8s tests, so they should be merged together)

@miq-bot
Copy link
Member

miq-bot commented Mar 5, 2018

Checked commit cben@4c9aebb with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 8 offenses detected

app/models/container_project.rb

app/models/container_quota.rb

app/models/container_quota_item.rb

app/models/ems_refresh/save_inventory_container.rb

app/models/manageiq/providers/container_manager.rb

@cben
Copy link
Contributor Author

cben commented Mar 12, 2018

@agrare @Fryguy Please review this and ManageIQ/manageiq-providers-kubernetes#198, we want these in next gaprindashvili-3(?) / 5.9.1 release.

@agrare
Copy link
Member

agrare commented Mar 12, 2018

@cben how frequently do container quotas change? Are they something which are changed pretty infrequently?
Can you add ContainerQuota and ContainerQuotaItem to the archived_entities_purge_timer so they're cleaned up on a schedule.

@cben
Copy link
Contributor Author

cben commented Mar 12, 2018

quota_desired & quota_enforced columns only change when an admin adds/changes/delete a quota —
pretty infrequently.
But quota_observed column changes a lot! It's not quite a metric that changes in real time, it's a function of inventory. For pod resources like cpu, it's the sum of pod requests, so will change as any pod in same namespace is created/deleted; for quotas that count another resource eg. count/secrets it will change as secrets are created/deleted...
On a big busy project, this may well change more frequently than we refresh, so we may archive each refresh.

I'd discussed with @yaacov whether I should archive on quota_observed changes. We concluded this is still "more inventory than metric" in nature, would be nice to have history of it, and while it may increase archived size significantly, it should be acceptable (hand-waving, haven't tested at scale!).

I'll do purging in separate PR.

@agrare agrare merged commit ffd2be0 into ManageIQ:master Mar 13, 2018
@agrare agrare added this to the Sprint 82 Ending Mar 26, 2018 milestone Mar 13, 2018
simaishi pushed a commit that referenced this pull request Mar 22, 2018
Keep container quota history by archiving
(cherry picked from commit ffd2be0)

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

Gaprindashvili backport details:

$ git log -1
commit b7feaddc588c85b4903a9859de382bf12267ae96
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Mar 13 15:42:23 2018 -0400

    Merge pull request #16722 from cben/quota-history
    
    Keep container quota history by archiving
    (cherry picked from commit ffd2be0fd1b9ec6b7816d2622bae408e6f75b741)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1559544

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