From eef3e49e217195fb741b0c42556288aaaeeaf6d9 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Sat, 31 Jan 2026 13:41:53 +0530 Subject: [PATCH] secondary-storage: delete temp directory while deleting entity download url When expired entity URLs are cleaned up, orphan directories are left behind. This change tries to delete the base temporary directory while cleanup. Signed-off-by: Abhishek Kumar --- .../storage/template/UploadManagerImpl.java | 44 +++++++++-- .../template/UploadManagerImplTest.java | 78 +++++++++++++++++++ .../main/java/com/cloud/utils/FileUtil.java | 15 ++++ 3 files changed, 131 insertions(+), 6 deletions(-) create mode 100644 services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/template/UploadManagerImplTest.java diff --git a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java index 828f61f89dc8..57a85c570949 100644 --- a/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java +++ b/services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java @@ -16,7 +16,10 @@ // under the License. package org.apache.cloudstack.storage.template; +import static com.cloud.utils.NumbersUtil.toHumanReadableSize; + import java.io.File; +import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; import java.nio.file.Path; @@ -30,10 +33,10 @@ import javax.naming.ConfigurationException; -import com.cloud.agent.api.Answer; - import org.apache.cloudstack.storage.resource.SecondaryStorageResource; +import org.apache.commons.lang3.StringUtils; +import com.cloud.agent.api.Answer; import com.cloud.agent.api.storage.CreateEntityDownloadURLAnswer; import com.cloud.agent.api.storage.CreateEntityDownloadURLCommand; import com.cloud.agent.api.storage.DeleteEntityDownloadURLCommand; @@ -48,15 +51,17 @@ import com.cloud.storage.template.TemplateUploader; import com.cloud.storage.template.TemplateUploader.Status; import com.cloud.storage.template.TemplateUploader.UploadCompleteCallback; +import com.cloud.utils.FileUtil; import com.cloud.utils.NumbersUtil; +import com.cloud.utils.UuidUtils; import com.cloud.utils.component.ManagerBase; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.script.Script; -import static com.cloud.utils.NumbersUtil.toHumanReadableSize; - public class UploadManagerImpl extends ManagerBase implements UploadManager { + protected static final String BASE_EXTRACT_DIR = "/var/www/html/userdata/"; + public class Completion implements UploadCompleteCallback { private final String jobId; @@ -266,7 +271,7 @@ public CreateEntityDownloadURLAnswer handleCreateEntityURLCommand(CreateEntityDo return new CreateEntityDownloadURLAnswer(errorString, CreateEntityDownloadURLAnswer.RESULT_FAILURE); } // Create the directory structure so that its visible under apache server root - String extractDir = "/var/www/html/userdata/"; + String extractDir = BASE_EXTRACT_DIR; extractDir = extractDir + cmd.getFilepathInExtractURL() + File.separator; Script command = new Script("/bin/su", logger); command.add("-s"); @@ -330,12 +335,15 @@ public Answer handleDeleteEntityDownloadURLCommand(DeleteEntityDownloadURLComman String extractUrl = cmd.getExtractUrl(); String result; if (extractUrl != null) { - command.add("unlink /var/www/html/userdata/" + extractUrl.substring(extractUrl.lastIndexOf(File.separator) + 1)); + String linkPath = extractUrl.substring(extractUrl.lastIndexOf(File.separator) + 1); + command.add("unlink " + BASE_EXTRACT_DIR + linkPath); result = command.execute(); if (result != null) { // FIXME - Ideally should bail out if you can't delete symlink. Not doing it right now. // This is because the ssvm might already be destroyed and the symlinks do not exist. logger.warn("Error in deleting symlink :" + result); + } else { + deleteEntitySymlinkRootPathIfNeeded(cmd, linkPath); } } @@ -356,6 +364,30 @@ public Answer handleDeleteEntityDownloadURLCommand(DeleteEntityDownloadURLComman return new Answer(cmd, true, ""); } + protected void deleteEntitySymlinkRootPathIfNeeded(DeleteEntityDownloadURLCommand cmd, String linkPath) { + if (StringUtils.isEmpty(linkPath)) { + return; + } + String[] parts = linkPath.split("/"); + if (parts.length == 0) { + return; + } + String rootDir = parts[0]; + if (StringUtils.isEmpty(rootDir) || !UuidUtils.isUuid(rootDir)) { + return; + } + logger.info("Deleting symlink root directory: {} for {}", rootDir, cmd.getExtractUrl()); + Path rootDirPath = Path.of(BASE_EXTRACT_DIR + rootDir); + String failMsg = "Failed to delete symlink root directory: {} for {}"; + try { + if (!FileUtil.deleteRecursively(rootDirPath)) { + logger.warn(failMsg, rootDir, cmd.getExtractUrl()); + } + } catch (IOException e) { + logger.warn(failMsg, rootDir, cmd.getExtractUrl(), e); + } + } + private String getInstallPath(String jobId) { // TODO Auto-generated method stub return null; diff --git a/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/template/UploadManagerImplTest.java b/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/template/UploadManagerImplTest.java new file mode 100644 index 000000000000..045e1df83a8f --- /dev/null +++ b/services/secondary-storage/server/src/test/java/org/apache/cloudstack/storage/template/UploadManagerImplTest.java @@ -0,0 +1,78 @@ +// 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 org.apache.cloudstack.storage.template; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; + +import java.nio.file.Path; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.agent.api.storage.DeleteEntityDownloadURLCommand; +import com.cloud.utils.FileUtil; + +@RunWith(MockitoJUnitRunner.class) +public class UploadManagerImplTest { + + @InjectMocks + UploadManagerImpl uploadManager; + + MockedStatic fileUtilMock; + + @Before + public void setup() { + fileUtilMock = mockStatic(FileUtil.class, Mockito.CALLS_REAL_METHODS); + fileUtilMock.when(() -> FileUtil.deleteRecursively(any(Path.class))).thenReturn(true); + } + + @After + public void tearDown() { + fileUtilMock.close(); + } + + @Test + public void doesNotDeleteWhenLinkPathIsEmpty() { + String emptyLinkPath = ""; + uploadManager.deleteEntitySymlinkRootPathIfNeeded(mock(DeleteEntityDownloadURLCommand.class), emptyLinkPath); + fileUtilMock.verify(() -> FileUtil.deleteRecursively(any(Path.class)), never()); + } + + @Test + public void doesNotDeleteWhenRootDirIsNotUuid() { + String invalidLinkPath = "invalidRootDir/file"; + uploadManager.deleteEntitySymlinkRootPathIfNeeded(mock(DeleteEntityDownloadURLCommand.class), invalidLinkPath); + fileUtilMock.verify(() -> FileUtil.deleteRecursively(any(Path.class)), never()); + } + + @Test + public void deletesSymlinkRootDirectoryWhenValidUuid() { + String validLinkPath = "123e4567-e89b-12d3-a456-426614174000/file"; + uploadManager.deleteEntitySymlinkRootPathIfNeeded(mock(DeleteEntityDownloadURLCommand.class), validLinkPath); + fileUtilMock.verify(() -> FileUtil.deleteRecursively(any(Path.class)), times(1)); + } +} diff --git a/utils/src/main/java/com/cloud/utils/FileUtil.java b/utils/src/main/java/com/cloud/utils/FileUtil.java index eea7c0f85617..8a1d3d32ec19 100644 --- a/utils/src/main/java/com/cloud/utils/FileUtil.java +++ b/utils/src/main/java/com/cloud/utils/FileUtil.java @@ -160,4 +160,19 @@ public static boolean writeToFile(String fileName, String content) { public static String readResourceFile(String resource) throws IOException { return IOUtils.toString(Objects.requireNonNull(Thread.currentThread().getContextClassLoader().getResourceAsStream(resource)), com.cloud.utils.StringUtils.getPreferredCharset()); } + + public static boolean deleteRecursively(Path path) throws IOException { + LOGGER.debug("Deleting path: {}", path); + if (Files.isDirectory(path)) { + try (Stream entries = Files.list(path)) { + List list = entries.collect(Collectors.toList()); + for (Path entry : list) { + if (!deleteRecursively(entry)) { + return false; + } + } + } + } + return Files.deleteIfExists(path); + } }