You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by karuturi <gi...@git.apache.org> on 2015/09/30 12:26:47 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8808: Successfully registered ...
GitHub user karuturi opened a pull request:
https://github.com/apache/cloudstack/pull/901
CLOUDSTACK-8808: Successfully registered VHD template is downloaded again due to missing virtualsize property in template.properties
We have multiple file processors to process different types of image
formats. The processor interface has two methods getVirtualSize() and
process().
1. getVirtualSize() as the name says, returns the virtual size of
the file and is used at get the size while copying files from NFS to s3
2. process() returns FormatInfo struct which has fileType, size,
virutalSize, filename. on successfully downloading a template, each
file is passed to all the processors.process() and whichever returns a
FormatInfo, that will be used to create template.properties file. If
process() throws an InternalErrorException, template installation fails.
But, if process() returns null, template registration is successful with
template.properties missing some attributes like virtualSize, file
format etc. which results in this bug on restart of ssvm/cloud
service/management server.
failing the template download if virutalsize or some other properties
cannot be determined.
The following changes are done:
getVirtualSize() to always return size(if it can calculate, get virtual
size else return file size). This would mean the following changes
1. QCOW2Processor.getVirtualSize() to return file size if virtual
size calculation fails
2. VHDProcessor.getVirtualSize() to return file size if virtual size
calculation fails
process() to throw InternalErrorException if virtual size calculation
fails or any other exceptions occur. This would mean the following
changes
1. OVAProcessor to throw InternalErrorException if untar fails
2. QCOW2Processor to throw InternalErrorException if virtual size
calculation fails
3. VHDProcessor to throw InternalErrorException if virtual size
calculation fails
Testing:
added unittests for the changes in the file processors.
manual test:
setup: host xenserver 6.5, management server centos 6.7
template: disk created using the process specified by andy at https://issues.apache.org/jira/browse/CLOUDSTACK-8808?focusedCommentId=14933368
tried to register the template and it failed with an error. Template never moved to Ready state.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/karuturi/cloudstack CLOUDSTACK-8808
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/901.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #901
----
commit 1056171aca8816492c16c5bdf8f963745f968b5d
Author: Rajani Karuturi <ra...@gmail.com>
Date: 2015-09-29T16:25:23Z
CLOUDSTACK-8808: Successfully registered VHD template is downloaded
again due to missing virtualsize property in template.properties
We have multiple file processors to process different types of image
formats. The processor interface has two methods getVirtualSize() and
process().
1. getVirtualSize() as the name says, returns the virtual size of
the file and is used at get the size while copying files from NFS to s3
2. process() returns FormatInfo struct which has fileType, size,
virutalSize, filename. on successfully downloading a template, each
file is passed to all the processors.process() and whichever returns a
FormatInfo, that will be used to create template.properties file. If
process() throws an InternalErrorException, template installation fails.
But, if process() returns null, template registration is successful with
template.properties missing some attributes like virtualSize, file
format etc. which results in this bug on restart of ssvm/cloud
service/management server.
failing the template download if virutalsize or some other properties
cannot be determined.
The following changes are done:
getVirtualSize() to always return size(if it can calculate, get virtual
size else return file size). This would mean the following changes
1. QCOW2Processor.getVirtualSize() to return file size if virtual
size calculation fails
2. VHDProcessor.getVirtualSize() to return file size if virtual size
calculation fails
process() to throw InternalErrorException if virtual size calculation
fails or any other exceptions occur. This would mean the following
changes
1. OVAProcessor to throw InternalErrorException if untar fails
2. QCOW2Processor to throw InternalErrorException if virtual size
calculation fails
3. VHDProcessor to throw InternalErrorException if virtual size
calculation fails
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8808: Successfully registered ...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/901#issuecomment-145036915
Thanks @borisroman, merged!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8808: Successfully registered ...
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/901#discussion_r40881668
--- Diff: core/src/com/cloud/storage/template/QCOW2Processor.java ---
@@ -75,6 +76,16 @@ public FormatInfo process(String templatePath, ImageFormat format, String templa
@Override
public long getVirtualSize(File file) throws IOException {
+ try {
+ long size = getTemplateVirtualSize(file);
--- End diff --
The utils one should be a recent implementation. I wonder why the author hasnt done cleanup and left two implementation of the same. I will take a look at the code later and see if it can be reused here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8808: Successfully registered ...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/901#issuecomment-144439566
Thanks @karuturi for picking this up. Will have a look soon!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8808: Successfully registered ...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/901#issuecomment-145001598
@remibergsma Anwser is sufficient. I'll cleanup after 4.6 is released.
Regarding the PR: :+1: LGTM
I ran the test_vm_lifecycle which succeeded and also manually ran the tests described in CLOUDSTACK-8808, same results as you showed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8808: Successfully registered ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/901
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8808: Successfully registered ...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/901#issuecomment-144847280
@karuturi Thanks a lot, works great! LGTM
I registered a template with invalid headers, as per CLOUDSTACK-8808.
At first everything is normal:
<img width="715" alt="screen shot 2015-09-30 at 21 09 29" src="https://cloud.githubusercontent.com/assets/1630096/10233628/027aa5da-6890-11e5-97fc-981033df91f5.png">
It then starts installing (and unzipping):
<img width="719" alt="screen shot 2015-09-30 at 21 11 22" src="https://cloud.githubusercontent.com/assets/1630096/10233631/0bdec520-6890-11e5-995e-f193691eb1e7.png">
And eventually disappears from the UI. The logs show `Format is invalid` as expected.
Full logs:
```
2015-10-01 20:55:00,624 WARN [storage.template.TemplateLocation] (pool-1-thread-2:null) Format is invalid
2015-10-01 20:55:00,625 DEBUG [storage.template.TemplateLocation] (pool-1-thread-2:null) Format: VHD size: 21475270656 virtualsize: 0 filename: 4db55a50-1775-3f5d-b982-962eb2d0f282.vhd
2015-10-01 20:55:00,625 DEBUG [storage.template.TemplateLocation] (pool-1-thread-2:null) format, filename cannot be null and size, virtual size should be > 0
2015-10-01 20:55:02,241 DEBUG [storage.template.TemplateLocation] (pool-1-thread-2:null) Remove /mnt/SecStorage/6cc407de-bf89-3f72-8985-1b9df6a63827/template/tmpl/2/201/4db55a50-1775-3f5d-b982-962eb2d0f282.vhd
2015-10-01 20:55:02,302 DEBUG [storage.template.TemplateLocation] (pool-1-thread-2:null) Remove /mnt/SecStorage/6cc407de-bf89-3f72-8985-1b9df6a63827/template/tmpl/2/201/template.properties
2015-10-01 20:55:02,867 DEBUG [cloud.agent.Agent] (agentRequest-Handler-2:null) Processing command: org.apache.cloudstack.storage.command.DownloadProgressCommand
2015-10-01 20:55:02,870 DEBUG [cloud.agent.Agent] (agentRequest-Handler-2:null) Seq 3-5925611209712730174: { Ans: , MgmtId: 90520732674659, via: 3, Ver: v1, Flags: 10, [{"com.cloud.agent.api.storage.DownloadAnswer":{"jobId":"d0965877-d8b8-4e12-a132-703fb7ee6b54","downloadPct":100,"errorString":"Failed post download script: Unable to install due to invalid file format","downloadStatus":"DOWNLOAD_ERROR","downloadPath":"/mnt/SecStorage/6cc407de-bf89-3f72-8985-1b9df6a63827/template/tmpl/2/201/dnld3817046421300354449tmp_","installPath":"template/tmpl/2/201/4db55a50-1775-3f5d-b982-962eb2d0f282.vhd","templateSize":0,"templatePhySicalSize":0,"checkSum":"49c737f858448e11eb181f250e4efaeb","result":true,"details":"Failed post download script: Unable to install due to invalid file format","wait":0}}] }
```
When someone else reviews this we can merge it and solve another blocker.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8808: Successfully registered ...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/901#discussion_r40784661
--- Diff: core/src/com/cloud/storage/template/QCOW2Processor.java ---
@@ -75,6 +76,16 @@ public FormatInfo process(String templatePath, ImageFormat format, String templa
@Override
public long getVirtualSize(File file) throws IOException {
+ try {
+ long size = getTemplateVirtualSize(file);
--- End diff --
@karuturi Could you please use the QCOW2Utils.getVirtualSize() from the com.cloud.utils.storage package instead of using the function defined in the QCOW2Processor? :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8808: Successfully registered ...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/901#issuecomment-144935255
@borisroman: Are the answers to your questions sufficient? Can you review again please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---