Update kpod inspect to use the new container state #106

umohnani8 · 2017-12-05T22:12:31Z

kpod inspect now uses the new libpod container state
and closely matches the output of docker inspect
some aspects of it are still WIP as the libpod container state
is still being worked on

Signed-off-by: umohnani8 umohnani@redhat.com

umohnani8 · 2017-12-05T22:15:46Z

@rhatdan @mheon @baude PTAL.
There is quite a bit of information that is not available yet, will add those later.
I have tried to match this as closely as possible to docker inspect.
If there are fields that we will definitely not be needing, let me know and I will take them out.
Also please double check that I am fetching the correct data for each field, and if I am overlooking any.
docker inspect does not omit empty fields, so I have done the same for kpod. What is our preference, omit or not?
Also I have no clue why ctrCreate in sql_internal_state.go is messed up with the spacing, will look into that later.

mheon · 2017-12-05T22:16:21Z

libpod/container.go

@@ -131,6 +132,8 @@ type ContainerConfig struct {
 	SharedNamespaceMap map[string]string `json:"sharedNamespaces"`
 	// Time container was created
 	CreatedTime time.Time `json:"createdTime"`
+	Volumes     []string  `json:"volumes"`


Can you parse this from the OCI runtime spec's Mounts field?

removed Volumes. fixed

mheon · 2017-12-05T22:18:06Z

libpod/container.go

+// A container FS is split into two parts.  The first is the top layer, a
+// mutable layer, and the rest is the RootFS: the set of immutable layers
+// that make up the image on which the container is based
+func (c *Container) RootFsSize() (int64, error) {


Do we need to use this outside of libpod? I think we should make this private, and let people get it via the inspect API

mheon · 2017-12-05T22:18:30Z

libpod/container.go

+}
+
+// RwSize Gets the size of the mutable top layer of the container
+func (c *Container) RwSize() (int64, error) {


Same as above - if possible, make private and allow access via stats API.

Or inspect API. Oops.

mheon · 2017-12-05T22:20:37Z

libpod/container_inspect.go

+	pidsLimit := getPidsInfo(spec)
+
+	args := config.Spec.Process.Args
+	path := args[0]


We don't do any validation on the spec, so someone could feed us a bad spec with args having 0 length. Can you do a check for that here, to make sure we don't segfault trying to read an empty array?

mheon · 2017-12-05T22:23:59Z

libpod/container_inspect.go

+		},
+		ImageID:         config.RootfsImageID,
+		ImageName:       config.RootfsImageName,
+		ResolvConfPath:  runtimeInfo.ConfigPath,                               // not sure


Can't get this yet, coming in networking patch - leave as "" for now

for the "" that are pending, can you add a TODO comment or perhaps PENDING.

mheon · 2017-12-05T22:25:06Z

libpod/container_inspect.go

+	"github.com/sirupsen/logrus"
+)
+
+func getContainerData(ctr *Container, size bool, driverData *driver.Data) (*ContainerData, error) {


can you make this func (ctr *Container) getContainerData(...) for consistency with the rest of our container functions?

mheon · 2017-12-05T22:25:52Z

libpod/container_inspect.go

+			Cmd:         config.Spec.Process.Args,
+		},
+		NetworkSettings: &NetworkSettings{
+			SandboxID: config.Pod,


Leave this blank for now, pods aren't really working yet.

mheon · 2017-12-05T22:28:24Z

libpod/container_inspect.go

+		AppArmorProfile: spec.Process.ApparmorProfile,
+		ExecIDs:         "", //can't get yet
+		HostConfig: &HostConfig{
+			Binds:                config.Volumes,


I really don't like storing this in Container. It doesn't directly affect any other part of the container, because the mounts are already in the spec. If possible, we should parse them out of the spec. Otherwise, we should either throw them in a label, or do something similar to the Artifacts functions we discussed on IRC.

Thinking about it more, we can't tell what the default mounts are, so even if it's not completely compliant with the Docker CLI we should just output all mounts in the spec.

If we already include the mounts (I see them below), I would probably just remove this and the container.config.Volumes field. From a libpod standpoint we don't know what is a user-created bind mount and what isn't - we just know about the mounts in the spec.

removed volumes and binds. fixed

mheon · 2017-12-05T22:33:57Z

libpod/image_inspect.go

+		Architecture:  ociv1Img.Architecture,
+		Os:            ociv1Img.OS,
+		Config:        &ociv1Img.Config,
+		DockerVersion: info.DockerVersion,


I think we should rename this, exact compatability be damned. @rhatdan agree?

I agree. We don't need to be outputing things that say Docker

rename it to what? OCIVersion?

Personally I'd just drop the 'Docker' part and leave it as Version:

but it should still get its data from info.DockerVersion?

I'd rename that too.

👍 Version works for me.

mheon · 2017-12-05T22:34:55Z

libpod/inspect_data.go

+	MountLabel      string           `json:"MountLabel"`
+	ProcessLabel    string           `json:"ProcessLabel"`
+	AppArmorProfile string           `json:"AppArmorProfile"`
+	ExecIDs         string           `json:"ExecIDs"` // don't know type yet, not implemented


Will be a []string

mheon · 2017-12-05T22:35:42Z

libpod/inspect_data.go

+	Config map[string]string `json:"Config"` //idk type, not implemented
+}
+
+// RestarPolicy holds the container's policy on restart


We aren't going to handle restart policy, so this can be removed

mheon · 2017-12-05T22:37:32Z

libpod/inspect_data.go

+
+// HostConfig represents the host configuration for the container
+type HostConfig struct {
+	Binds                []string                    `json:"Binds"`


Strongly in favor of dropping this or leaving it empty always, makes no sense in a libpod context

mheon · 2017-12-05T22:38:15Z

libpod/inspect_data.go

+// HostConfig represents the host configuration for the container
+type HostConfig struct {
+	Binds                []string                    `json:"Binds"`
+	ContainerIDFile      string                      `json:"ContainerIDFile"` //not implemented


I think we do create this in kpod run - we can store it as an annotation and unmarshal it into here?

mheon · 2017-12-05T22:39:02Z

libpod/inspect_data.go

+	Binds                []string                    `json:"Binds"`
+	ContainerIDFile      string                      `json:"ContainerIDFile"` //not implemented
+	LogConfig            *LogConfig                  `json:"LogConfig"`       //not implemented
+	NetworkMode          string                      `json:"NetworkMode"`     //not implemented


Can probably be parsed out of the namespaces info in the OCI spec

sort of fixed. Will need to move to cmd/kpod if decide to break the inspect struct

mheon · 2017-12-05T22:40:09Z

libpod/inspect_data.go

+	NetworkMode          string                      `json:"NetworkMode"`     //not implemented
+	PortBindings         map[string]struct{}         `json:"PortBindings"`    //not implemented
+	RestartPolicy        *RestartPolicy              `json:"RestartPolicy"`   //not implemented
+	AutoRemove           bool                        `json:"AutoRemove"`      //not implemented


This is handled in kpod run as --rm - again, we can probably add an annotation for it and check for it

mheon · 2017-12-05T22:41:15Z

libpod/inspect_data.go

+	AutoRemove           bool                        `json:"AutoRemove"`      //not implemented
+	VolumeDriver         string                      `json:"VolumeDriver"`    //not implemented
+	VolumesFrom          string                      `json:"VolumesFrom"`     // dont know type, not implemented
+	CapAdd               string                      `json:"CapAdd"`          //dont know type, not implemented


Implemented in kpod run as --cap-add and --cap-drop. Given the amount of stuff we need from kpod run, maybe we do need to store artifacts - maybe not the entire create config, but certainly a subset of it?

Also, if libpod can't get these (CapAdd, CapDrop make no sense inside libpod), I would be in favor of removing them from the structs libpod returns (and maybe adding them back in in cmd/kpod?)

mheon · 2017-12-05T22:41:30Z

libpod/inspect_data.go

+	Dns                  []string                    `json:"Dns"`             //not implemented
+	DnsOptions           []string                    `json:"DnsOptions"`      //not implemented
+	DnsSearch            []string                    `json:"DnsSearch"`       //not implemented
+	ExtraHosts           string                      `json:"ExtraHosts"`      //dont know type, not implemented


Probably a []string

mheon · 2017-12-05T22:42:21Z

libpod/inspect_data.go

+	DnsOptions           []string                    `json:"DnsOptions"`      //not implemented
+	DnsSearch            []string                    `json:"DnsSearch"`       //not implemented
+	ExtraHosts           string                      `json:"ExtraHosts"`      //dont know type, not implemented
+	GroupAdd             string                      `json:"GroupAdd"`        //not implemented


AdditionalGIDs in the spec

mheon · 2017-12-05T22:42:41Z

libpod/inspect_data.go

+	DnsSearch            []string                    `json:"DnsSearch"`       //not implemented
+	ExtraHosts           string                      `json:"ExtraHosts"`      //dont know type, not implemented
+	GroupAdd             string                      `json:"GroupAdd"`        //not implemented
+	IpcMode              string                      `json:"IpcMode"`         //not implemented


Can probably be parsed out of the spec's namespaces info

mheon · 2017-12-05T22:44:00Z

libpod/inspect_data.go

+	Links                string                      `json:"Links"` // dont know type, not implemented
+	OomScoreAdj          *int                        `json:"OomScoreAdj"`
+	PidMode              string                      `json:"PidMode"` //not implemented
+	Priveleged           bool                        `json:"Priveleged"`


Typo, Privileged

mheon · 2017-12-05T22:46:14Z

I think we need to have separate ContainerInfo structs in libpod and cmd/kpod. There's a lot of stuff in the existing docker-based structs that makes no sense in libpod and will clutter the struct for people trying to use the API. The cmd/kpod structs can include the missing information, which we can store as labels or container artifacts

TomSweeneyRedHat · 2017-12-06T12:03:19Z

docs/kpod-inspect.1.md

-    "WorkingDir": "/data"
-}
+```
+# kpod inspect umohnani/get-started:part1


you may want to consider changing the 'umohani' to something generic like 'myusername' or some such.

or perhaps doing redis:alpine again, an image people would be able to easily play with on their system.

Wanted to use an image that had a bunch of things configured like ports and all, but using fedora now. fixed

TomSweeneyRedHat · 2017-12-06T12:07:53Z

libpod/container.go

+// RootFsSize gets the size of the container's root filesystem
+// A container FS is split into two parts.  The first is the top layer, a
+// mutable layer, and the rest is the RootFS: the set of immutable layers
+// that make up the image on which the container is based


nit of a nit, missing period at end of line. Actually it looks like you did this through out. I'd suggest adding a period to all. That way people reading it a year from now won't wonder if you forgot to make a point or not.

TomSweeneyRedHat · 2017-12-06T12:24:19Z

libpod/image_inspect.go

+
+	ociv1Img, err := imgRef.OCIConfig()
+	if err != nil {
+		return nil, errors.Wrapf(err, "error gettin oci image info %q", img.ID)


gettin to getting

mheon · 2017-12-06T15:50:21Z

@rhatdan I think we need to restructure this so we have different inspect structs coming out of libpod and 'kpod inspect' - libpod doesn't know about a lot of things that should be in the kpod inspect struct (what mounts were user specified volume mounts, what namespaces were specified as host or another container, etc). CRI-O and other libpod API users won't know or care about these because they don't have a kpod CLI. We should output only the things libpod knows about from libpod, and then we can save the kpod metadata separately and use that for kpod inspect

umohnani8 · 2017-12-06T16:46:38Z

Made changes based on reviews. Will make further changes based on @rhatdan response to having inspect be broken in two, 1 for kpod and the other for the libpod api.

rhatdan · 2017-12-06T17:34:02Z

@mheon I agree. I think kpod create/run should save this information on their own, and leave to libpod stuff that it knows about. Therefore cri-o inspect would return less information then kpod run/kpod inspect.
Some of the information that kpod inspect of a cri-o container, could probably reconstructed.

mheon · 2017-12-06T17:45:51Z

@rhatdan I talked with @umohnani8 and we agreed on an interface for storing metadata. We'll need such a thing for CRI-O anyways, so we'll build it now and use it to store information from kpod run (CLI arguments) so we can reconstruct the rest of what we need.

umohnani8 · 2017-12-11T16:58:18Z

rebased

rhatdan · 2017-12-11T18:05:52Z

cmd/kpod/create_cli.go

-	if config.resources.memory != 0 && config.resources.memory < linuxMinMemory {
-		return warnings, fmt.Errorf("minimum memory limit allowed is 4MB")
+	if config.Resources.Memory != 0 && config.Resources.Memory < linuxMinMemory {
+		return warnings, fmt.Errorf("minimum Memory limit allowed is 4MB")
