CLOUDSTACK-9503: Increased the VR script timeout. Most of the changes… by abhinandanprateek · Pull Request #1678 · apache/cloudstack

abhinandanprateek · 2016-09-20T07:34:34Z

… are about converting int/long time values to Duration.

yadvr · 2016-09-20T10:43:01Z

@blueorangutan package

blueorangutan · 2016-09-20T10:44:09Z

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

blueorangutan · 2016-09-20T11:29:49Z

Packaging result: ✖centos6 ✖centos7 ✖debian. JID-8

yadvr · 2016-09-20T11:34:48Z

@abhinandanprateek please fix noredist/vmware issue:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.2:compile (default-compile) on project cloud-plugin-hypervisor-vmware: Compilation failure: Compilation failure:
[ERROR] /jenkins/workspace/acs-pr-centos6-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.9.1.0-SNAPSHOT/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java:[1269,15] error: no suitable method found for executeInVR(String,String,String,int)
[ERROR] method VmwareResource.executeInVR(String,String,String,Duration) is not applicable
[ERROR](actual argument int cannot be converted to Duration by method invocation conversion)
[ERROR] method VmwareResource.executeInVR(String,String,String) is not applicable
[ERROR](actual and formal argument lists differ in length)
[ERROR] /jenkins/workspace/acs-pr-centos6-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.9.1.0-SNAPSHOT/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java:[1284,41] error: cannot find symbol
[ERROR] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
[ERROR]
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR] mvn -rf :cloud-plugin-hypervisor-vmware

abhinandanprateek · 2016-09-20T11:41:00Z

@rhtyd fixed it, not sure why that was not caught by jenkins build.

yadvr · 2016-09-20T11:42:07Z

@abhinandanprateek jenkins does not do noredist builds

blueorangutan · 2016-09-21T04:38:57Z

Packaging result: ✖centos6 ✖centos7 ✖debian. JID-12

yadvr · 2016-09-21T04:40:19Z

@abhinandanprateek error:

[INFO] Compiling 51 source files to /jenkins/workspace/acs-pr-centos6-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.9.1.0-SNAPSHOT/plugins/hypervisors/vmware/target/classes
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /jenkins/workspace/acs-pr-centos6-pkg-builder/dist/rpmbuild/BUILD/cloudstack-4.9.1.0-SNAPSHOT/plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java:[1284,41] error: cannot find symbol

abhinandanprateek · 2016-09-21T09:49:20Z

@rhtyd thanks for sharing nordist repo. I have sanitized the vmware plugin too.

jburwell

@abhinandanprateek this is a really nice change. Moving from using an int to Duration will make time handling code far less error prone.

Reading through the PR, I think we need to run smoke tests with requires_hardware=true + the VR component test suites on VMware, KVM, and XenServer. Do you agree?

jburwell · 2016-09-22T15:04:32Z

core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java

To avoid any potential unit conversion issues, consider using the ``Duration. multipliedBy(int)` method.

jburwell · 2016-09-22T15:08:45Z

core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java

Please consider extract 120L to a constant of type Duration to intent more clear and provide further unit conversion safety (e.g. MINIMUM_VR_RESTART_TIMEOUT).

jburwell · 2016-09-22T15:09:10Z

core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java

Consider using the previously defined constant here for ensured consistency between he check and the value used.

jburwell · 2016-09-22T15:12:05Z

...hypervisors/hyperv/src/com/cloud/hypervisor/hyperv/resource/HypervDirectConnectResource.java

Please consider defining an override of the SshHelper.sshExecute method that accepts a Duration in order to encapsulate this conversion. We should also consider deprecating the version that accepts int.

jburwell · 2016-09-22T15:13:27Z

plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

Please consider adding an overridden version of the Script(String, int, Logger) constructor that accepts a Duration instance to encapsulate this conversion.

Scripts is an external package, hope they will upgrade to Duration after Java 1.8.

jburwell · 2016-09-22T15:17:17Z

...isors/kvm/src/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtPvlanSetupCommandWrapper.java

Please consider adding an overridden version of the Script(String, int, Logger) constructor that accepts Duration to encapsulate this type conversion.

Scripts is an external package, hope they will upgrade to Duration after Java 1.8.

@abhinandanprateek the Script class is in the com.cloud.utils.script which is part of the project. What prevents us from adding a constructor that class?

@jburwell yes you are right, we should modify the script class to take duration as timeout instead of long.

jburwell · 2016-09-22T15:18:16Z

...isors/ovm3/src/main/java/com/cloud/hypervisor/ovm3/resources/Ovm3VirtualRoutingResource.java

Please consider extracting Duration.standardSeconds(600L) to constant to clarify the intent.

jburwell · 2016-09-22T15:19:29Z

plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java

Please consider extracting Duration.standardSeconds(120L) to a constant to clarify intent

jburwell · 2016-09-22T15:20:48Z

plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java

Please consider adding an overridden version of the SshHelper.sshExecute(String, int, String, String, ??, String, int, int, int) method that accepts Duration to encapsulate this type conversion.

jburwell · 2016-09-22T15:21:08Z

...ns/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java

Please consider adding an overridden version of the SshHelper.sshExecute(String, int, String, String, ??, String, int, int, int) method that accepts Duration to encapsulate this type conversion.

abhinandanprateek · 2016-09-26T05:00:11Z

@jburwell updated Script to use Duration as timeout.

jburwell · 2016-09-26T05:13:38Z

core/src/com/cloud/agent/resource/virtualnetwork/VirtualRoutingResource.java

-                int timeout = answerCounts * _eachTimeout;
-                if (timeout < 120) {
-                    timeout = 120;
+                Duration timeout = _eachTimeout.withDurationAdded(_eachTimeout.getStandardSeconds(), answerCounts);


Why use withDurationAdded rather multipliedBy? Passing _eachTimeout into the method is an indication that multipliedBy may be a more concise expression of intent.

@jburwell the java Duration that has multipliedBy is available from Java 8. Right now we are using joda.time.

jburwell · 2016-09-26T05:19:58Z

@abhinandanprateek could you please create a ticket to refactor Script to use Duration internally? Also, are there any component tests that need to be run in addition to smoke tests to verify this bug fix?

jburwell · 2016-09-26T05:21:47Z

@abhinandanprateek how difficult would it be to change the base branch of this PR to 4.8?

abhinandanprateek · 2016-09-26T08:30:45Z

@jburwell ticket filed to refactor Script and SshHelper to use Duration. https://issues.apache.org/jira/browse/CLOUDSTACK-9508

abhinandanprateek · 2016-09-27T08:27:05Z

@karuturi Jenkins fail on find-bug: [ERROR] Failed to execute goal org.codehaus.mojo:findbugs-maven-plugin:3.0.3:findbugs (findbugs) on project cloud-plugin-network-bigswitch: Execution findbugs of goal org.codehaus.mojo:findbugs-maven-plugin:3.0.3:findbugs failed: Java returned: 143

I did not make any changes there, do you know if it is failing elsewhere too ? Or it is just some spurious failure.

karuturi · 2016-09-27T10:26:32Z

I think the build timed out.

[INFO] --- findbugs-maven-plugin:3.0.3:findbugs (findbugs) @ cloud-plugin-network-bigswitch ---
[INFO] Fork Value is true
Build timed out (after 96 minutes). Marking the build as aborted.
Build was aborted

jburwell · 2016-09-27T12:46:19Z

@abhinandanprateek it appears that there is a Findbugs issue that needs to be addressed.

… are about converting int/long time values to joda Duration.

abhinandanprateek · 2016-09-28T01:12:15Z

@jburwell there was some issue with jenkins, force pushing PR resolved it.

yadvr · 2016-10-21T05:05:15Z

@blueorangutan package

blueorangutan · 2016-10-21T05:32:13Z

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

blueorangutan · 2016-10-21T08:46:21Z

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

jburwell · 2016-10-28T16:28:45Z

@blueorangutan test centos6 kvm

blueorangutan · 2016-10-28T16:29:02Z

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

jburwell · 2016-11-01T02:34:00Z

@abhinandanprateek this fix seems like a good fix for 4.8 users as well which I would like to get included in 4.8.2.0, as well as, 4.9.1.0 and 4.10.0.0. Could rebase this PR and change the base branch to 4.8?

abhinandanprateek · 2016-11-01T04:17:06Z

@jburwell Created a new PR for 4.8 #1745, closing this one.

abhinandanprateek force-pushed the vr_timeout branch from e90511d to f46f774 Compare September 20, 2016 11:40

abhinandanprateek force-pushed the vr_timeout branch from f46f774 to 659cb71 Compare September 21, 2016 09:46

jburwell suggested changes Sep 22, 2016

View reviewed changes

abhinandanprateek force-pushed the vr_timeout branch 2 times, most recently from 81d0d9c to 6fdfd9d Compare September 26, 2016 04:59

jburwell suggested changes Sep 26, 2016

View reviewed changes

abhinandanprateek force-pushed the vr_timeout branch from 6fdfd9d to 04c990e Compare September 26, 2016 08:00

CLOUDSTACK-9503: Increased the VR script timeout. Most of the changes…

4032a5e

… are about converting int/long time values to joda Duration.

abhinandanprateek force-pushed the vr_timeout branch from 04c990e to 4032a5e Compare September 27, 2016 13:00

abhinandanprateek changed the base branch from 4.9 to 4.8 November 1, 2016 03:36

abhinandanprateek changed the base branch from 4.8 to 4.9 November 1, 2016 03:37

abhinandanprateek mentioned this pull request Nov 1, 2016

CLOUDSTACK-9503: Increased the VR script timeout. Most of the changes… #1745

Merged

abhinandanprateek closed this Nov 1, 2016

Conversation

abhinandanprateek commented Sep 20, 2016

Uh oh!

yadvr commented Sep 20, 2016

Uh oh!

blueorangutan commented Sep 20, 2016

Uh oh!

blueorangutan commented Sep 20, 2016

Uh oh!

yadvr commented Sep 20, 2016

Uh oh!

abhinandanprateek commented Sep 20, 2016

Uh oh!

yadvr commented Sep 20, 2016

Uh oh!

blueorangutan commented Sep 21, 2016

Uh oh!

yadvr commented Sep 21, 2016

Uh oh!

abhinandanprateek commented Sep 21, 2016

Uh oh!

jburwell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinandanprateek commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jburwell commented Sep 26, 2016

Uh oh!

jburwell commented Sep 26, 2016

Uh oh!

abhinandanprateek commented Sep 26, 2016

Uh oh!

abhinandanprateek commented Sep 27, 2016

Uh oh!

karuturi commented Sep 27, 2016

Uh oh!

jburwell commented Sep 27, 2016

Uh oh!

abhinandanprateek commented Sep 28, 2016

Uh oh!

yadvr commented Oct 21, 2016

Uh oh!

blueorangutan commented Oct 21, 2016

Uh oh!

blueorangutan commented Oct 21, 2016

Uh oh!

jburwell commented Oct 28, 2016

Uh oh!