Fix: cgroupsv2 never returns ErrCgroupDeleted#5739
Fix: cgroupsv2 never returns ErrCgroupDeleted#5739thaJeztah wants to merge 1 commit intocontainerd:mainfrom
Conversation
|
Build succeeded.
|
|
Build succeeded.
|
Actually, this is not correct, because it won't create the parent path, so we will fail in this case; https://play.golang.org/p/6noxWN225Dh package main
import (
"fmt"
"io/ioutil"
)
func main() {
err := ioutil.WriteFile("/no/such/path/myfile.txt", []byte("hello world"), 0755)
fmt.Println(err)
} |
|
So (if I see it correctly) Perhaps LoadManager should return Alternatively, we modify |
|
If this is ready to be reviewed, can you rebase to get the tests passing? |
I noticed that this code is checking for `cgroupsv2.ErrCgroupDeleted`
However, `cgroupsv2.LoadManager()` will never return this error type, rendering
the `cgroupsv2.NewManager()` code to be unused.
Even _if_ `cgroupsv2.` would be used, calling `NewManager()` with a `nil` resources
would produce an error ("resources reference is nil");
https://github.com/containerd/cgroups/blob/v1.0.1/v2/manager.go#L169-L172
func NewManager(mountpoint string, group string, resources *Resources) (*Manager, error) {
if resources == nil {
return nil, errors.New("resources reference is nil")
}
This patch removes checks for `cgroupsv2.ErrCgroupDeleted`, as it is not used
in cgroups v2. As outlined above, this change should not affect the current
behavior.
Question remains: *should* containerd create a new manager? In the current
codebase, `cgroupsv2.NewManager()` is not used, which means that the cgroup
path is never (explicitly) created, and controllers are never enabled (by
containerd); https://github.com/containerd/cgroups/blob/v1.0.1/v2/manager.go#L176-L192
Cgroup paths *will* be created implicitly, as the code continues with `cg.AddProc()`,
which:
- calls `writeValues()` https://github.com/containerd/cgroups/blob/v1.0.1/v2/manager.go#L319
- calls `Value.write()` https://github.com/containerd/cgroups/blob/v1.0.1/v2/manager.go#L162
- calls `ioutil.Writefile()` https://github.com/containerd/cgroups/blob/v1.0.1/v2/manager.go#L147.
which creates files when missing: https://pkg.go.dev/io/ioutil#WriteFile
> WriteFile writes data to a file named by filename. If the file does not exist,
> WriteFile creates it with permissions perm (before umask) (...)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
cgroupv2_deleted_detection
branch
from
3f3dbd6 to
031c749
Compare
|
Build succeeded.
|
|
Rebased. Yes, I think this should be ok to review; at least it changes the code to match the actual situation (and We'd still need to look at if it actually should return a specific error (and thus, if the code that's removed here should be performed in this situation); that requires a follow-up in any case (with changes in containerd/cgroups) Perhaps @AkihiroSuda could provide input on that (he's more familiar with the cgroups v2) |
|
This was included in 18d483b, so no longer needed |
This commit brings the resource leak fix below. containerd/cgroups#212 - The upgrade PR in main was containerd#6498. I didn't cherry-pick the commit due to conflicts. - ErrCgroupDeleted check is removed since LoadManager won't return the error (see containerd#5739) and the variable was removed in 1.0.3. Signed-off-by: Kazuyoshi Kato <[email protected]>
This commit brings the resource leak fix below. containerd/cgroups#212 - The original upgrade PR was containerd#6498. I didn't cherry-pick the commit due to conflicts. - ErrCgroupDeleted check is removed since LoadManager won't return the error (see containerd#5739) and the variable was removed in 1.0.3. Signed-off-by: Kazuyoshi Kato <[email protected]>
2 participants
I noticed that this code is checking for
cgroupsv2.ErrCgroupDeletedHowever,
cgroupsv2.LoadManager()will never return this error type, renderingthe
cgroupsv2.NewManager()code to be unused.Even if
cgroupsv2.would be used, callingNewManager()with anilresourceswould produce an error ("resources reference is nil");
https://github.com/containerd/cgroups/blob/v1.0.1/v2/manager.go#L169-L172
This patch removes checks for
cgroupsv2.ErrCgroupDeleted, as it is not usedin cgroups v2. As outlined above, this change should not affect the current
behavior.
Question remains: should containerd create a new manager? In the current
codebase,
cgroupsv2.NewManager()is not used, which means that the cgrouppath is never (explicitly) created, and controllers are never enabled (by
containerd); https://github.com/containerd/cgroups/blob/v1.0.1/v2/manager.go#L176-L192
Cgroup paths will be created implicitly, as the code continues with
cg.AddProc(),which:
calls
writeValues()https://github.com/containerd/cgroups/blob/v1.0.1/v2/manager.go#L319calls
Value.write()https://github.com/containerd/cgroups/blob/v1.0.1/v2/manager.go#L162calls
ioutil.Writefile()https://github.com/containerd/cgroups/blob/v1.0.1/v2/manager.go#L147.which creates files when missing: https://pkg.go.dev/io/ioutil#WriteFile