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

systemd: Fix -Dpid arg passing to systemd usage service #3210

Merged
merged 3 commits into from Mar 14, 2019

Conversation

Projects
None yet
5 participants
@rhtyd
Copy link
Member

commented Mar 12, 2019

This fixes regression introduced by refactoring PR #3163 where -Dpid
was incorrectly passed string $$ instead of parent PID integer.

Fixes #3203

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
systemd: Fix -Dpid arg passing to systemd usage service
This fixes regression introduced by refactoring PR #3163 where `-Dpid`
was incorrectly passed string `$$` instead of parent PID integer.

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

@rhtyd rhtyd added this to the 4.12.0.0 milestone Mar 12, 2019

@rhtyd

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@rhtyd

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@blueorangutan package

@rhtyd rhtyd referenced this pull request Mar 12, 2019

Closed

Fix #3203 usage server broken in 4.11/4.12 #3207

1 of 5 tasks complete
@blueorangutan

This comment has been minimized.

Copy link

commented Mar 12, 2019

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

This comment has been minimized.

Copy link

commented Mar 12, 2019

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2627

@rhtyd

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@blueorangutan

This comment has been minimized.

Copy link

commented Mar 12, 2019

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@GabrielBrascher
Copy link
Member

left a comment

@rhtyd when debugging it I get the PID as a string "$JAVA_PID". With the silenced exception java.lang.NumberFormatException: For input string: "$JAVA_PID"

The solution on PR #3207 is not pretty, I agree with you. However, the systemd unit process the Environment as a string, not a script. Thus, had to execute a script with the ExecStartPost to provide a workaround on the PID.

@@ -24,7 +24,8 @@ After=network.target network-online.target
[Service]
Type=simple
EnvironmentFile=/etc/default/cloudstack-usage
ExecStart=/usr/bin/java $JAVA_OPTS -cp $CLASSPATH $JAVA_CLASS
Environment=JAVA_PID=$$

This comment has been minimized.

Copy link
@rhtyd

rhtyd Mar 12, 2019

Author Member

@GabrielBrascher @andrijapanic moved the JAVA_PID here so users can't change/mess it around. The systemd variable referencing is limiting, the fix was to use the ${VAR} syntax and not $VAR syntax. Can you believe it! https://www.freedesktop.org/software/systemd/man/systemd.service.html#Command%20lines

@rhtyd

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@GabrielBrascher fixed the issue, it was systemd variable referencing/syntax issue which per following docs should be ${VAR} instead of $VAR:
https://www.freedesktop.org/software/systemd/man/systemd.service.html#Command%20lines

After my last commit, it's working again.

@rhtyd

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@blueorangutan package

@blueorangutan

This comment has been minimized.

Copy link

commented Mar 12, 2019

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

This comment has been minimized.

Copy link

commented Mar 12, 2019

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2628

@andrijapanicsb

This comment has been minimized.

Copy link

commented Mar 12, 2019

LGTM

Tested and the job does kick in as expected.

@GabrielBrascher

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@rhtyd @andrijapanic are you testing it with CentOS? On Ubuntu I still get error. Now the exception is: java.lang.NumberFormatException: For input string: "$$"

@andrijapanicsb

This comment has been minimized.

Copy link

commented Mar 12, 2019

yes, Centos7
let me build Ubuntu lab - 18.04 sounds fine ?

@GabrielBrascher

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@rhtyd @andrijapanic nevermind, my mistake when updating the script at this time. Due to the /bin/sh -ec '' it works now. Thanks!

@andrijapanicsb

This comment has been minimized.

Copy link

commented Mar 12, 2019

Cool - let's LGTM this one and close if you are OK with it @GabrielBrascher ?

@GabrielBrascher

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

@rhtyd it looks good. Thanks!

Can you please also remove the lines that are silencing the exception? I don't see any reason for print on the stack trace (console) the exceptions.

} catch (Exception e) {
    e.printStackTrace();
}
@blueorangutan

This comment has been minimized.

Copy link

