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

libct/devices: move config to libct/cg/devices/config #4577

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jan 4, 2025

This is the next step to facilitate separation of libct/cgroups as per https://github.com/opencontainers/tob/blob/main/proposals/cgroups.md

Currently, libcontainer/devices contains two things:

  1. Device-related configuration data structures and accompanying
    methods. Those are used by runc itself, mostly by libct/cgroups.

  2. A few functions (HostDevices, DeviceFromPath, GetDevices).
    Those are not used by runc directly, but have some external users
    (cri-o, microsoft/hcsshim), and they also have a few forks
    (containerd/pkg/oci, podman/pkg/util).

This commit moves (1) to a new separate package, config (under
libcontainer/cgroups/devices), adding a backward-compatible aliases
(marked as deprecated so we will be able to remove those later).

(As for (2), I think something like moby/sys#181 should be done)

Currently, libcontainer/devices contains two things:

1. Device-related configuration data structures and accompanying
   methods. Those are used by runc itself, mostly by libct/cgroups.

2. A few functions (HostDevices, DeviceFromPath, GetDevices).
   Those are not used by runc directly, but have some external users
   (cri-o, microsoft/hcsshim), and they also have a few forks
   (containerd/pkg/oci, podman/pkg/util).

This commit moves (1) to a new separate package, config (under
libcontainer/cgroups/devices), adding a backward-compatible aliases
(marked as deprecated so we will be able to remove those later).

Alas it's not possible to move this to libcontainer/cgroups directly
because some IDs (Type, Rule, Permissions) are too generic, and renaming
them (to DeviceType, DeviceRule, DevicePermissions) will break backward
compatibility (mostly due to Rule being embedded into Device).

Signed-off-by: Kir Kolyshkin <[email protected]>
Use the old package name as an alias to minimize the patch.

No functional change; this just eliminates a bunch of deprecation
warnings.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

So,

  1. this CAN NOT be moved to libcontainer/cgroups (like we did with the rest of cgroups-related configs in libct/configs: move cgroup configs to libct/cgroups #4472) because some IDs (Type, Rule, Permissions) are too generic, and renaming them (to DeviceType, DeviceRule, DevicePermissions) will break backward compatibility (mostly due to Rulebeing embedded intoDevice`);
  2. this CAN NOT be moved to libcontainer/cgroups/devices as it is a special package (importing it enables cgroup managers to manage devices (see https://github.com/opencontainers/runc/blob/main/libcontainer/cgroups/devices/devices.go) and makes binaries larger due to more dependencies like cilium/ebpf);
  3. this CAN be moved to any other new package under libct/cgroups, e.g. libcontainer/cgroups/devconfig or something like this) -- please let me know it it's better than devices/config;
  4. this COULD HAVE BEEN moved to libcontainer/cgroups/configs but in libct/configs: move cgroup configs to libct/cgroups #4472 we've decided to go without this package (although I should have thought about devices at that time, and I missed it).

So, it's either the way it is now in this PR, or we can break the backward compatibility and go with 1, or we go with 3.

WDYT @thaJeztah ?

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