collect container definition limits #22

zeari · 2017-05-22T12:09:14Z

Depends on and completes ManageIQ/manageiq#15180
More info: https://kubernetes.io/docs/tasks/administer-cluster/cpu-memory-limit/

~$ oc describe pod nginx-1-deploy
Name:			nginx-1-deploy
Namespace:		aris-project
Security Policy:	restricted
Node:			---
Start Time:		Sun, 21 May 2017 12:18:34 +0300
Containers:
  deployment:
...
    Limits:      <-------------------------------------
      cpu:	300m
      memory:	200Mi
    Requests:
      cpu:		200m
      memory:		100Mi
    State:		Terminated
      Reason:		Error
      Exit Code:	1
      Started:		Sun, 21 May 2017 12:18:40 +0300
 ...

@simon3z @cben @moolitayer

cben · 2017-05-23T11:48:03Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

@@ -643,6 +643,15 @@ def parse_container_definition(container_def, pod_id)
        parse_container_env_var(env_var)
      end

+      limits_hash = container_def.try(:resources).try(:limits).to_h
+      new_result[:container_limit_items] = limits_hash.to_h.collect do |resource, value|


redundant .to_h, already did it in line above

simon3z · 2017-05-29T22:40:41Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

+    BINARY_SUFFICES = ["Ki", "Mi", "Gi", "Ti", "Pi", "Ei"].freeze
+    SI_SUFFICES = ["K", "M", "G", "T", "P", "E"].freeze
+
+    def parse_quantity(resource) # convert to bytes\cores


@zeari can't we use iec_60027_2_to_i as for the capacity fields?

@simon3z we can but just for Ki,Mi,Gi...
i can try to get this code into https://github.com/ManageIQ/more_core_extensions instead

@zeari yes let's extend that or anyway implement it in more_core_extensions

simon3z · 2017-06-01T12:26:27Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

@@ -646,6 +650,28 @@ def parse_container_definition(container_def, pod_id)
      new_result
    end

+    BINARY_SUFFICES = ["Ki", "Mi", "Gi", "Ti", "Pi", "Ei"].freeze
+    SI_SUFFICES = ["K", "M", "G", "T", "P", "E"].freeze


@zeari s/SUFFICES/SUFFIXES/ and I think we're missing m (which is different from M) and k is lowercase. Anyway check the quantity.go file.

simon3z · 2017-06-19T16:40:31Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

@@ -646,6 +655,19 @@ def parse_container_definition(container_def, pod_id)
      new_result
    end

+    BINARY_SUFFIXES = ["Ki", "Mi", "Gi", "Ti", "Pi", "Ei"].freeze


@zeari this is spreading the internals of iec_60027_2_to_i. What if iec_60027_2_to_i will add a new suffix?

What if you try to parse iec_60027_2_to_i and on ArgumentError you fallback to decimal_si_to_f?

Or anyway anything that keeps the logic in the iec_60027_2_to_i module. E.g. adding a is_60027_2?
(Now that I think of it this second option seems nicer than the ArgumentError)

simon3z · 2017-06-20T20:53:17Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

+
+      # covert to milicores
+      new_result[:limit_cpu] = (new_result[:limit_cpu] * 1000).to_i unless new_result[:limit_cpu].nil?
+      new_result[:request_cpu] = (new_result[:request_cpu] * 1000).to_i unless new_result[:request_cpu].nil?


@zeari if we parse this with iec_60027_2_to_i which is using floats we won't recover the rounding by multiplying by 1000.

Either we do the entire parsing in integers or we may as well keep these in floats.

i guess youre correct but are we expecting cpu to appear in binary form? thats seems weird to me. '1Mi' cores?

Limits are user-supplied, right?
If user is allowed to write 1Ki, somebody will :-), and we should support it.

so we can make a float version (iec_60027_2_to_f) in core_extensions if thats what we want to do there. We can instead add a 'target units' option where we can choose in which units we want the result to be. for example "1.1111Mi".iec_60027_2_to_f("m") would give us the result in milicores(milianything).

simon3z · 2017-07-08T12:25:48Z

LGTM 👍 waiting for the schema ManageIQ/manageiq-schema#25 to get merged.

simon3z · 2017-07-18T08:36:59Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

+        :privileged           => container_def.securityContext.try(:privileged),
+        :run_as_user          => container_def.securityContext.try(:runAsUser),
+        :run_as_non_root      => container_def.securityContext.try(:runAsNonRoot),
+        :security_context     => parse_security_context(container_def.securityContext),


@zeari let's not make this dependent on the container definition removal.

If you just add the limit/request_cpu_cores and limit/request_memory_bytes then we can merge now.

cben · 2017-07-18T08:37:00Z

close-cycling is unlikely to help, Travis fails on bin/setup due to ManageIQ/manageiq#15585 (comment)

zeari · 2017-07-18T12:30:55Z

