The /tmp/src directory is never removed. #84
Comments
|
@GrahamDumpleton I think we mount that directory to the container, so it should not be present in the output image. Will verify that. |
|
Maybe it is because I am running 's2i' on the command line and not within OpenShift. Is how it is executed going to be different? |
|
|
@GrahamDumpleton ok, that smells like a bug. We should not keep the source in the |
|
And just to confirm files are copied in, rather than mounted, where copying in is what I thought happened, here are verbose logs. |
|
@rhcarvalho can you please take a look at this? |
|
@bparees FYI, this is one of the things we were talking about yesterday on IRC, before I see @GrahamDumpleton complaining about it :) This is the same for all other images. In the discussion yesterday I was raising the point why don't we copy the source to the final location == |
|
We use a docker COPY instruction to copy the source code into the image. @mfojtik what do you think about changing the |
|
@rhcarvalho that will require to change all assemble scripts in images we already released, right? |
|
@mfojtik yes, we'd need to simplify the assemble scripts, remove that line. The quick fix for this issue is to change all assemble scripts to also |
|
@rhcarvalho why not remove that folder in S2I directly? We can add that removal to a wrapper for the assemble script, so that folder is always removed when the assemble finishes. Similar to the wrapper I'm doing for removing the secrets. One less thing to worry about in assemble script then. |
|
And why not the "wrapper" put the source in the right place as well? :) I would like to tell people that their assemble scripts need not to be any more complex than: And we'd autogenerate those and bake it into their image so nobody needs to write a single line of code in the easy case, and can customize it with 2-lines if needed. |
|
Customize it by starting from the 2-liner we'd have generated in the first place... |
|
@rhcarvalho the wrapper has to know what is the "right place", right? It is already known for all images we own as OpenShift, but you don't know what the right place is for upstream images. |
|
For me the right place is always the WORKDIR. That's the case for ONBUILD Docker images and all other circumstances I've seem. |
|
@rhcarvalho some images I've seen on Dockerhub does not even set WORKDIR, what would you do in that case? Copy the sources to |
|
yes, because when you docker exec or oc exec into your container, that's where you'll end up, and your code will be there for you. I had in mind the golang image that we use in https://github.com/openshift/golang-ex, it puts your code in the WORKDIR that happens to be in the GOPATH. |
|
I can't see that you have a choice but to leave the current behaviour as the default. You will break the builder of anyone out there who has You also can't just assume you can instead copy to What I suggest going forward to make it more flexible is to optionally allow a S2I builder to set a label like: So something like: Needs a better label name, but the idea is that you could specify in a S2I builder where the source was unpacked to. The This could be set in This would then allow the Use: if it wanted it copied direct into place, be it the recommended place or elsewhere. Use: if wanted to use a different location, where it then either copied the whole lot into a single place as often done, or even where it copied different parts of the directory into different locations. In this case it would be the To make sure not duplicating information in different places, the That way the |
|
btw another historic reason we use /tmp/src is that's not just the application source code that gets put in /tmp. We also put the run/assemble scripts there if they came from an external location, because it all comes in from one single tar file. So the motivation was "we're going to inject this tar of stuff, some of which is source, some of which will be other stuff, so we'll just extract it in /tmp and then let the assemble script worry about which bits it cares about for what" if we start injecting directly to a different location, the external scripts are going to show up in that directory also unless we create a more sophisticate tar structure. In anycase the "quickfix" to this is simply to change the cp to a mv, for assemble scripts where that makes sense. |
|
Just got hit by this problem with It not being removed means you cannot use layered S2I builds with the default Python S2I builder, without needing to provide your own That is, use the S2I builder to create a new image using This layered build might for example be used to create a new S2I builder pre-populated with Python packages or applications to effectively create a new base image, but using S2I. If as part of that first phase, files were removed from |
|
I think i stand by my above comment that the easy fix here is to change our default assemble scripts to "mv" instead of "cp" the source. i'd be happy to accept a PR of that form. changing the basic s2i flow to remove the /tmp/src content is unlikely because for compiled languages, we want to (at least by default) include the source that compiled the artifacts in the image. |
|
Depending on how you intend doing it, In respect of the compiled languages, are you saying that the source code is compiled out of the |
I was just thinking we'd change: to: (with some dotglob magic to make sure we get dot-files) that will run into problems w/ merging directories if there is overlap, so if you want to get really fancy i suppose we could use rsync, but at some point i feel like if you're really trying to do what you're doing (provide a new s2i builder image that's built on top of the python one, but includes some framework source code) then maybe you really should be providing a custom assemble script to handle merging the user's source code into what you are providing in your new s2i builder image.
well i was thinking it was, but i'm clearly wrong, at least for wildfly: |
|
as an update, it looks like we are going to adopt the mv approach in the php image: |
Minor issue.
After the 'assemble' script does:
Nothing ever appears to remove '/tmp/src'.
This means you have two copies of the application. If the source code and data files for the application were very large, that is an unnecessary waste of space.
Is that intentional for any specific reason?
The text was updated successfully, but these errors were encountered: