Fix #3203 usage server broken in 4.11/4.12 by GabrielBrascher · Pull Request #3207 · apache/cloudstack

GabrielBrascher · 2019-03-11T02:14:49Z

Fixes issue #3203.

Due to the migration to systemd, the configuration -Dpid=$$ does not return the pid value but the string '$$'; thus, the _pid variable received '$$', causing a silenced exception.

This PR adds the file /etc/cloudstack/usage/cloudstack-usage.service.pid that has the process pid. The file is configured after the ExecStart, by running the systemctl show command configured as:

ExecStartPost=/bin/sh -c "systemctl show -p MainPID cloudstack-usage.service 2>/dev/null | cut -d= -f2 > /etc/cloudstack/usage/cloudstack-usage.service.pid"

Description

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)

Screenshots (if appropriate):

How Has This Been Tested?

CloudStack usage service run as expected after updating with the changes from this PR.

'$$'; thus, the _pid variable received '$$', causing an exception.

GabrielBrascher · 2019-03-11T02:15:04Z

@blueorangutan package

blueorangutan · 2019-03-11T02:15:54Z

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

GabrielBrascher · 2019-03-11T03:31:44Z

@blueorangutan package

blueorangutan · 2019-03-11T03:32:49Z

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

konstantinjfk · 2019-03-11T07:31:35Z

Would you think to put pid file to the /run folder? like /run/cloudstack-usage.service.pid

wido · 2019-03-11T09:21:31Z

I would say that /run or /var/run would be better as well.

But why does the usage server actually needs it's PID?

andrijapanicsb · 2019-03-11T10:33:12Z

I would also ask (sorry if silly question) - why any difference to 4.11.2 at all (did the code around systemd changed) ? - i.e. I observe no pid with 4.11.2, as far as I can see.

