Re: [PR] ssvm: delete temp directory while deleting entity download url [cloudstack]
DaanHoogland merged PR #12562: URL: https://github.com/apache/cloudstack/pull/12562 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] ssvm: delete temp directory while deleting entity download url [cloudstack]
shwstppr commented on PR #12562: URL: https://github.com/apache/cloudstack/pull/12562#issuecomment-3869532352 @RosiKyu can you please re-check. Should be okay with latest changes, ``` 2026-02-09T05:51:41,784 WARN [storage.template.UploadManagerImpl] (AgentRequest-Handler-6:[]) handleDeleteEntityDownloadURLCommand Path:volumes/2/3/1c3c53be-c155-47e4-8f44-6949bb4dd7f2.qcow2 Type:VOLUME 2026-02-09T05:51:41,785 DEBUG [storage.template.UploadManagerImpl] (AgentRequest-Handler-6:[]) Executing command [/bin/bash -c unlink /var/www/html/userdata/031ecc55-a58d-48c5-9ef1-28df687e8a9e/ROOT-3.qcow2 ]. 2026-02-09T05:51:41,798 DEBUG [storage.template.UploadManagerImpl] (AgentRequest-Handler-6:[]) Successfully executed process [4579] for command [/bin/bash -c unlink /var/www/html/userdata/031ecc55-a58d-48c5-9ef1-28df687e8a9e/ROOT-3.qcow2 ]. 2026-02-09T05:51:41,800 INFO [storage.template.UploadManagerImpl] (AgentRequest-Handler-6:[]) Deleting symlink root directory: 031ecc55-a58d-48c5-9ef1-28df687e8a9e for http://10.0.53.42/userdata/031ecc55-a58d-48c5-9ef1-28df687e8a9e/ROOT-3.qcow2 2026-02-09T05:51:41,803 WARN [storage.template.UploadManagerImpl] (AgentRequest-Handler-6:[]) null/volumes/2/3/1c3c53be-c155-47e4-8f44-6949bb4dd7f2.qcow2 2026-02-09T05:51:41,805 DEBUG [storage.template.UploadManagerImpl] (AgentRequest-Handler-6:[]) Executing command [/bin/bash -c rm -rf /mnt/SecStorage/f595fbca-4040-386a-9b7a-1e95584597fb/volumes/2/3/1c3c53be-c155-47e4-8f44-6949bb4dd7f2.qcow2 ]. 2026-02-09T05:51:42,004 DEBUG [storage.template.UploadManagerImpl] (AgentRequest-Handler-6:[]) Successfully executed process [4580] for command [/bin/bash -c rm -rf /mnt/SecStorage/f595fbca-4040-386a-9b7a-1e95584597fb/volumes/2/3/1c3c53be-c155-47e4-8f44-6949bb4dd7f2.qcow2 ]. root@s-2-VM:~# ls -lahi /var/www/html/userdata/ total 12K 131804 drwxr-xr-x 2 www-data www-data 4.0K Feb 9 05:51 . 131436 drwxr-xr-x 5 www-data www-data 4.0K Feb 9 05:27 .. 130933 -rwxr-xr-x 1 www-data www-data 17 Feb 6 09:30 .htaccess ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] ssvm: delete temp directory while deleting entity download url [cloudstack]
blueorangutan commented on PR #12562: URL: https://github.com/apache/cloudstack/pull/12562#issuecomment-3859341717 Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16726 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] ssvm: delete temp directory while deleting entity download url [cloudstack]
blueorangutan commented on PR #12562: URL: https://github.com/apache/cloudstack/pull/12562#issuecomment-3858933192 @shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] ssvm: delete temp directory while deleting entity download url [cloudstack]
shwstppr commented on PR #12562: URL: https://github.com/apache/cloudstack/pull/12562#issuecomment-3858928303 @blueorangutan package -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] ssvm: delete temp directory while deleting entity download url [cloudstack]
Copilot commented on code in PR #12562:
URL: https://github.com/apache/cloudstack/pull/12562#discussion_r2767281324
##
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)) {
Review Comment:
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.
##
services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/UploadManagerImpl.java:
##
@@ -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);
Review Comment:
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.
```suggestion
Path rootDirPath = Path.of(BASE_EXTRACT_DIR, rootDir);
```
##
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 doesNotDeleteWhenRootDir
Re: [PR] ssvm: delete temp directory while deleting entity download url [cloudstack]
shwstppr commented on PR #12562: URL: https://github.com/apache/cloudstack/pull/12562#issuecomment-3851200239 Thanks @RosiKyu . I was maybe working with main branch env for testing and didn't face the problem. I'll look into it and add a fix -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
