Add support for CPU and memory isolators #610
Conversation
The rkt community added supprt for these isolators recently
| @@ -149,6 +149,19 @@ func (d *RktDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle, e | |||
| cmd_args = append(cmd_args, fmt.Sprintf("--exec=%v", exec_cmd)) | |||
| } | |||
|
|
|||
| if task.Resources.MemoryMB == 0 { | |||
There was a problem hiding this comment.
how do you say "infinite" or "don't care at all" ?
There was a problem hiding this comment.
@jippi Drivers that support resource control, shouldn't allow infinite resources in my opinion since from the point of view of the scheduler finite resources were being assigned to a task.
|
@diptanu thanks, updated |
| @@ -133,33 +133,46 @@ func (d *RktDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle, e | |||
| envVars.ClearAllocDir() | |||
|
|
|||
| for k, v := range envVars.Map() { | |||
| cmd_args = append(cmd_args, fmt.Sprintf("--set-env=%v=%v", k, v)) | |||
| cmdArgs = append(cmdArgs, fmt.Sprintf("--set-env=%v=%v", k, v)) | |||
| } | |||
|
|
|||
| // Disble signature verification if the trust command was not run. | |||
| if !trust_cmd { | |||
There was a problem hiding this comment.
this one should be camelCase too.
| } | ||
|
|
||
| // Add memory isolator | ||
| cmdArgs = append(cmdArgs, fmt.Sprintf("--memory=%vM", int64(task.Resources.MemoryMB)*1024*1024)) |
There was a problem hiding this comment.
IIRC this was not previously supported in rkt. Does this require a particular version of rkt? If so we should check in the fingerprinter that we have a version that supports this.
There was a problem hiding this comment.
Yes, this was added in 0.14.0, rkt/rkt#1851
I assumed we were targeting to converge after post 1.0, so that it's okay to not be backwards compatible for now. If we think the check is necessary, I can add it.
There was a problem hiding this comment.
It's fine not to be BC here but we should still check that the version is supported so we can tell the user to upgrade instead of failing later when a task is started.
There was a problem hiding this comment.
@cbednarski @achanda I almost think we should not make the rkt driver available if the version is less than 0.14
There was a problem hiding this comment.
@diptanu I agree, in favor of uniformity across all the drivers.
There was a problem hiding this comment.
Move 1024*1024 to a constant at the top of the file
| @@ -85,6 +86,13 @@ func (d *RktDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool, e | |||
| node.Attributes["driver.rkt.version"] = rktMatches[1] | |||
| node.Attributes["driver.rkt.appc.version"] = appcMatches[1] | |||
|
|
|||
| minVersion, _ := version.NewVersion("0.14.0") | |||
There was a problem hiding this comment.
I would also put this in a const/variable at the top and put a comment on why we only support 0.14.0 and above.
|
@achanda Thanks! |
Add support for CPU and memory isolators
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
No description provided.