From 6a3fe1618f5166e5c44f21714736049bac9c02cb Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 10 Feb 2022 19:53:38 -0800 Subject: [PATCH] libcontainer: remove LinuxFactory Since LinuxFactory has become the means to specify containers state top directory (aka --root), and is only used by two methods (Create and Load), it is easier to pass root to them directly. Modify all the users and the docs accordingly. While at it, fix Create and Load docs (those that were originally moved from the Factory interface docs). Signed-off-by: Kir Kolyshkin --- libcontainer/README.md | 18 ++----- libcontainer/cgroups/manager/new.go | 8 +-- libcontainer/factory_linux.go | 57 ++++++++------------- libcontainer/factory_linux_test.go | 27 ++-------- libcontainer/integration/checkpoint_test.go | 7 ++- libcontainer/integration/utils_test.go | 6 +-- libcontainer/restored_process.go | 4 +- list.go | 7 +-- utils_linux.go | 16 ++---- 9 files changed, 44 insertions(+), 106 deletions(-) diff --git a/libcontainer/README.md b/libcontainer/README.md index 4210eacc5..addd6a956 100644 --- a/libcontainer/README.md +++ b/libcontainer/README.md @@ -40,18 +40,7 @@ func init() { } ``` -Then to create a container you first have to initialize an instance of a factory -that will handle the creation and initialization for a container. - -```go -factory, err := libcontainer.New("/var/lib/container") -if err != nil { - logrus.Fatal(err) - return -} -``` - -Once you have an instance of the factory created we can create a configuration +Then to create a container you first have to create a configuration struct describing how the container is to be created. A sample would look similar to this: ```go @@ -242,10 +231,11 @@ config := &configs.Config{ } ``` -Once you have the configuration populated you can create a container: +Once you have the configuration populated you can create a container +with a specified ID under a specified state directory: ```go -container, err := factory.Create("container-id", config) +container, err := libcontainer.Create("/run/containers", "container-id", config) if err != nil { logrus.Fatal(err) return diff --git a/libcontainer/cgroups/manager/new.go b/libcontainer/cgroups/manager/new.go index 5df120d0f..a7bf155cf 100644 --- a/libcontainer/cgroups/manager/new.go +++ b/libcontainer/cgroups/manager/new.go @@ -55,10 +55,10 @@ func NewWithPaths(config *configs.Cgroup, paths map[string]string) (cgroups.Mana return fs.NewManager(config, paths) } -// getUnifiedPath is an implementation detail of libcontainer factory. -// Historically, it saves cgroup paths as per-subsystem path map (as returned -// by cm.GetPaths(""), but with v2 we only have one single unified path -// (with "" as a key). +// getUnifiedPath is an implementation detail of libcontainer. +// Historically, libcontainer.Create saves cgroup paths as per-subsystem path +// map (as returned by cm.GetPaths(""), but with v2 we only have one single +// unified path (with "" as a key). // // This function converts from that map to string (using "" as a key), // and also checks that the map itself is sane. diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index e9c8cb23a..9d4bc4cfe 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -27,35 +27,18 @@ const ( var idRegex = regexp.MustCompile(`^[\w+-\.]+$`) -// New returns a linux based container factory based in the root directory. -func New(root string) (*LinuxFactory, error) { - if err := os.MkdirAll(root, 0o700); err != nil { - return nil, err - } - return &LinuxFactory{ - Root: root, - }, nil -} - -// LinuxFactory implements the default factory interface for linux based systems. -type LinuxFactory struct { - // Root directory for the factory to store state. - Root string -} - -// Create creates a new container with the given id and starts the initial -// process inside it. +// Create creates a new container with the given id inside a given state +// directory (root), and returns a Container object. // -// The id must not be empty and consists of only the following characters: +// The root is a state directory which many containers can share. It can be +// used later to get the list of containers, or to get information about a +// particular container (see Load). +// +// The id must not be empty and consist of only the following characters: // ASCII letters, digits, underscore, plus, minus, period. The id must be -// unique and non-existent for the factory with same root path. -// -// Returns the new container with a running process. -// -// On error, any partially created container parts are cleaned up (the -// operation is atomic). -func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, error) { - if l.Root == "" { +// unique and non-existent for the given root path. +func Create(root, id string, config *configs.Config) (Container, error) { + if root == "" { return nil, errors.New("root not set") } if err := validateID(id); err != nil { @@ -64,7 +47,10 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err if err := validate.Validate(config); err != nil { return nil, err } - containerRoot, err := securejoin.SecureJoin(l.Root, id) + if err := os.MkdirAll(root, 0o700); err != nil { + return nil, err + } + containerRoot, err := securejoin.SecureJoin(root, id) if err != nil { return nil, err } @@ -108,7 +94,8 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err return nil, errors.New("container's cgroup unexpectedly frozen") } - if err := os.MkdirAll(containerRoot, 0o711); err != nil { + // Parent directory is already created above, so Mkdir is enough. + if err := os.Mkdir(containerRoot, 0o711); err != nil { return nil, err } c := &linuxContainer{ @@ -122,18 +109,18 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err return c, nil } -// Load takes an ID for an existing container and returns the container -// information from the state. This presents a read only view of the -// container. -func (l *LinuxFactory) Load(id string) (Container, error) { - if l.Root == "" { +// Load takes a path to the state directory (root) and an id of an existing +// container, and returns a Container object reconstructed from the saved +// state. This presents a read only view of the container. +func Load(root, id string) (Container, error) { + if root == "" { return nil, errors.New("root not set") } // when load, we need to check id is valid or not. if err := validateID(id); err != nil { return nil, err } - containerRoot, err := securejoin.SecureJoin(l.Root, id) + containerRoot, err := securejoin.SecureJoin(root, id) if err != nil { return nil, err } diff --git a/libcontainer/factory_linux_test.go b/libcontainer/factory_linux_test.go index b6512fdb9..c9e2b0e09 100644 --- a/libcontainer/factory_linux_test.go +++ b/libcontainer/factory_linux_test.go @@ -12,26 +12,9 @@ import ( "github.com/opencontainers/runtime-spec/specs-go" ) -func TestFactoryNew(t *testing.T) { - root := t.TempDir() - factory, err := New(root) - if err != nil { - t.Fatal(err) - } - if factory == nil { - t.Fatal("factory should not be nil") - } - if factory.Root != root { - t.Fatalf("expected factory root to be %q but received %q", root, factory.Root) - } -} - func TestFactoryLoadNotExists(t *testing.T) { - factory, err := New(t.TempDir()) - if err != nil { - t.Fatal(err) - } - _, err = factory.Load("nocontainer") + stateDir := t.TempDir() + _, err := Load(stateDir, "nocontainer") if err == nil { t.Fatal("expected nil error loading non-existing container") } @@ -77,11 +60,7 @@ func TestFactoryLoadContainer(t *testing.T) { if err := marshal(filepath.Join(root, id, stateFilename), expectedState); err != nil { t.Fatal(err) } - factory, err := New(root) - if err != nil { - t.Fatal(err) - } - container, err := factory.Load(id) + container, err := Load(root, id) if err != nil { t.Fatal(err) } diff --git a/libcontainer/integration/checkpoint_test.go b/libcontainer/integration/checkpoint_test.go index c86ec8a0c..cc45134b6 100644 --- a/libcontainer/integration/checkpoint_test.go +++ b/libcontainer/integration/checkpoint_test.go @@ -62,10 +62,9 @@ func testCheckpoint(t *testing.T, userns bool) { } config := newTemplateConfig(t, &tParam{userns: userns}) - factory, err := libcontainer.New(t.TempDir()) - ok(t, err) + stateDir := t.TempDir() - container, err := factory.Create("test", config) + container, err := libcontainer.Create(stateDir, "test", config) ok(t, err) defer destroyContainer(container) @@ -143,7 +142,7 @@ func testCheckpoint(t *testing.T, userns bool) { ok(t, err) // reload the container - container, err = factory.Load("test") + container, err = libcontainer.Load(stateDir, "test") ok(t, err) restoreStdinR, restoreStdinW, err := os.Pipe() diff --git a/libcontainer/integration/utils_test.go b/libcontainer/integration/utils_test.go index def29fc0c..c54a40bbe 100644 --- a/libcontainer/integration/utils_test.go +++ b/libcontainer/integration/utils_test.go @@ -169,11 +169,7 @@ func newContainer(t *testing.T, config *configs.Config) (libcontainer.Container, name := strings.ReplaceAll(t.Name(), "/", "_") + strconv.FormatInt(-int64(time.Now().Nanosecond()), 35) root := t.TempDir() - f, err := libcontainer.New(root) - if err != nil { - return nil, err - } - return f.Create(name, config) + return libcontainer.Create(root, name, config) } // runContainer runs the container with the specific config and arguments diff --git a/libcontainer/restored_process.go b/libcontainer/restored_process.go index cdffbd3af..2e81cdf68 100644 --- a/libcontainer/restored_process.go +++ b/libcontainer/restored_process.go @@ -79,8 +79,8 @@ func (p *restoredProcess) forwardChildLogs() chan error { } // nonChildProcess represents a process where the calling process is not -// the parent process. This process is created when a factory loads a container from -// a persisted state. +// the parent process. This process is created when Load loads a container +// from a persisted state. type nonChildProcess struct { processPid int processStartTime uint64 diff --git a/list.go b/list.go index 80721e70c..01e282714 100644 --- a/list.go +++ b/list.go @@ -121,11 +121,6 @@ func getContainers(context *cli.Context) ([]containerState, error) { // Report other errors, including non-existent custom --root. return nil, err } - factory, err := libcontainer.New(root) - if err != nil { - return nil, err - } - var s []containerState for _, item := range list { if !item.IsDir() { @@ -146,7 +141,7 @@ func getContainers(context *cli.Context) ([]containerState, error) { owner.Name = fmt.Sprintf("#%d", uid) } - container, err := factory.Load(item.Name()) + container, err := libcontainer.Load(root, item.Name()) if err != nil { fmt.Fprintf(os.Stderr, "load container %s: %v\n", item.Name(), err) continue diff --git a/utils_linux.go b/utils_linux.go index 7ec7b8a79..4124d9a7e 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -23,19 +23,15 @@ import ( var errEmptyID = errors.New("container id cannot be empty") -// getContainer returns the specified container instance by loading it from state -// with the default factory. +// getContainer returns the specified container instance by loading it from +// a state directory (root). func getContainer(context *cli.Context) (libcontainer.Container, error) { id := context.Args().First() if id == "" { return nil, errEmptyID } root := context.GlobalString("root") - factory, err := libcontainer.New(root) - if err != nil { - return nil, err - } - return factory.Load(id) + return libcontainer.Load(root, id) } func getDefaultImagePath() string { @@ -185,11 +181,7 @@ func createContainer(context *cli.Context, id string, spec *specs.Spec) (libcont } root := context.GlobalString("root") - factory, err := libcontainer.New(root) - if err != nil { - return nil, err - } - return factory.Create(id, config) + return libcontainer.Create(root, id, config) } type runner struct {