Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runscript should be derived from Dockerfile's CMD/ENTRYPOINT #49

Closed
rgov opened this issue May 22, 2019 · 14 comments
Closed

runscript should be derived from Dockerfile's CMD/ENTRYPOINT #49

rgov opened this issue May 22, 2019 · 14 comments

Comments

@rgov
Copy link

rgov commented May 22, 2019

https://github.com/singularityware/docker2singularity/blob/master/docker2singularity.sh#L255

This is not a valid way to transform a CMD or ENTRYPOINT. From the Dockerfile documentation:

RUN has 2 forms:
RUN <command> (shell form, the command is run in a shell, which by default is /bin/sh -c on Linux or cmd /S /C on Windows).
RUN ["executable", "param1", "param2"] (exec form).

@vsoch
Copy link
Member

vsoch commented May 23, 2019

hey @rgov the master is a (now older) version of the script - I am going to be updating for Singularity 3.* family, would you care to work with me on that to fix up the issue here?

@vsoch vsoch changed the title runscript is a butchered version of Dockerfile's CMD runscript should be derived from Dockerfile's CMD/ENTRYPOINT May 23, 2019
@vsoch
Copy link
Member

vsoch commented May 23, 2019

You should consider being more professional and kind in your issue title, I have changed it for you.

@rgov
Copy link
Author

rgov commented May 24, 2019

No offense intended by 'butchered', sorry, just meant that chopping out characters isn't a safe transformation.

I fixed it in a fork here: rgov@eb0a760

@vsoch
Copy link
Member

vsoch commented May 24, 2019

Great! If it's okay with you, I'll grab that for the 3.* family update. Do you need previous versions here (2.x family branches) updated as well?

@vsoch
Copy link
Member

vsoch commented May 24, 2019

hey @rgov I'm doing some testing and I'm not sure this is exactly right. As it is now, we use either an entrypoint OR a CMD:

if [ -n "$ENTRYPOINT" ]; then
    echo exec "$ENTRYPOINT" '"$@"' >> $TMPDIR/singularity;
else
    if [ -n "$CMD" ]; then
        echo exec "$CMD" '"$@"' >> $TMPDIR/singularity;
    fi
fi

However, it should be the case that the CMD is added to an entrypoint. Try running vanessa/salad,
the entrypoint is ["/code/salad"] and the cmd is ["fork"]

$ docker inspect --format='{{json .Config.Entrypoint}}' vanessa/salad
["/code/salad"]
vanessa@vanessa-ThinkPad-T460s:/tmp/test$ docker inspect --format='{{json .Config.Cmd}}' vanessa/salad
["fork"]

When we run the container via docker, we get the full line "/code/salad fork" and the result is showing fork puns:

$ docker run -it vanessa/salad

 Fork off, already.  

          _________________  .========
         [_________________>< :======
                             '======== 

but with this change, the Singularity container just runs "/code/salad"

t$ singularity run vanessa_salad-2019-04-11-34373b0c6e21.img 
NAME:
   Salad Fork - Stick this in your salad, with caution, grasshopper!

USAGE:
   salad [global options] command [command options] [arguments...]

VERSION:
   0.0.1

COMMANDS:
     fork     Fork
     spoon    Spoon
     help, h  Shows a list of commands or help for one command

GLOBAL OPTIONS:
   --help, -h     show help
   --version, -v  print the version

so we see help. Instead, I think we want to honor the entrypoint and command:

# First preference goes to both entrypoint / cmd, then individual
if [ -n "$ENTRYPOINT" ] && [ -n "$CMD" ]; then
    echo exec "$ENTRYPOINT" "$CMD" '"$@"' >> $TMPDIR/singularity;
elif [ -n "$ENTRYPOINT" ]; then
    echo exec "$ENTRYPOINT" '"$@"' >> $TMPDIR/singularity;
elif [ -n "$CMD" ]; then
    echo exec "$CMD" '"$@"' >> $TMPDIR/singularity;
fi

That runs correctly:

$ singularity run vanessa_salad-2019-04-11-5b5c19bc9c20.img 

 Have a knife day? Fork you!  

                       /\
                      //\\
                     //  \\
                 ^   \\  //   ^
                / \   )  (   / \ 
                ) (   )  (   ) (
                \  \_/ /\ \_/  /
                 \__  _)(_  __/
                    \ \  / /
                     ) \/ (
                     | /\ |
                     | )( |
                     | )( |
                     | \/ |
                     )____(
                    /      \
                    \______/ 

And the runscript looks correct:

#!/bin/sh -e
exec /code/salad fork "$@"

Did you test the method you used on your containers?

@vsoch
Copy link
Member

vsoch commented May 24, 2019

The previous method (without the python dependency) also seems to work fine. Did you have issue with it for some reason?

@vsoch
Copy link
Member

vsoch commented May 24, 2019

@rgov I have updated all branches and master, and also added WORKDIR. Please close this issue at your leisure if it has been addressed. If you have some spare cycles, testing for master / v3.1 would be appreciated!

@rgov
Copy link
Author

rgov commented May 24, 2019

I am running Singularity 2.5.2 so I'd need it to work there (though I can continue to use my fork).

Yes, I think you're correct that both ENTRYPOINT and CMD need to be honored and I am only honoring one. In my source Dockerfile I only needed CMD so I did not attempt to make a combination work. The logic in b6fa033 can be applied on top of this change also.

The reason why the previous method does not work is because JSON has a context-free grammar (like HTML) and cannot be safely modified with regular expressions (using sed). If one of the command line arguments contains " or , or [ or ], the regular expression will modify the argument. So this will be broken:

CMD echo "a,b,c"

You will need to use a real JSON parser, and then properly escape the arguments so that things like dollar signs in the argument don't get interpreted as variable expansion. (JSON also has an escape character \ that needs to be properly handled.)

Note that I used Python 2 because it is specified in your Dockerfile and so this does not add a new dependency.

Would it not be easier to have the master branch support multiple versions of Singularity? Wherever the functionality diverges you could make the code conditional. You could use if ... fi, case ... esac, or source a separate file that defines functions used for each.

@vsoch
Copy link
Member

vsoch commented May 24, 2019

hey @rgov thanks for the details - I wound up liking the python approach better (I had forgotten that we added it as a dependency for later versions anyway) and you are completely right about issues with json. I think at the time I had hoped to "escape" needing that, but only tested for very simple use cases.

I chose to have different versions because (hopefully) the older wouldn't need to be frequently updated, and it works nicely to build on Docker Hub. It's also the case that the builds have changed a bit drastically over time (Singularity 3.x has golang, for example). I think the crunk of issues that piled up resulted from nobody taking care of the repo - I'll make sure that doesn't happen again :)

@rgov
Copy link
Author

rgov commented May 24, 2019

I tested the v2.5 branch and it worked for me. Please push a new version to the Docker Hub when you have a chance.

@vsoch
Copy link
Member

vsoch commented May 24, 2019

I think I already did - have you tested?

https://hub.docker.com/r/singularityware/docker2singularity/tags

@rgov
Copy link
Author

rgov commented May 24, 2019

I tried the Hub version first and had trouble and then built my own. I was debugging something else so maybe I got the two conflated.

@rgov rgov closed this as completed May 24, 2019
@vsoch
Copy link
Member

vsoch commented May 24, 2019

I'll build and push again just in case, give me a few minutes!

@vsoch
Copy link
Member

vsoch commented May 24, 2019

Forgot to post here - I rebuild and pushed, please test again and let me know the issues you run into!

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

No branches or pull requests

2 participants