Merge pull request opencontainers#3373 from kolyshkin/less-interfaces

Remove Factory and LinuxFactory

Author: thaJeztah

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)

libcontainer/README.md

@@ -32,27 +32,15 @@ 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--")
 	}
 }
 ```
 
-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

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.

libcontainer/factory.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
 	}

libcontainer/factory_linux.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)
 	}

libcontainer/factory_linux_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()

libcontainer/integration/checkpoint_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)
 	}
 }