commented Mar 12, 2019

Trillian test result (tid-3418)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 22699 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3210-t3418-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Smoke tests completed. 68 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
@rhtyd

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@GabrielBrascher okay I'll move that to logging tomorrow.
@blueorangutan test

@blueorangutan

This comment has been minimized.

Copy link

commented Mar 12, 2019

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan

This comment has been minimized.

Copy link

commented Mar 13, 2019

Trillian test result (tid-3419)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 23399 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3210-t3419-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_outofbandmanagement.py
Intermittent failure detected: /marvin/tests/smoke/test_host_maintenance.py
Smoke tests completed. 68 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
@ustcweizhou

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

tested ok in 4.11.2
LGTM

@GabrielBrascher

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

All checks have passed (Travis, Jenkins, Trillian) passed and we have 3 LGTMs. I also tested and it worked. @rhtyd this PR is looking good.

@rhtyd just to keep track on the exception silenced; will you be able to add a commit addressing that exception silenced? Adding the JAVA_DEBUG is great, fixing that exception is also a great plus to help users on debugging.

Thanks for the work!

@rhtyd

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

@GabrielBrascher sorry forgot about that amidst dayjob work. I can do that tomorrow, or if you'd prefer you nay push your changes to this PR's branch.

@GabrielBrascher

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

No problem @rhtyd, thanks!

@rhtyd

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

@GabrielBrascher GabrielBrascher merged commit f7327c7 into apache:4.11 Mar 14, 2019

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GabrielBrascher

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Thanks, @rhtyd! I am merging this PR.

@rhtyd

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

@GabrielBrascher, I've forward merged it to master now, in case you want to cut the RC or address another issue. Thanks.

@GabrielBrascher

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Time to spin out RC5 then, thanks!

gmueller-ewerk added a commit to gmueller-ewerk/cloudstack that referenced this pull request Mar 25, 2019

systemd: Fix -Dpid arg passing to systemd usage service (apache#3210)
* systemd: Fix -Dpid arg passing to systemd usage service

This fixes regression introduced by refactoring PR apache#3163 where `-Dpid`
was incorrectly passed string `$$` instead of parent PID integer.

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

* fix systemd limitation, exec using /bin/sh instead and wrap in ${} syntax

https://www.freedesktop.org/software/systemd/man/systemd.service.html#Command%20lines

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

* usage: don't hide exception from Gabriel's https://github.com/apache/cloudstack/pull/3207/files#diff-062fcf5ae32de59dfd6cd4f780e1d7cd

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

pbankonier added a commit to pbankonier/cloudstack that referenced this pull request Apr 24, 2019

systemd: Fix -Dpid arg passing to systemd usage service (apache#3210)
* systemd: Fix -Dpid arg passing to systemd usage service

This fixes regression introduced by refactoring PR apache#3163 where `-Dpid`
was incorrectly passed string `$$` instead of parent PID integer.

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

* fix systemd limitation, exec using /bin/sh instead and wrap in ${} syntax

https://www.freedesktop.org/software/systemd/man/systemd.service.html#Command%20lines

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

* usage: don't hide exception from Gabriel's https://github.com/apache/cloudstack/pull/3207/files#diff-062fcf5ae32de59dfd6cd4f780e1d7cd

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

mweber92 added a commit to mweber92/cloudstack that referenced this pull request Apr 25, 2019

systemd: Fix -Dpid arg passing to systemd usage service (apache#3210)
* systemd: Fix -Dpid arg passing to systemd usage service

This fixes regression introduced by refactoring PR apache#3163 where `-Dpid`
was incorrectly passed string `$$` instead of parent PID integer.

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

* fix systemd limitation, exec using /bin/sh instead and wrap in ${} syntax

https://www.freedesktop.org/software/systemd/man/systemd.service.html#Command%20lines

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>

* usage: don't hide exception from Gabriel's https://github.com/apache/cloudstack/pull/3207/files#diff-062fcf5ae32de59dfd6cd4f780e1d7cd

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.