Repository: opencontainers/runc
Pull request #3373 (2022-02-11 04:07:17)

Remove Factory and LinuxFactory
~~_Based on and currently includes #3374. Draft until that one is merged._~~

Continuing the spring cleaning started in #3354. This dismantles Factory and LinuxFactory, doing some cleanups along the way.

Loosely based on parts of #3316.

kolyshkin: No longer a draft; PTAL @opencontainers/runc-maintainers @thaJeztah @AkihiroSuda 
kolyshkin: PTAL @opencontainers/runc-maintainers @thaJeztah @AkihiroSuda (I have a few more draft PRs on top of this).
kolyshkin: This is mostly code removal, and is organized to ease the review as much as possible.
kolyshkin: Anything I can do to move this forward, @opencontainers/runc-maintainers? I have a few more PRs based on top of this.
AkihiroSuda: :+1: but godoc might not be correct

Patch:
diff --git a/init.go b/init.go
index bddc237f..2f44ce8a 100644
--- a/init.go
+++ b/init.go
@@ -32,8 +32,7 @@ func init() {
 		logrus.SetFormatter(new(logrus.JSONFormatter))
 		logrus.Debug("child process in init()")
 
-		factory, _ := libcontainer.New("")
-		if err := factory.StartInitialization(); err != nil {
+		if err := libcontainer.StartInitialization(); err != nil {
 			// as the error is sent back to the parent there is no need to log
 			// or write it to stderr because the parent process will handle this
 			os.Exit(1)
diff --git a/libcontainer/README.md b/libcontainer/README.md
index 2850db2a..addd6a95 100644
--- a/libcontainer/README.md
+++ b/libcontainer/README.md
@@ -32,8 +32,7 @@ func init() {
 	if len(os.Args) > 1 && os.Args[1] == "init" {
 		runtime.GOMAXPROCS(1)
 		runtime.LockOSThread()
-		factory, _ := libcontainer.New("")
-		if err := factory.StartInitialization(); err != nil {
+		if err := libcontainer.StartInitialization(); err != nil {
 			logrus.Fatal(err)
 		}
 		panic("--this line should have never been executed, congratulations--")
@@ -41,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
@@ -243,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 5df120d0..a7bf155c 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.go b/libcontainer/factory.go
deleted file mode 100644
index 9f9e8fc5..00000000
--- a/libcontainer/factory.go
+++ /dev/null
@@ -1,30 +0,0 @@
-package libcontainer
-
-import (
-	"github.com/opencontainers/runc/libcontainer/configs"
-)
-
-type Factory interface {
-	// Creates a new container with the given id and starts the initial process inside it.
-	// id must be a string containing only letters, digits and underscores and must contain
-	// between 1 and 1024 characters, inclusive.
-	//
-	// The id must not already be in use by an existing container. Containers created using
-	// a factory with the same path (and filesystem) must have distinct ids.
-	//
-	// Returns the new container with a running process.
-	//
-	// On error, any partially created container parts are cleaned up (the operation is atomic).
-	Create(id string, config *configs.Config) (Container, error)
-
-	// 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.
-	Load(id string) (Container, error)
-
-	// StartInitialization is an internal API to libcontainer used during the reexec of the
-	// container.
-	StartInitialization() error
-
-	// Type returns info string about factory type (e.g. lxc, libcontainer...)
-	Type() string
-}
diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go
index a0ea4fe3..9d4bc4cf 100644
--- a/libcontainer/factory_linux.go
+++ b/libcontainer/factory_linux.go
@@ -10,7 +10,6 @@ import (
 	"strconv"
 
 	securejoin "github.com/cyphar/filepath-securejoin"
-	"github.com/moby/sys/mountinfo"
 	"golang.org/x/sys/unix"
 
 	"github.com/opencontainers/runc/libcontainer/cgroups/manager"
@@ -28,60 +27,30 @@ const (
 
 var idRegex = regexp.MustCompile(`^[\w+-\.]+$`)
 
-// TmpfsRoot is an option func to mount LinuxFactory.Root to tmpfs.
-func TmpfsRoot(l *LinuxFactory) error {
-	mounted, err := mountinfo.Mounted(l.Root)
-	if err != nil {
-		return err
-	}
-	if !mounted {
-		if err := mount("tmpfs", l.Root, "", "tmpfs", 0, ""); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-// New returns a linux based container factory based in the root directory and
-// configures the factory with the provided option funcs.
-func New(root string, options ...func(*LinuxFactory) error) (Factory, error) {
-	if root != "" {
-		if err := os.MkdirAll(root, 0o700); err != nil {
-			return nil, err
-		}
-	}
-	l := &LinuxFactory{
-		Root: root,
-	}
-
-	for _, opt := range options {
-		if opt == nil {
-			continue
-		}
-		if err := opt(l); err != nil {
-			return nil, err
-		}
-	}
-	return l, nil
-}
-
-// LinuxFactory implements the default factory interface for linux based systems.
-type LinuxFactory struct {
-	// Root directory for the factory to store state.
-	Root string
-}
-
-func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, error) {
-	if l.Root == "" {
+// Create creates a new container with the given id inside a given state
+// directory (root), and returns a Container object.
+//
+// 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 given root path.
+func Create(root, id string, config *configs.Config) (Container, error) {
+	if root == "" {
 		return nil, errors.New("root not set")
 	}
-	if err := l.validateID(id); err != nil {
+	if err := validateID(id); err != nil {
 		return nil, 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
 	}
@@ -125,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{
@@ -139,19 +109,22 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err
 	return c, nil
 }
 
-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 := l.validateID(id); err != nil {
+	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
 	}
-	state, err := l.loadState(containerRoot)
+	state, err := loadState(containerRoot)
 	if err != nil {
 		return nil, err
 	}
@@ -181,13 +154,10 @@ func (l *LinuxFactory) Load(id string) (Container, error) {
 	return c, nil
 }
 
-func (l *LinuxFactory) Type() string {
-	return "libcontainer"
-}
-
-// StartInitialization loads a container by opening the pipe fd from the parent to read the configuration and state
-// This is a low level implementation detail of the reexec and should not be consumed externally
-func (l *LinuxFactory) StartInitialization() (err error) {
+// StartInitialization loads a container by opening the pipe fd from the parent
+// to read the configuration and state. This is a low level implementation
+// detail of the reexec and should not be consumed externally.
+func StartInitialization() (err error) {
 	// Get the INITPIPE.
 	envInitPipe := os.Getenv("_LIBCONTAINER_INITPIPE")
 	pipefd, err := strconv.Atoi(envInitPipe)
@@ -269,7 +239,7 @@ func (l *LinuxFactory) StartInitialization() (err error) {
 	return i.Init()
 }
 
-func (l *LinuxFactory) loadState(root string) (*State, error) {
+func loadState(root string) (*State, error) {
 	stateFilePath, err := securejoin.SecureJoin(root, stateFilename)
 	if err != nil {
 		return nil, err
@@ -289,7 +259,7 @@ func (l *LinuxFactory) loadState(root string) (*State, error) {
 	return state, nil
 }
 
-func (l *LinuxFactory) validateID(id string) error {
+func validateID(id string) error {
 	if !idRegex.MatchString(id) || string(os.PathSeparator)+id != utils.CleanPath(string(os.PathSeparator)+id) {
 		return ErrInvalidID
 	}
diff --git a/libcontainer/restored_process.go b/libcontainer/restored_process.go
index cdffbd3a..2e81cdf6 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 80721e70..01e28271 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 7ec7b8a7..4124d9a7 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 {


Test patch:
diff --git a/libcontainer/factory_linux_test.go b/libcontainer/factory_linux_test.go
index 47f30699..c9e2b0e0 100644
--- a/libcontainer/factory_linux_test.go
+++ b/libcontainer/factory_linux_test.go
@@ -7,89 +7,14 @@ import (
 	"reflect"
 	"testing"
 
-	"github.com/moby/sys/mountinfo"
 	"github.com/opencontainers/runc/libcontainer/configs"
 	"github.com/opencontainers/runc/libcontainer/utils"
 	"github.com/opencontainers/runtime-spec/specs-go"
-
-	"golang.org/x/sys/unix"
 )
 
-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")
-	}
-	lfactory, ok := factory.(*LinuxFactory)
-	if !ok {
-		t.Fatal("expected linux factory returned on linux based systems")
-	}
-	if lfactory.Root != root {
-		t.Fatalf("expected factory root to be %q but received %q", root, lfactory.Root)
-	}
-
-	if factory.Type() != "libcontainer" {
-		t.Fatalf("unexpected factory type: %q, expected %q", factory.Type(), "libcontainer")
-	}
-}
-
-func TestFactoryNewTmpfs(t *testing.T) {
-	root := t.TempDir()
-	factory, err := New(root, TmpfsRoot)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if factory == nil {
-		t.Fatal("factory should not be nil")
-	}
-	lfactory, ok := factory.(*LinuxFactory)
-	if !ok {
-		t.Fatal("expected linux factory returned on linux based systems")
-	}
-	if lfactory.Root != root {
-		t.Fatalf("expected factory root to be %q but received %q", root, lfactory.Root)
-	}
-
-	if factory.Type() != "libcontainer" {
-		t.Fatalf("unexpected factory type: %q, expected %q", factory.Type(), "libcontainer")
-	}
-	mounted, err := mountinfo.Mounted(lfactory.Root)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if !mounted {
-		t.Fatalf("Factory Root is not mounted")
-	}
-	mounts, err := mountinfo.GetMounts(mountinfo.SingleEntryFilter(lfactory.Root))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(mounts) != 1 {
-		t.Fatalf("Factory Root is not listed in mounts list")
-	}
-	m := mounts[0]
-	if m.FSType != "tmpfs" {
-		t.Fatalf("FSType of root: %s, expected %s", m.FSType, "tmpfs")
-	}
-	if m.Source != "tmpfs" {
-		t.Fatalf("Source of root: %s, expected %s", m.Source, "tmpfs")
-	}
-	err = unix.Unmount(root, unix.MNT_DETACH)
-	if err != nil {
-		t.Error("failed to unmount root:", err)
-	}
-}
-
 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")
 	}
@@ -135,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 c86ec8a0..cc45134b 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/init_test.go b/libcontainer/integration/init_test.go
index effcde06..b6a3714d 100644
--- a/libcontainer/integration/init_test.go
+++ b/libcontainer/integration/init_test.go
@@ -19,11 +19,7 @@ func init() {
 	}
 	runtime.GOMAXPROCS(1)
 	runtime.LockOSThread()
-	factory, err := libcontainer.New("")
-	if err != nil {
-		logrus.Fatalf("unable to initialize for container: %s", err)
-	}
-	if err := factory.StartInitialization(); err != nil {
+	if err := libcontainer.StartInitialization(); err != nil {
 		logrus.Fatal(err)
 	}
 }
diff --git a/libcontainer/integration/utils_test.go b/libcontainer/integration/utils_test.go
index def29fc0..c54a40bb 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