From 4ef7ebbded87bcc5db04678d0b63e590aca7e128 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 12 Jun 2023 12:32:42 +0530 Subject: [PATCH] ssvm: pass all accessible secondary storage to ssvm (#7410) * ssvm: pass all accessible secondary storage to ssvm Fixes #7162 Signed-off-by: Abhishek Kumar * changes Signed-off-by: Abhishek Kumar * test Signed-off-by: Abhishek Kumar * license Signed-off-by: Abhishek Kumar --------- Signed-off-by: Abhishek Kumar --- .../SecondaryStorageManagerImpl.java | 49 ++++++---- .../SecondaryStorageManagerImplTest.java | 89 +++++++++++++++++++ systemvm/agent/scripts/ssvm-check.sh | 27 +++--- 3 files changed, 139 insertions(+), 26 deletions(-) create mode 100644 services/secondary-storage/controller/src/test/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImplTest.java diff --git a/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java b/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java index aa03211d9c0..f93d3e28a38 100644 --- a/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java +++ b/services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java @@ -26,6 +26,7 @@ import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import javax.inject.Inject; import javax.naming.ConfigurationException; @@ -1065,11 +1066,12 @@ public class SecondaryStorageManagerImpl extends ManagerBase implements Secondar Map details = _vmDetailsDao.listDetailsKeyPairs(vm.getId()); vm.setDetails(details); - DataStore secStore = _dataStoreMgr.getImageStoreWithFreeCapacity(dest.getDataCenter().getId()); - if (secStore == null) { + List secStores= _dataStoreMgr.listImageStoresWithFreeCapacity(dest.getDataCenter().getId()); + if (CollectionUtils.isEmpty(secStores)) { s_logger.warn(String.format("Unable to finalize virtual machine profile [%s] as it has no secondary storage available to satisfy storage needs for zone [%s].", profile.toString(), dest.getDataCenter().getUuid())); return false; } + Collections.shuffle(secStores); final Map sshAccessDetails = _networkMgr.getSystemVMAccessDetails(profile.getVirtualMachine()); final Map ipAddressDetails = new HashMap<>(sshAccessDetails); @@ -1163,7 +1165,7 @@ public class SecondaryStorageManagerImpl extends ManagerBase implements Secondar if (dc.getDns2() != null) { buf.append(" dns2=").append(dc.getDns2()); } - String nfsVersion = imageStoreDetailsUtil != null ? imageStoreDetailsUtil.getNfsVersion(secStore.getId()) : null; + String nfsVersion = imageStoreDetailsUtil != null ? imageStoreDetailsUtil.getNfsVersion(secStores.get(0).getId()) : null; buf.append(" nfsVersion=").append(nfsVersion); buf.append(" keystore_password=").append(VirtualMachineGuru.getEncodedString(PasswordGenerator.generateRandomPassword(16))); String bootArgs = buf.toString(); @@ -1175,27 +1177,44 @@ public class SecondaryStorageManagerImpl extends ManagerBase implements Secondar s_logger.debug(String.format("Setting UseHttpsToUpload config on cmdline with [%s] value.", useHttpsToUpload)); buf.append(" useHttpsToUpload=").append(useHttpsToUpload); - addSecondaryStorageServerAddressToBuffer(buf, secStore, vmName); + addSecondaryStorageServerAddressToBuffer(buf, secStores, vmName); return true; } /** - * Adds the secondary storage address to the buffer if it is in the following pattern: //
/... + * Adds the secondary storages address to the buffer if it is in the following pattern: //
/... */ - protected void addSecondaryStorageServerAddressToBuffer(StringBuilder buffer, DataStore dataStore, String vmName) { - String url = dataStore.getTO().getUrl(); - String[] urlArray = url.split("/"); + protected void addSecondaryStorageServerAddressToBuffer(StringBuilder buffer, List dataStores, String vmName) { + List addresses = new ArrayList<>(); + for (DataStore dataStore: dataStores) { + String url = dataStore.getTO().getUrl(); + String[] urlArray = url.split("/"); - s_logger.debug(String.format("Found [%s] as secondary storage's URL for SSVM [%s].", url, vmName)); - if (ArrayUtils.getLength(urlArray) < 3) { - s_logger.debug(String.format("Could not retrieve secondary storage address from URL [%s] of SSVM [%s].", url, vmName)); + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Found [%s] as secondary storage [%s] URL for SSVM [%s].", dataStore.getName(), url, vmName)); + } + if (ArrayUtils.getLength(urlArray) < 3) { + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("Could not retrieve secondary storage [%s] address from URL [%s] of SSVM [%s].", dataStore.getName(), url, vmName)); + } + continue; + } + + String address = urlArray[2]; + s_logger.info(String.format("Using [%s] as address of secondary storage [%s] of SSVM [%s].", address, dataStore.getName(), vmName)); + if (!addresses.contains(address)) { + addresses.add(address); + } + + } + if (addresses.isEmpty()) { + if (s_logger.isDebugEnabled()) { + s_logger.debug(String.format("No address found for the secondary storages: [%s] of SSVM: [%s]", StringUtils.join(dataStores.stream().map(DataStore::getName).collect(Collectors.toList()), ","), vmName)); + } return; } - - String address = urlArray[2]; - s_logger.info(String.format("Using [%s] as address of secondary storage of SSVM [%s].", address, vmName)); - buffer.append(" secondaryStorageServerAddress=").append(address); + buffer.append(" secondaryStorageServerAddress=").append(StringUtils.join(addresses, ",")); } @Override diff --git a/services/secondary-storage/controller/src/test/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImplTest.java b/services/secondary-storage/controller/src/test/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImplTest.java new file mode 100644 index 00000000000..6df205e1aac --- /dev/null +++ b/services/secondary-storage/controller/src/test/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImplTest.java @@ -0,0 +1,89 @@ +// 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.secondarystorage; + +import java.security.SecureRandom; +import java.util.ArrayList; +import java.util.List; + +import org.apache.cloudstack.engine.subsystem.api.storage.DataStore; +import org.apache.commons.lang3.StringUtils; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.agent.api.to.DataStoreTO; +import com.cloud.utils.net.NetUtils; +import com.google.common.net.InetAddresses; + +@RunWith(MockitoJUnitRunner.class) +public class SecondaryStorageManagerImplTest { + private final SecureRandom secureRandom = new SecureRandom(); + + @Spy + @InjectMocks + private SecondaryStorageManagerImpl secondaryStorageManager; + + private List mockDataStoresForTestAddSecondaryStorageServerAddressToBuffer(List addresses) { + List dataStores = new ArrayList<>(); + for (String address: addresses) { + DataStore dataStore = Mockito.mock(DataStore.class); + DataStoreTO dataStoreTO = Mockito.mock(DataStoreTO.class); + Mockito.when(dataStoreTO.getUrl()).thenReturn(NetUtils.isValidIp4(address) ? String.format("http://%s", address) : address); + Mockito.when(dataStore.getTO()).thenReturn(dataStoreTO); + dataStores.add(dataStore); + } + return dataStores; + } + + private void runAddSecondaryStorageServerAddressToBufferTest(List addresses, String expected) { + List dataStores = mockDataStoresForTestAddSecondaryStorageServerAddressToBuffer(addresses); + StringBuilder builder = new StringBuilder(); + secondaryStorageManager.addSecondaryStorageServerAddressToBuffer(builder, dataStores, "VM"); + String result = builder.toString(); + result = result.contains("=") ? result.split("=")[1] : null; + Assert.assertEquals(expected, result); + } + + @Test + public void testAddSecondaryStorageServerAddressToBufferDifferentAddress() { + String randomIp1 = InetAddresses.fromInteger(secureRandom.nextInt()).getHostAddress(); + String randomIp2 = InetAddresses.fromInteger(secureRandom.nextInt()).getHostAddress(); + List addresses = List.of(randomIp1, randomIp2); + String expected = StringUtils.join(addresses, ","); + runAddSecondaryStorageServerAddressToBufferTest(addresses, expected); + } + + @Test + public void testAddSecondaryStorageServerAddressToBufferSameAddress() { + String randomIp1 = InetAddresses.fromInteger(secureRandom.nextInt()).getHostAddress(); + List addresses = List.of(randomIp1, randomIp1); + runAddSecondaryStorageServerAddressToBufferTest(addresses, randomIp1); + } + + @Test + public void testAddSecondaryStorageServerAddressToBufferInvalidAddress() { + String randomIp1 = InetAddresses.fromInteger(secureRandom.nextInt()).getHostAddress(); + String randomIp2 = InetAddresses.fromInteger(secureRandom.nextInt()).getHostAddress(); + List addresses = List.of(randomIp1, "garbage", randomIp2); + runAddSecondaryStorageServerAddressToBufferTest(addresses, StringUtils.join(List.of(randomIp1, randomIp2), ",")); + } +} diff --git a/systemvm/agent/scripts/ssvm-check.sh b/systemvm/agent/scripts/ssvm-check.sh index 5fdc244debd..b2721a93b3f 100644 --- a/systemvm/agent/scripts/ssvm-check.sh +++ b/systemvm/agent/scripts/ssvm-check.sh @@ -101,7 +101,7 @@ then else echo "ERROR: Storage $storage is not currently mounted" echo "Verifying if we can at least ping the storage" - STORAGE_ADDRESS=`grep "secondaryStorageServerAddress" $CMDLINE | sed -E 's/.*secondaryStorageServerAddress=([^ ]*).*/\1/g'` + STORAGE_ADDRESSES=`grep "secondaryStorageServerAddress" $CMDLINE | sed -E 's/.*secondaryStorageServerAddress=([^ ]*).*/\1/g'` if [[ -z "$STORAGE_ADDRESS" ]] then @@ -117,16 +117,21 @@ else route -n fi else - echo "Storage address is $STORAGE_ADDRESS, trying to ping it" - ping -c 2 $STORAGE_ADDRESS - if [ $? -eq 0 ] - then - echo "Good: Can ping $storage storage address" - else - echo "WARNING: Cannot ping $storage storage address" - echo routing table follows - route -n - fi + echo "Storage address(s): $STORAGE_ADDRESSES, trying to ping" + STORAGE_ADDRESS_LIST=$(echo $STORAGE_ADDRESSES | tr ",") + for STORAGE_ADDRESS in $STORAGE_ADDRESS_LIST + do + echo "Pinging storage address: $STORAGE_ADDRESS" + ping -c 2 $STORAGE_ADDRESS + if [ $? -eq 0 ] + then + echo "Good: Can ping $storage storage address" + else + echo "WARNING: Cannot ping $storage storage address" + echo routing table follows + route -n + fi + done fi fi