[Vmware] Fix for OVF parsing error by nvazquez · Pull Request #2567 · apache/cloudstack

nvazquez · 2018-04-12T17:20:14Z

Description

OVF files contained on OVA template files, with missing 'ovf:populatedSize' attribute fail the post download installation.

After template is downloaded, installation fails with error:

2018-04-12 13:36:47,959 INFO  [storage.template.OVAProcessor] (pool-1-thread-6:null) The ovf file /mnt/SecStorage/3544cb8f-d19d-3ebb-b8f2-bdc7ee4b7a4c/template/tmpl/2/206/3283498e6608-10f2-34c0-9ad4-808f
f593bea9.ovf is invalid
java.lang.NumberFormatException: For input string: ""

at java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)

at java.lang.Long.parseLong(Long.java:601)

at java.lang.Long.parseLong(Long.java:631)

at com.cloud.agent.api.storage.OVFHelper.getOVFVolumeInfo(OVFHelper.java:116)

at com.cloud.storage.template.OVAProcessor.process(OVAProcessor.java:105)

at org.apache.cloudstack.storage.template.DownloadManagerImpl.postLocalDownload(DownloadManagerImpl.java:462)

at org.apache.cloudstack.storage.template.DownloadManagerImpl.setDownloadStatus(DownloadManagerImpl.java:302)

at org.apache.cloudstack.storage.template.DownloadManagerImpl$Completion.downloadComplete(DownloadManagerImpl.java:105)

at com.cloud.storage.template.HttpTemplateDownloader.download(HttpTemplateDownloader.java:228)

at com.cloud.storage.template.HttpTemplateDownloader.runInContext(HttpTemplateDownloader.java:426)

at org.apache.cloudstack.managed.context.ManagedContextRunnable$1.run(ManagedContextRunnable.java:49)

at org.apache.cloudstack.managed.context.impl.DefaultManagedContext$1.call(DefaultManagedContext.java:56)

at org.apache.cloudstack.managed.context.impl.DefaultManagedContext.callWithContext(DefaultManagedContext.java:103)

at org.apache.cloudstack.managed.context.impl.DefaultManagedContext.runWithContext(DefaultManagedContext.java:53)

at org.apache.cloudstack.managed.context.ManagedContextRunnable.run(ManagedContextRunnable.java:46)

at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)

at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)

at java.lang.Thread.run(Thread.java:748)
2018-04-12 13:36:47,962 ERROR [storage.template.DownloadManagerImpl] (pool-1-thread-6:null) Template process exception
com.cloud.exception.InternalErrorException: OVA package has bad ovf file For input string: ""

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?

Tested on Vmware environment registering a Windows 2012 OVA template.
OVF file didn't include the 'ovf:populatedSize' attribute

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

@blueorangutan package

blueorangutan · 2018-04-12T17:21:07Z

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

rafaelweingartner · 2018-04-12T17:30:46Z

                od._diskId = disk.getAttribute("ovf:diskId");
                od._fileRef = disk.getAttribute("ovf:fileRef");
-                od._populatedSize = Long.parseLong(disk.getAttribute("ovf:populatedSize") == null ? "0" : disk.getAttribute("ovf:populatedSize"));
+                od._populatedSize = Long.parseLong(StringUtils.isBlank(disk.getAttribute("ovf:populatedSize")) ? "0" : disk.getAttribute("ovf:populatedSize"));


Why not use org.apache.commons.lang3.math.NumberUtils.toLong(String) instead?
Then, you do not even need this inline IF

Excellent, thanks @rafaelweingartner

blueorangutan · 2018-04-12T17:48:01Z

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

nvazquez · 2018-04-12T17:49:01Z

@blueorangutan package

blueorangutan · 2018-04-12T17:50:07Z

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

blueorangutan · 2018-04-12T18:16:00Z

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

nvazquez · 2018-04-12T18:39:34Z

@blueorangutan test

blueorangutan · 2018-04-12T18:40:06Z

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

DaanHoogland

👍

borisstoyanov

LGTM

blueorangutan · 2018-04-13T09:20:07Z

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

yadvr · 2018-04-13T09:47:37Z

                od._diskId = disk.getAttribute("ovf:diskId");
                od._fileRef = disk.getAttribute("ovf:fileRef");
-                od._populatedSize = Long.parseLong(disk.getAttribute("ovf:populatedSize") == null ? "0" : disk.getAttribute("ovf:populatedSize"));
+                od._populatedSize = NumberUtils.toLong(disk.getAttribute("ovf:populatedSize"));


@nvazquez this can cause exception still -- this should be used with a default value like 0L in case of error. For example like this -- NumberUtils.toLong("string-param", 0L) . Also no tests were ran /cc @DaanHoogland

@rhtyd NumberUtils.toLong(Sting) calls NumberUtils.toLong(Sting,0L)

sorry @rhtyd , you are right about the tests. I saw @borisstoyanov 's lgtm and assumed.

@DaanHoogland cool, I did not know that internally it falls to NumberUtils.toLong(String,0L)

remains teh question: are we settling for the ci on this one (i did check that before merging) @rhtyd @rafaelweingartner @borisstoyanov ?

Well, now it is already merged. However, I do not see the need to run those tests again; they are the same tests used when the code was merged, right? Testes passed then, they will pass now. Unless you had a case, which could cause the case that @nvazquez addressed here.

@DaanHoogland let's kick tests both on master and 4.11 over the weekend. I don't think Nicolas's pr will cause any fail, besides Travis and pkging has passed.

blueorangutan · 2018-04-13T10:06:19Z

Packaging result: ✔centos6 ✖centos7 ✔debian. JID-1919

nvazquez · 2018-04-13T13:30:28Z

@blueorangutan package

blueorangutan · 2018-04-13T13:31:07Z

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

blueorangutan · 2018-04-13T13:58:15Z

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

blueorangutan · 2018-04-14T02:44:38Z

Trillian test result (tid-2501)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 113772 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2567-t2501-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_public_ip_range.py
Intermitten failure detected: /marvin/tests/smoke/test_routers.py
Intermitten failure detected: /marvin/tests/smoke/test_templates.py
Intermitten failure detected: /marvin/tests/smoke/test_usage.py
Intermitten failure detected: /marvin/tests/smoke/test_volumes.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 63 look OK, 4 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_04_restart_network_wo_cleanup	`Failure`	4.06	test_routers.py
test_04_extract_template	`Failure`	128.28	test_templates.py
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
test_06_download_detached_volume	`Failure`	137.59	test_volumes.py

nvazquez requested review from DaanHoogland and yadvr April 12, 2018 17:20

nvazquez added this to the 4.11.1.0 milestone Apr 12, 2018

yadvr approved these changes Apr 12, 2018

View reviewed changes

rafaelweingartner reviewed Apr 12, 2018

View reviewed changes

Fix for OVF parsing error

537fa18

nvazquez force-pushed the ovfparsefix branch from d64126c to 537fa18 Compare April 12, 2018 17:45

DaanHoogland approved these changes Apr 13, 2018

View reviewed changes

borisstoyanov approved these changes Apr 13, 2018

View reviewed changes

DaanHoogland merged commit b69c378 into apache:4.11 Apr 13, 2018

yadvr reviewed Apr 13, 2018

View reviewed changes

nvazquez deleted the ovfparsefix branch April 6, 2020 14:49

Uh oh!

Conversation

nvazquez commented Apr 12, 2018

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

blueorangutan commented Apr 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Apr 12, 2018

Uh oh!

nvazquez commented Apr 12, 2018

Uh oh!

blueorangutan commented Apr 12, 2018

Uh oh!

blueorangutan commented Apr 12, 2018

Uh oh!

nvazquez commented Apr 12, 2018

Uh oh!

blueorangutan commented Apr 12, 2018

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Apr 13, 2018

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!

blueorangutan commented Apr 13, 2018

Uh oh!

nvazquez commented Apr 13, 2018

Uh oh!

blueorangutan commented Apr 13, 2018

Uh oh!

blueorangutan commented Apr 13, 2018

Uh oh!

blueorangutan commented Apr 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants