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

feat(memlimit): replace containerd/cgroup with own cgroup implementation #24

Merged
merged 4 commits into from
Dec 8, 2024

Conversation

KimMachineGun
Copy link
Owner

#21

@KimMachineGun KimMachineGun force-pushed the cgroup branch 7 times, most recently from 60d49bb to ae07440 Compare December 8, 2024 10:09
@KimMachineGun KimMachineGun marked this pull request as ready for review December 8, 2024 10:15
@KimMachineGun KimMachineGun requested a review from Copilot December 8, 2024 10:16
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 10 out of 18 changed files in this pull request and generated 2 suggestions.

Files not reviewed (8)
  • examples/dynamic/go.mod: Language not supported
  • examples/gosigar/go.mod: Language not supported
  • examples/logger/go.mod: Language not supported
  • examples/system/go.mod: Language not supported
  • go.mod: Language not supported
  • memlimit/memlimit_common_test.go: Evaluated as low risk
  • memlimit/cgroups_linux.go: Evaluated as low risk
  • memlimit/cgroups_test.go: Evaluated as low risk
Comments skipped due to low confidence (6)

memlimit/cgroups_linux_test.go:35

  • The error message should refer to FromCgroupHybrid instead of FromCgroup.
t.Fatalf("FromCgroup() error = %v, wantErr %v", err, ErrNoCgroup)

memlimit/cgroups.go:269

  • The error message 'invalid separator' is not descriptive enough. Specify that the separator '-' is missing or misplaced.
return mountInfo{}, fmt.Errorf("invalid separator")

memlimit/cgroups.go:359

  • Specify the expected number of fields in the error message for clarity.
return cgroupHierarchy{}, fmt.Errorf("not enough fields: %v", fields)

memlimit/memlimit_test.go:1

  • The removal of the TestMain function might cause issues with the setup of the expected, expectedSystem, and cgVersion variables. Ensure that the tests are still valid without this setup.
package memlimit

memlimit/memlimit_linux_test.go:15

  • [nitpick] Consider using an integer type for 'cgVersion' as it represents version numbers which are typically small integers.
cgVersion      uint64

memlimit/memlimit_linux_test.go:174

  • [nitpick] The error message for the 'SetGoMemLimitWithProvider' assertion could be more descriptive to indicate which test case failed.
t.Errorf("SetGoMemLimitWithProvider() error = %v, wantErr %v", err, tt.wantErr)

memlimit/cgroups.go Outdated Show resolved Hide resolved
memlimit/memlimit_linux_test.go Outdated Show resolved Hide resolved
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 10 out of 18 changed files in this pull request and generated 1 suggestion.

Files not reviewed (8)
  • examples/dynamic/go.mod: Language not supported
  • examples/gosigar/go.mod: Language not supported
  • examples/logger/go.mod: Language not supported
  • examples/system/go.mod: Language not supported
  • go.mod: Language not supported
  • memlimit/memlimit_common_test.go: Evaluated as low risk
  • .github/workflows/test.yml: Evaluated as low risk
  • memlimit/cgroups_linux.go: Evaluated as low risk
Comments skipped due to low confidence (6)

memlimit/cgroups.go:269

  • [nitpick] The error message 'invalid separator' is not descriptive enough. It should specify the expected separator and where the error occurred.
return mountInfo{}, fmt.Errorf("invalid separator")

memlimit/cgroups.go:408

  • [nitpick] The error message 'invalid cgroup path: %s is not under root %s' should specify that the relative path starts with '..', which is why it's invalid.
return "", fmt.Errorf("invalid cgroup path: %s is not under root %s", cgroupRelPath, root)

memlimit/cgroups.go:225

  • [nitpick] The error message 'too many fields for hierarchical_memory_limit' is vague. It should specify the expected format and the actual number of fields encountered.
return 0, fmt.Errorf("failed to parse memory.stats %q: too many fields for hierarchical_memory_limit", line)

memlimit/cgroups_linux_test.go:11

  • Add tests to cover scenarios where expected is 0 to ensure edge cases are handled.
if expected == 0 {

memlimit/cgroups_linux_test.go:47

  • Ensure that the cgVersion checks are explicit in the tests to verify the correct version is being tested.
if expected == 0 || cgVersion != 1 {

memlimit/cgroups_linux_test.go:60

  • Ensure that the cgVersion checks are explicit in the tests to verify the correct version is being tested.
if expected == 0 || cgVersion != 2 {

memlimit/memlimit_test.go Outdated Show resolved Hide resolved

Choose a reason for hiding this comment

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

Copilot reviewed 10 out of 18 changed files in this pull request and generated no suggestions.

Files not reviewed (8)
  • examples/dynamic/go.mod: Language not supported
  • examples/gosigar/go.mod: Language not supported
  • examples/logger/go.mod: Language not supported
  • examples/system/go.mod: Language not supported
  • go.mod: Language not supported
  • memlimit/memlimit_common_test.go: Evaluated as low risk
  • memlimit/cgroups_linux.go: Evaluated as low risk
  • .github/workflows/test.yml: Evaluated as low risk
Comments skipped due to low confidence (6)

memlimit/memlimit_linux_test.go:14

  • [nitpick] The variable 'cgVersion' should be renamed to 'cgroupVersion' for clarity.
var (

memlimit/memlimit_linux_test.go:14

  • [nitpick] The variable 'cgVersion' should be of type 'int' instead of 'uint64' to better represent the cgroup version (0, 1, or 2).
var (

memlimit/cgroups.go:25

  • The function name 'fromCgroup' should be 'fromCGroup' to follow Go naming conventions and be consistent with other function names.
func fromCgroup(versionDetector func(mis []mountInfo) (bool, bool)) (uint64, error) {

memlimit/cgroups.go:269

  • The error message 'invalid separator' should be more descriptive, indicating what was expected and what was found.
return mountInfo{}, fmt.Errorf("invalid separator")

memlimit/memlimit_test.go:102

  • [nitpick] The test case name 'Limit_0.9_math.MaxUint64' is ambiguous. It should be renamed to something more descriptive, like 'Limit_90PercentOfMaxUint64'.
name: "Limit_0.9_math.MaxUint64",

memlimit/memlimit_test.go:168

  • [nitpick] The error message 'failed to set GOMEMLIMIT: unknown error' could be improved for clarity. Consider changing it to 'failed to set Go memory limit: unknown error'.
wantErr: fmt.Errorf("failed to set GOMEMLIMIT: unknown error"),
@KimMachineGun KimMachineGun merged commit 0506e2d into main Dec 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant