You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/06/11 17:38:27 UTC

[GitHub] [cloudstack] div8cn opened a new pull request #4144: Fix Usage failed to get pid

div8cn opened a new pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144


   
   
   ## Description
   <!--- Describe your changes in detail -->
   cloudstack-usage use System.getProperty("pid") to get the process PID
   System.getProperty cannot get the Pid, which will cause the service to stay in the config stage. Cannot continue to start, and there will be no log output.
   
   This PR uses ManagementFactory instead of System.getProperty to get Pid, and throws an exception when Pid cannot be obtained
   
   <!-- For new features, provide link to FS, dev ML discussion etc. -->
   <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
   
   <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
   <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
   <!-- Fixes: # -->
   
   ## Types of changes
   <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [x] 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?
   <!-- Please describe in detail how you tested your changes. -->
   <!-- Include details of your testing environment, and the tests you ran to -->
   <!-- see how your change affects other areas of the code, etc. -->
   
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-697904669


   @blueorangutan package


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#discussion_r465259826



##########
File path: usage/src/main/java/com/cloud/usage/UsageManagerImpl.java
##########
@@ -275,7 +277,14 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
             s_logger.error("Unhandled exception configuring UsageManger", e);
             throw new ConfigurationException("Unhandled exception configuring UsageManager " + e.toString());
         }
-        _pid = Integer.parseInt(System.getProperty("pid"));
+
+        try {
+            String processName = ManagementFactory.getRuntimeMXBean().getName();
+            _pid = Integer.parseInt(processName.split("@")[0]);
+        } catch (Exception e) {
+            s_logger.error("Unable to get process pid ", e);
+            throw new ConfigurationException("Unable to get process pid " + e.toString());

Review comment:
       I'm all for clarity @ravening . what do you suggest?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-665076769


   @blueorangutan package


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-682423096


   @blueorangutan package


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] div8cn edited a comment on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
div8cn edited a comment on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-643551945


   I encountered this problem when building cloudstack-usage into docker and started using supervisord。
   
   cloudstack 4.13.1 or 4.11.3


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-698341080


   though the ha error is a bit offsetting I see not how any of these errors can be related, @rhtyd agree?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd merged pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-697938369


   Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2071


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] ravening commented on a change in pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
ravening commented on a change in pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#discussion_r447245386



##########
File path: usage/src/main/java/com/cloud/usage/UsageManagerImpl.java
##########
@@ -275,7 +277,14 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
             s_logger.error("Unhandled exception configuring UsageManger", e);
             throw new ConfigurationException("Unhandled exception configuring UsageManager " + e.toString());
         }
-        _pid = Integer.parseInt(System.getProperty("pid"));
+
+        try {
+            String processName = ManagementFactory.getRuntimeMXBean().getName();
+            _pid = Integer.parseInt(processName.split("@")[0]);
+        } catch (Exception e) {
+            s_logger.error("Unable to get process pid ", e);
+            throw new ConfigurationException("Unable to get process pid " + e.toString());

Review comment:
       Should we add a more meaningful message here so its more clear?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] div8cn commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
div8cn commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-685207683


   @rhtyd  Thanks, I've updated it to ProcessHandle.current ().pid().


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-717836556


   Yup LGTM


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-649444455


   @blueorangutan package


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] weizhouapache commented on a change in pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#discussion_r481904326



##########
File path: usage/src/main/java/com/cloud/usage/UsageManagerImpl.java
##########
@@ -280,8 +278,7 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
 
         String processName = null;
         try {
-            processName = ManagementFactory.getRuntimeMXBean().getName();
-            _pid = Integer.parseInt(processName.split("@")[0]);
+            _pid = (int) ProcessHandle.current().pid();
         } catch (Exception e) {
             String msg = String.format("Unable to get process Id for %s!", processName);

Review comment:
       processName is always null .




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-682454136


   Packaging result: ✔centos7 ✖centos8 ✔debian. JID-1841


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#discussion_r465524355



##########
File path: usage/src/main/java/com/cloud/usage/UsageManagerImpl.java
##########
@@ -275,7 +277,16 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
             s_logger.error("Unhandled exception configuring UsageManger", e);
             throw new ConfigurationException("Unhandled exception configuring UsageManager " + e.toString());
         }
-        _pid = Integer.parseInt(System.getProperty("pid"));
+
+        String processName = null;
+        try {
+            processName = ManagementFactory.getRuntimeMXBean().getName();
+            _pid = Integer.parseInt(processName.split("@")[0]);
+        } catch (Exception e) {
+            String msg = String.format("Unable to get process Id for %s!", processName);
+            s_logger.debug(msg , e);
+            throw new ConfigurationException(msg + " " + e.toString());

Review comment:
       `logger.debug(msg, e)` should log the entire stacktrace @GabrielBrascher . and it gives the user to filter or direct the output to a certain stream/file. `e.printStacktrace()` only logs to stdout (or stderr, i forgot). in short i think not too much of it. I agree toString does not seem the best, but we have alreadyt output the stacktrace by then. A change from `toString()` to `getLocalizedMessage()` maybe, @div8cn? Of course we can add the entire exception again i.e. `throw new ConfigurationException(msg, e)`, that's fine as well.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-697940160


   @blueorangutan test


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-653725052


   @div8cn can you check if in the docker container the pid parameter was passed? Depending on how the service runs perhaps this was not passed, look at the cloudstack-usage systemd service for example.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-687655209


   Packaging result: ✖centos7 ✖centos8 ✖debian. JID-1898


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-697905918


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-698317533


   <b>Trillian test result (tid-2844)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 57660 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4144-t2844-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
   Smoke tests completed. 83 look OK, 2 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_deploy_kubernetes_cluster | `Error` | 3618.75 | test_kubernetes_clusters.py
   test_02_deploy_kubernetes_ha_cluster | `Error` | 0.04 | test_kubernetes_clusters.py
   test_04_deploy_and_upgrade_kubernetes_cluster | `Failure` | 1984.43 | test_kubernetes_clusters.py
   test_05_deploy_and_upgrade_kubernetes_ha_cluster | `Error` | 0.04 | test_kubernetes_clusters.py
   test_07_deploy_and_scale_kubernetes_cluster | `Failure` | 906.89 | test_kubernetes_clusters.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 941.71 | test_kubernetes_clusters.py
   test_hostha_kvm_host_fencing | `Error` | 175.18 | test_hostha_kvm.py
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-697940447


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-649444977


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-689500693


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-649451535


   Packaging result: ✖centos7 ✖debian. JID-1459


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on a change in pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#discussion_r480958381



##########
File path: usage/src/main/java/com/cloud/usage/UsageManagerImpl.java
##########
@@ -275,7 +277,16 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
             s_logger.error("Unhandled exception configuring UsageManger", e);
             throw new ConfigurationException("Unhandled exception configuring UsageManager " + e.toString());
         }
-        _pid = Integer.parseInt(System.getProperty("pid"));
+
+        String processName = null;
+        try {
+            processName = ManagementFactory.getRuntimeMXBean().getName();

Review comment:
       @div8cn master is on java 11 now, can you use `ProcessHandle.current().pid()`? I did a quick test:
   ```
   > jshell 
   |  Welcome to JShell -- Version 11.0.7
   |  For an introduction type: /help intro
   
   jshell> ProcessHandle.current().pid()
   $1 ==> 20380
   
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-682423578


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#discussion_r465441006



##########
File path: usage/src/main/java/com/cloud/usage/UsageManagerImpl.java
##########
@@ -275,7 +277,16 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
             s_logger.error("Unhandled exception configuring UsageManger", e);
             throw new ConfigurationException("Unhandled exception configuring UsageManager " + e.toString());
         }
-        _pid = Integer.parseInt(System.getProperty("pid"));
+
+        String processName = null;
+        try {
+            processName = ManagementFactory.getRuntimeMXBean().getName();
+            _pid = Integer.parseInt(processName.split("@")[0]);
+        } catch (Exception e) {
+            String msg = String.format("Unable to get process Id for %s!", processName);
+            s_logger.debug(msg , e);
+            throw new ConfigurationException(msg + " " + e.toString());

Review comment:
       @DaanHoogland @div8cn What do you think of using `e.printStackTrace()` to add the stack on the log/ConfigurationException?
   
   With `toString` (or simply s_logger.debug(msg , e)) it logs only the exception name and the error message (if there is any message). On the other hand, `printStackTrace()` presents the whole stack trace of an exception, which is very helpful for debugging.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-687650199


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-687650129


   @blueorangutan package


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-665077347






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-697146442


   @blueorangutan package


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-698317533


   <b>Trillian test result (tid-2844)</b>
   Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
   Total time taken: 57660 seconds
   Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4144-t2844-kvm-centos7.zip
   Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
   Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
   Smoke tests completed. 83 look OK, 2 have error(s)
   Only failed tests results shown below:
   
   
   Test | Result | Time (s) | Test File
   --- | --- | --- | ---
   test_01_deploy_kubernetes_cluster | `Error` | 3618.75 | test_kubernetes_clusters.py
   test_02_deploy_kubernetes_ha_cluster | `Error` | 0.04 | test_kubernetes_clusters.py
   test_04_deploy_and_upgrade_kubernetes_cluster | `Failure` | 1984.43 | test_kubernetes_clusters.py
   test_05_deploy_and_upgrade_kubernetes_ha_cluster | `Error` | 0.04 | test_kubernetes_clusters.py
   test_07_deploy_and_scale_kubernetes_cluster | `Failure` | 906.89 | test_kubernetes_clusters.py
   ContextSuite context=TestKubernetesCluster>:teardown | `Error` | 941.71 | test_kubernetes_clusters.py
   test_hostha_kvm_host_fencing | `Error` | 175.18 | test_hostha_kvm.py
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-643529656


   @div8cn where did you reproduce the issue, OS/distro, CloudStack version etc? Thanks


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on a change in pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#discussion_r481937498



##########
File path: usage/src/main/java/com/cloud/usage/UsageManagerImpl.java
##########
@@ -275,7 +275,15 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
             s_logger.error("Unhandled exception configuring UsageManger", e);
             throw new ConfigurationException("Unhandled exception configuring UsageManager " + e.toString());
         }
-        _pid = Integer.parseInt(System.getProperty("pid"));
+
+        String processName = null;

Review comment:
       processName is not used and can be removed @div8cn 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-698341080


   though the ha error is a bit offsetting I see not how any of these errors can be related, @rhtyd agree?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] div8cn commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
div8cn commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-643551945


   I encountered this problem when building cloudstack-usage into docker and started using supervisord。


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] blueorangutan commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-689531148


   Packaging result: ✔centos7 ✖centos8 ✖debian. JID-1932


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on a change in pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#discussion_r461636110



##########
File path: usage/src/main/java/com/cloud/usage/UsageManagerImpl.java
##########
@@ -275,7 +277,14 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
             s_logger.error("Unhandled exception configuring UsageManger", e);
             throw new ConfigurationException("Unhandled exception configuring UsageManager " + e.toString());
         }
-        _pid = Integer.parseInt(System.getProperty("pid"));
+
+        try {
+            String processName = ManagementFactory.getRuntimeMXBean().getName();
+            _pid = Integer.parseInt(processName.split("@")[0]);
+        } catch (Exception e) {
+            s_logger.error("Unable to get process pid ", e);
+            throw new ConfigurationException("Unable to get process pid " + e.toString());
+        }

Review comment:
       1. twice the same message should be in a string constand
   1. rethrown should not abandon the stack trace for the original problem
   ```suggestion
           String processName;
           try {
               processName = ManagementFactory.getRuntimeMXBean().getName();
               _pid = Integer.parseInt(processName.split("@")[0]);
           } catch (Exception e) {
               String msg = String.format("Unable to get process Id for %s!", processName);
               s_logger.error(msg);
               throw new ConfigurationException(msg, e);
           }
   ```
   or
   ```suggestion
           String processName;
           try {
               processName = ManagementFactory.getRuntimeMXBean().getName();
               _pid = Integer.parseInt(processName.split("@")[0]);
           } catch (Exception e) {
               String msg = String.format("Unable to get process Id for %s!", processName);
               s_logger.debug(msg , e);
               throw new ConfigurationException(msg, e);
           }
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] rhtyd commented on pull request #4144: Fix Usage failed to get pid

Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4144:
URL: https://github.com/apache/cloudstack/pull/4144#issuecomment-689500337


   @blueorangutan package


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org