From 1056171aca8816492c16c5bdf8f963745f968b5d Mon Sep 17 00:00:00 2001 From: Rajani Karuturi Date: Tue, 29 Sep 2015 21:55:23 +0530 Subject: [PATCH] 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 --- .../cloud/storage/template/OVAProcessor.java | 2 +- .../storage/template/QCOW2Processor.java | 17 ++- .../storage/template/TemplateLocation.java | 4 +- .../cloud/storage/template/VhdProcessor.java | 17 ++- .../storage/template/OVAProcessorTest.java | 138 ++++++++++++++++++ .../storage/template/QCOW2ProcessorTest.java | 109 ++++++++++++++ .../storage/template/VhdProcessorTest.java | 110 ++++++++++++++ .../storage/template/DownloadManagerImpl.java | 5 +- 8 files changed, 393 insertions(+), 9 deletions(-) create mode 100644 core/test/com/cloud/storage/template/OVAProcessorTest.java create mode 100644 core/test/com/cloud/storage/template/QCOW2ProcessorTest.java create mode 100644 core/test/com/cloud/storage/template/VhdProcessorTest.java diff --git a/core/src/com/cloud/storage/template/OVAProcessor.java b/core/src/com/cloud/storage/template/OVAProcessor.java index 3d7f7a23bd7..e0003b1609c 100644 --- a/core/src/com/cloud/storage/template/OVAProcessor.java +++ b/core/src/com/cloud/storage/template/OVAProcessor.java @@ -71,7 +71,7 @@ public class OVAProcessor extends AdapterBase implements Processor { String result = command.execute(); if (result != null) { s_logger.info("failed to untar OVA package due to " + result + ". templatePath: " + templatePath + ", templateName: " + templateName); - return null; + throw new InternalErrorException("failed to untar OVA package"); } FormatInfo info = new FormatInfo(); diff --git a/core/src/com/cloud/storage/template/QCOW2Processor.java b/core/src/com/cloud/storage/template/QCOW2Processor.java index 2c66415bf96..096b0b6f32a 100644 --- a/core/src/com/cloud/storage/template/QCOW2Processor.java +++ b/core/src/com/cloud/storage/template/QCOW2Processor.java @@ -27,6 +27,7 @@ import java.util.Map; import javax.ejb.Local; import javax.naming.ConfigurationException; +import com.cloud.exception.InternalErrorException; import org.apache.log4j.Logger; import com.cloud.storage.Storage.ImageFormat; @@ -42,7 +43,7 @@ public class QCOW2Processor extends AdapterBase implements Processor { private StorageLayer _storage; @Override - public FormatInfo process(String templatePath, ImageFormat format, String templateName) { + public FormatInfo process(String templatePath, ImageFormat format, String templateName) throws InternalErrorException { if (format != null) { s_logger.debug("We currently don't handle conversion from " + format + " to QCOW2."); return null; @@ -64,10 +65,10 @@ public class QCOW2Processor extends AdapterBase implements Processor { info.size = _storage.getSize(qcow2Path); try { - info.virtualSize = getVirtualSize(qcow2File); + info.virtualSize = getTemplateVirtualSize(qcow2File); } catch (IOException e) { s_logger.error("Unable to get virtual size from " + qcow2File.getName()); - return null; + throw new InternalErrorException("unable to get virtual size from qcow2 file"); } return info; @@ -75,6 +76,16 @@ public class QCOW2Processor extends AdapterBase implements Processor { @Override public long getVirtualSize(File file) throws IOException { + try { + long size = getTemplateVirtualSize(file); + return size; + } catch (Exception e) { + s_logger.info("[ignored]" + "failed to get template virtual size for QCOW2: " + e.getLocalizedMessage()); + } + return file.length(); + } + + protected long getTemplateVirtualSize(File file) throws IOException { byte[] b = new byte[8]; try (FileInputStream strm = new FileInputStream(file)) { if (strm.skip(VIRTUALSIZE_HEADER_LOCATION) != VIRTUALSIZE_HEADER_LOCATION) { diff --git a/core/src/com/cloud/storage/template/TemplateLocation.java b/core/src/com/cloud/storage/template/TemplateLocation.java index ea785b206bd..e52a635dc68 100644 --- a/core/src/com/cloud/storage/template/TemplateLocation.java +++ b/core/src/com/cloud/storage/template/TemplateLocation.java @@ -180,7 +180,9 @@ public class TemplateLocation { deleteFormat(newInfo.format); if (!checkFormatValidity(newInfo)) { - s_logger.warn("Format is invalid "); + s_logger.warn("Format is invalid"); + s_logger.debug("Format: " + newInfo.format + " size: " + newInfo.size + " virtualsize: " + newInfo.virtualSize + " filename: " + newInfo.filename); + s_logger.debug("format, filename cannot be null and size, virtual size should be > 0 "); return false; } diff --git a/core/src/com/cloud/storage/template/VhdProcessor.java b/core/src/com/cloud/storage/template/VhdProcessor.java index 2974c75c190..0b68f895fd6 100644 --- a/core/src/com/cloud/storage/template/VhdProcessor.java +++ b/core/src/com/cloud/storage/template/VhdProcessor.java @@ -27,6 +27,7 @@ import java.util.Map; import javax.ejb.Local; import javax.naming.ConfigurationException; +import com.cloud.exception.InternalErrorException; import org.apache.log4j.Logger; import com.cloud.storage.Storage.ImageFormat; @@ -52,7 +53,7 @@ public class VhdProcessor extends AdapterBase implements Processor { private byte[][] citrixCreatorApp = { {0x74, 0x61, 0x70, 0x00}, {0x43, 0x54, 0x58, 0x53}}; /*"tap ", and "CTXS"*/ @Override - public FormatInfo process(String templatePath, ImageFormat format, String templateName) { + public FormatInfo process(String templatePath, ImageFormat format, String templateName) throws InternalErrorException { if (format != null) { s_logger.debug("We currently don't handle conversion from " + format + " to VHD."); return null; @@ -72,10 +73,10 @@ public class VhdProcessor extends AdapterBase implements Processor { info.size = _storage.getSize(vhdPath); try { - info.virtualSize = getVirtualSize(vhdFile); + info.virtualSize = getTemplateVirtualSize(vhdFile); } catch (IOException e) { s_logger.error("Unable to get the virtual size for " + vhdPath); - return null; + throw new InternalErrorException("unable to get virtual size from vhd file"); } return info; @@ -83,6 +84,16 @@ public class VhdProcessor extends AdapterBase implements Processor { @Override public long getVirtualSize(File file) throws IOException { + try { + long size = getTemplateVirtualSize(file); + return size; + } catch (Exception e) { + s_logger.info("[ignored]" + "failed to get template virtual size for VHD: " + e.getLocalizedMessage()); + } + return file.length(); + } + + protected long getTemplateVirtualSize(File file) throws IOException { byte[] currentSize = new byte[8]; byte[] creatorApp = new byte[4]; diff --git a/core/test/com/cloud/storage/template/OVAProcessorTest.java b/core/test/com/cloud/storage/template/OVAProcessorTest.java new file mode 100644 index 00000000000..f2f0f4712dc --- /dev/null +++ b/core/test/com/cloud/storage/template/OVAProcessorTest.java @@ -0,0 +1,138 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package com.cloud.storage.template; + +import com.cloud.exception.InternalErrorException; +import com.cloud.storage.Storage; +import com.cloud.storage.StorageLayer; +import com.cloud.utils.script.Script; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.junit4.PowerMockRunner; + +import java.io.File; +import java.util.HashMap; +import java.util.Map; + +@RunWith(PowerMockRunner.class) +@PrepareForTest(OVAProcessor.class) +public class OVAProcessorTest { + OVAProcessor processor; + + @Mock + StorageLayer mockStorageLayer; + + @Before + public void setUp() throws Exception { + processor = PowerMockito.spy(new OVAProcessor()); + Map params = new HashMap(); + params.put(StorageLayer.InstanceConfigKey, mockStorageLayer); + processor.configure("OVA Processor", params); + } + + @Test(expected = InternalErrorException.class) + public void testProcessWhenUntarFails() throws Exception { + String templatePath = "/tmp"; + String templateName = "template"; + + Mockito.when(mockStorageLayer.exists(Mockito.anyString())).thenReturn(true); + + Script mockScript = Mockito.mock(Script.class); + PowerMockito.whenNew(Script.class).withAnyArguments().thenReturn(mockScript); + PowerMockito.when(mockScript.execute()).thenReturn("error while untaring the file"); + + processor.process(templatePath, null, templateName); + } + + @Test(expected = InternalErrorException.class) + public void testProcessWhenVirtualSizeThrowsException() throws Exception { + String templatePath = "/tmp"; + String templateName = "template"; + + Mockito.when(mockStorageLayer.exists(Mockito.anyString())).thenReturn(true); + Mockito.when(mockStorageLayer.getSize(Mockito.anyString())).thenReturn(1000l); + Mockito.doThrow(new InternalErrorException("virtual size calculation failed")).when(processor).getTemplateVirtualSize(Mockito.anyString(), Mockito.anyString()); + + Script mockScript = Mockito.mock(Script.class); + PowerMockito.whenNew(Script.class).withAnyArguments().thenReturn(mockScript); + PowerMockito.when(mockScript.execute()).thenReturn(null); + + processor.process(templatePath, null, templateName); + } + + @Test + public void testProcess() throws Exception { + String templatePath = "/tmp"; + String templateName = "template"; + long virtualSize = 2000; + long actualSize = 1000; + + Mockito.when(mockStorageLayer.exists(Mockito.anyString())).thenReturn(true); + Mockito.when(mockStorageLayer.getSize(Mockito.anyString())).thenReturn(actualSize); + Mockito.doReturn(virtualSize).when(processor).getTemplateVirtualSize(Mockito.anyString(), Mockito.anyString()); + + Script mockScript = Mockito.mock(Script.class); + PowerMockito.whenNew(Script.class).withAnyArguments().thenReturn(mockScript); + PowerMockito.when(mockScript.execute()).thenReturn(null); + + Processor.FormatInfo info = processor.process(templatePath, null, templateName); + Assert.assertEquals(Storage.ImageFormat.OVA, info.format); + Assert.assertEquals("actual size:", actualSize, info.size); + Assert.assertEquals("virtual size:", virtualSize, info.virtualSize); + Assert.assertEquals("template name:", templateName + ".ova", info.filename); + } + + @Test + public void testGetVirtualSizeWhenVirtualSizeThrowsException() throws Exception { + long virtualSize = 2000; + long actualSize = 1000; + String templatePath = "/tmp"; + String templateName = "template"; + File mockFile = Mockito.mock(File.class); + Mockito.when(mockFile.length()).thenReturn(actualSize); + Mockito.when(mockFile.getParent()).thenReturn(templatePath); + Mockito.when(mockFile.getName()).thenReturn(templateName); + Mockito.doThrow(new InternalErrorException("virtual size calculation failed")).when(processor).getTemplateVirtualSize(templatePath, templateName); + Assert.assertEquals(actualSize, processor.getVirtualSize(mockFile)); + Mockito.verify(mockFile, Mockito.times(1)).length(); + } + + @Test + public void testGetVirtualSize() throws Exception { + long virtualSize = 2000; + long actualSize = 1000; + String templatePath = "/tmp"; + String templateName = "template"; + File mockFile = Mockito.mock(File.class); + Mockito.when(mockFile.length()).thenReturn(actualSize); + Mockito.when(mockFile.getParent()).thenReturn(templatePath); + Mockito.when(mockFile.getName()).thenReturn(templateName); + Mockito.doReturn(virtualSize).when(processor).getTemplateVirtualSize(templatePath, templateName); + Assert.assertEquals(virtualSize, processor.getVirtualSize(mockFile)); + Mockito.verify(mockFile, Mockito.times(0)).length(); + } + +} \ No newline at end of file diff --git a/core/test/com/cloud/storage/template/QCOW2ProcessorTest.java b/core/test/com/cloud/storage/template/QCOW2ProcessorTest.java new file mode 100644 index 00000000000..c268c413fc7 --- /dev/null +++ b/core/test/com/cloud/storage/template/QCOW2ProcessorTest.java @@ -0,0 +1,109 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package com.cloud.storage.template; + +import com.cloud.exception.InternalErrorException; +import com.cloud.storage.Storage; +import com.cloud.storage.StorageLayer; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.runners.MockitoJUnitRunner; + +import java.io.File; +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; + +@RunWith(MockitoJUnitRunner.class) +public class QCOW2ProcessorTest { + QCOW2Processor processor; + + @Mock + StorageLayer mockStorageLayer; + + @Before + public void setUp() throws Exception { + processor = Mockito.spy(new QCOW2Processor()); + Map params = new HashMap(); + params.put(StorageLayer.InstanceConfigKey, mockStorageLayer); + processor.configure("VHD Processor", params); + } + + @Test(expected = InternalErrorException.class) + public void testProcessWhenVirtualSizeThrowsException() throws Exception { + String templatePath = "/tmp"; + String templateName = "template"; + + Mockito.when(mockStorageLayer.exists(Mockito.anyString())).thenReturn(true); + File mockFile = Mockito.mock(File.class); + + Mockito.when(mockStorageLayer.getFile(Mockito.anyString())).thenReturn(mockFile); + Mockito.when(mockStorageLayer.getSize(Mockito.anyString())).thenReturn(1000L); + Mockito.doThrow(new IOException("virtual size calculation failed")).when(processor).getTemplateVirtualSize((File)Mockito.any()); + + processor.process(templatePath, null, templateName); + } + + @Test + public void testProcess() throws Exception { + String templatePath = "/tmp"; + String templateName = "template"; + long virtualSize = 2000; + long actualSize = 1000; + + Mockito.when(mockStorageLayer.exists(Mockito.anyString())).thenReturn(true); + File mockFile = Mockito.mock(File.class); + + Mockito.when(mockStorageLayer.getFile(Mockito.anyString())).thenReturn(mockFile); + Mockito.when(mockStorageLayer.getSize(Mockito.anyString())).thenReturn(actualSize); + Mockito.doReturn(virtualSize).when(processor).getTemplateVirtualSize((File)Mockito.any()); + + Processor.FormatInfo info = processor.process(templatePath, null, templateName); + Assert.assertEquals(Storage.ImageFormat.QCOW2, info.format); + Assert.assertEquals(actualSize, info.size); + Assert.assertEquals(virtualSize, info.virtualSize); + Assert.assertEquals(templateName + ".qcow2", info.filename); + } + + @Test + public void testGetVirtualSizeWhenVirtualSizeThrowsException() throws Exception { + long virtualSize = 2000; + long actualSize = 1000; + File mockFile = Mockito.mock(File.class); + Mockito.when(mockFile.length()).thenReturn(actualSize); + Mockito.doThrow(new IOException("virtual size calculation failed")).when(processor).getTemplateVirtualSize((File)Mockito.any()); + Assert.assertEquals(actualSize, processor.getVirtualSize(mockFile)); + Mockito.verify(mockFile, Mockito.times(1)).length(); + } + + @Test + public void testGetVirtualSize() throws Exception { + long virtualSize = 2000; + long actualSize = 1000; + File mockFile = Mockito.mock(File.class); + Mockito.when(mockFile.length()).thenReturn(actualSize); + Mockito.doReturn(virtualSize).when(processor).getTemplateVirtualSize((File)Mockito.any()); + Assert.assertEquals(virtualSize, processor.getVirtualSize(mockFile)); + Mockito.verify(mockFile, Mockito.times(0)).length(); + } +} \ No newline at end of file diff --git a/core/test/com/cloud/storage/template/VhdProcessorTest.java b/core/test/com/cloud/storage/template/VhdProcessorTest.java new file mode 100644 index 00000000000..b8b362a017b --- /dev/null +++ b/core/test/com/cloud/storage/template/VhdProcessorTest.java @@ -0,0 +1,110 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package com.cloud.storage.template; + +import com.cloud.exception.InternalErrorException; +import com.cloud.storage.Storage; +import com.cloud.storage.StorageLayer; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.runners.MockitoJUnitRunner; + +import java.io.File; +import java.io.IOException; +import java.util.HashMap; +import java.util.Map; + +@RunWith(MockitoJUnitRunner.class) +public class VhdProcessorTest { + VhdProcessor processor; + + @Mock + StorageLayer mockStorageLayer; + + @Before + public void setUp() throws Exception { + processor = Mockito.spy(new VhdProcessor()); + Map params = new HashMap(); + params.put(StorageLayer.InstanceConfigKey, mockStorageLayer); + processor.configure("VHD Processor", params); + } + + @Test(expected = InternalErrorException.class) + public void testProcessWhenVirtualSizeThrowsException() throws Exception { + String templatePath = "/tmp"; + String templateName = "template"; + + Mockito.when(mockStorageLayer.exists(Mockito.anyString())).thenReturn(true); + File mockFile = Mockito.mock(File.class); + + Mockito.when(mockStorageLayer.getFile(Mockito.anyString())).thenReturn(mockFile); + Mockito.when(mockStorageLayer.getSize(Mockito.anyString())).thenReturn(1000L); + Mockito.doThrow(new IOException("virtual size calculation failed")).when(processor).getTemplateVirtualSize((File) Mockito.any()); + + processor.process(templatePath, null, templateName); + } + + @Test + public void testProcess() throws Exception { + String templatePath = "/tmp"; + String templateName = "template"; + long virtualSize = 2000; + long actualSize = 1000; + + Mockito.when(mockStorageLayer.exists(Mockito.anyString())).thenReturn(true); + File mockFile = Mockito.mock(File.class); + + Mockito.when(mockStorageLayer.getFile(Mockito.anyString())).thenReturn(mockFile); + Mockito.when(mockStorageLayer.getSize(Mockito.anyString())).thenReturn(actualSize); + Mockito.doReturn(virtualSize).when(processor).getTemplateVirtualSize((File) Mockito.any()); + + Processor.FormatInfo info = processor.process(templatePath, null, templateName); + Assert.assertEquals(Storage.ImageFormat.VHD, info.format); + Assert.assertEquals(actualSize, info.size); + Assert.assertEquals(virtualSize, info.virtualSize); + Assert.assertEquals(templateName + ".vhd", info.filename); + } + + @Test + public void testGetVirtualSizeWhenVirtualSizeThrowsException() throws Exception { + long virtualSize = 2000; + long actualSize = 1000; + File mockFile = Mockito.mock(File.class); + Mockito.when(mockFile.length()).thenReturn(actualSize); + Mockito.doThrow(new IOException("virtual size calculation failed")).when(processor).getTemplateVirtualSize((File) Mockito.any()); + Assert.assertEquals(actualSize, processor.getVirtualSize(mockFile)); + Mockito.verify(mockFile,Mockito.times(1)).length(); + } + + @Test + public void testGetVirtualSize() throws Exception{ + long virtualSize = 2000; + long actualSize = 1000; + File mockFile = Mockito.mock(File.class); + Mockito.when(mockFile.length()).thenReturn(actualSize); + Mockito.doReturn(virtualSize).when(processor).getTemplateVirtualSize((File) Mockito.any()); + Assert.assertEquals(virtualSize, processor.getVirtualSize(mockFile)); + Mockito.verify(mockFile,Mockito.times(0)).length(); + } +} \ No newline at end of file diff --git a/services/secondary-storage/server/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java b/services/secondary-storage/server/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java index 3bc0de7e9df..431a20425a2 100644 --- a/services/secondary-storage/server/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java +++ b/services/secondary-storage/server/src/org/apache/cloudstack/storage/template/DownloadManagerImpl.java @@ -482,7 +482,10 @@ public class DownloadManagerImpl extends ManagerBase implements DownloadManager return e.toString(); } if (info != null) { - loc.addFormat(info); + if(!loc.addFormat(info)) { + loc.purge(); + return "Unable to install due to invalid file format"; + } dnld.setTemplatesize(info.virtualSize); dnld.setTemplatePhysicalSize(info.size); break;