But definitively (if PID required) would put it to default location as /var/run/*

Happy to test, just want to avoid doing it 2 times... ?

ustcweizhou · 2019-03-11T10:49:07Z

I agree with @wido @konstantinjfk that it would be better to save pid file as "/var/run/cloudstack-usage.service.pid"

I have applied @GabrielBrascher 's patch in cloudstack 4.11.2, changed pid file to "/var/run/cloudstack-usage.service.pid" and tested it. all seems fine.

so this pull request is good for me, except the pid file location.

thanks @GabrielBrascher for your work.

exception in case of null or blank pid (NumberUtils.toInt returns zero if the conversion fails).

ustcweizhou · 2019-03-11T13:13:45Z

code looks good to me.

GabrielBrascher · 2019-03-11T13:32:58Z

Thanks for the feedback @wido @andrijapanic @konstantinjfk @ustcweizhou!
I have changed the pid file to /var/run/cloudstack-usage.service.pid.

GabrielBrascher · 2019-03-11T13:33:27Z

@wido, as far as I know, the implementation used the pid to have a unique id regarding the cloudstack-usage.service (in case of multiple cloudstack-usage nodes running). To change this approach some refactoring is needed.

Regarding the codebase (prior to this PR), the exception was happening at line 274 from UsageManagerImpl.java.
_pid = Integer.parseInt(System.getProperty("pid")); and silenced at the UsageServer as it would print the stack only in case of running the service from the command line and not a systemd service (which was not the case for any of us). Explaining why the usage.log did not print details and/or an exception.

try {
    ComponentContext.initComponentsLifeCycle();
} catch (Exception e) {
    e.printStackTrace();
}

If the _pid is not a valid pid for the service (for instance zero) the runInContextInternal execution flow is ignored.

cloudstack/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java

Line 353 in 1d05fea

UsageJobVO job = _usageJobDao.isOwner(_hostname, _pid);

cloudstack/usage/src/main/java/com/cloud/usage/UsageManagerImpl.java

Line 354 in 1d05fea

if (job != null) {

GabrielBrascher · 2019-03-11T13:34:51Z

@andrijapanic the _pid is there at 4.11.2. However, I am not sure if the service was alrady migrate to systemd at 4.11.2 or it was migrated in the 4.11 branch after 4.11.2 was released.
https://github.com/apache/cloudstack/blob/4.11.2.0/usage/src/com/cloud/usage/UsageManagerImpl.java#L353

andrijapanicsb · 2019-03-11T13:43:53Z

@GabrielBrascher thx a lot for the fixes ! I'll wait for commit and kick packaging once more.

GabrielBrascher · 2019-03-11T13:50:57Z

Fixed! Thanks for the help @andrijapanicsb!

wido

LGTM

ustcweizhou · 2019-03-11T21:07:53Z

@GabrielBrascher could you squash the commits to one commit ?

GabrielBrascher · 2019-03-11T22:05:06Z

@ustcweizhou sure, I will Squash when merging (with the Squash and merge).

GabrielBrascher · 2019-03-11T23:17:55Z

@blueorangutan package

blueorangutan · 2019-03-11T23:18:52Z

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

blueorangutan · 2019-03-11T23:40:11Z

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

GabrielBrascher · 2019-03-12T00:04:53Z

@blueorangutan test

blueorangutan · 2019-03-12T00:05:52Z

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

yadvr · 2019-03-12T06:11:28Z

 # specific language governing permissions and limitations
 # under the License.

-JAVA_OPTS="-Dpid=$$ -Xms256m -Xmx2048m"


@GabrielBrascher What's the issue, this should work?

Nevermind, re-read the description. Alright keep this change, I'll advise you a simpler fix.

Btw, this without quotes would work

yadvr · 2019-03-12T06:13:26Z

 [Service]
 Type=simple
 EnvironmentFile=/etc/default/cloudstack-usage
-ExecStart=/usr/bin/java $JAVA_OPTS -cp $CLASSPATH $JAVA_CLASS


Put this and this would fix the pid issue:

Environment=JAVA_PID=$$ ExecStart=/usr/bin/java $JAVA_DEBUG -Dpid=$JAVA_PID $JAVA_OPTS -cp $CLASSPATH $JAVA_CLASS

Comment: this would need to be fixed for 4.11 as well, let me send a quick fix instead.

yadvr

Too complicated, I'll submit a simpler change.

yadvr · 2019-03-12T06:24:29Z

Please review/test - #3210

blueorangutan · 2019-03-12T08:31:34Z

Trillian test result (tid-3416)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28344 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3207-t3416-kvm-centos7.zip
Smoke tests completed. 70 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

andrijapanicsb · 2019-03-12T11:22:33Z

I did not review this one (test it), but already had env for #3210 and confirm it works fine - usage jobs does kick in as it should.

GabrielBrascher · 2019-03-12T17:09:41Z

Closing this in favor of #3210

the java application for the cloudstack-usage.service was retrieving

9a3a67a

'$$'; thus, the _pid variable received '$$', causing an exception.

GabrielBrascher added type:bug component:usage-server Severity:BLOCKER labels Mar 11, 2019

GabrielBrascher added this to the 4.12.0.0 milestone Mar 11, 2019

GabrielBrascher self-assigned this Mar 11, 2019

GabrielBrascher changed the title ~~Fix #3203 usage server broken in 4.12.0.0~~ Fix #3203 usage server broken in 4.11 / 4.12 Mar 11, 2019

GabrielBrascher changed the title ~~Fix #3203 usage server broken in 4.11 / 4.12~~ Fix #3203 usage server broken in 4.11/4.12 Mar 11, 2019

GabrielBrascher mentioned this pull request Mar 11, 2019

usage server broken in 4.12.0.0 #3203

Closed

Change pid file to /var/run/cloudstack-usage.service.pid and throw

353251b

exception in case of null or blank pid (NumberUtils.toInt returns zero if the conversion fails).

GabrielBrascher commented Mar 11, 2019

View reviewed changes

Comment thread usage/src/main/java/com/cloud/usage/UsageManagerImpl.java Outdated

Fix typo

4c948e0

GabrielBrascher force-pushed the usage-aggregation-issue branch from 3510470 to 4c948e0 Compare March 11, 2019 14:32

wido approved these changes Mar 11, 2019

View reviewed changes

Removing tests that are not runned by maven and have no asserts

7a5649e

GabrielBrascher force-pushed the usage-aggregation-issue branch from 4d7ad0f to d9d3da9 Compare March 11, 2019 19:03

Add unit test for UsageManagerImpl.configureUsageManagerServicePid()

a42bc9f

GabrielBrascher force-pushed the usage-aggregation-issue branch from d9d3da9 to a42bc9f Compare March 11, 2019 22:01

yadvr reviewed Mar 12, 2019

View reviewed changes

yadvr requested changes Mar 12, 2019

View reviewed changes

GabrielBrascher closed this Mar 12, 2019

Conversation

GabrielBrascher commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

GabrielBrascher commented Mar 11, 2019

Uh oh!

blueorangutan commented Mar 11, 2019

Uh oh!

GabrielBrascher commented Mar 11, 2019

Uh oh!

blueorangutan commented Mar 11, 2019

Uh oh!

konstantinjfk commented Mar 11, 2019

Uh oh!

wido commented Mar 11, 2019

Uh oh!

andrijapanicsb commented Mar 11, 2019

Uh oh!

ustcweizhou commented Mar 11, 2019

Uh oh!

ustcweizhou commented Mar 11, 2019

Uh oh!

GabrielBrascher commented Mar 11, 2019

Uh oh!

GabrielBrascher commented Mar 11, 2019

Uh oh!

GabrielBrascher commented Mar 11, 2019

Uh oh!

Uh oh!

andrijapanicsb commented Mar 11, 2019

Uh oh!

GabrielBrascher commented Mar 11, 2019

Uh oh!

wido left a comment

Choose a reason for hiding this comment

Uh oh!

ustcweizhou commented Mar 11, 2019

Uh oh!

GabrielBrascher commented Mar 11, 2019

Uh oh!

GabrielBrascher commented Mar 11, 2019

Uh oh!

blueorangutan commented Mar 11, 2019

Uh oh!

blueorangutan commented Mar 11, 2019

Uh oh!

GabrielBrascher commented Mar 12, 2019

Uh oh!

blueorangutan commented Mar 12, 2019

Uh oh!

yadvr Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

yadvr Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

yadvr Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

yadvr Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

yadvr Mar 12, 2019

Choose a reason for hiding this comment

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Mar 12, 2019

Uh oh!

blueorangutan commented Mar 12, 2019

Uh oh!

andrijapanicsb commented Mar 12, 2019

Uh oh!

GabrielBrascher commented Mar 12, 2019

Uh oh!

GabrielBrascher commented Mar 11, 2019 •

edited

Loading