Fix automatic SystemVM template download to S3 secondary storage

api/src/main/java/org/apache/cloudstack/api/command/admin/storage/AddImageStoreS3CMD.java

@@ -46,6 +46,7 @@
 import org.apache.cloudstack.api.Parameter;
 import org.apache.cloudstack.api.ServerApiException;
 import org.apache.cloudstack.api.response.ImageStoreResponse;
+import org.apache.cloudstack.api.response.ZoneResponse;
 
 import com.cloud.exception.ConcurrentOperationException;
 import com.cloud.exception.DiscoveryException;
@@ -61,6 +62,9 @@ public final class AddImageStoreS3CMD extends BaseCmd implements ClientOptions {
 
     private static final String s_name = "addImageStoreS3Response";
 
+    @Parameter(name = ApiConstants.ZONE_ID, type = CommandType.UUID, entityType = ZoneResponse.class, description = "The Zone ID for the S3 image store")
+    private Long zoneId;
+
     @Parameter(name = S3_ACCESS_KEY, type = STRING, required = true, description = "S3 access key")
     private String accessKey;
 
@@ -128,7 +132,7 @@ public void execute() throws ResourceUnavailableException, InsufficientCapacityE
         }
 
         try{
-            ImageStore result = _storageService.discoverImageStore(null, null, "S3", null, dm);
+            ImageStore result = _storageService.discoverImageStore(null, null, "S3", zoneId, dm);
             ImageStoreResponse storeResponse;
             if (result != null) {
                 storeResponse = _responseGenerator.createImageStoreResponse(result);

api/src/main/java/org/apache/cloudstack/api/response/ImageStoreResponse.java

@@ -71,6 +71,14 @@ public class ImageStoreResponse extends BaseResponseWithAnnotations {
     @Param(description = "The host's currently used disk size")
     private Long diskSizeUsed;
 
+    @SerializedName(ApiConstants.S3_END_POINT)
+    @Param(description = "The S3 endpoint URL")
+    private String s3Endpoint;
+
+    @SerializedName(ApiConstants.S3_BUCKET_NAME)
+    @Param(description = "The S3 bucket name")
+    private String s3BucketName;
+
     public ImageStoreResponse() {
     }
 
@@ -156,4 +164,20 @@ public void setDiskSizeTotal(Long diskSizeTotal) {
     public void setDiskSizeUsed(Long diskSizeUsed) {
         this.diskSizeUsed = diskSizeUsed;
     }
+
+    public String getS3Endpoint() {
+        return s3Endpoint;
+    }
+
+    public void setS3Endpoint(String s3Endpoint) {
+        this.s3Endpoint = s3Endpoint;
+    }
+
+    public String getS3BucketName() {
+        return s3BucketName;
+    }
+
+    public void setS3BucketName(String s3BucketName) {
+        this.s3BucketName = s3BucketName;
+    }
 }

core/src/main/java/com/cloud/storage/template/S3TemplateDownloader.java

@@ -215,9 +215,16 @@ public void progressChanged(ProgressEvent progressEvent) {
         try {
             // Wait for the upload to complete.
             upload.waitForCompletion();
+            // Set status directly to avoid race condition.
+            status = Status.DOWNLOAD_FINISHED;
         } catch (InterruptedException e) {
-            // Interruption while waiting for the upload to complete.
-            logger.warn("Interruption occurred while waiting for upload of " + downloadUrl + " to complete");
+            errorString = "Interruption occurred while waiting for upload of " + downloadUrl + " to complete";
+            logger.warn(errorString);
+            status = Status.UNRECOVERABLE_ERROR;
+        } catch (Exception e) {
+            errorString = "S3 upload failed for " + downloadUrl + ": " + e.getMessage();
+            logger.warn(errorString, e);
+            status = Status.UNRECOVERABLE_ERROR;
         }
 
         downloadTime = new Date().getTime() - start.getTime();

engine/schema/src/main/java/com/cloud/upgrade/SystemVmTemplateRegistration.java

@@ -959,6 +959,10 @@ public void registerTemplates(List<Pair<Hypervisor.HypervisorType, CPU.CPUArch>>
                     @Override
                     public void doInTransactionWithoutResult(final TransactionStatus status) {
                         List<Long> zoneIds = getEligibleZoneIds();
+                        if (zoneIds.isEmpty()) {
+                            updateSystemVmTemplateUrlsForNonNfsStores();
+                            return;
+                        }
                         for (Long zoneId : zoneIds) {
                             String filePath = null;
                             try {
@@ -984,6 +988,21 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
         }
     }
 
+    public void updateSystemVmTemplateUrlsForNonNfsStores() {
+        for (Pair<Hypervisor.HypervisorType, CPU.CPUArch> hypervisorArch :
+                clusterDao.listDistinctHypervisorsArchAcrossClusters(null)) {
+            MetadataTemplateDetails templateDetails = getMetadataTemplateDetails(hypervisorArch.first(), hypervisorArch.second());
+            if (templateDetails == null) {
+                continue;
+            }
+            VMTemplateVO templateVO = vmTemplateDao.findLatestTemplateByTypeAndHypervisorAndArch(
+                    templateDetails.getHypervisorType(), templateDetails.getArch(), Storage.TemplateType.SYSTEM);
+            if (templateVO != null) {
+                updateTemplateUrlChecksumAndGuestOsId(templateVO, templateDetails);
+            }
+        }
+    }
+
     private void updateRegisteredTemplateDetails(Long templateId, MetadataTemplateDetails templateDetails) {
         VMTemplateVO templateVO = vmTemplateDao.findById(templateId);
         templateVO.setTemplateType(Storage.TemplateType.SYSTEM);

engine/storage/src/main/java/org/apache/cloudstack/storage/endpoint/DefaultEndPointSelector.java

@@ -400,8 +400,8 @@ 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) {
-                return LocalHostEndpoint.getEndpoint(); // for bootstrap system vm template downloading to region image store
+            if (tmplInfo.getTemplateType() == TemplateType.SYSTEM) {
+                return LocalHostEndpoint.getEndpoint(); // for bootstrap system vm template downloading when no SSVM is available
             }
         }
         return null;

plugins/storage/image/s3/src/main/java/org/apache/cloudstack/storage/datastore/lifecycle/S3ImageStoreLifeCycleImpl.java

@@ -22,6 +22,7 @@
 
 import javax.inject.Inject;
 
+import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
 
@@ -72,28 +73,34 @@ public S3ImageStoreLifeCycleImpl() {
     @Override
     public DataStore initialize(Map<String, Object> dsInfos) {
 
+        Long dcId = (Long)dsInfos.get("zoneId");
         String url = (String)dsInfos.get("url");
         String name = (String)dsInfos.get("name");
         String providerName = (String)dsInfos.get("providerName");
         ScopeType scope = (ScopeType)dsInfos.get("scope");
         DataStoreRole role = (DataStoreRole)dsInfos.get("role");
         Map<String, String> details = (Map<String, String>)dsInfos.get("details");
 
-        logger.info("Trying to add a S3 store with endpoint: " + details.get(ApiConstants.S3_END_POINT));
+        String endPoint = details.get(ApiConstants.S3_END_POINT);
+        String bucketName = details.get(ApiConstants.S3_BUCKET_NAME);
+        logger.info("Trying to add a S3 store with endpoint: " + endPoint);
+
+        if (StringUtils.isEmpty(url) && StringUtils.isNotEmpty(endPoint) && StringUtils.isNotEmpty(bucketName)) {
+            url = "s3://" + endPoint.replaceFirst("^https?://", "") + "/" + bucketName;
+        }
+        if (StringUtils.isEmpty(name) && StringUtils.isNotEmpty(url)) {
+            name = url;
+        }
 
         Map<String, Object> imageStoreParameters = new HashMap();
         imageStoreParameters.put("name", name);
+        imageStoreParameters.put("zoneId", dcId);
         imageStoreParameters.put("url", url);
-        String protocol = "http";
-        String useHttps = details.get(ApiConstants.S3_HTTPS_FLAG);
-        if (useHttps != null && Boolean.parseBoolean(useHttps)) {
-            protocol = "https";
-        }
-        imageStoreParameters.put("protocol", protocol);
+        imageStoreParameters.put("protocol", "s3");
         if (scope != null) {
             imageStoreParameters.put("scope", scope);
         } else {
-            imageStoreParameters.put("scope", ScopeType.REGION);
+            imageStoreParameters.put("scope", ScopeType.ZONE);
         }
         imageStoreParameters.put("providerName", providerName);
         imageStoreParameters.put("role", role);

plugins/storage/image/s3/src/main/java/org/apache/cloudstack/storage/datastore/provider/S3ImageStoreProviderImpl.java

@@ -90,9 +90,7 @@ public Set<DataStoreProviderType> getTypes() {
 
     @Override
     public boolean isScopeSupported(ScopeType scope) {
-        if (scope == ScopeType.REGION)
-            return true;
-        return false;
+        return scope == ScopeType.ZONE || scope == ScopeType.REGION;
     }
 
     @Override

server/src/main/java/com/cloud/api/query/dao/ImageStoreJoinDaoImpl.java

@@ -18,6 +18,7 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
 
 import javax.inject.Inject;
 
@@ -26,7 +27,10 @@
 import com.cloud.user.AccountManager;
 import org.apache.cloudstack.annotation.AnnotationService;
 import org.apache.cloudstack.annotation.dao.AnnotationDao;
+import org.apache.cloudstack.api.ApiConstants;
 import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreProvider;
+import org.apache.cloudstack.storage.datastore.db.ImageStoreDetailsDao;
 import org.springframework.stereotype.Component;
 
 import org.apache.cloudstack.api.response.ImageStoreResponse;
@@ -48,6 +52,8 @@ public class ImageStoreJoinDaoImpl extends GenericDaoBase<ImageStoreJoinVO, Long
     private AnnotationDao annotationDao;
     @Inject
     private AccountManager accountManager;
+    @Inject
+    private ImageStoreDetailsDao imageStoreDetailsDao;
 
     private final SearchBuilder<ImageStoreJoinVO> dsSearch;
 
@@ -92,6 +98,14 @@ public ImageStoreResponse newImageStoreResponse(ImageStoreJoinVO ids) {
         osResponse.setHasAnnotation(annotationDao.hasAnnotations(ids.getUuid(), AnnotationService.EntityType.SECONDARY_STORAGE.name(),
                 accountManager.isRootAdmin(CallContext.current().getCallingAccount().getId())));
 
+        if (DataStoreProvider.S3_IMAGE.equalsIgnoreCase(ids.getProviderName())) {
+            Map<String, String> s3Details = imageStoreDetailsDao.getDetails(ids.getId());
+            if (s3Details != null) {
+                osResponse.setS3Endpoint(s3Details.get(ApiConstants.S3_END_POINT));
+                osResponse.setS3BucketName(s3Details.get(ApiConstants.S3_BUCKET_NAME));
+            }
+        }
+
         osResponse.setObjectName("imagestore");
         return osResponse;
     }

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

@@ -3537,16 +3537,6 @@ public ImageStore discoverImageStore(String name, String url, String providerNam
             throw new InvalidParameterValueException("Image store provider " + providerName + " does not support scope " + scopeType);
         }
 
-        // check if we have already image stores from other different providers,
-        // we currently are not supporting image stores from different
-        // providers co-existing
-        List<ImageStoreVO> imageStores = _imageStoreDao.listImageStores();
-        for (ImageStoreVO store : imageStores) {
-            if (!store.getProviderName().equalsIgnoreCase(providerName)) {
-                throw new InvalidParameterValueException("You can only add new image stores from the same provider " + store.getProviderName() + " already added");
-            }
-        }
-
         if (zoneId != null) {
             // Check if the zone exists in the system
             DataCenterVO zone = _dcDao.findById(zoneId);
@@ -3585,6 +3575,7 @@ public ImageStore discoverImageStore(String name, String url, String providerNam
 
         if (((ImageStoreProvider)storeProvider).needDownloadSysTemplate()) {
             // trigger system vm template download
+            new SystemVmTemplateRegistration().updateSystemVmTemplateUrlsForNonNfsStores();
             _imageSrv.downloadBootstrapSysTemplate(store);
         } else {
             // populate template_store_ref table

services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java

@@ -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("/");
 

services/secondary-storage/controller/src/test/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImplTest.java

@@ -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;

services/secondary-storage/server/src/main/java/org/apache/cloudstack/storage/template/DownloadManagerImpl.java

@@ -316,6 +316,8 @@ public void setDownloadStatus(String jobId, Status status) {
                 // For S3 and Swift, which are considered "remote",
                 // as in the file cannot be accessed locally,
                 // we run the postRemoteDownload() method.
+                // Set early so status polls see DOWNLOADED; overridden on error below
+                td.setStatus(Status.POST_DOWNLOAD_FINISHED);
                 td.setDownloadError("Download success, starting install ");
                 String result = postRemoteDownload(jobId);
                 if (result != null) {
@@ -324,7 +326,6 @@ public void setDownloadStatus(String jobId, Status status) {
                     td.setDownloadError("Failed post download install: " + result);
                     ((S3TemplateDownloader) td).cleanupAfterError();
                 } else {
-                    td.setStatus(Status.POST_DOWNLOAD_FINISHED);
                     td.setDownloadError("Install completed successfully at " + new SimpleDateFormat().format(new Date()));
                 }
             } else {

ui/src/config/section/infra/secondaryStorages.js

@@ -36,7 +36,7 @@ export default {
     return fields
   },
   details: () => {
-    var fields = ['name', 'id', 'url', 'protocol', 'provider', 'scope', 'zonename']
+    var fields = ['name', 'id', 'url', 'protocol', 'provider', 'scope', 'zonename', 'endpoint', 'bucket']
     if (store.getters.apis.listImageStores.params.filter(x => x.name === 'readonly').length > 0) {
       fields.push('readonly')
     }

ui/src/views/infra/AddSecondaryStorage.vue

@@ -347,7 +347,7 @@ export default {
           data.url = url
         }
         data.provider = provider
-        if (values.zone && !['Swift', 'S3'].includes(provider)) {
+        if (values.zone && !['Swift'].includes(provider)) {
           data.zoneid = values.zone
         }
 

ui/src/views/infra/zone/ZoneWizardAddResources.vue

@@ -1094,19 +1094,12 @@ export default {
       })
     },
     fetchProvider () {
-      const storageProviders = []
-      api('listImageStores', { provider: 'S3' }).then(json => {
-        const s3stores = json.listimagestoresresponse.imagestore
-        if (s3stores != null && s3stores.length > 0) {
-          storageProviders.push({ id: 'S3', description: 'S3' })
-        } else {
-          storageProviders.push({ id: 'NFS', description: 'NFS' })
-          storageProviders.push({ id: 'SMB', description: 'SMB/CIFS' })
-          storageProviders.push({ id: 'S3', description: 'S3' })
-          storageProviders.push({ id: 'Swift', description: 'Swift' })
-        }
-        this.storageProviders = storageProviders
-      })
+      this.storageProviders = [
+        { id: 'NFS', description: 'NFS' },
+        { id: 'SMB', description: 'SMB/CIFS' },
+        { id: 'S3', description: 'S3' },
+        { id: 'Swift', description: 'Swift' }
+      ]
     },
     fetchPrimaryStorageProvider () {
       this.primaryStorageProviders = []

ui/src/views/infra/zone/ZoneWizardLaunchZone.vue

@@ -1596,6 +1596,7 @@ export default {
         params['details[2].value'] = this.prefillContent.secondaryStorageSMBDomain
       } else if (this.prefillContent.secondaryStorageProvider === 'S3') {
         params.provider = this.prefillContent.secondaryStorageProvider
+        params.zoneid = this.stepData.zoneReturned.id
         params['details[0].key'] = 'accesskey'
         params['details[0].value'] = this.prefillContent.secondaryStorageAccessKey
         params['details[1].key'] = 'secretkey'
@@ -2195,13 +2196,11 @@ export default {
     },
     createSecondaryStagingStore (args) {
       return new Promise((resolve, reject) => {
-        let message = ''
-
         api('createSecondaryStagingStore', args).then(json => {
-          const result = json.addimagestoreresponse.secondarystorage
+          const result = json.createsecondarystagingstoreresponse.secondarystorage
           resolve(result)
         }).catch(error => {
-          message = error.response.headers['x-description']
+          const message = error?.response?.headers?.['x-description'] || error.message || error
           reject(message)
         })
       })