-
Notifications
You must be signed in to change notification settings - Fork 199
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
cgroups: fix incorrect cgroups cpuset via systemd #2230
Conversation
Ephemeral COPR build failed. @containers/packit-build please check. |
pkg/cgroups/systemd_linux.go
Outdated
@@ -260,3 +262,35 @@ func resourcesToProps(res *configs.Resources, v2 bool) (map[string]uint64, map[s | |||
|
|||
return uMap, sMap, bMap, iMap, structMap | |||
} | |||
|
|||
func RangeToBits(str string) ([]byte) { |
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.
since it is used only here, can we make it private (rangeToBits
)?
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 made it to private (rangeToBits
).
pkg/cgroups/systemd_linux.go
Outdated
start, _ := strconv.ParseUint(startr, 10, 32) | ||
end, _ := strconv.ParseUint(endr, 10, 32) |
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.
please propagate these errors, if they happen
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 added error handling. However, in Podman, values are checked in advance, so no errors occur.
I also added a test to check the value of the converted cpuset.
pkg/cgroups/systemd_linux.go
Outdated
bits.SetBit(bits, int(i), 1) | ||
} | ||
} else { | ||
val, _ := strconv.ParseUint(startr, 10, 32) |
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 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.
I added error handling.
4406064
to
9b32d3e
Compare
pkg/cgroups/systemd_linux.go
Outdated
// fit cpuset parsing order in systemd | ||
for l, r := 0, len(ret)-1; l < r; l, r = l+1, r-1 { | ||
ret[l], ret[r] = ret[r], ret[l] | ||
} |
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 is reversing the slice I assume? Please use slices.Reverse() instead
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've used slices.Reverse()
.
pkg/cgroups/systemd_linux.go
Outdated
uMap, sMap, bMap, iMap, structMap, res_err := resourcesToProps(resources, v2) | ||
if res_err != nil { | ||
return res_err | ||
} |
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.
the error var should be called err
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.
the error var changed to err
.
Signed-off-by: Yuta Maeda <[email protected]> Signed-off-by: Tatsuya Ono <[email protected]>
9b32d3e
to
d63a212
Compare
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, mmmmaeda The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
The cpuset value setting for cgroup is not set correctly.
Fixes #2229