v2: Fix inotify fd leak when cgroup is deleted

[jepio]

This resolves the leak of inotify fd by additionally subscribing to cgroup.events notifications, so that we can handle container exits and cgroup deletion. This is best tested in the context of k8s, the same manifest as containerd/containerd#6323 can be used - increase the replica count and watch the inotify instances get exhausted.

See issue: containerd/containerd#5670

[jepio]

Hi @AkihiroSuda, can this be merged? Does this repository need to be tagged so that it can be updated in containerd/containerd?

[AkihiroSuda]

Hi @AkihiroSuda, can this be merged?

Needs another LGTM from the committers.

Does this repository need to be tagged so that it can be updated in containerd/containerd?

Yes, we will probably create a tag right after merging this.

[ktock]

LGTM

[fuweid]

LGTM

[invidian]

Should there be some tests for this behavior change so we make sure it does not regress?

[jbehling]

Any word on when we can get this merged? This is blocking us from rolling out cgroups v2

[jepio]

@invidian you're welcome to add tests, I'm currently working on other things and don't have the time.

Is there any one else that wants to review this? I was hoping to get this into containerd 1.6.0 - containerd/containerd#5670 is marked as a priority 1 bug.

[invidian]

@invidian you're welcome to add tests, I'm currently working on other things and don't have the time.

I can have a look, but no promises and I guess it shouldn't be blocking this PR.

[invidian]

Quick draft of test which could be written, though it relies on go.uber.org/goleak for detecting leaking goroutines, maybe that could be improved. This currently fails on main branch , but passes on this PR. But this PR modifies much more so perhaps more tests should be added.

While reading the code and writing tests for it I also spotted some potentially breaking changes, so mentioned them in the comments.

package v2

import (
  "fmt"
  "io/fs"
  "os"
  "path/filepath"
  "strings"
  "testing"
  "time"

  "go.uber.org/goleak"
)

func Test_EventWatch_stops_watching_memory_events_when_cgroup_is_removed(t *testing.T) {
  skipWhenNotRunAsRoot(t)
  checkCgroupMode(t)

  group := "/cpu-test-cg"
  groupPath := fmt.Sprintf("%s-%d", group, os.Getpid())

  c, err := NewManager(defaultCgroup2Path, groupPath, &Resources{})
  if err != nil {
    t.Fatal("failed to init new cgroup manager: ", err)
  }

  evCh, errCh := c.EventChan()

  data := `low 0
high 0
max 0
oom 0
oom_kill 0
`

  ticker := time.NewTicker(100 * time.Millisecond)
  done := make(chan bool)

  go func() {
    for {
      select {
      case <-done:
        return
      case <-ticker.C:
        err := os.WriteFile(filepath.Join(defaultCgroup2Path, groupPath, "memory.events"), []byte(data), 0o444)
        if !strings.HasSuffix(err.Error(), fs.ErrInvalid.Error()) {
          t.Logf("Unexpected error writing event to cgroup: %v", err)
          t.Fail()
        }
      }
    }
  }()

  select {
  case ev := <-evCh:
    t.Logf("event received: %v", ev)
  case err := <-errCh:
    if err != nil {
      t.Fatalf("Received error event: %v", err)
    }
  }

  t.Cleanup(func() {
    if err := os.Remove(filepath.Join(defaultCgroup2Path, groupPath)); err != nil {
      t.Fatalf("Removing cgroup path failed: %v", err)
    }

    ticker.Stop()
    done <- true

    goleak.VerifyNone(t)
  })
}

[invidian]

This change is unrelated I think and could be done at least in a separate commit. Tests shouldn't be hard either, I think you just need to pass it an empty string (so it has NULL as first byte).

[jepio]

I'm touching the if condition for this branch and harmonizing the error handling with the other InotifyAddWatch so this change is related. It's not worth breaking out into a separate commit.

[invidian]

Thanks for addressing my comments @jepio!

[dmcgowan]

The vendor directory got pulled into one of the commits.

[jepio]

@dmcgowan got rid of the vendor directory again, I believe that should cover everything.

[invidian]

LGTM

[dcantah]

LGTM, minor nitpick

[estesp]

LGTM