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

Issue #10696 - Addressing start-stop-daemon behaviors in jetty.sh #10700

Closed
wants to merge 8 commits into from

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Oct 10, 2023

  • moving the touch <file> tests to only execute if the JETTY_USER is unspecified. (ie: no user id change)
  • disabling internal pid-file management on start-stop-daemon, allowing Jetty's PidFile and jetty.sh manage it.
  • re-introducing the USE_START_STOP_DAEMON=1 to the jetty.sh to allow disabling use of start-stop-daemon based on configuration.

@joakime joakime added High Priority Bug For general bugs on Jetty side Build labels Oct 10, 2023
@joakime joakime added this to the 10.0.x milestone Oct 10, 2023
@joakime joakime self-assigned this Oct 10, 2023
…t, or process will switch to JETTY_USER

Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime
Copy link
Contributor Author

joakime commented Oct 10, 2023

@olamy i'd like to use testcontainers and add a set of docker testing for jetty.sh

I've been doing it manually with the docker images i have at https://github.com/joakime/jettysh-tests
But I think we should bring this into regular jetty unit/integration testing.
Got any pointers?

Ideally we would build a Docker image from our own committed Dockerfiles, and we can test via shell script or direct bash commands?

@joakime
Copy link
Contributor Author

joakime commented Oct 10, 2023

I don't think we can use the testcontainers / docker to test the linux service startup techniques (init.d or systemd).

@joakime
Copy link
Contributor Author

joakime commented Oct 10, 2023

Note: this change is for jetty-10.0.x (and jetty-11.0.x), This jetty.sh isn't 100% compatible with jetty-12.0.x.

The only difference in the jetty.sh script between jetty-10.0.x and jetty-12.0.x is the addition of the env option on --dry-run on jetty-12.0.x.
Users of Jetty 12 will have to wait for the merge of this change up to jetty-12.0.x before they can use this fixed jetty.sh

@joakime joakime marked this pull request as ready for review October 12, 2023 14:03
Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime joakime requested a review from gregw October 13, 2023 19:43
@joakime
Copy link
Contributor Author

joakime commented Oct 13, 2023

@gregw i'm happy with this version, it's been testing well in various virtual machines (so I can test service start/stop behaviors), and also basic docker setups (not our image), and even command line.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

This still fails the test I described in #10696 :

[1011] ../jetty-home/bin/jetty.sh start
ERROR : Unrecognized argument: "dir" in <command-line>

Usage: java -jar $JETTY_HOME/start.jar [options] [properties] [configs]
       java -jar $JETTY_HOME/start.jar --help  # for more information
Starting Jetty: .Unrecognized option: --start-log-file=/home/gregw/src/jetty-10.0.x/jetty-home/target/base/jetty/jetty-start.log
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
...

This diff fix I posted there still applies, but no longer fixes that test.

@gregw
Copy link
Contributor

gregw commented Oct 15, 2023

@joakime I'm reposting this comment I made in the issue here. This is still failing with your latest version.

I've found and "fixed" another pathological test case (that we should include). I created a base dir like:

[1205] tree .
.
├── dir one
│   └── test1.xml
├── dir two
│   └── test2.xml
├── etc
│   └── jetty.conf
├── jetty
├── modules
│   └── test.mod
├── resources
│   └── jetty-logging.properties
└── start.d
    ├── server.ini
    └── test.ini

modules/test.mod is

[exec]
-showversion
-XX:+PrintCommandLineFlags
-Xlog:gc*:file=/tmp/logs/gc.log:time,level,tags
-XX:ErrorFile=/tmp/logs/jvm_crash_pid_%p.log

etc/jetty.conf is

# test configurations with spaces
dir one/test1.xml
dir two

The xml files are both just:

<?xml version="1.0"?>
<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "https://www.eclipse.org/jetty/configure_10_0.dtd">

<Configure id="Server" class="org.eclipse.jetty.server.Server">
</Configure>

This died in numerous ways. The "fix" I have is

diff --git a/jetty-home/src/main/resources/bin/jetty.sh b/jetty-home/src/main/resources/bin/jetty.sh
index 9fd13d0281..c6882a543d 100755
--- a/jetty-home/src/main/resources/bin/jetty.sh
+++ b/jetty-home/src/main/resources/bin/jetty.sh
@@ -471,14 +471,14 @@ then
       do
         if [ -r "$XMLFILE" ] && [ -f "$XMLFILE" ]
         then
-          JETTY_ARGS=(${JETTY_ARGS[*]} "$XMLFILE")
+          JETTY_ARGS=(${JETTY_ARGS[*]} "'$XMLFILE'")
         else
           echo "** WARNING: Cannot read '$XMLFILE' specified in '$JETTY_CONF'"
         fi
       done
     else
       # assume it's a command line parameter (let start.jar deal with its validity)
-      JETTY_ARGS=(${JETTY_ARGS[*]} "$CONF")
+      JETTY_ARGS=(${JETTY_ARGS[*]} "'$CONF'")
     fi
   done < "$JETTY_CONF"
 fi
@@ -566,7 +566,7 @@ then
 fi
 
 # Collect the dry-run (of opts,path,main,args) from the jetty.base configuration
-JETTY_DRY_RUN=$("$JAVA" -jar "$JETTY_START" --dry-run=opts,path,main,args ${JETTY_ARGS[*]} ${JAVA_OPTIONS[*]})
+JETTY_DRY_RUN=$({ echo -jar "$JETTY_START" --dry-run=opts,path,main,args ; echo ${JETTY_ARGS[*]} ${JAVA_OPTIONS[*]} ; } | xargs "$JAVA" )
 RUN_ARGS=($JETTY_SYS_PROPS ${JETTY_DRY_RUN[@]})
 
 if (( DEBUG ))

Specifically I am adding single quotes around what I know are single arguments.
Then I'm using the xargs technique for running the dry-run command as well.

Edit: I applied this fix to the latest version and it no longer fixes it. So more work is needed.

@joakime
Copy link
Contributor Author

joakime commented Oct 16, 2023

That's a great testcase / scenario / usecase @gregw

I really need to get these into a set of docker tests (with the important configurations), i'll prioritize this over the next 2 days.

@joakime
Copy link
Contributor Author

joakime commented Oct 25, 2023

Closing in favor of PR #10753

@joakime joakime closed this Oct 25, 2023
@joakime joakime deleted the fix/10.0.x/jetty-sh-user-updates branch October 25, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Build High Priority
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

jetty.sh doesn't work with JETTY_USER in Jetty 10.0.17 thru Jetty 12.0.2
2 participants