-
Notifications
You must be signed in to change notification settings - Fork 201
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
Add support for atomic command #83
Conversation
Can one of the admins verify this patch? |
\${OPT1} \${IMAGE} /usr/share/container-layer/mysql/atomic/uninstall.sh | ||
LABEL RUN docker run -t -i --rm --name=\${NAME} -e OPT1 -e OPT2 -e OPT3 \${OPT2} \ | ||
\${IMAGE} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do these all as a single layer to save build time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fact combine them into the label statement above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'd rather just merge them together, but let all the Atomic labels as one LABEL instruction. The time saved might be 1s but the Dockerfile will be better readable.
couple nits, mostly looks good to me. |
Petr Pisar had some opinion on my atomic install.sh script, which could be valid also for your install.sh, so I'm forwarding it here:
|
@hanzz Thanks for pointing to that, will use quotes more :) |
@@ -20,10 +20,23 @@ LABEL io.k8s.description="MySQL is a multi-user, multi-threaded SQL database ser | |||
io.openshift.expose-services="3306:mysql" \ | |||
io.openshift.tags="database,mysql,mysql55" | |||
|
|||
# Support for atomic run/install/uninstall | |||
LABEL INSTALL="docker run -t -i --rm --privileged -u 0:0 -v /:/host --net=host \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is incredibly ugly :-) @vpavlin are there any plans to make this crypto commands more human-friendly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ugly? This is transparent for the user, so why does it matter? Anyway, this may be raised in the atomic itself (https://github.com/projectatomic/atomic/), I don't think we can do anything differently in this image.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hhorak just from looking at this, does it seem readable to you? It matters because we want users to build the images that will run on Atomic. Users will eventually copy the patterns we have here. My concern which this pattern is that many users will not really understand what this magic options are for and will end up copying this blindly.
I don't think we can solve this here and now, that is what I tagged @vpavlin to provide his thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @bparees
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hhorak also, I don't want to block this PR from being merged, just want to have discussion about these labels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfojtik No, I didn't say we don't know the story. Just that it might not be obvious for the first glance. The story is to let user run the image properly without being forced to search for READMEs and How-tos. He just needs to know that there is an atomic command which is able to run images that support it.
Also, I didn't say it's ok that users copy random stuff and run it (Hey, have you tried to use Kubernetes and run examples without knowing what that lines mean?:) ). What I am saying is that users just do it that way and the only thing we can do is to provide them with accessible and straightforward documentation.
Your questions about backward compatibilty are very good, but you are asking wrong people at wrong place. @rhatdan is trying to get attention to this for some time already at https://github.com/projectatomic/ContainerApplicationGenericLabels
You need to write out the service file here. Or some configuration (f.e. in case of rsyslog image).
I like @hanzz's proposal/idea but again - this is a wrong place to solve it because not all interested people are following this particular pull request:-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$OPTx
is not only for environment variables, but for modifying the command overall. Let's say you have the --verbose
switch which is off by default, but you can turn it on if there is $OPTx
on the right place.
Images should work without these options being used, but they should let you tweak the behaviour of image in special cases (like debuging as I described above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The $OPTx
in this image are used to get user/pass/dbname from user, which are required to create a container. I don't have any other way to get those data except $OPTx
, do I?
@hanzz not sure I got your crazy idea correctly, but when I tried something like atomic install --unknown ...
, then I got /bin/atomic: unrecognized arguments: --unknown
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree with @mfojtik reading this is hurting my eyes. We've (OpenShift team) put a lot of effort to simplify how to create builder images, with this addition I'm more than 100% sure people will refuse to build those by themselves. One big questions that comes into my head is:
if these are directives for atomic command, can't these just point the necessary information (dirs, ports, etc.) you'll pass down to container engine (be that docker or rocket in the future) and atomic will know how to call docker/rkt run etc. Similarly to how we do with s2i build, we don't put entire logic into the builder image, we just put the necessary bits there to run the command, can't atomic do the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hanzz not sure I got your crazy idea correctly, but when I tried something like atomic install --unknown ..., then I got /bin/atomic: unrecognized arguments: --unknown...
I was proposing the way how atomic could work in the future. It's not implemented like that right now :).
Perhaps it would be better to create a BlueJeans meeting to discuss these issues. I am open to ideas on how to fix these issues. I think Vaclav has done a good idea describing what the goal was. BTW Current atomic will allow you to append content on to the end of the INSTALL/RUN/UNINSTALL lines atomic install mysql --user=dwalsh --passwd=foobar Should endup taking the INSTALL line and appending --user=dwalsh --passwd=foobar to the end. |
@rhatdan +1 (I will schedule something for next week, what day suits you best?) |
@hhorak can you rebase this? |
@mfojtik as for one of you comments above.. "4) Having this labels in one liner saves some time during a build, but parsing this just with eyes is very hard. I mean I completely missed RUN ;-)" -- that was how it looked first time and i made one command from it after feedback here. If you prefer one command per LABEL, so as I do, shouldn't we return to the previous variant? |
No, I think it is fine (it makes the build faster)... We should work on On Wed, Sep 2, 2015 at 4:30 PM, Honza Horak notifications@github.com
Michal Fojtik <mi@mifo.sk mfojtik@mifo.sk> |
Conflicts: 5.5/Dockerfile 5.5/Dockerfile.rhel7 5.6/Dockerfile 5.6/Dockerfile.rhel7
Monday is Holiday in US. Tuesday After 10-15 EDT is open. Also have scattered time Wednesday-Friday |
@@ -0,0 +1,12 @@ | |||
# Check whether all required variables are set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hhorak cab you rename this file to "variables.sh" or "vars.sh" ? As this file really just define variables used by other scripts.
one renaming suggestion, otherwise LGTM, we will discuss the labels/format on Tuesday. |
How would you run this image with atomic and specify a location for the data volume (/var/lib/mysql/data)? |
OK, I see that you can use However, that's just weird and kind of defeats the purpose of making this generic for atomic. I can understand if the labels are meant to be used from shell, for example. But if you are running atomic {run,install,uninstall}, you should be able to use some "normal" metadata for guessing how to run it, no? For example, the metadata might say you need to mount the host rootfs, run privileged and execute a given script (or have a standard location for an atomic install script, so this would be unnecessary). You would get the VOLUME metadata for free. |
OPT1, OPT2 and OPT3 should not be used unless the user/tester wants to override the atomic command. We want
To just work. If the user wanted to do something like atomic install --opt1="--debug" mariadb Then it could replace $OPT1 in the LABEL with "--debug" when it executes the command and potentially run the command in debug mode. But OPT1,OPT2, OPT3 should not be part of the standard command that users execute.
The developer of mariadb would just specify the RUN LABEL to be
You might have LABEL INSTALL command do something like running myslqd init to create content in the hosts /var/lib/mysql directory. If you wanted to support multiple versions of mariadb you could do it based on $NAME
Which would allow you to run multiple containers with different names.
|
@rhatdan would it be a normal use-case for atomic to have a DB on host? Would a user running The database is initialized automatically, when none is detected, so there should be no need for the install label to handle this. |
Well you would not have a /var/lib/mysql on the host unless you do an install. |
@rhatdan Thanks for the feedback, I've tried to take all that into account when creating reworked version at #92, but there is one difference from your description (except the fact that some environment variables are compulsory in mysql image now, so running just The difference is caused by projectatomic/atomic#151 and especially by not using systemd unit for watching the container. How @vpavlin explained me it should work (correct me if I'm wrong) is that
(this workflow defines However, from what I see in atomic docs, it looks like the
Any advise which way is better here? |
atomic run will do a start if the container exists, if it does not exists it will execute the LABEL RUN |
I think these would all work.
|
I still struggle with some elegant implementation for So, for example in case we call
then what we get is
while what we need is
The easiest idea to fix this was to use script similar to install.sh, something like this:
and having RUN label like this:
but I got the following error:
Any idea? |
This should probably be in the label. Why not have the User name and password passed after the command on the run, they will get appended as options on the command. $ atomic run -n myc4 mysql-55-rhel7 --user=user --password=secretpass --db=db1 |
Then the setup script could use these parameters or prompt for them if they are not given. BTW this is the goal of nulecule/atomicapp to handle problems like this. |
Now I see why it didn't work for me, I simply had too old version of |
Closing this one in favor of #92, the same feature is gonna be done by that new PR. |
Add cgroup utility functions
Atomic command is standard way to run containers on Atomic host. This adds support for running this image on such a system using
atomic install/uninstall/run
, as described at https://github.com/projectatomic/atomic/.@omron93 @hanzz have you got to something similar to that?