Support cgroup2 #44

emadolsky · 2022-01-04T21:27:08Z

CGroup-V2 has a different architecture which is unified. Since it is not yet supported in go, it makes sense to currently have the
support. This commit adds the functionality to set GOMAXPROCS in cgroup2 enabled systems.

Also there were lint errors because of build annotation comments which are fixed in a separate commit.

This PR resolves #21.

CLAassistant · 2022-01-04T21:27:12Z

All committers have signed the CLA.

CGroup-V2 has a different architecture which is unified. Since it is not yet supported in go, it makes sense to currently have the support. This commit adds the functionality to set GOMAXPROCS in cgroup2 enabled systems.

ardan-bkennedy · 2022-01-27T14:14:35Z

Can someone at Uber provide information about if this project is still being supported? I'm considering to fork the project so it can be maintained.

alxn · 2022-01-27T23:39:40Z

Thanks for the PR - we are looking for a reviewer internally. @prashantv and @akshayjshah have left the building...

akshayjshah · 2022-01-27T23:41:37Z

Happy to look at this PR, Alun, but I'd love to also get a stamp from someone who can speak authoritatively for Uber infra.

brettbuddin · 2022-02-09T11:40:34Z

Has anyone had a chance to review this yet? We've recently upgraded Flatcar and are running into cgroup V2 issues. If this is on track to be merged, we can just wait for it instead of forking and applying a patch.

Thanks for good work here, everyone. 😄

ardan-bkennedy · 2022-02-09T14:10:16Z

You don't need to wait. Run this command at the root of your project where the go.mod file is located

go get github.com/emadolsky/automaxprocs@master

Then switch your import to using this.

"github.com/emadolsky/automaxprocs/maxprocs"

Everything will work.

dprotaso · 2022-03-14T15:26:50Z

@alxn are you able to find an internal reviewer?

dprotaso · 2022-03-14T15:42:29Z

cc @abhinav

akshayjshah

Thanks for the PR, @emadolsky! I'm not at Uber any more and didn't actually author this code, but it's nice to see this package still providing some value for the community.

I've left some comments on the PR (which hopefully the current Uber folks agree with). I'd wait for a review from one of the current maintainers before making any changes, though - in practice, they may not be accepting PRs for this package at all. In the meantime, it looks like you're the maintainer of the new automaxprocs! 😸

internal/cgroups/cgroup.go

internal/cgroups/cgroups.go

maciej · 2022-03-28T14:30:13Z

internal/cgroups/cgroups.go

