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

Simplify cgroup paths handling in v2 via unified v1/v2 API #2386

Merged
merged 1 commit into from
May 12, 2020

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented May 7, 2020

This unties the Gordian Knot of using GetPaths in cgroupv2 code.

The problem is, the current code uses GetPaths for three kinds of things:

  1. Get all the paths to cgroup v1 controllers to save its state (see
    (*linuxContainer).currentState(), (*LinuxFactory).loadState()
    methods).

  2. Get all the paths to cgroup v1 controllers to have the setns process
    enter the proper cgroups in (*setnsProcess).start().

  3. Get the path to a specific controller (for example,
    m.GetPaths()["devices"]).

Now, for cgroup v2 instead of a set of per-controller paths, we have only
one single unified path, and a dedicated function GetUnifiedPath() to get it.

This discrepancy between v1 and v2 cgroupManager API leads to the
following problems with the code:

  • multiple if/else code blocks that have to treat v1 and v2 separately;

  • backward-compatible GetPaths() methods in v2 controllers;

  • repeated writing of the PID into the same cgroup for v2;

Overall, it's hard to write the right code with all this, and the code
that is written is kinda hard to follow.

The solution is to slightly change the API to do the 3 things outlined
above in the same manner for v1 and v2:

  1. Use GetPaths() for state saving and setns process cgroups entering.

  2. Introduce and use Path(subsys string) to obtain a path to a
    subsystem. For v2, the argument is ignored and the unified path is
    returned.

This commit converts all the controllers to the new API, and modifies
all the users to use it.

@kolyshkin
Copy link
Contributor Author

kolyshkin commented May 7, 2020

Personally, I am very happy with the result. The only ugliness that is left is

  1. function func getUnifiedPath() in libcontainer/factory_linux.go. It can be simplified but we'll lose backward compatibility (not sure if we need it so I'm not touching it).
  2. a requirement to pass an (unused) string parameter to Path() for v2. I think it's an OK price to pay for the API which works for v1 and v2, otherwise we'd need an if/else and it will be uglier.
  3. A bunch of functions in libcontainer/factory_linux.go to init cgroup manager. I don't know what to do about it, but it's not a big problem.
  4. Public members of fs.Manager and systemd.LegacyManager. Will address it later. addressed

@kolyshkin
Copy link
Contributor Author

@AkihiroSuda PTAL

@AkihiroSuda
Copy link
Member

AkihiroSuda commented May 8, 2020

LGTM

Approved with PullApprove

@kolyshkin
Copy link
Contributor Author

I think I'll split it into two PRs for the sake of easier review.

Part I: #2387 (relatively easy to review)
Part II is this one.

@kolyshkin kolyshkin changed the title Simplify cgroup paths handling in v2, unify API Simplify cgroup paths handling in v2 via unified v1/v2 API May 8, 2020
@kolyshkin kolyshkin force-pushed the gordian-knot branch 3 times, most recently from 13f45c7 to e2c593b Compare May 8, 2020 17:20
This unties the Gordian Knot of using GetPaths in cgroupv2 code.

The problem is, the current code uses GetPaths for three kinds of things:

1. Get all the paths to cgroup v1 controllers to save its state (see
   (*linuxContainer).currentState(), (*LinuxFactory).loadState()
   methods).

2. Get all the paths to cgroup v1 controllers to have the setns process
    enter the proper cgroups in `(*setnsProcess).start()`.

3. Get the path to a specific controller (for example,
   `m.GetPaths()["devices"]`).

Now, for cgroup v2 instead of a set of per-controller paths, we have only
one single unified path, and a dedicated function `GetUnifiedPath()` to get it.

This discrepancy between v1 and v2 cgroupManager API leads to the
following problems with the code:

 - multiple if/else code blocks that have to treat v1 and v2 separately;

 - backward-compatible GetPaths() methods in v2 controllers;

 -  - repeated writing of the PID into the same cgroup for v2;

Overall, it's hard to write the right code with all this, and the code
that is written is kinda hard to follow.

The solution is to slightly change the API to do the 3 things outlined
above in the same manner for v1 and v2:

1. Use `GetPaths()` for state saving and setns process cgroups entering.

2. Introduce and use Path(subsys string) to obtain a path to a
   subsystem. For v2, the argument is ignored and the unified path is
   returned.

This commit converts all the controllers to the new API, and modifies
all the users to use it.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

@AkihiroSuda @mrunalp @cyphar PTAL

func (m *manager) GetPaths() map[string]string {
m.mu.Lock()
defer m.mu.Unlock()
return m.paths
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if the locking here is needed. Might help in the case when another goroutine is populating the paths in Apply(), but practically I don't think this is a possibility. Nevertheless, nothing wrong to have it here.


func (m *legacyManager) GetPaths() map[string]string {
m.mu.Lock()
defer m.mu.Unlock()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto wrt locking

@AkihiroSuda
Copy link
Member

AkihiroSuda commented May 9, 2020

LGTM

Approved with PullApprove

@kolyshkin
Copy link
Contributor Author

@mrunalp @cyphar PTAL

@mrunalp
Copy link
Contributor

mrunalp commented May 12, 2020

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit 867c9f5 into opencontainers:master May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants