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 ContainerQuotaScope model, save them in save_inventory #16655

Merged
merged 3 commits into from
Dec 20, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented Dec 13, 2017

Kubernetes namespaces (aka openshift projects) may have quotas.
Each quota may have one or more scopes, which we didn't store before but should:
https://kubernetes.io/docs/concepts/policy/resource-quotas/#quota-scopes
https://docs.openshift.com/container-platform/3.7/admin_guide/quota.html#quota-scopes
Schema was added in ManageIQ/manageiq-schema#111

Core side, followed by ManageIQ/manageiq-providers-kubernetes#190.
I think this PR is safe to merge first.

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

@cben
Copy link
Contributor Author

cben commented Dec 13, 2017

@zeari @enoodle please review

@enoodle
Copy link

enoodle commented Dec 14, 2017

LGTM, travis sais you have to create a factory class for this though

@@ -0,0 +1,3 @@
class ContainerQuotaScope < ApplicationRecord
belongs_to :container_quota
Copy link
Member

Choose a reason for hiding this comment

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

what does this class do ?
why we need this class ?

Copy link
Member

Choose a reason for hiding this comment

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

from discussion in ManageIQ/manageiq-providers-kubernetes#190 is sounds like this class only hold one string field (:scope [string] the name of the scope ) that can currently hold only 4 possible strings ...
Do we need a class to hold a list of pre defined strings, shouldn't we use a text field here ?

Copy link
Contributor Author

@cben cben Dec 18, 2017

Choose a reason for hiding this comment

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

This is a model class. We need it to access the table, because Rails ;-)
As we're discussing on that PR:

  1. schema might be suboptimal, but gaprindashvili schema is frozen and we need this feature there.
  2. since we're going to retain full quotas history by archiving instead of in-place updates, there is a case where this schema may be more effecient to refresh.

Copy link
Member

Choose a reason for hiding this comment

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

@cben does a quata has only 4 possible set of scopes ?
a. terminating and best effort
b. terminating and not best effort
c. not terminating and best effort
d not terminating and not best effort

@miq-bot
Copy link
Member

miq-bot commented Dec 18, 2017

Checked commits cben/manageiq@6b525f4~...70d840b with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. ⭐

@cben
Copy link
Contributor Author

cben commented Dec 18, 2017

Thanks, Travis :) Added factory, PTAL.

@cben
Copy link
Contributor Author

cben commented Dec 18, 2017

@agrare @Fryguy please review

@miq-bot add-label enhancement, gaprindashvili/yes

@@ -2,5 +2,6 @@ class ContainerQuota < ApplicationRecord
belongs_to :ext_management_system, :foreign_key => "ems_id"
belongs_to :container_project

has_many :container_quota_scopes, :dependent => :destroy
Copy link
Member

Choose a reason for hiding this comment

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

@cben you mention in the BZ keep history instead of overwriting in refresh. ref is that something that you want to accomplish in this PR? Should this disconnect instead of destroy the quota scopes?

What is a quota scope BTW?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see answer below #16655 (comment)

@cben
Copy link
Contributor Author

cben commented Dec 18, 2017 via email

@cben
Copy link
Contributor Author

cben commented Dec 18, 2017 via email

Copy link

@enoodle enoodle left a comment

Choose a reason for hiding this comment

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

LGTM,
@cben I think it is worth mentioning here the PR with VCR tests for this.

@cben
Copy link
Contributor Author

cben commented Dec 20, 2017

worth mentioning here the PR with VCR tests for this.

Don't have it yet :-) Need to finish ManageIQ/manageiq-providers-openshift#75 first, then copy new cassettes to this repo.

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

@agrare agrare merged commit 56c179d into ManageIQ:master Dec 20, 2017
@agrare agrare modified the milestones: Roadmap, Sprint 76 Ending Jan 1, 2018 Dec 20, 2017
@simaishi
Copy link
Contributor

simaishi commented Jan 3, 2018

Gaprindashvili backport details:

$ git log -1
commit 8e2347ee43383f9cda7fce277791fcda98db1566
Author: Adam Grare <[email protected]>
Date:   Wed Dec 20 07:58:09 2017 -0500

    Merge pull request #16655 from cben/quota-scopes
    
    Add ContainerQuotaScope model, save them in save_inventory
    (cherry picked from commit 56c179db4cca8000a913f5fed899f24ef12dc065)

simaishi pushed a commit that referenced this pull request Jan 3, 2018
Add ContainerQuotaScope model, save them in save_inventory
(cherry picked from commit 56c179d)
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.

6 participants