Add cgroups v2 support to resourcemanager

[Tristan-Wilson]

If the --node.resource-mgmt.mem-limit-percent command line option is set, now Nitro will auto-detect whether memory stats can be checked using cgroups v2 or v1, instead of only supporting v1.

Testing done

Same testing was done as in #1738 after resetting my system to use cgroups v2:

sudo grubby --update-kernel=ALL --remove-args="systemd.unified_cgroup_hierarchy"

Additionally confirmed that cgroups v2 was correctly detected by checking log messages:

sequencer_1                    | INFO [07-07|23:46:49.218] Cgroups v2 detected, enabling memory limit RPC throttling 

[anodar]

nit:

_, err = ....
return err

[Tristan-Wilson]

Fixed

[anodar]

nit: inline. Same below

if _, err := ... ; err == nil {
}

[Tristan-Wilson]

Done.

[anodar]

When errors are not nil, there's probably no use of continuing the test, so this should be Fatal instead.

Same below/up.

[Tristan-Wilson]

Thanks, I had used the wrong one by mistake.

[anodar]

This test basically has duplicate code for 3 subtests. You can just describe each as a test-case and de-duplicate the code. (Sorry overlooked on the first review).

https://go.dev/blog/subtests

[Tristan-Wilson]

Fixed.

[anodar]

As I've mentioned in original review where this was introduced, please return concrete type here, as returning interfaces are not idiomatic in Go. You can change this function to:

func newLimitChecker(conf *Config) (cgroupsMemoryLimitChecker, error) {
       if c := newCgroupsMemoryLimitChecker(cgroupsV1MemoryFiles, conf.MemoryLimitPercent); isSupported(c) {
		return c, nil
	}
      if c := newCgroupsMemoryLimitChecker(cgroupsV2MemoryFiles, conf.MemoryLimitPercent); isSupported(c) {
            return c, nil
      }
      return nil, errNotSupported
}

Then in the call site you can call this, check with error.Is(err, errNotSupported) to instantiate trivialLimitChecker{} when needed.

[Tristan-Wilson]

Refactored it as suggested.

[anodar]

func TestCgroupsv1MemoryLimit(t *testing.T) {

  for _, tc := range []struct {
        desc string
        inactive int
        want bool
} {
    {  
       desc: "limit should be exceeded", 
      inactive: 51,
      want: true,

    },
    {  
       desc: "limit should not be exceeded", 
      inactive: 51,
      want: false,

    },
} {
   t.Run(tc.desc, func(t *testing.T) {
         dir := makeCgroupsTestDir(t.TempDir())
         if err := updateFakeCgroupFiles(c, 1000, 1000, tc.inactive); err != nil {
      		t.Fatal("Updating cgroup files: %v", err)
         }
         exceeded, err := c.isLimitExceeded()
         if err != nil {
            t.Fatalf("Checking if limit exceeded: %v", err)
         }
         if exceeded != tc.want {
            t.Errorf("isLimitExceeded() = %t, want %t", exceeded, tc.want)
         }
	}
   })
}
}

You can either also have TestCgroupsFailIfCantOpen as another case in the test, or leave as is.

[anodar]

You can see examples of table-driven tests at: https://github.com/OffchainLabs/nitro/blob/ac0a2de65e86b008ff10ded1f7b003ca01737315/arbnode/dataposter/storage_test.go

[Tristan-Wilson]

Thanks, I'll use this pattern more often. Took your code largely as suggested.

[anodar]

Doesn't matter too much in this case, but once you change this to being table-driven tests, this should be Errorf.

https://google.github.io/styleguide/go/decisions#keep-going

[Tristan-Wilson]

Thanks, that's interesting to know. I had always followed the opposite pattern (fail fast), but I'll try to use this where it makes sense.

[anodar]

Thanks for addressing the comments.

[anodar]

nit: any reason why not just c,err := ... ; and drop declaration of those variables (two lines above this one) ?

[Tristan-Wilson]

I wanted to have c be of the limitChecker interface type so I can assign the trivialLimitChecker to it. I moved the declaration err into the same line as the limit checker creation.

[anodar]

nit: go tends to encourage shorter names. You can drop "IfSupported" suffix and just add in the comment above the function that this returns errNotSupported if it's not supported.

[Tristan-Wilson]

I'm not sure what to rename it to since newCgroupsMemoryLimitChecker is already the factory method for the CgroupsMemoryLimitChecker and this version handles auto-detecting cgroups v1 or v2.

[anodar]

nit: same here, you don't need to define err before, you can just do

if _, err := ... ; err == nil {
}

It's more concise and limits the scope of err variable (less error prone in general that way for consequent error checks)

[Tristan-Wilson]

Fixed.