rkt: add --memory and --cpu to override isolators #1851
Conversation
TODO: add tests. |
8367f65
to
66fd872
Compare
66fd872
to
88b2e74
Compare
Updated with tests. |
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") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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')
88b2e74
to
4fe70cc
Compare
Branch updated. |
if err != nil { | ||
return err | ||
} | ||
// Just don't accept anything less than 4Ki. It's not reasonnable and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reasonable*
LGTM after fixing the typo |
8da3780
to
bd9060b
Compare
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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will update.
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? |
bd9060b
to
b671960
Compare
@jellonek I think that would be an increase of complexity in the UX... For the IO isolator, that would give something like:
|
Rebased. |
I agree with @alban |
LGTM |
rkt: add --memory and --cpu to override isolators
The new rkt parameters --memory and --cpu were added without documentation via rkt#1851. Add the documentation.
The new rkt parameters --memory and --cpu were added without documentation via rkt#1851. Add the documentation.
Fixes #1735
Depends on appc/spec#552 / appc/spec#553