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

Run post build hook with /bin/sh -ic #7379

Merged
merged 1 commit into from
Feb 23, 2016

Conversation

rhcarvalho
Copy link
Contributor

We need either /bin/bash -c or /bin/sh -ic to make our supported SCL-powered images to work properly. That's due to a hack to auto-enable SCLs:
https://github.com/openshift/sti-base/blob/8d95148/Dockerfile#L23-L29

In CentOS and RHEL images, /bin/sh is a symlink to /bin/bash. In that case, the ENV environment variable is only evaluated when the shell is started in interactive mode, which happens when there's and attached tty (not the case for post build hook) or when the -i argument is passed explicitly.

Making the shell interactive with -i is ugly, but is the only solution we know so far without requiring Bash.

@rhcarvalho
Copy link
Contributor Author

@bparees @smarterclayton @mfojtik PTAL

[test][extended:core(build without output image)]

@@ -145,6 +145,9 @@
"name": "origin-ruby-sample:latest"
}
},
"postCommit": {
"script": "bundle exec rake test"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should omit this example, as the hook is not run by default for custom builds, @bparees wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, unless we're going to update the origin-custom-docker-builder image to actually respect/invoke the hook, we probably shouldn't define it since it implies behavior that isn't present.

@smarterclayton
Copy link
Contributor

Um, the problem you described is a problem with the sti-base image, and isn't a reason to switch to /bin/bash. By convention people call /bin/sh - so we need to as well.

@rhcarvalho
Copy link
Contributor Author

We started with /bin/bash and just later with #6715 under review we made the decision to use /bin/sh, and I was the one pushing for that. I failed to realize our images didn't work with /bin/sh -c.
My bad.

We've already spent a lot of effort trying to make SCL packages work in our favor, nothing much left to squeeze (#2001 was my original complaint on SCL-powered images not working out-of-the-box).

The changes to the hook that would make our images work are the two I mentioned:

  • /bin/bash -c; or
  • /bin/sh -ic (notice the extra "i" for interactive mode)

The latter seems clumsy.
Changing sti-base would not help, as we already do all trickery known-to-man to make SCL work as out-of-the-box as possible.

I did some grepping here and there and found that some actions in OpenShift use sh, some bash.
In particular, oc rsh uses Bash.

@mfojtik
Copy link
Contributor

mfojtik commented Feb 17, 2016

@smarterclayton the pure /bin/sh wont work (never worked without -i) because as @rhcarvalho described already, there is no way to enable SCL using that restricted shell (and we complained to SCL many times about that...).

For post-build, I think it is OK to use "/bin/bash" as default (when user does not specify something else, because user can do that). That way, we enable our own images to work, without breaking other folks too much :)

@rhcarvalho
Copy link
Contributor Author

When I argued for /bin/sh, I wanted to make script "just work" with the widest range of images possible, including minimal images based on busybox, alpine, etc. Just unfortunately that rules out working with our own CentOS/RHEL images.

In practice, images built on CentOS, RHEL, Ubuntu, etc, will all work the same. For the minimalist images, they'd need to go for the Command/Args syntax. Really minimal images (without /bin/sh) would need that anyway.

@mfojtik
Copy link
Contributor

mfojtik commented Feb 17, 2016

@rhcarvalho I think that as long as there is a way to allow minimal images to roll (by updating Args/Command), we're fine.

@bparees
Copy link
Contributor

bparees commented Feb 17, 2016

@rhcarvalho what are the negatives associated with /bin/sh -ic? I agree with you that using /bin/sh seems preferable for more universal support (though as noted people can always specific /bin/sh if their image doesn't have bash)

@rhcarvalho
Copy link
Contributor Author

what are the negatives associated with /bin/sh -ic?

As we discussed on IRC, mainly that the script (and whatever it may invoke) would be thinking it is running in an interactive shell, and might behave differently than if the shell was non-interactive.

@bparees
Copy link
Contributor

bparees commented Feb 17, 2016

lgtm but will need a docs pull I believe also?

@rhcarvalho
Copy link
Contributor Author

For the record, all tests passed. I'm removing the example from examples/sample-app/application-template-custombuild.json as per discussion. Too late?

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5070/) (Image: devenv-rhel7_3512)

@rhcarvalho
Copy link
Contributor Author

will need a docs pull I believe also?

No, the original docs has not merged yet. I've already synced with @PI-Victor this morning to include this update.

@smarterclayton
Copy link
Contributor

Merge removed, still not comfortable with this.

@smarterclayton
Copy link
Contributor

So the information I need to hear from you guys is:

  1. Explain clearly why we have to use bash for SCL (why a symlink from /bin/sh to /bin/bash or to another binary in s2i-base won't work), and what is special about the name /bin/sh vs /bin/bash
  2. Identify which images in the ecosystem might potentially not include /bin/bash - I want actual quantifiable evidence, including the following: Alpine, Ubuntu, Busybox, Centos 6, Centos 7, Fedora images, and any other "popular" base image.

@rhcarvalho
Copy link
Contributor Author

@smarterclayton

  1. In most distros /bin/sh is a symlink to /bin/bash. As per the Bash man page, several things happen when Bash is invoked with the name "sh", enabling a compatibility mode. When we were saying and running /bin/sh it means that for Fedora, CentOS, RHEL and Ubuntu images we're in fact running Bash in this compatibility mode. In Busybox and Alpine we're running the actual non-Bash sh. The choice of sh or bash wouldn't matter that much if it wasn't for...

    Now on to SCL. SCL packages are installed at custom paths not to conflict with system packages managed by yum. By design, they require the user to "enable" a collection/package every time its use in intended. One of the ways to do it is a line like source scl_source enable ruby200 ror40 nodejs010.

    We don't want to force our users to have to know that their assemble and run scripts or Dockerfiles need to worry about SCL every time. We don't want users to run oc exec or oc rsh and have to enable certain collections before then can start interacting with their apps, use a ruby console, etc.

    The most clever "solution" we have so far to pretend that certain collections are always enabled is to use three env vars that try to cover for most scenarios: ENV, BASH_ENV and PROMPT. @mfojtik or myself can give more details about this approach if needed.

    BASH_ENV is loaded by /bin/bash -c, thus enabling our collections, while the man page prose made me feel dubious about the correct behavior of ENV. Bug or intended behavior, it is only loaded when the shell is interactive (the -i flag) and the base name of the path is sh. Talking with other people in the office, we came to some memories about limitations @mfojtik has run into when first adding the trick. Before that things were just worse.

    As I mentioned above, we chose to use /bin/bash for oc rsh, but there was likely really no choice, since if it was using /bin/sh we'd have run into the same problem: oc rsh would leave users in a shell without "ruby" in the PATH in their "ruby image"...

  2. I don't really know where to start if we want statistical data like that, unless taking the time to scrape Docker Hub...

    AFAIK all the major distros come with Bash. This is true for Fedora, CentOS, RHEL, Ubuntu. Also sure that Busybox and Alpine do not include /bin/bash in their base image, both only have Busybox's /bin/sh implementation.

Notice that I changed the tests to use centos:7 instead of busybox:1. That's really what would be happening, images based on busybox/alpine, unless patched to include bash, will only be able to use the Command+Args form, and not the Script form. Images build from Scratch also can use Command+Args and not Script (unless again the image happen to have /bin/bash...)

@smarterclayton
Copy link
Contributor

So the impact of this change is that Alpine and Busybox images (which are
what Docker pushes for base images) will be straight up broken with oc rsh,
this behavior, and any future behavior we add to /bin/bash? That's a
pretty serious problem - it means that the community is being pushed to
develop images one way, but we develop another way. It also means X
percentage of images won't work with these features ootb. That's a pretty
terrible default.

POSIX compatibility mode is not significantly different than regular bash,
but will break scripts in certain ways. What is the expectation of the
user base? Bash, or POSIX compat?

I think I need more detail on why -i is dangerous.

On Wed, Feb 17, 2016 at 12:51 PM, Rodolfo Carvalho <notifications@github.com

wrote:

@smarterclayton https://github.com/smarterclayton

In most distros /bin/sh is a symlink to /bin/bash. As per the Bash man
page, several things happen when Bash is invoked with the name "sh",
enabling a compatibility mode. When we were saying and running /bin/sh
it means that for Fedora, CentOS, RHEL and Ubuntu images we're in fact
running Bash in this compatibility mode. In Busybox and Alpine we're
running the actual non-Bash sh. The choice of sh or bash wouldn't
matter that much if it wasn't for...

Now on to SCL. SCL packages are installed at custom paths not to
conflict with system packages managed by yum. By design, they require the
user to "enable" a collection/package every time its use in intended. One
of the ways to do it is a line like source scl_source enable ruby200
ror40 nodejs010.

We don't want to force our users to have to know that their assemble
and run scripts or Dockerfiles need to worry about SCL every time. We don't
want users to run oc exec or oc rsh and have to enable certain
collections before then can start interacting with their apps, use a ruby
console, etc.

The most clever "solution" we have so far to pretend that certain
collections are always enabled is to use three env vars that try to cover
for most scenarios: ENV, BASH_ENV and PROMPT. @mfojtik
https://github.com/mfojtik or myself can give more details about
this approach if needed.

BASH_ENV is loaded by /bin/bash -c, thus enabling our collections,
while the man page prose made me feel dubious about the correct behavior of
ENV. Bug or intended behavior, it is only loaded when the shell is
interactive (the -i flag) and the base name of the path is sh. Talking
with other people in the office, we came to some memories about limitations
@mfojtik https://github.com/mfojtik has run into when first adding
the trick. Before that things were just worse.

As I mentioned above, we chose to use /bin/bash for oc rsh, but there
was likely really no choice, since if it was using /bin/sh we'd have
run into the same problem: oc rsh would leave users in a shell without
"ruby" in the PATH in their "ruby image"...
2.

I don't really know where to start if we want statistical data like
that, unless taking the time to scrape Docker Hub...

AFAIK all the major distros come with Bash. This is true for Fedora,
CentOS, RHEL, Ubuntu. Also sure that Busybox and Alpine do not include
/bin/bash in their base image, both only have Busybox's /bin/sh
implementation.

Notice that I changed the tests to use centos:7 instead of busybox:1.
That's really what would be happening, images based on busybox/alpine,
unless patched to include bash, will only be able to use the Command+Args
form, and not the Script form. Images build from Scratch also can use
Command+Args and not Script (unless again the image happen to have
/bin/bash...)


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

@mfojtik
Copy link
Contributor

mfojtik commented Feb 17, 2016

On Feb 17, 2016 19:21, "Clayton Coleman" notifications@github.com wrote:

So the impact of this change is that Alpine and Busybox images (which are
what Docker pushes for base images) will be straight up broken with oc
rsh,
this behavior, and any future behavior we add to /bin/bash? That's a

Alpine does not use SCL. In worst case we can default to /bin/sh -ic

pretty serious problem - it means that the community is being pushed to
develop images one way, but we develop another way. It also means X
percentage of images won't work with these features ootb. That's a pretty
terrible default.

POSIX compatibility mode is not significantly different than regular bash,
but will break scripts in certain ways. What is the expectation of the
user base? Bash, or POSIX compat?

I think I need more detail on why -i is dangerous.

On Wed, Feb 17, 2016 at 12:51 PM, Rodolfo Carvalho <
notifications@github.com

wrote:

@smarterclayton https://github.com/smarterclayton

In most distros /bin/sh is a symlink to /bin/bash. As per the Bash man
page, several things happen when Bash is invoked with the name "sh",
enabling a compatibility mode. When we were saying and running /bin/sh
it means that for Fedora, CentOS, RHEL and Ubuntu images we're in fact
running Bash in this compatibility mode. In Busybox and Alpine we're
running the actual non-Bash sh. The choice of sh or bash wouldn't
matter that much if it wasn't for...

Now on to SCL. SCL packages are installed at custom paths not to
conflict with system packages managed by yum. By design, they require
the
user to "enable" a collection/package every time its use in intended.
One
of the ways to do it is a line like source scl_source enable ruby200
ror40 nodejs010.

We don't want to force our users to have to know that their assemble
and run scripts or Dockerfiles need to worry about SCL every time. We
don't
want users to run oc exec or oc rsh and have to enable certain
collections before then can start interacting with their apps, use a
ruby
console, etc.

The most clever "solution" we have so far to pretend that certain
collections are always enabled is to use three env vars that try to
cover
for most scenarios: ENV, BASH_ENV and PROMPT. @mfojtik
https://github.com/mfojtik or myself can give more details about

this approach if needed.

BASH_ENV is loaded by /bin/bash -c, thus enabling our collections,
while the man page prose made me feel dubious about the correct
behavior of
ENV. Bug or intended behavior, it is only loaded when the shell is
interactive (the -i flag) and the base name of the path is sh. Talking
with other people in the office, we came to some memories about
limitations
@mfojtik https://github.com/mfojtik has run into when first adding

the trick. Before that things were just worse.

As I mentioned above, we chose to use /bin/bash for oc rsh, but there
was likely really no choice, since if it was using /bin/sh we'd have
run into the same problem: oc rsh would leave users in a shell without
"ruby" in the PATH in their "ruby image"...
2.

I don't really know where to start if we want statistical data like
that, unless taking the time to scrape Docker Hub...

AFAIK all the major distros come with Bash. This is true for Fedora,
CentOS, RHEL, Ubuntu. Also sure that Busybox and Alpine do not include
/bin/bash in their base image, both only have Busybox's /bin/sh
implementation.

Notice that I changed the tests to use centos:7 instead of busybox:1.
That's really what would be happening, images based on busybox/alpine,
unless patched to include bash, will only be able to use the
Command+Args
form, and not the Script form. Images build from Scratch also can use
Command+Args and not Script (unless again the image happen to have
/bin/bash...)


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


Reply to this email directly or view it on GitHub.

@rhcarvalho
Copy link
Contributor Author

So the impact of this change is that Alpine and Busybox images (which are
what Docker pushes for base images) will be straight up broken with oc rsh,

This PR is not changing oc rsh.

I think I need more detail on why -i is dangerous.

I can't say "-i is dangerous".

Either using /bin/bash -c or /bin/sh -ic would get SCL-based images working ootb.
(What I would rather prefer is to forget "scl enable" ever existed for containers)

The former seemed more reasonable than the latter, but I don't have strong options for any.

What we discussed so far today, "-i" is at least running scripts in an "unexpected way" from the script's point of view, since normally scripts are not run in interactive mode, and there might be some code somewhere that checks whether the shell is interactive or not to do something different.

@smarterclayton
Copy link
Contributor

Can you quantify that concern?

How are we going to fix SCL in the future? I think this needs to be fixed
in SCL, so who owns pushing that and making that solved?

On Wed, Feb 17, 2016 at 1:46 PM, Rodolfo Carvalho notifications@github.com
wrote:

So the impact of this change is that Alpine and Busybox images (which are
what Docker pushes for base images) will be straight up broken with oc rsh,

This PR is not changing oc rsh.

I think I need more detail on why -i is dangerous.

I can't say "-i is dangerous".

Either using /bin/bash -c or /bin/sh -ic would get SCL-based images
working ootb.
(What I would rather prefer is to forget "scl enable" ever existed for
containers)

The former seemed more reasonable than the latter, but I don't have strong
options for any.

What we discussed so far today, "-i" is at least running scripts in an
"unexpected way" from the script's point of view, since normally scripts
are not run in interactive mode, and there might be some code somewhere
that checks whether the shell is interactive or not to do something
different.


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

@rhcarvalho
Copy link
Contributor Author

@hhorak ^^ can you clarify the situation with SCL and requirement for enabling collections before using them?

@bparees
Copy link
Contributor

bparees commented Feb 17, 2016

How are we going to fix SCL in the future? I think this needs to be fixed in SCL, so who owns pushing that and making that solved?

i don't see that this is a "problem" in SCL, the whole point of SCL is that i should be able to install multiple versions and pick which one i want to use. That necessitates a mechanism for setting up my paths correctly based on the version i want to use.

The challenge for us (as image authors) is having a way to ensure those paths are always setup correctly.

Depending what exactly SCL enable is doing, could we perhaps just update the appropriate env vars in the image as the default values? (does scl enable do anything other than setup env vars?)

@rhcarvalho
Copy link
Contributor Author

@smarterclayton please note that using Bash for the Script form is not preventing Busybox and Alpine users from using build hooks. We baked in full control to the image entrypoint and cmd, and the Script is just a shortcut for the common cases.

Let's consider an Alpine-based golang image with the Go toolchain and some source code. The API allows for so many possibilities that we could easily run our tests with either:

spec:
  postCommit:
    Command: ["go test"]

or

spec:
  postCommit:
    Args: ["go test"]

@smarterclayton
Copy link
Contributor

The choice of default shell is a foundational value that will guide
behavior of the system for years to come. It cannot be changed after the
fact. So this decision requires a very clear set of choices and tradeoffs,
and cannot be rushed. This is part of the definition of the execution
environment of the container cluster itself.

On Wed, Feb 17, 2016 at 2:57 PM, Rodolfo Carvalho notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton please note that
using Bash for the Script form is not preventing Busybox and Alpine users
from using build hooks. We baked in full control to the image entrypoint
and cmd, and the Script is just a shortcut for the common cases.

Let's consider an Alpine-based golang image with the Go toolchain and some
source code. The API allows for so many possibilities that we could easily
run our tests with either:

spec:
postCommit:
Command: ["go test"]

or

spec:
postCommit:
Args: ["go test"]


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

@bparees
Copy link
Contributor

bparees commented Feb 19, 2016

@smarterclayton @rhcarvalho given that oc rsh already presumes /bin/bash, i'm inclined to say we do the same thing here.

@smarterclayton do you consider it a bug that oc rsh presumes /bin/bash? If so, then obviously we would not want to presume /bin/bash here either, in which case we're stuck with /bin/sh -i and whatever ramifications that has for users (who can always set the entrypoint explicitly if that is problem)

@smarterclayton
Copy link
Contributor

Yes, I consider oc rsh depending on bin/bash a bug. Do we know what
ramifications /bin/sh -i has in practical use?

On Fri, Feb 19, 2016 at 10:55 AM, Ben Parees notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton @rhcarvalho
https://github.com/rhcarvalho given that oc rsh already presumes
/bin/bash, i'm inclined to say we do the same thing here.

@smarterclayton https://github.com/smarterclayton do you consider it a
bug that oc rsh presumes /bin/bash? If so, then obviously we would not
want to presume /bin/bash here either, in which case we're stuck with /bin/sh
-i and whatever ramifications that has for users (who can always set the
entrypoint explicitly if that is problem)


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

@bparees
Copy link
Contributor

bparees commented Feb 19, 2016

@smarterclayton we aren't aware of any, the only thing we have right now is theoretical speculation that certain scripts/programs might behave differently(and incorrectly) when they think they are running in an interactive shell. eg attempting to prompt the user for information.

@smarterclayton
Copy link
Contributor

Can we do some research and prove it?

On Fri, Feb 19, 2016 at 12:06 PM, Ben Parees notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton we aren't aware of
any, the only thing we have right now is theoretical speculation that
certain scripts/programs might behave differently(and incorrectly) when
they think they are running in an interactive shell. eg attempting to
prompt the user for information.


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

@bparees
Copy link
Contributor

bparees commented Feb 19, 2016

Can we do some research and prove it?

it's trivial to prove the problem is possible, we can write a script today that exhibits that behavior.

Determining whether or not it's commonplace is pretty hard/impossible. (we might be able to find examples that prove it is a problem, we'll never be able to prove it's not).

@smarterclayton
Copy link
Contributor

Is it a problem on our images. Has anyone opened an issue on this for us
recently. Do a representative swath of the docker hub images exhibit
problems with this? Can we get an expert in bash to say how common this
is? Can we do a google search or check stack overflow and see how often
people hit it?

Need data to make the right decision.

On Fri, Feb 19, 2016 at 1:23 PM, Ben Parees notifications@github.com
wrote:

Can we do some research and prove it?

it's trivial to prove the problem is possible, we can write a script today
that exhibits that behavior.

Determining whether or not it's commonplace is pretty hard/impossible. (we
might be able to find examples that prove it is a problem, we'll never be
able to prove it's not).


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

@bparees
Copy link
Contributor

bparees commented Feb 19, 2016

@smarterclayton it's not a matter of it being a problem for a given image, it's not. It all depends what you are trying to run under a "/bin/sh -i" environment. That thing may or may not be broken. In all likelihood, it won't be.

if it is broken, the user would have to switch to "/bin/sh" at which point scl enablement is broken, so then they'd have to switch to "/bin/bash" if they also need the SCL content enabled.

This really just comes down to two distinct options:

  1. use /bin/bash and force users to override if their image doesn't have it
  2. use "/bin/sh -i" and force users to override it if the thing they want to run can't tolerate an interactive shell.

So if you don't like (1), let's just do (2) and move on, with a longer term term of getting SCL to provide a way to explicitly enable within an image, so we don't need to do either of these things.

@smarterclayton
Copy link
Contributor

Is /bin/sh -i going to break alpine / busy box containers that have .bashrc
or .bash_profile? How many of those exist.

On Fri, Feb 19, 2016 at 1:47 PM, Ben Parees notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton it's not a matter of
it being a problem for a given image, it's not. It all depends what you are
trying to run under a "/bin/sh -i" environment. That thing may or may not
be broken. In all likelihood, it won't be.

if it is broken, the user would have to switch to "/bin/sh" at which point
scl enablement is broken, so then they'd have to switch to "/bin/bash" if
they also need the SCL content enabled.

This really just comes down to two distinct options:

  1. use /bin/bash and force users to override if their image doesn't have it
  2. use "/bin/sh -i" and force users to override it if the thing they want
    to run can't tolerate an interactive shell.

So if you don't like (1), let's just do (2) and move on, with a longer
term term of getting SCL to provide a way to explicitly enable within an
image, so we don't need to do either of these things.


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

@bparees
Copy link
Contributor

bparees commented Feb 19, 2016

why would an alpline or busybox container have a .bashrc when it doesn't have bash?

regardless:
"According to the bash man page, .bash_profile is executed for login shells, while .bashrc is executed for interactive non-login shells."

so .bash_profile isn't going to be run for any of these cases (/bin/bash or /bin/sh -i), and .bashrc would be run for "/bin/sh -i" but not for "/bin/bash".

@bparees
Copy link
Contributor

bparees commented Feb 19, 2016

ubuntu based image:

$ docker run  --entrypoint="/bin/sh" cloudgear/ruby:2.2 "-ic" "ruby --version"
/bin/sh: 0: can't access tty; job control turned off
ruby 2.2.0p0 (2014-12-25 revision 49005) [x86_64-linux-gnu]

alpine:

$ docker run  --entrypoint="/bin/sh" frolvlad/alpine-ruby "-ic" "ruby --version"
/bin/sh: can't access tty; job control turned off
ruby 2.2.4p230 (2015-12-16 revision 53155) [x86_64-linux-musl]

docker library images:

$ docker run  --entrypoint="/bin/sh" library/ruby "-ic" "ruby --version"
/bin/sh: 0: can't access tty; job control turned off
ruby 2.3.0p0 (2015-12-25 revision 53290) [x86_64-linux]
$ docker run  --entrypoint="/bin/sh" library/node "-ic" "node --version"
/bin/sh: 0: can't access tty; job control turned off
v5.6.0

our image:

$ docker run  --entrypoint="/bin/sh" centos/ruby-22-centos7 "-ic" "ruby --version"
sh: cannot set terminal process group (-1): Inappropriate ioctl for device
sh: no job control in this shell
ruby 2.2.2p95 (2015-04-13 revision 50295) [x86_64-linux]

Basically no shell binary is happy about running with "-i" when there's not actually a terminal, but they all seem to tolerate it.

@bparees
Copy link
Contributor

bparees commented Feb 19, 2016

Incidentally:
http://unix.stackexchange.com/questions/4921/sh-startup-files-over-ssh/4927#4927

we do set ENV, just like we set BASH_ENV, so i don't know why this is not working with /bin/sh for our SCL images. It sounds like ENV should be exactly what we need to work with "/bin/sh" and get scl enabled.

(this may be what @rhcarvalho was discussing earlier, that ENV is only processed for interactive shells)

@mfojtik
Copy link
Contributor

mfojtik commented Feb 19, 2016

I do remember there was a problem with ENV, to make it work you need -i :(
/bin/sh has very limited options to inject the scl vars...
On Feb 19, 2016 21:05, "Ben Parees" notifications@github.com wrote:

Incidentally:

http://unix.stackexchange.com/questions/4921/sh-startup-files-over-ssh/4927#4927

we do set ENV, just like we set BASH_ENV, so i don't know why this is not
working with /bin/sh for our SCL images. It sounds like ENV should be
exactly what we need to work with "/bin/sh" and get scl enabled.


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

@mfojtik
Copy link
Contributor

mfojtik commented Feb 19, 2016

A non-interactive shell invoked with the name sh does not attempt to read
any other startup files.

(ignoring ENV as well, which is why you have to pass -i). I think oc rsh is
indeed interactive shell, so adding -i makes sense there.
On Feb 19, 2016 21:57, "Michal Fojtik" mi@mifo.sk wrote:

I do remember there was a problem with ENV, to make it work you need -i :(
/bin/sh has very limited options to inject the scl vars...
On Feb 19, 2016 21:05, "Ben Parees" notifications@github.com wrote:

Incidentally:

http://unix.stackexchange.com/questions/4921/sh-startup-files-over-ssh/4927#4927

we do set ENV, just like we set BASH_ENV, so i don't know why this is not
working with /bin/sh for our SCL images. It sounds like ENV should be
exactly what we need to work with "/bin/sh" and get scl enabled.


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

@mfojtik
Copy link
Contributor

mfojtik commented Feb 19, 2016

and for the record, I consider using ENV as pretty ugly hack. however I
dont see any better option other than making "scl" tooling docker aware.
On Feb 19, 2016 22:11, "Michal Fojtik" mi@mifo.sk wrote:

A non-interactive shell invoked with the name sh does not attempt to
read any other startup files.

(ignoring ENV as well, which is why you have to pass -i). I think oc rsh
is indeed interactive shell, so adding -i makes sense there.
On Feb 19, 2016 21:57, "Michal Fojtik" mi@mifo.sk wrote:

I do remember there was a problem with ENV, to make it work you need -i
:( /bin/sh has very limited options to inject the scl vars...
On Feb 19, 2016 21:05, "Ben Parees" notifications@github.com wrote:

Incidentally:

http://unix.stackexchange.com/questions/4921/sh-startup-files-over-ssh/4927#4927

we do set ENV, just like we set BASH_ENV, so i don't know why this is
not working with /bin/sh for our SCL images. It sounds like ENV should be
exactly what we need to work with "/bin/sh" and get scl enabled.


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

@rhcarvalho
Copy link
Contributor Author

I've updated this to use /bin/sh -ic instead. Testing before I push the changes.

We need either `/bin/bash -c` or `/bin/sh -ic` to make our supported
SCL-powered images to work properly. That's due to a hack to auto-enable
SCLs:
https://github.com/openshift/sti-base/blob/8d95148/Dockerfile#L23-L29

In CentOS and RHEL images, `/bin/sh` is a symlink to `/bin/bash`. In
that case, the ENV environment variable is only evaluated when the shell
is started in interactive mode, which happens when there's and attached
tty (not the case for post build hook) or when the `-i` argument is
passed explicitly.

Making the shell interactive with `-i` is ugly, but is the only solution
we know so far without requiring Bash.
@rhcarvalho
Copy link
Contributor Author

Build logs of sample-app with S2I build:

...
Your bundle is complete!
It was installed into ./bundle
---> Cleaning up unused ruby gems ...
I0222 16:54:35.617087       1 common.go:134] Running post commit hook with image demo/ruby-sample-build-1:27879944 ...
/opt/rh/rh-ruby22/root/usr/bin/ruby -I"lib" -I"/opt/app-root/src/bundle/ruby/gems/rake-10.3.2/lib" "/opt/app-root/src/bundle/ruby/gems/rake-10.3.2/lib/rake/rake_test_loader.rb" "test/*_test.rb" 
Run options: --seed 60816
# Running:
.
Finished in 0.003172s, 315.2892 runs/s, 315.2892 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips
I0222 16:54:39.885720       1 sti.go:243] Using provided push secret for pushing 172.30.33.230:5000/demo/origin-ruby-sample:latest image
I0222 16:54:39.885756       1 sti.go:247] Pushing 172.30.33.230:5000/demo/origin-ruby-sample:latest image ...
I0222 16:55:38.673326       1 sti.go:263] Successfully pushed 172.30.33.230:5000/demo/origin-ruby-sample:latest

@bparees FYI no funny messages like you observed with docker run, good for us 🎉

@rhcarvalho rhcarvalho changed the title Run post build hook with /bin/bash Run post build hook with /bin/sh -ic Feb 22, 2016
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 46e7e91

@rhcarvalho
Copy link
Contributor Author

@bparees @smarterclayton I've updated the PR title/description/code to use /bin/sh -ic, waiting for Jenkins results. PTAL.

No problems running bundle exec rake test with our sample-app. Will check other languages/images.

@rhcarvalho
Copy link
Contributor Author

Also works for the Python image (django-ex):

...
75 static files copied to '/opt/app-root/src/staticfiles'.
I0222 17:39:37.933959       1 common.go:134] Running post commit hook with image demo/django-example-1:2fdae629 ...
...
----------------------------------------------------------------------
Ran 3 tests in 0.024s
OK
Creating test database for alias 'default'...
Destroying test database for alias 'default'...
I0222 17:39:42.540090       1 sti.go:243] Using provided push secret for pushing 172.30.33.230:5000/demo/django-example:latest image
I0222 17:39:42.540511       1 sti.go:247] Pushing 172.30.33.230:5000/demo/django-example:latest image ...
I0222 17:41:02.814566       1 sti.go:263] Successfully pushed 172.30.33.230:5000/demo/django-example:latest

@bparees
Copy link
Contributor

bparees commented Feb 22, 2016

Thanks @rhcarvalho, lgtm.

@smarterclayton are you ok with this or do you want it marked experimental?

@mfojtik
Copy link
Contributor

mfojtik commented Feb 22, 2016

lgtm
On Feb 22, 2016 19:13, "Ben Parees" notifications@github.com wrote:

Thanks @rhcarvalho https://github.com/rhcarvalho, lgtm.

@smarterclayton https://github.com/smarterclayton are you ok with this
or do you want it marked experimental?


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

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/1482/) (Extended Tests: core(build without output image))

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 22, 2016 via email

@bparees
Copy link
Contributor

bparees commented Feb 22, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 46e7e91

openshift-bot pushed a commit that referenced this pull request Feb 23, 2016
@openshift-bot openshift-bot merged commit 2479b96 into openshift:master Feb 23, 2016
@rhcarvalho rhcarvalho deleted the build-post-hook-bash branch February 23, 2016 09:57
@rhcarvalho
Copy link
Contributor Author

@richm as I mentioned in IRC, seems that you're using the newest version of the template (master), with an older version of openshift (which happens to be the latest release).

The solution, if anyone else runs into the same problem, is to use a version of the template that is not master, but the same tag as the version of the openshift binary you're using. For v1.1.3 that would be:

$ oc new-app https://github.com/raw/openshift/origin/v1.1.3/examples/sample-app/application-template-stibuild.json

I'll update the sample-app README to include a note about that.

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

Successfully merging this pull request may close these issues.

6 participants