Skip to content
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

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

mmmmaeda
Copy link
Contributor

@mmmmaeda mmmmaeda commented Nov 5, 2024

The cpuset value setting for cgroup is not set correctly.

Fixes #2229

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@rhatdan
Copy link
Member

rhatdan commented Nov 5, 2024

@giuseppe @Luap99 PTAL

@@ -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) {
Copy link
Member

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)?

Copy link
Contributor Author

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).

Comment on lines 278 to 279
start, _ := strconv.ParseUint(startr, 10, 32)
end, _ := strconv.ParseUint(endr, 10, 32)
Copy link
Member

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

Copy link
Contributor Author

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.

bits.SetBit(bits, int(i), 1)
}
} else {
val, _ := strconv.ParseUint(startr, 10, 32)
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added error handling.

@mmmmaeda mmmmaeda force-pushed the fix-cgroup-cpuset branch 2 times, most recently from 4406064 to 9b32d3e Compare November 6, 2024 08:37
Comment on lines 319 to 322
// 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]
}
Copy link
Member

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

Copy link
Contributor Author

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().

Comment on lines 59 to 64
uMap, sMap, bMap, iMap, structMap, res_err := resourcesToProps(resources, v2)
if res_err != nil {
return res_err
}
Copy link
Member

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

Copy link
Contributor Author

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]>
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented Nov 7, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Nov 7, 2024
@rhatdan
Copy link
Member

rhatdan commented Nov 7, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Nov 7, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 19bcf2b into containers:main Nov 7, 2024
16 checks passed
@ttyono ttyono deleted the fix-cgroup-cpuset branch November 7, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cgroup cpuset is not set correctly
4 participants