+}
+
+func cpuQuotaV2(cgroupv2MountPoint, cgroupv2CPUMax string) (float64, bool, error) {
+	cpuMaxParams, err := os.Open(path.Join(cgroupv2MountPoint, cgroupv2CPUMax))


Here you should also include the cgroup-path from /proc/self/cgroup, see https://man7.org/linux/man-pages/man7/cgroups.7.html

codecov · 2022-03-29T16:42:23Z

Codecov Report

Merging #44 (57aa42c) into master (7ff767a) will increase coverage by 0.88%.
The diff coverage is 96.00%.

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
+ Coverage   92.07%   92.95%   +0.88%     
==========================================
  Files           8        9       +1     
  Lines         164      213      +49     
==========================================
+ Hits          151      198      +47     
- Misses         10       11       +1     
- Partials        3        4       +1

Impacted Files	Coverage Δ
internal/cgroups/cgroups2.go	`95.34% <95.34%> (ø)`
internal/runtime/cpu_quota_linux.go	`62.50% <100.00%> (+22.50%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ff767a...57aa42c. Read the comment docs.

https://github.com/uber-go/guide/blob/master/style.md#group-similar-declarations

abhinav

Hey folks, sorry for the delay. @mway and I will review this.
I've made an initial pass in terms of style and organization and made some updates.
In particular, I've moved the bulk of the v2 logic into separate files.

On top of that, I have limited knowledge of the underlying systems so I need some help understanding this:
Why is the mountpoint hard-coded to be /sys/fs/cgroup? As far as I can tell, mountinfo says "this is where cgroups2 is mounted", so should we not use whatever path is specified in mountinfo? Is it absolutely required for cgroups2 to mount to that path?

internal/cgroups/cgroups2.go

Naming wise, "give" and "want" are equally informative and much shorter.

This CGroups2 has the same interface as the CGroups object. This allows the logic in cpu_quota_linux to remain largely unchanged.

Turn the NewCGroupsForCurrentProcess and NewCGroups2ForCurrentProcess functions into function references so that we can replace them from tests.

abhinav

I've made changes for readability, added more tests, and changed cpu_quota_linux so that the changes to the core logic are minimal.

This change LGTM now. I'm going to have someone else take a quick look over it, but it should be good to go otherwise.

mway

lgtm

internal/cgroups/cgroups2.go

ardan-bkennedy · 2022-04-05T23:18:13Z

This change has not fixed the problem. I switched my code from "github.com/emadolsky/automaxprocs/maxprocs" to v1.5.0 and I am still getting that the CPU Quota is not defined.

ardan-bkennedy · 2022-04-05T23:23:36Z

I noticed that in v1.5.0

https://github.com/uber-go/automaxprocs/blob/v1.5.0/internal/runtime/cpu_quota_linux.go#L35

func CPUQuotaToGOMAXPROCS(minValue int) (int, CPUQuotaStatus, error) {[Jason Lai, 5 years ago: • Import `automaxprocs/x/runtime`](https://sourcegraph.com/github.com/uber-go/automaxprocs/-/commit/572239b375b1ca6b76babeb8b0ee139dc35fd4d8)
	cgroups, err := newQueryer()
	if err != nil {
		return -1, CPUQuotaUndefined, err
	}

	quota, defined, err := cgroups.CPUQuota()
	if !defined || err != nil {
		return -1, CPUQuotaUndefined, err
	}

	maxProcs := int(math.Floor(quota))
	if minValue > 0 && maxProcs < minValue {
		return minValue, CPUQuotaMinUsed, nil
	}
	return maxProcs, CPUQuotaUsed, nil
}

var (
	_newCgroups2 = cg.NewCGroups2ForCurrentProcess
	_newCgroups  = cg.NewCGroupsForCurrentProcess
)

func newQueryer() (queryer, error) {
	cgroups, err := _newCgroups2()
	if err == nil {
		return cgroups, nil
	}
	if errors.Is(err, cg.ErrNotV2) {
		return _newCgroups()
	}
	return nil, err
}

In the emadolsky repo, it's working. I don't know enough to tell you why.

func CPUQuotaToGOMAXPROCS(minValue int) (int, CPUQuotaStatus, error) {
	var quota float64
	var defined bool
	var err error

	isV2, err := cg.IsCGroupV2()
	if err != nil {
		return -1, CPUQuotaUndefined, err
	}

	if isV2 {
		quota, defined, err = cg.CPUQuotaV2()
		if !defined || err != nil {
			return -1, CPUQuotaUndefined, err
		}
	} else {
		cgroups, err := cg.NewCGroupsForCurrentProcess()
		if err != nil {
			return -1, CPUQuotaUndefined, err
		}

		quota, defined, err = cgroups.CPUQuota()
		if !defined || err != nil {
			return -1, CPUQuotaUndefined, err
		}
	}

	maxProcs := int(math.Floor(quota))
	if minValue > 0 && maxProcs < minValue {
		return minValue, CPUQuotaMinUsed, nil
	}
	return maxProcs, CPUQuotaUsed, nil
}

emadolsky · 2022-04-06T13:09:48Z

@abhinav - I also verify that the merged version doesn't work properly.

In #44, we accidentally turned v2 mountpoint detection from "are any of the mountpoints v2 to "is the last mountpoint v2". This fixes that condition and updates the test case to catch that in the future. Resolves #49

bsergean · 2024-02-22T04:43:56Z

I've add issue getting this working on k8s 1.26 (on google cloud), where apparently we use cgroup v2 ; I'm not positive that this is working.

emadolsky added 2 commits January 4, 2022 22:37

Add support for cgroup-v2

fa06d43

CGroup-V2 has a different architecture which is unified. Since it is not yet supported in go, it makes sense to currently have the support. This commit adds the functionality to set GOMAXPROCS in cgroup2 enabled systems.

Fix build file annotations

0ae5491

emadolsky force-pushed the support-cgroup2 branch from 32bb5b7 to 0ae5491 Compare January 4, 2022 21:38

emadolsky mentioned this pull request Jan 5, 2022

Memory limiter: Support cgroups v2 and its unified hierarchy open-telemetry/opentelemetry-collector#4457

Closed

embano1 mentioned this pull request Jan 27, 2022

maxprocs: CPU quota undefined #43

Closed

dprotaso mentioned this pull request Mar 14, 2022

use automaxprocs to calculate the number of CPUs ko-build/ko#649

Merged

akshayjshah reviewed Mar 14, 2022

View reviewed changes

daxmc99 mentioned this pull request Mar 25, 2022

Add Datadog profiler sourcegraph/zoekt#314

Merged

Apply review comments

748a33b

maciej suggested changes Mar 28, 2022

View reviewed changes

abhinav added 6 commits March 29, 2022 09:45

Merge branch 'master' into support-cgroup2

8a8d286

Merge branch 'master' into support-cgroup2

2cc0849

internal/cgroups: Pull v2 logic into separate files

91616e5

cpu_quota_linux: group var decls

55be037

https://github.com/uber-go/guide/blob/master/style.md#group-similar-declarations

Rename testTable to tests, use subtests

99817e4

test: Merge nonexistent into test table

bab65bf

abhinav reviewed Mar 29, 2022

View reviewed changes

internal/cgroups/cgroups2.go Outdated Show resolved Hide resolved

abhinav added 4 commits April 4, 2022 20:21

test table: Use "want" instead of "expected" and "should"

072a91c

Naming wise, "give" and "want" are equally informative and much shorter.

cpuQuotaV2: more coverage

6f26022

v2: Pull state and logic into a CGroups2 object

91b17b1

This CGroups2 has the same interface as the CGroups object. This allows the logic in cpu_quota_linux to remain largely unchanged.

cpu_quota_linux: Inject NewCGroups*ForCurrentProcess

512703b

Turn the NewCGroupsForCurrentProcess and NewCGroups2ForCurrentProcess functions into function references so that we can replace them from tests.

abhinav added 2 commits April 4, 2022 23:37

Stub out NewCGroups* and test newQueryer

13d648d

isCGroupV2: test with NewCgroups2 constructor

0375f50

abhinav approved these changes Apr 4, 2022

View reviewed changes

mway approved these changes Apr 5, 2022

View reviewed changes

internal/cgroups/cgroups2.go Outdated Show resolved Hide resolved

internal/cgroups/cgroups2.go Outdated Show resolved Hide resolved

mway added 2 commits April 5, 2022 10:16

misc formatting

556f5ad

misc formatting

2bab625

mway reviewed Apr 5, 2022

View reviewed changes

internal/cgroups/cgroups2.go Show resolved Hide resolved

misc formatting

5076afc

mway reviewed Apr 5, 2022

View reviewed changes

internal/cgroups/cgroups2.go Outdated Show resolved Hide resolved

internal/cgroups/cgroups2.go Outdated Show resolved Hide resolved

mway and others added 3 commits April 5, 2022 10:32

fix format

72542df

fix format

9ffafbe

fix format

57aa42c

abhinav merged commit ad6f05b into uber-go:master Apr 5, 2022

dprotaso mentioned this pull request Apr 6, 2022

v1.5.0 I am still getting that the CPU Quota is not defined. #49

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support cgroup2 #44

Support cgroup2 #44

emadolsky commented Jan 4, 2022

CLAassistant commented Jan 4, 2022 •

edited

Loading

ardan-bkennedy commented Jan 27, 2022

alxn commented Jan 27, 2022

akshayjshah commented Jan 27, 2022

brettbuddin commented Feb 9, 2022 •

edited

Loading

ardan-bkennedy commented Feb 9, 2022

dprotaso commented Mar 14, 2022

dprotaso commented Mar 14, 2022

akshayjshah left a comment •

edited

Loading

maciej Mar 28, 2022

codecov bot commented Mar 29, 2022 •

edited

Loading

abhinav left a comment

abhinav left a comment

mway left a comment

ardan-bkennedy commented Apr 5, 2022

ardan-bkennedy commented Apr 5, 2022 •

edited

Loading

emadolsky commented Apr 6, 2022

bsergean commented Feb 22, 2024

Support cgroup2 #44

Support cgroup2 #44

Conversation

emadolsky commented Jan 4, 2022

CLAassistant commented Jan 4, 2022 • edited Loading

ardan-bkennedy commented Jan 27, 2022

alxn commented Jan 27, 2022

akshayjshah commented Jan 27, 2022

brettbuddin commented Feb 9, 2022 • edited Loading

ardan-bkennedy commented Feb 9, 2022

dprotaso commented Mar 14, 2022

dprotaso commented Mar 14, 2022

akshayjshah left a comment • edited Loading

Choose a reason for hiding this comment

maciej Mar 28, 2022

Choose a reason for hiding this comment

codecov bot commented Mar 29, 2022 • edited Loading

Codecov Report

abhinav left a comment

Choose a reason for hiding this comment

abhinav left a comment

Choose a reason for hiding this comment

mway left a comment

Choose a reason for hiding this comment

ardan-bkennedy commented Apr 5, 2022

ardan-bkennedy commented Apr 5, 2022 • edited Loading

emadolsky commented Apr 6, 2022

bsergean commented Feb 22, 2024

CLAassistant commented Jan 4, 2022 •

edited

Loading

brettbuddin commented Feb 9, 2022 •

edited

Loading

akshayjshah left a comment •

edited

Loading

codecov bot commented Mar 29, 2022 •

edited

Loading

ardan-bkennedy commented Apr 5, 2022 •

edited

Loading