Skip to content
This repository has been archived by the owner on Oct 10, 2020. It is now read-only.

Revisiting the defaults for atomic run with and without --spc #75

Open
bexelbie opened this issue Jun 26, 2015 · 12 comments
Open

Revisiting the defaults for atomic run with and without --spc #75

bexelbie opened this issue Jun 26, 2015 · 12 comments

Comments

@bexelbie
Copy link
Contributor

The current default behavior is inconsistent for SPC and non-SPC invocations (see below).

In short, the default for atomic run --spc drops you into an interactive session. The default for atomic run is to create a running container that can later be used for commands.

PR #74 fixes this behavior.

However, I'd like us to rethink this. I believe a better solution would be something like this:

atomic run [--spc] image [command]:
if (CREATE LABEL present)
execute CREATE label passing command if present in an environment variable
if (RUN LABEL present)
execute RUN LABEL passing command if present in an environment variable
else
use docker create to launch the container and then docker exec with /bin/bash or command if present

atomic start [--spc] image [command]:
if (CREATE LABEL present)
execute CREATE label passing command if present in an environment variable
else
use docker create to launch the container passing command if present


Example of current behavior

./atomic run --spc busybox /bin/ps

Container 'busybox-spc' must be running before executing a command into it.
Execute the following to create the container:
./atomic run --spc --name busybox-spc busybox

./atomic run --spc busybox

/usr/bin/docker run -t -i --rm --privileged -v /:/host -v /run:/run -v /etc/localtime:/etc/localtime --net=host --ipc=host --pid=host -e HOST=/host -e NAME=${NAME} -e IMAGE=${IMAGE} ${IMAGE} /bin/sh

./atomic run busybox /bin/ps

Container 'busybox' must be running before executing a command into it.
Execute the following to create the container:
./atomic run busybox

./atomic run busybox

/usr/bin/docker create -t -i --name ${NAME} ${IMAGE} /bin/sh
busybox

docker ps

CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
e7f88fef782d busybox:latest "/bin/sh" 5 seconds ago Up 4 seconds busybox

@rhatdan
Copy link
Member

rhatdan commented Jul 8, 2015

@sctweedie What do you think?

I am not crazy about breaking current behaviour, but I would like a way to differentiate between a RUN Once container, and a run some container multiple times.

atomic run currently handles the rhel-tools container fairly well, although users get confused on the first run. But people building containers to run once and specify a command can get confused.

@rhatdan
Copy link
Member

rhatdan commented Jul 8, 2015

@riekrh Comment?

@ghost
Copy link

ghost commented Jul 9, 2015

I think the original idea was to RUN the initial command for the rhel-tools image and the leave the container around for quick execution of subsequent RUNs (vs. recreating multiple instances of the container - which is not intended - or using --rm - which is slow).

I agree that consistency for --spc is desirable and differentiating between CREATE and RUN is a good idea. But perhaps we need an explicit flag for the re-use existing vs create new instance point?

@bexelbie
Copy link
Contributor Author

bexelbie commented Jul 9, 2015

Maybe the default case should be the simplest one and we should allow containers like rhel-tools to have sufficient freedom to define the create/exec functionality?

@ghost
Copy link

ghost commented Jul 9, 2015

I agree - at the end it should be defined in the container, so the end-user
is not exposed to decisions.

On Thu, Jul 9, 2015 at 10:13 AM, Brian Exelbierd notifications@github.com
wrote:

Maybe the default case should be the simplest one and we should allow
containers like rhel-tools to have sufficient freedom to define the
create/exec functionality?


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

@bexelbie
Copy link
Contributor Author

bexelbie commented Jul 9, 2015

@riekrh would you support the proposal above, with the addition that CREATE is not executed if the container is already running?

@ghost
Copy link

ghost commented Jul 9, 2015

I think there should be a 'reuse_existing_container' LABEL controlling whether run (and start) will re-create the container.

@rhatdan
Copy link
Member

rhatdan commented Jul 9, 2015

Can we get a little less awful label.

LABEL shared_container

But the problem is how do we handle existing containers. I guess we could continue to look to see if a container exists, but if the container does not exist and the LABEL shared_container does not exist it just does the run command not the create

@ghost
Copy link

ghost commented Jul 9, 2015

On Thu, Jul 9, 2015 at 12:13 PM, Daniel J Walsh notifications@github.com
wrote:

Can we get a little less awful label.

LABEL shared_container

But the problem is how do we handle existing containers. I guess we could
continue to look to see if a container exists, but if the container does
not exist and the LABEL shared_container does not exist it just does the
run command not the create

That should work for the transition. Can drop it later.

@bexelbie
Copy link
Contributor Author

I am not 100% sure we need a label for a shared container. Wouldn't it be enough for the RUN to not include --rm? This assumes that the default isn't trying to support multiple style use cases. Perhaps we could move to this default:

atomic run NAME COMMAND

if container exists
   verify it is the same version as the image of the same name, if not warning message
if container is STOPPED or CREATED
  if START label
    run START label and continue
  else
    run `docker start NAME` and continue
if container is RUNNING
   if EXEC label
     EXECUTE EXEC label and return
   else
     EXECUTE `docker exec -it NAME [COMMAND] and return
if RUN label
  EXECUTE RUN label and return
else
  cmd = `docker run -i -t --rm --name NAME NAME COMMAND`
  if SPC
    cmd also gets `--privileged -v /:/host -v /run:/run -v /etc/loaltime:/etc/loaltime --net=host --ipc=h
ost --pid=host -e HOST=/host -e NAME=NAME -e IMAGE=IMAGE`
  if COMMAND
    cmd += COMMAND
  EXECUTE cmd and return

If you need a container that persists you will need to include a custom RUN that either daemonizes the container or doesn't clean it up. rhel-tools does this. We could also just state SPC loses the --rm if we wanted to in the default.

@ghost
Copy link

ghost commented Jul 15, 2015

Maybe that is is sufficient. Can someone play through the scenarios?

On Wed, Jul 15, 2015 at 6:43 AM, Brian Exelbierd notifications@github.com
wrote:

I am not 100% sure we need a label for a shared container. Wouldn't it be
enough for the RUN to not include --rm? This assumes that the default isn't
trying to support multiple style use cases. Perhaps we could move to this
default:

atomic run NAME COMMAND

if container exists
verify it is the same version as the image of the same name, if not warning message

if container is STOPPED or CREATED
if START label
run START label and continue
else
run docker start NAME and continue

if container is RUNNING
if EXEC label
EXECUTE EXEC label and return
else
EXECUTE `docker exec -it NAME [COMMAND] and return

if RUN label
EXECUTE RUN label and return
else
cmd = docker run -i -t --rm --name NAME NAME COMMAND
if SPC
cmd also gets --privileged -v /:/host -v /run:/run -v /etc/loaltime:/etc/loaltime --net=host --ipc=h ost --pid=host -e HOST=/host -e NAME=NAME -e IMAGE=IMAGE
if COMMAND
cmd += COMMAND
EXECUTE cmd and return

If you need a container that persists you will need to include a custom
RUN that either daemonizes the container or doesn't clean it up. rhel-tools
does this. We could also just state SPC loses the --rm if we wanted to in
the default.


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

@rhatdan
Copy link
Member

rhatdan commented Jul 15, 2015

That seems to work for me. We need @sctweedie to comment on this. When is he back from PTO?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants