Issue #63 Bypass start.jar

[gregw]

While this is working, I'm not sure this PR is ready to merge yet, I just wanted to create it as the basis of discussion.

This modified docker-entrypoint.sh , so that after the args have been processed, it looks to see if it is a jetty command and if so:

• If it has a terminating argument (eg --dry-run or --version), then just exec the command
• Else If a /jetty-quickstart files exists and is newer than $JETTY_BASE/start.d then set the contents of that file as the command
• Else set the command as the output of the jetty command with --dry-run appended

This approach:

• always avoid spawning a nested JVM and the main jetty process is PID 1.
• allows a pre-generated jetty-quickstart file to bypass running start.jar at all
• still supports passing in args and/or commands and acts as you would expect

Some things I'm not really happy with:

• my sh-foo is not so great, so this could perhaps be done more simply... and I'm a bit worried about any args that have spaces in them.
• listing the terminating arguments is a little fragile
• on reflection the file should be called something like /jetty-command as it is unrelated to quickstart
• perhaps we should warn if a /jetty-command exists but is older than start.d?

[gregw]

For Issue #63

[gregw]

@md5 @SchulteMarkus your thoughts?

It's a little bit more complexity than I'd like in the entrypoint, but I think it works well enough and has some good benefits.

[SchulteMarkus]

I think this is the way to go for solving the issue. There is more complexity, and I agree, it is kind of fragile (what if Jetty adds a new program-argument?), but I think, this is worth it, as many users will benefit.

[md5]

I'm not sure that the complexity here is worth the benefit, but I'm to concede that the benefit may be greater than I perceive.

From my testing, the Jetty process is already PID 1 with the default behavior of this image. There may be some overhead associated with setting up a new ClassLoader and executing the main method of the Jetty server, but that complexity is hidden from the user. Only when the --exec argument is used do I see a case where the Jetty server is a different PID than 1.

The second listed benefit is that a pre-generated script can be used to avoid start.jar entirely, but this benefit can already be realized without adding any complication to the entrypoint. The generate-jetty-start.sh script would look substantially the same either way.

The only benefit that remains is the ability to pass command line arguments to the container invocation while still retaining the ability to avoid start.jar, but I find this benefit to be dubious in the face of the complexity introduced. In my mind, if you're trying to shave milliseconds (or perhaps a second) off the startup time, it's worth having your own entrypoint as well.

That being said, I don't want to stand in the way of the Jetty team taking ownership over the direction of this image. I think the Shell scripting looks fine and appears to work as expected on Alpine as well as Debian. I agree that the terminating arguments list is fragile, but it seems like the sort of thing that would break infrequently and in hopefully obvious ways.

[gregw]

@md5 Mike,

I too am a little cautious about the approach. However, what I do like is that if we are to encourage more users to use quickstart, then we will retain a single entrypoint and a single approach. So having initially been sceptical that this was the right thing, I find myself now probably on the side that it is a good thing.

note that it is not only --exec that will cause the behaviour, but enabling any module that has system properties or jvm args also has an implicit --exec. In my work with other docker images there has definitely been confused/surprised users when enabling a module (eg Java Util Logging) caused the PID to no longer be 1.

With regards to processing command line args - I don't think that is a huge benefit other than to maintain the standard behaviour that args can be passed in. It is again about not surprising users rather than any huge technical benefit.

So I currently think this is the right way to go, However, I wont merge just yet. I'll ponder and test it a bit more just to be really sure there is not a better way. I'll comment again before taking any action either way,

cheers

[md5]

Sounds good 👍

[gregw]

@md5 I've worked on this PR a little bit to make it more robust if we are not rigorous in maintaining the list of terminating options. If fact we could almost get by without the list or at least a very minimal version of exceptional cases.

I'd like to merge this soon, so can you double check that you are still OK with it.

[md5]

Looks like some bad rebasing snuck in here? I think it would be good to squash and rebase these changes into a single commit on top of 9c03b274e981c7405b956f6f158e8694c55d0838 before merging, once we settle on the set -e thing.

[gregw]

@md5 The reference to #53 was just me having finger troubles typing commit comments. So I repeated all the comments in my final commit.

I'm happy with retaining the -e and only doing a log and exit 0 for additional stdout lines. The branch is like that now, so I'm good to squash/merge and correct the comments.

I'll give it another 24h before doing so, so you can comment if there is something I missed.

[md5]

Looks good to me 👍

[Phocea]

Hello,

Do you know when this merge is going to also happen on the Alpine version please ?

Thanks

[gregw]

@Phocea this PR did leave out the generate script from the alpine Dockerfile, but it has subsequently been fixed and the master branch is up to date.

I will look at updating the official images as soon as I get an opportunity.

[Phocea]

Great. Thanks for the update

[gregw]

official images update has been merged.