Skip to content

Fix automatic SystemVM template download to S3 secondary storage #12426

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

Open
Damans227 wants to merge 4 commits into
base: 4.20
Choose a base branch
from
+48 −1
Open

Fix automatic SystemVM template download to S3 secondary storage #12426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
387f958
Allow endpoint selection logic for system templates in region and zon…
Jan 14, 2026
dd39393
Skip null or missing URLs for S3
Jan 14, 2026
d6bf807
Enable path-style access for S3-compatible storage
Jan 14, 2026
9a2ba84
Add test for handling null entries in secondary storage
Jan 14, 2026
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
4 changes: 3 additions & 1 deletion ...storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,9 @@ public EndPoint select(DataObject object) {
}
if (object instanceof TemplateInfo) {
TemplateInfo tmplInfo = (TemplateInfo)object;
if (store.getScope().getScopeType() == ScopeType.ZONE && store.getScope().getScopeId() == null && tmplInfo.getTemplateType() == TemplateType.SYSTEM) {
if (tmplInfo.getTemplateType() == TemplateType.SYSTEM &&
(store.getScope().getScopeType() == ScopeType.REGION ||
(store.getScope().getScopeType() == ScopeType.ZONE && store.getScope().getScopeId() == null))) {
Comment on lines +403 to +405
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The modified endpoint selection logic for REGION-scoped SYSTEM templates lacks test coverage. Consider adding unit tests in the engine/storage module to verify that LocalHostEndpoint is correctly returned for REGION-scoped stores with SYSTEM templates, similar to the existing test coverage in SecondaryStorageManagerImplTest.

Copilot uses AI. Check for mistakes.
return LocalHostEndpoint.getEndpoint(); // for bootstrap system vm template downloading to region image store
}
}
Expand Down
4 changes: 4 additions & 0 deletions ...ler/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1247,6 +1247,10 @@ public boolean finalizeVirtualMachineProfile(VirtualMachineProfile profile, Depl
protected void addSecondaryStorageServerAddressToBuffer(StringBuilder buffer, List<DataStore> dataStores, String vmName) {
List<String> addresses = new ArrayList<>();
for (DataStore dataStore: dataStores) {
// S3 and other object stores may not have a URL, so better to skip them
if (dataStore == null || dataStore.getTO() == null || dataStore.getTO().getUrl() == null) {
continue;
}
String url = dataStore.getTO().getUrl();
String[] urlArray = url.split("/");

Expand Down
39 changes: 39 additions & 0 deletions ...src/test/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,45 @@ public void testAddSecondaryStorageServerAddressToBufferInvalidAddress() {
runAddSecondaryStorageServerAddressToBufferTest(addresses, StringUtils.join(List.of(randomIp1, randomIp2), ","));
}

@Test
public void testAddSecondaryStorageServerAddressToBufferWithNullEntries() {
String randomIp1 = InetAddresses.fromInteger(secureRandom.nextInt()).getHostAddress();
String randomIp2 = InetAddresses.fromInteger(secureRandom.nextInt()).getHostAddress();

List<DataStore> dataStores = new ArrayList<>();

DataStore validStore1 = Mockito.mock(DataStore.class);
DataStoreTO validStoreTO1 = Mockito.mock(DataStoreTO.class);
when(validStoreTO1.getUrl()).thenReturn(String.format("http://%s", randomIp1));
when(validStore1.getTO()).thenReturn(validStoreTO1);
dataStores.add(validStore1);

dataStores.add(null);

DataStore nullToStore = Mockito.mock(DataStore.class);
when(nullToStore.getTO()).thenReturn(null);
dataStores.add(nullToStore);

DataStore nullUrlStore = Mockito.mock(DataStore.class);
DataStoreTO nullUrlStoreTO = Mockito.mock(DataStoreTO.class);
when(nullUrlStoreTO.getUrl()).thenReturn(null);
when(nullUrlStore.getTO()).thenReturn(nullUrlStoreTO);
dataStores.add(nullUrlStore);

DataStore validStore2 = Mockito.mock(DataStore.class);
DataStoreTO validStoreTO2 = Mockito.mock(DataStoreTO.class);
when(validStoreTO2.getUrl()).thenReturn(String.format("http://%s", randomIp2));
when(validStore2.getTO()).thenReturn(validStoreTO2);
dataStores.add(validStore2);

StringBuilder builder = new StringBuilder();
secondaryStorageManager.addSecondaryStorageServerAddressToBuffer(builder, dataStores, "VM");
String result = builder.toString();
result = result.contains("=") ? result.split("=")[1] : null;

assertEquals(StringUtils.join(List.of(randomIp1, randomIp2), ","), result);
}

@Test
public void testCreateSecondaryStorageVm_New() {
long dataCenterId = 1L;
Expand Down
2 changes: 2 additions & 0 deletions utils/src/main/java/com/cloud/utils/storage/S3/S3Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ public static TransferManager getTransferManager(final ClientOptions clientOptio
LOGGER.debug(format("Setting the end point for S3 client with access key %1$s to %2$s.", clientOptions.getAccessKey(), clientOptions.getEndPoint()));

client.setEndpoint(clientOptions.getEndPoint());
// Enable path-style access for S3-compatible storage
client.setS3ClientOptions(com.amazonaws.services.s3.S3ClientOptions.builder().setPathStyleAccess(true).build());
Copy link
Member

@vishesh92 vishesh92 Jan 16, 2026

Choose a reason for hiding this comment

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

Why is this required?

Comment on lines +117 to +118
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

Path-style access is being enabled unconditionally for all S3 endpoints, including AWS S3 which deprecated path-style access in favor of virtual-hosted-style. This could cause compatibility issues with AWS S3. Consider making path-style access configurable through ClientOptions, or only enabling it when a custom endpoint is detected (non-AWS S3).

Copilot uses AI. Check for mistakes.
}

TRANSFERMANAGER_ACCESSKEY_MAP.put(clientOptions.getAccessKey(), new TransferManager(client));
Expand Down
Loading