Skip to content

ssvm: delete temp directory while deleting entity download url #12562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
shwstppr wants to merge 1 commit into
base: 4.20
Choose a base branch
from
+131 −6
Draft

ssvm: delete temp directory while deleting entity download url #12562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 38 additions & 6 deletions ...torage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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);
Comment on lines +338 to +346
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linkPath extraction logic is incorrect. Given extractUrl format http://host/userdata/UUID/filename, using lastIndexOf(File.separator) will find the last "/" before the filename, resulting in linkPath being just "filename" instead of "UUID/filename". This breaks the deleteEntitySymlinkRootPathIfNeeded method which expects linkPath to contain the UUID directory name as the first path component. The code should extract everything after "/userdata/" using extractUrl.substring(extractUrl.indexOf("/userdata/") + "/userdata/".length()) instead, with proper validation that "/userdata/" exists in the URL.

Suggested change
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);
final String userDataSegment = "/userdata/";
int userDataIndex = extractUrl.indexOf(userDataSegment);
if (userDataIndex == -1) {
logger.warn("Extract URL does not contain expected segment '/userdata/': " + extractUrl);
} else {
String linkPath = extractUrl.substring(userDataIndex + userDataSegment.length());
if (StringUtils.isBlank(linkPath)) {
logger.warn("Extract URL does not contain a valid link path after '/userdata/': " + extractUrl);
} else {
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);
}
}

Copilot uses AI. Check for mistakes.
}
}

Expand All @@ -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);
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using Path.of(BASE_EXTRACT_DIR, rootDir) instead of string concatenation for path construction. This is more robust and handles path separators correctly across different operating systems, even if BASE_EXTRACT_DIR is modified in the future to not include a trailing separator.

Suggested change
Path rootDirPath = Path.of(BASE_EXTRACT_DIR + rootDir);
Path rootDirPath = Path.of(BASE_EXTRACT_DIR, rootDir);

Copilot uses AI. Check for mistakes.
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;
Expand Down
78 changes: 78 additions & 0 deletions ...ge/server/src/test/java/org/apache/cloudstack/storage/template/UploadManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<FileUtil> 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));
}
Comment on lines +72 to +77
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a test case for linkPath that contains only a UUID without a filename (e.g., "123e4567-e89b-12d3-a456-426614174000"). While this may not occur in normal operation given the URL format, it's a valid edge case that should be handled correctly.

Copilot uses AI. Check for mistakes.
}
15 changes: 15 additions & 0 deletions utils/src/main/java/com/cloud/utils/FileUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deleteRecursively method may behave unexpectedly when encountering symbolic links to directories. Files.isDirectory(path) follows symlinks by default, which means if a symlink points to a directory, this method will recursively delete the contents of the target directory instead of just removing the symlink. Consider using Files.isDirectory(path, LinkOption.NOFOLLOW_LINKS) to treat symlinks as files and delete them directly without following them.

Copilot uses AI. Check for mistakes.
try (Stream<Path> entries = Files.list(path)) {
List<Path> list = entries.collect(Collectors.toList());
for (Path entry : list) {
if (!deleteRecursively(entry)) {
return false;
}
}
}
}
return Files.deleteIfExists(path);
}
}
Loading