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

Ensure pidfile directory exists and RUN_USER has access #20

Merged
merged 1 commit into from
May 31, 2012
Merged

Ensure pidfile directory exists and RUN_USER has access #20

merged 1 commit into from
May 31, 2012

Conversation

fpauser
Copy link

@fpauser fpauser commented May 30, 2012

As /var/run is mounted as tmpfs at least under Debian/Ubuntu.

@fpauser
Copy link
Author

fpauser commented May 30, 2012

Initial creation of the $PIDFILE directory during init-script generation is not enaugh as for example under debian/ubuntu /var/run is a tempfs meaning all information is none persistent and gone after reboot.

As long as no $RUN_USER was specified and jsvc is invoked as root-user this is not a problem because jsvc manages to create its missing $PIDFILE directory (strange enough, only when run as root).

If a $RUN_USER was specified, jsvc is started using sudo su $RUN_USER thus requiring rights to create its $PIDFILE as $RUN_USER (thats why I added the ugly chown part).

But jsvc has a -user option which makes jscv manage (create/destroy) its $PIDFILE as root user, running afterwards as $RUN_USER which is exactly what we need - but in that case jsvc does not create the missing $PIDFILE directory (maybe a jsvc-bug / missing feature?).

Anyway. My patch now ensures that the $PIDFILE directory exists and that jsvc - in case a $RUN_USER was specified - is started using the -user option instead of running it through sudo su $RUN_USER.

What do you think?

@kares
Copy link
Member

kares commented May 31, 2012

ok, you kind of convinced me with making the dir due tempfs (but without chown-ing it each and every time).
sounds like we should make sure the dir is accessible with $RUN_USER while creating it, is that ok for you ?
something like a mashup of your original code :

PIDFILE_DIR=$(dirname $PIDFILE)
if [ ! -d $PIDFILE_DIR ] ; then
  mkdir -p $PIDFILE_DIR
  [ ! -z "$RUN_USER" ] && chown -R $RUN_USER $PIDFILE_DIR
fi

unfortunately, the added jsvc -user option is kind of a step "back" - it does not work for everybody see #12.
we'd like to keep what works for all of us by default and sudo sounds like it. there was an attempt to bring back jsvc -user already at #16 and I do not mind changing it back but than we need to get the bottom of why it did not work for all previously ... (maybe it's a jsvc bug not sure)

@fpauser
Copy link
Author

fpauser commented May 31, 2012

From the jsvc docs:
[..] Jsvc is a daemon process so it should be started as root and the -user parameter allows to downgrade to an unprivilegded user. [..]

I think the -user option should really be favored instead of invoking the jsvc daemon as unprivileged user using "sudo".

The issues #12 (2012-01-20) and #16 (2012-02-22) were reported before jsvc-1.0.10 was released (2012-02-24) and could well be fixed in jsvc-1.0.10, see https://issues.apache.org/jira/browse/DAEMON-242.

Maybe you can try to reproduce #12 using jsvc-1.0.10?

My Environment:
OS: Ubuntu 10.04.4 LTS (server)
JRuby: jruby 1.6.7.2 (ruby-1.9.2-p312) (2012-05-01 26e08ba) (Java HotSpot(TM) Client VM 1.7.0_04) [linux-i386-java]

@kares
Copy link
Member

kares commented May 31, 2012

@fpauser we all know and appreciate the jsvc -user option it's just that it did not work for some of us !
I promise I'll look into it, but this pull was originally not about jsvc -user do you think some of the code is still worth pulling without changing the sudo semantics for now ? If so could you review the commits please and open another issue for changing the jsvc -user option (we'd most likely keep a documented switch for troubleshooting or attempt to check whether it really works during script generation), thanks a lot

@kares
Copy link
Member

kares commented May 31, 2012

one last thing could you maybe squash things into a single commit thus it's more consistent with what we're doing ?
if not I'll do it myself when I get to it in the evening ... thanks a lot !

kares added a commit that referenced this pull request May 31, 2012
Ensure pidfile directory exists and RUN_USER has access
@kares kares merged commit a41a527 into trinidad:master May 31, 2012
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.

None yet

2 participants