Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

rkt: add --memory and --cpu to override isolators #1851

Merged
merged 1 commit into from Dec 15, 2015

Conversation

alban
Copy link
Member

@alban alban commented Dec 8, 2015

Fixes #1735


Depends on appc/spec#552 / appc/spec#553

@alban
Copy link
Member Author

alban commented Dec 8, 2015

TODO: add tests.

@alban
Copy link
Member Author

alban commented Dec 8, 2015

Updated with tests.

@iaguis
Copy link
Member

iaguis commented Dec 8, 2015

Pending on a spec release

@@ -86,6 +86,8 @@ func init() {
cmdRun.Flags().Var((*appAsc)(&rktApps), "signature", "local signature file to use in validating the preceding image")
cmdRun.Flags().Var((*appExec)(&rktApps), "exec", "override the exec command for the preceding image")
cmdRun.Flags().Var((*appMount)(&rktApps), "mount", "mount point binding a volume to a path within an app")
cmdRun.Flags().Var((*appMemoryLimit)(&rktApps), "memory", "memory limit")
Copy link
Member

Choose a reason for hiding this comment

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

I think we should give more information here:

  • it's for the preceding app
  • syntax

Copy link
Member Author

Choose a reason for hiding this comment

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

Done:

OPTIONS:
      --cpu=                    cpu limit for the preceding image (example: '--cpu=500m')
      --exec=                   override the exec command for the preceding image
      --inherit-env[=false]         inherit all environment variables not set by apps
      --interactive[=false]         run pod interactively. If true, only one image may be supplied.
      --mds-register[=false]            register pod with metadata service. needs network connectivity to the host (--net=(default|default-restricted|host)
      --memory=                 memory limit for the preceding image (example: '--memory=16Mi', '--memory=50M', '--memory=1G')

@alban
Copy link
Member Author

alban commented Dec 9, 2015

Branch updated.

if err != nil {
return err
}
// Just don't accept anything less than 4Ki. It's not reasonnable and
Copy link
Member

Choose a reason for hiding this comment

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

reasonable*

@iaguis
Copy link
Member

iaguis commented Dec 9, 2015

LGTM after fixing the typo

@alban alban force-pushed the alban/limit-cmdline branch 2 times, most recently from 8da3780 to bd9060b Compare December 9, 2015 11:22
@jonboulle
Copy link
Contributor

someone please pick up #1859 :-)

// it's most likely a mistake from the user, such as passing
// --memory=16m (milli-bytes!) instead of --memory=16M (megabytes).
if isolator.Limit().Value() < 4096 {
return fmt.Errorf("memory limit too low. Try a reasonable value, such as --memory=16M")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe print the value, so the user may get the clue immediately what is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I will update.

@jellonek
Copy link
Contributor

Little question - why this PR is adding 2 specific options (cpu, memory) instead of providing general purpose way of overriding values for isolators?

If in future we will have a way to limit I/O (storage, network) in some way, if we would have general purpose mechanism parsing small json part of manifest which would cover values from pod/apps manifests - there would be no need to add new parameters, codebase would be more compact, and probably it would be maintainable with less effort.

What You think about this?

@alban
Copy link
Member Author

alban commented Dec 11, 2015

@jellonek I think that would be an increase of complexity in the UX... For the IO isolator, that would give something like:

$ rkt run image --isolator='{"name": "resource/block-bandwidth", "value": {"default": true, "limit": "2M"}'} 

@alban
Copy link
Member Author

alban commented Dec 11, 2015

Rebased.

@iaguis
Copy link
Member

iaguis commented Dec 15, 2015

I agree with @alban

@iaguis
Copy link
Member

iaguis commented Dec 15, 2015

LGTM

iaguis added a commit that referenced this pull request Dec 15, 2015
rkt: add --memory and --cpu to override isolators
@iaguis iaguis merged commit 00eb94e into rkt:master Dec 15, 2015
alban added a commit to kinvolk/rkt that referenced this pull request Dec 15, 2015
The new rkt parameters --memory and --cpu were added without
documentation via rkt#1851.

Add the documentation.
@alban alban mentioned this pull request Dec 15, 2015
alban added a commit to kinvolk/rkt that referenced this pull request Dec 15, 2015
The new rkt parameters --memory and --cpu were added without
documentation via rkt#1851.

Add the documentation.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants