adds list command #507

mikebrow · 2016-01-23T01:33:40Z

Adds a runc list command. Show minimal information about the container and their current status.

Signed-off-by: Mike Brown [email protected]

mikebrow · 2016-01-23T01:35:53Z

Replacement for #412

crosbymichael · 2016-01-23T01:38:15Z

list.go

You don't need the status as an int here because we already have it as a string

kk... removing was there for debug because I was getting undefined (container status is broken atm) removing it.... done... removed.

The undefined stuff will be fixed in #499

mikebrow · 2016-01-23T18:01:56Z

@crosbymichael I've completed the suggestions. Tabwriter's cool, thx!

mikebrow · 2016-01-25T14:55:17Z

@mrunalp @duglin @vishh comments on this runc list PR?

duglin · 2016-01-25T16:30:45Z

list.go

I find the use of duration here less helpful than the actual time itself. While clocks will vary between systems, at least with a concrete time you know what exact time something started on that system. With a duration I now need to know when my query was executed and take that into account to get the real date/time. Let use a time instead.

mikebrow · 2016-01-25T21:46:01Z

@duglin Per our discussion since the current time value that we are storing along with container state is mics since boot of the host for the init process, and thus isn't very informative for the user... I've added a new field for the container time stamp to indicate in UTC when the container was first started on a host.

vishh · 2016-01-25T21:48:14Z

libcontainer/container.go

nit: Can we instead say System Time?

changed the new one to creation timestamp per your suggestion..

This old one for init process start is a legacy naming issue.. in so far as it's been named and used this way for a while... I was just updating the comment to be more explanatory

mikebrow · 2016-01-25T22:45:34Z

@vishh Comments addressed, Thx!!

hqhq · 2016-01-26T04:02:42Z

list.go

I prefer the order to be: "ID\tPID\tSTATUS\tSTARTED\tROOTFS\n", it's more consistent with linux ps output, and maybe list BUNDLE instead of ROOTFS, WDYT?

@hqhq Order: I have no preference on the order so... sure changed.

Thoughts on BUNDLE vs. ROOTFS:
We are making container id a required field on all of the runc commands that previously needed you to either be in the bundle or pass the old bundle as an optional parameter. Decision made at the last face to face. Thus the utility of the bundle as a thing you have to know from one command to the next on a running container is changing.

I think there is good utility for knowing the rootfs of the currently running container.
My understanding of the bundle on the other hand, is that it is an optional path argument to runc start/create or the cwd of the runc command. The bundle path is not being stored ATM (I don't believe). We take a snap shot of everything that was there at start/create and then we are done with it. Eg. we make a copy of the config.json and that copy is stored as state. The original is not used again unless they start a new container from the same bundle. From one runc start to the next, the config.json in the bundle directory could have been edited and it's not required to still be there.

So... I'd rather have either an option on list (-d etc.) to output more detail in which we add things like BUNDLE and maybe a few other items... or have a different command entirely that dumps as much or as little as you might like to see about a particular container.

runc is a bit loose on the optionality of the bundle path. While runc itself lets it be optional, that's only because it then defaults to "." - but the spec itself says that its a required field. It could be argued that we should require it on the "start" cmd, if we want to promote spec compliance especially since at the f2f there was a bit of a push to have less defaults being used.

As for rootfs vs bundle in the output. Irrespective of whether runc saves that info directly or not, it does seem like showing the bundle dir might make more sense since that's the source of all info used to create the container. Also, from there you can add something like /config.json to get that info if needed. Granted from rootfs you can remove the tail of it and calculate the bundle dir, but I'm not sure I see the value in forcing people to do that. The only downside is that to get the rootfs dir they'd have to parse the config.json to find its name - but it all depends on which data we think people want to see most often. We could also just show the rootfs dir name w/o the path as a column.

Ok.. Right.. my thoughts regarding pointing them to the bundle directory so they can list the config.json are back with the concern that the config.json might not even be there any more, and certainly could have been edited by then by some higher level tool that generated it and/or is managing it. I was thinking runc list is more for what is going on after runc has loaded the config.json than telling them where the files were loaded from to launch it. At one point we were discussing a need to dump the state data including the copy of the config.json that was used to start the container.. I think it would make sense to have a command at some point that regenerates that which created the container.. this to enable cloning and introspection operations.

That said.. yeah sure as long as rootfs is allways going to be a subdirectory of the bundle path and never a full path.., I can just tail the rootfs path (using our copy of rootfs in the config.json in memory) to form the bundle path.. and then output the bundle (path) and the relative offset rootfs. WDYT?

I would put the bundle as one of the columns. Its helpful for the user and makes sense

Sounds good will update splitting the root fs into the bundle and rootfs as separate columns...

crosbymichael · 2016-01-26T19:05:23Z

libcontainer/container_linux.go

Why don't you use the start time?

Start time is clock ticks since boot .. it's not a timestamp.

duglin · 2016-01-26T19:09:47Z

Been thinking that since this might be one of the first commands we have that might be used by non-humans I think it would be good to add a --json type of flag to show the output in a true machine readable format - not tabs. I'd like us to get into the habit of having json output on all commands that we expect to be used my scripts/machines. If not json I'm ok with other options, but human readable output isn't ideal. what do others think?

crosbymichael · 2016-01-26T19:11:18Z

@duglin yes, that is good to have

vishh · 2016-01-26T19:13:42Z

How about --output or --format which can include possibly multiple
formats in the future?

On Tue, Jan 26, 2016 at 11:10 AM, Doug Davis [email protected]
wrote:

Been thinking that since this might be one of the first commands we have
that might be used by non-humans I think it would be good to add a --json
type of flag to show the output in a true machine readable format - not
tabs. I'd like us to get into the habit of having json output on all
commands that we expect to be used my scripts/machines. If not json I'm ok
with other options, but human readable output isn't ideal. what do others
think?

—
Reply to this email directly or view it on GitHub
#507 (comment).

duglin · 2016-01-26T19:14:18Z

yep - I like --format. e.g --format=json

duglin · 2016-01-26T19:20:58Z

@mikebrow it make sure this PR doesn't keep going, we could add the --format flag in a separate PR :-)

mikebrow · 2016-01-26T19:31:56Z

Love the --format options will put a few in on the next PR :-)

mikebrow · 2016-01-26T22:54:08Z

All comments addressed except the --format=json option which I'll do in the next PR. The list command now outputs the original bundle directory (cwd) from when the container was started and the rootfs path. We've prefix trimmed the bundle directory from the absolute rootfs path to make it take less space and look better.

ashahab-altiscale · 2016-01-27T01:53:39Z

list.go

Do you check if the PID actually exists?

@ashahab-altiscale Short answer... no.

I'm listing all containers for which there is an instance. Today that means started and has not been cleaned up (no kill or standard exit processing). Soon that should mean created or started.

This line of code (41) merely caches up the column headings for the tab writer.

With this "runc list" command I'm just loading up the containers and state using existing apis and using the existing error checking that those apis provide. The loaded containers may have just been created or started or paused, etc. If there is no init pid stored away for the container the int should be zero. If we don't like it (someone might try to perform an operation on pid zero..?) then I could substitute zero for "n/a" or "none" or some other default text. But folks might prefer to see zero as an indicator that there is no init pid yet? Let's see what it looks like when dug's create/start split drops.

Ok, so a follow up question: What are we reporting in the status column? IMHO, If we are reporting 'running', we should check if the pid actually exists.

Now I see what your question was about... :)

I'm just reporting the currently "stored" values for the containers and their state as stored on disk. I'm not validating whether or not the stored values accurately reflect the actual state of said containers.

To address your point/issue I could make a call to container.refreshstate() hmm... Let me sleep on that and see what the others think about it. I suppose at issue is should the runc list command be true to the stored state on disk or should I run refresh on those containers to fact check the state at the point of reporting it... then if I do refresh should I report or have the option of reporting the differences... Hmm :-)

Ok.. the decision from multiple points of input is to not add any additional verification in the runc list command over and above what I already have in there. If there is an issue / disconnect in reported state we'll fix the issues when they are reported.

vishh · 2016-01-27T21:20:43Z

libcontainer/container.go

Why is this field a string and not time.Time?

Just putting it in a human readable form.. time.Time would work fine. FYI Time is a struct ... two int64s seconds and nanoseconds and a string for the timezone.. nill if utc.

WDYT keep it a string or let json marshal the golang unix time.Time structure?

Updated to use time.Time in canonical form in memory and serialized.. now waits to put it into RFC3339Nano string format until it outputs it on the list command.

vishh · 2016-01-27T23:52:17Z

LGTM.

crosbymichael · 2016-01-28T00:06:27Z

libcontainer/container_linux.go

This is incorrect because cwd is not right at the libcontainer level

It reported the correct directory on all my tests..

Suggestion?

For things like docker it will not. Can we maybe leave this one out and i'll think about how we want to handle this in libcontainer?

Ok.. removing it for now.

mikebrow · 2016-01-28T05:21:48Z

@duglin @crosbymichael All comments addressed. Removed the bundle and rootfs columns for now. Should be ready to move forward.

duglin · 2016-01-28T14:18:40Z

libcontainer/container.go

nit: for consistency we should remove the "Stamp" part - e.g. we don't have "InitProcessStartTimeStamp"

I've stamped out the use of "stamp" :-)

duglin · 2016-01-28T14:24:32Z

aside from the nit, non-binding-LGTM

mikebrow · 2016-01-28T16:28:11Z

@duglin comment re: naming of terms is addressed.

duglin · 2016-01-28T17:51:18Z

thanks

crosbymichael · 2016-01-28T18:48:42Z

list.go

Why is this last column STARTED and everywhere else this is creationtime?

Cause I originally called the variable container_timestamp with an output at the runc list command column name of "STARTED" so as to indicate the time when the container was started. Then vish wanted to call the variable creation_timestamp... then dug wanted to call the variable creation_time. But you're the first one to ask about the output column heading name. :-)

All that said I have no druthers, what column heading would you like to see? "CREATION TIME" does not really roll off the tongue.

for those of us with simple/anal minds... for consistency: s/creation/created/ then :-)

on the vars

"CREATED" it is :-) oh and +1 i like that better.

mikebrow · 2016-01-28T20:01:17Z

@duglin @crosbymichael PR updated with CREATED column..

Signed-off-by: Mike Brown <[email protected]>

mikebrow · 2016-01-28T20:26:03Z

@duglin Ok... all variables now indicate that is this the created time for the container. Will update the point at which the timestamp is taken when the create/start split PR is accepted. The column now says CREATED and the comments have been refreshed to indicate this is the time when the container has been created. Cheers.

duglin · 2016-01-28T20:30:46Z

looks good!

crosbymichael · 2016-01-28T21:19:25Z

LGTM

adds list command

runtime: Add 'creating' to state status

GordonTheTurtle added the status/0-triage label Jan 23, 2016

crosbymichael reviewed Jan 23, 2016
View reviewed changes

mikebrow force-pushed the runc-ls-command branch 2 times, most recently from 20e5773 to d693244 Compare January 23, 2016 17:58

duglin reviewed Jan 25, 2016
View reviewed changes

mikebrow force-pushed the runc-ls-command branch 2 times, most recently from 6b70978 to 74302b1 Compare January 25, 2016 21:27

vishh reviewed Jan 25, 2016
View reviewed changes

mikebrow force-pushed the runc-ls-command branch from 74302b1 to fb4e79b Compare January 25, 2016 22:45

hqhq reviewed Jan 26, 2016
View reviewed changes

mikebrow force-pushed the runc-ls-command branch from fb4e79b to eb13b2a Compare January 26, 2016 13:56

crosbymichael reviewed Jan 26, 2016
View reviewed changes

mikebrow force-pushed the runc-ls-command branch from eb13b2a to 8dfe9f7 Compare January 26, 2016 22:43

ashahab-altiscale reviewed Jan 27, 2016
View reviewed changes

vishh reviewed Jan 27, 2016
View reviewed changes

mikebrow force-pushed the runc-ls-command branch from 8dfe9f7 to b6f2001 Compare January 27, 2016 23:46

crosbymichael reviewed Jan 28, 2016
View reviewed changes

mikebrow force-pushed the runc-ls-command branch 2 times, most recently from 276e151 to e930efc Compare January 28, 2016 05:19

duglin reviewed Jan 28, 2016
View reviewed changes

mikebrow force-pushed the runc-ls-command branch from e930efc to f8adb96 Compare January 28, 2016 16:23

crosbymichael reviewed Jan 28, 2016
View reviewed changes

mikebrow force-pushed the runc-ls-command branch from f8adb96 to dc028b7 Compare January 28, 2016 20:00

adds list command, and a timestamp in the container state

4c87126

Signed-off-by: Mike Brown <[email protected]>

mikebrow force-pushed the runc-ls-command branch from dc028b7 to 4c87126 Compare January 28, 2016 20:21

crosbymichael added a commit that referenced this pull request Jan 28, 2016

Merge pull request #507 from mikebrow/runc-ls-command

480e5f4

adds list command

crosbymichael merged commit 480e5f4 into opencontainers:master Jan 28, 2016

mikebrow deleted the runc-ls-command branch February 9, 2016 20:38

This was referenced Feb 10, 2016

Format options for list command #547

Closed

Add bundle and rootfs to the list command #548

Closed

no bundlePath in state.json #559

Closed

stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017

Merge pull request opencontainers#507 from wking/add-creating-status

1f408dc

runtime: Add 'creating' to state status

adds list command #507

adds list command #507

Uh oh!

Conversation

mikebrow commented Jan 23, 2016

Uh oh!

mikebrow commented Jan 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Jan 23, 2016

Uh oh!

mikebrow commented Jan 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Jan 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Jan 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Jan 26, 2016

Uh oh!

crosbymichael commented Jan 26, 2016

Uh oh!

vishh commented Jan 26, 2016

Uh oh!

duglin commented Jan 26, 2016

Uh oh!

duglin commented Jan 26, 2016

Uh oh!

mikebrow commented Jan 26, 2016

Uh oh!

mikebrow commented Jan 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishh commented Jan 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!