-
Notifications
You must be signed in to change notification settings - Fork 205
Add numa cpuset support #334
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
Conversation
d393477
to
fc75aa0
Compare
15a8fd2
to
ce46c24
Compare
runtime/cpuset/cpuset_builder.go
Outdated
sep = "" | ||
} | ||
|
||
str += fmt.Sprintf("%s%d-%d", sep, r.min, r.max) |
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.
nit: _range
seems like a good candidate for a String()
method
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
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.
Looks good to me. Just a few suggestions about commenting and documentation.
runtime/cpuset/cpuset_builder.go
Outdated
} | ||
|
||
// AddMemRange adds a range of memory nodes by utilizing the minimum node to | ||
// use to the maximum node to use. |
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'm not quite sure what you're trying to say here.
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.
Fixed
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.
It looks like there are merge conflicts. Can you rebase this PR on top of the current master
branch and resolve them?
|
||
func stringify(elems []int, ranges []_range) string { | ||
str := "" | ||
for _, elem := range elems { |
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.
Any reason for doing this instead of using strings.Join
?
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.
Gah, good call out. Just slipped the mind.
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.
@xibz Can you update this to use strings.Join
?
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.
This returns a string using Join. Was there something else you had in mind? Cause I think this is probably the simplest way to go?
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.
@xibz My apologies, I misread this. I might use strconv.Itoa
instead of fmt.Sprintf
, but that's really minor.
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.
Ah cool, and no worries! I'll go ahead and fix the fmt.Sprintf
issue. Thanks!
str += fmt.Sprintf("%s%d", sep, elem) | ||
} | ||
|
||
for _, r := range ranges { |
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.
Same question here.
a33efce
to
c471534
Compare
runtime/jailer_integ_test.go
Outdated
func TestJailerCPUSet_Isolated(t *testing.T) { | ||
prepareIntegTest(t, withJailer()) | ||
|
||
t.Run("Jailer NumaNode", func(t *testing.T) { |
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.
Can you update the name to reflect the new function name?
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!
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.
Changes look good to me. Please just clean up the commit history at this point.
This change adds support for specifying a set of cpu and memory nodes during the CreateVM call. This will run the jailed process on those set of cpus and restrict access to the specified nodes. This change also adds the cpuset.Builder which allows for easy creation of cpus and mems strings Signed-off-by: xibz <[email protected]>
221995e
to
29ee484
Compare
Fix BuildKite's race condition
This change adds support for specifying a set of cpu and memory nodes
during the CreateVM call. This will run the jailed process on those set
of cpus and restrict access to the specified nodes. This change also
adds the cpuset.Builder which allows for easy creation of cpus and mems
strings
Signed-off-by: xibz [email protected]
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.