Also, we need a new more_core_extensions that would have decimal_si_to_f ManageIQ/more_core_extensions#43 (comment)

miq-bot · 2017-07-26T21:14:07Z

This pull request is not mergeable. Please rebase and repush.

zeari · 2017-07-27T11:50:04Z

more_core_extensions was updated which makes the tests green :)

cben · 2017-07-27T14:36:26Z

LGTM but can you add a test?

zeari · 2017-07-30T13:08:13Z

LGTM but can you add a test?

Added a test for parse_quantity + parse_container_spec

miq-bot · 2017-07-30T13:09:06Z

Checked commit zeari@e2ddbf6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

moolitayer

LGTM 👍

zeari force-pushed the get_anon_limits branch from 704418f to 0304e7f Compare May 22, 2017 13:51

cben reviewed May 23, 2017

View reviewed changes

simon3z self-assigned this May 24, 2017

simon3z added the enhancement label May 24, 2017

zeari force-pushed the get_anon_limits branch from 0304e7f to b870dfc Compare May 25, 2017 08:12

zeari changed the title ~~collect container defninition limits~~ collect container definition limits May 25, 2017

zeari force-pushed the get_anon_limits branch 4 times, most recently from b202f81 to 25b7caf Compare May 28, 2017 11:53

zeari mentioned this pull request May 28, 2017

Collect limit items for container definitions ManageIQ/manageiq#15180

Closed

simon3z reviewed May 29, 2017

View reviewed changes

simon3z reviewed Jun 1, 2017

View reviewed changes

zeari force-pushed the get_anon_limits branch 5 times, most recently from 7224621 to c82bc10 Compare June 19, 2017 13:18

zeari mentioned this pull request Jun 19, 2017

add decimal_si_to_f ManageIQ/more_core_extensions#43

Merged

simon3z reviewed Jun 19, 2017

View reviewed changes

simon3z reviewed Jun 20, 2017

View reviewed changes

zeari force-pushed the get_anon_limits branch 4 times, most recently from 032ed97 to e3c644c Compare June 22, 2017 14:08

zeari force-pushed the get_anon_limits branch from e3c644c to b598b86 Compare July 17, 2017 14:19

simon3z closed this Jul 18, 2017

simon3z reopened this Jul 18, 2017

simon3z reviewed Jul 18, 2017

View reviewed changes

miq-bot added the unmergeable label Jul 26, 2017

zeari force-pushed the get_anon_limits branch from b598b86 to 5481e7e Compare July 27, 2017 10:41

miq-bot removed the unmergeable label Jul 27, 2017

zeari force-pushed the get_anon_limits branch from 5481e7e to 5eb7818 Compare July 27, 2017 11:51

collect container defninition limits

e2ddbf6

zeari force-pushed the get_anon_limits branch from 5eb7818 to e2ddbf6 Compare July 30, 2017 13:07

cben approved these changes Jul 30, 2017

View reviewed changes

cben mentioned this pull request Jul 30, 2017

Convert quotas to numeric values #69

Merged

moolitayer approved these changes Jul 30, 2017

View reviewed changes

moolitayer added the inventory label Jul 30, 2017

moolitayer merged commit 0c4e816 into ManageIQ:master Jul 30, 2017

moolitayer added this to the Sprint 66 Ending Aug 7, 2017 milestone Aug 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

collect container definition limits #22

collect container definition limits #22

zeari commented May 22, 2017 •

edited

Loading

cben May 23, 2017 •

edited

Loading

simon3z May 29, 2017

zeari Jun 1, 2017

simon3z Jun 1, 2017

simon3z Jun 1, 2017

simon3z Jun 19, 2017

simon3z Jun 20, 2017

zeari Jun 22, 2017

cben Jun 23, 2017

zeari Jul 2, 2017

simon3z commented Jul 8, 2017

simon3z Jul 18, 2017

cben commented Jul 18, 2017

zeari commented Jul 18, 2017 •

edited

Loading

miq-bot commented Jul 26, 2017

zeari commented Jul 27, 2017 •

edited

Loading

cben commented Jul 27, 2017

zeari commented Jul 30, 2017

miq-bot commented Jul 30, 2017

moolitayer left a comment

collect container definition limits #22

collect container definition limits #22

Conversation

zeari commented May 22, 2017 • edited Loading

cben May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon3z commented Jul 8, 2017

Choose a reason for hiding this comment

cben commented Jul 18, 2017

zeari commented Jul 18, 2017 • edited Loading

miq-bot commented Jul 26, 2017

zeari commented Jul 27, 2017 • edited Loading

cben commented Jul 27, 2017

zeari commented Jul 30, 2017

miq-bot commented Jul 30, 2017

moolitayer left a comment

Choose a reason for hiding this comment

zeari commented May 22, 2017 •

edited

Loading

cben May 23, 2017 •

edited

Loading

zeari commented Jul 18, 2017 •

edited

Loading

zeari commented Jul 27, 2017 •

edited

Loading