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

support cgroup v2 #121

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

support cgroup v2 #121

wants to merge 1 commit into from

Conversation

Curl-Li
Copy link

@Curl-Li Curl-Li commented Sep 10, 2022

适配CGroupv2,兼容v1版本,暂未支持混用挂载

@mosn-community-bot
Copy link

Hi @Curl-Li, welcome to mosn community, Please sign Contributor License Agreement!

After you signed CLA, we will automatically sync the status of this pull request in 3 minutes.

@Jun10ng
Copy link
Contributor

Jun10ng commented Sep 14, 2022

Hi Curl, thanks for your PR, this is a large PR for holmes, I need more time to figure out them, and these changes seems very like automaxprocs/tree/master/internal/cgroups.

It seems to be a common solution, but I'm not sure whether it is related to the Golang version

@Curl-Li
Copy link
Author

Curl-Li commented Sep 26, 2022

Hi Curl, thanks for your PR, this is a large PR for holmes, I need more time to figure out them, and these changes seems very like automaxprocs/tree/master/internal/cgroups.

It seems to be a common solution, but I'm not sure whether it is related to the Golang version

I referenced part of automaxprocs's code and wrapped it. About issue of version, I tried golang1.9rc1 and golang1.19.1, both are working well.

@Jun10ng
Copy link
Contributor

Jun10ng commented Jan 9, 2023

LGTM.
It seems standard modifications about CGroups v1/v2.

@doujiang24
Copy link
Member

Thanks, I will take a deeper look when I have a long free time.

Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

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

Thanks for your nice work. LGTM mostly with a few small comments.

Also, I think it's worth adding comments for the files where copying from.
so that we could easier to trace updates and not break the copyrights.


if cpuQuota, err = readUint(cgroupCpuQuotaPath); err != nil {
return 0, err
if quota == -1 {
Copy link
Member

Choose a reason for hiding this comment

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

better add a comment for this -1.

Also, seems -1 means an error happened, so, maybe return 0 + error should be better.

if err != nil {
return 0, err
}
limit := uint64(math.Min(float64(usage), float64(machineMemory.Total)))
if usage == -1 {
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

return 0, err
}
}
quota, _, err := globalCG.CPUQuota()
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not ignore the _, otherwise, it may returns -1, nil, thoughts?

we should check if the _ = true instead.

return 0, err
}
}
limit, _, err := globalCG.MemLimit()
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

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.

3 participants