From 3b60d38769d732604e7e8cae5e531e461d14f478 Mon Sep 17 00:00:00 2001
From: Matthew Heon <mheon@redhat.com>
Date: Wed, 25 Oct 2017 12:04:52 -0400
Subject: [PATCH] Address review comments

Signed-off-by: Matthew Heon <mheon@redhat.com>
---
 libpod/container.go | 13 ++++++++++---
 libpod/oci.go       |  2 +-
 libpod/storage.go   | 20 +++++++++-----------
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/libpod/container.go b/libpod/container.go
index 6b3e1484..0582fa6d 100644
--- a/libpod/container.go
+++ b/libpod/container.go
@@ -54,7 +54,7 @@ type Container struct {
 }
 
 // containerState contains the current state of the container
-// It is stored as on disk in a per-boot directory
+// It is stored on disk in a tmpfs and recreated on reboot
 type containerRuntimeInfo struct {
 	// The current state of the running container
 	State ContainerState `json:"state"`
@@ -75,12 +75,11 @@ type containerRuntimeInfo struct {
 	ExitCode int32 `json:"exitCode,omitempty"`
 
 	// TODO: Save information about image used in container if one is used
-	// TODO save start time, create time (create time in the containerConfig?)
 }
 
 // containerConfig contains all information that was used to create the
 // container. It may not be changed once created.
-// It is stored as an unchanging part of on-disk state
+// It is stored, read-only, on disk
 type containerConfig struct {
 	Spec *spec.Spec `json:"spec"`
 	ID   string     `json:"id"`
@@ -281,6 +280,8 @@ func (c *Container) Create() (err error) {
 		c.state.Mounted = true
 		c.state.Mountpoint = mountPoint
 
+		logrus.Debugf("Created root filesystem for container %s at %s", c.ID(), c.state.Mountpoint)
+
 		defer func() {
 			if err != nil {
 				if err2 := c.runtime.storageService.StopContainer(c.ID()); err2 != nil {
@@ -310,12 +311,16 @@ func (c *Container) Create() (err error) {
 	}
 	c.state.ConfigPath = jsonPath
 
+	logrus.Debugf("Created OCI spec for container %s at %s", c.ID(), jsonPath)
+
 	// With the spec complete, do an OCI create
 	// TODO set cgroup parent in a sane fashion
 	if err := c.runtime.ociRuntime.createContainer(c, "/libpod_parent"); err != nil {
 		return err
 	}
 
+	logrus.Debugf("Created container %s in runc", c.ID())
+
 	// TODO should flush this state to disk here
 	c.state.State = ContainerStateCreated
 
@@ -340,6 +345,8 @@ func (c *Container) Start() error {
 		return err
 	}
 
+	logrus.Debugf("Started container %s", c.ID())
+
 	// TODO should flush state to disk here
 	c.state.StartedTime = time.Now()
 	c.state.State = ContainerStateRunning
diff --git a/libpod/oci.go b/libpod/oci.go
index 15f46709..0ed1c1f6 100644
--- a/libpod/oci.go
+++ b/libpod/oci.go
@@ -119,7 +119,7 @@ func (r *OCIRuntime) createContainer(ctr *Container, cgroupParent string) error
 
 	childStartPipe, parentStartPipe, err := newPipe()
 	if err != nil {
-		return errors.Wrapf(err, "error creating socket pair")
+		return errors.Wrapf(err, "error creating socket pair for start pipe")
 	}
 
 	defer parentPipe.Close()
diff --git a/libpod/storage.go b/libpod/storage.go
index 678b73b3..09378fcf 100644
--- a/libpod/storage.go
+++ b/libpod/storage.go
@@ -22,12 +22,11 @@ func getStorageService(store storage.Store) (*storageService, error) {
 	return &storageService{store: store}, nil
 }
 
-// ContainerInfo wraps a subset of information about a container: its ID and
-// the locations of its nonvolatile and volatile per-container directories,
-// along with a copy of the configuration blob from the image that was used to
-// create the container, if the image had a configuration.
+// ContainerInfo wraps a subset of information about a container: the locations
+// of its nonvolatile and volatile per-container directories, along with a copy
+// of the configuration blob from the image that was used to create the
+// container, if the image had a configuration.
 type ContainerInfo struct {
-	ID     string
 	Dir    string
 	RunDir string
 	Config *v1.Image
@@ -35,11 +34,11 @@ type ContainerInfo struct {
 
 // RuntimeContainerMetadata is the structure that we encode as JSON and store
 // in the metadata field of storage.Container objects.  It is used for
-// specifying attributes of pod sandboxes and containers when they are being
-// created, and allows a container's MountLabel, and possibly other values, to
-// be modified in one read/write cycle via calls to
-// RuntimeServer.ContainerMetadata, RuntimeContainerMetadata.SetMountLabel,
-// and RuntimeServer.SetContainerMetadata.
+// specifying attributes containers when they are being created, and allows a
+// container's MountLabel, and possibly other values, to be modified in one
+// read/write cycle via calls to storageService.ContainerMetadata,
+// RuntimeContainerMetadata.SetMountLabel, and
+// storageService.SetContainerMetadata.
 type RuntimeContainerMetadata struct {
 	// The provided name and the ID of the image that was used to
 	// instantiate the container.
@@ -158,7 +157,6 @@ func (r *storageService) CreateContainerStorage(systemContext *types.SystemConte
 	logrus.Debugf("container %q has run directory %q", container.ID, containerRunDir)
 
 	return ContainerInfo{
-		ID:     container.ID, // not needed
 		Dir:    containerDir,
 		RunDir: containerRunDir,
 		Config: imageConfig,