You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by da...@apache.org on 2021/07/19 15:18:56 UTC
[cloudstack] 01/01: Revert "Externalize kvm agent storage timeout
configuration (#4585)"
This is an automated email from the ASF dual-hosted git repository.
dahn pushed a commit to branch revert-4585-externalize-kvm-agent-storage-timeout-configuration
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit 3bc60b2f4b81c7d23f863e5637115cbada01f250
Author: dahn <da...@gmail.com>
AuthorDate: Mon Jul 19 17:18:28 2021 +0200
Revert "Externalize kvm agent storage timeout configuration (#4585)"
This reverts commit 05a978c2495d1c93c798d2b845c5030ad78f6a14.
---
agent/conf/agent.properties | 4 -
.../cloud/agent/properties/AgentProperties.java | 52 -------
.../properties/AgentPropertiesFileHandler.java | 70 ---------
.../properties/AgentPropertiesFileHandlerTest.java | 143 -----------------
.../hypervisor/kvm/resource/KVMHAMonitor.java | 171 +++++++++++----------
5 files changed, 90 insertions(+), 350 deletions(-)
diff --git a/agent/conf/agent.properties b/agent/conf/agent.properties
index bc17d96..06d8f3f 100644
--- a/agent/conf/agent.properties
+++ b/agent/conf/agent.properties
@@ -266,7 +266,3 @@ iscsi.session.cleanup.enabled=false
# Automatically clean up iscsi sessions not attached to any VM.
# Should be enabled for users using managed storage for example solidfire.
# Should be disabled for users with unmanaged iscsi connections on their hosts
-
-# This parameter specifies the heartbeat update timeout in ms; The default value is 60000ms (1 min).
-# Depending on the use case, this timeout might need increasing/decreasing.
-# heartbeat.update.timeout=60000
\ No newline at end of file
diff --git a/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java b/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java
deleted file mode 100644
index b685484..0000000
--- a/agent/src/main/java/com/cloud/agent/properties/AgentProperties.java
+++ /dev/null
@@ -1,52 +0,0 @@
-/*
- *
- * Licensed 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 com.cloud.agent.properties;
-
-/**
- * Class of constant agent's properties available to configure on
- * "agent.properties".
- *<br><br>
- * Not all available agent properties are defined here, but we should work to
- * migrate them on demand to this class.
- *
- * @param <T> type of the default value.
- */
-public class AgentProperties{
-
- /**
- * Heartbeat update timeout. <br>
- * Data type: int. <br>
- * Default value: 60000 (ms).
- */
- public static final Property<Integer> HEARTBEAT_UPDATE_TIMEOUT = new Property<Integer>("heartbeat.update.timeout", 60000);
-
- public static class Property <T>{
- private final String name;
- private final T defaultValue;
-
- private Property(String name, T value) {
- this.name = name;
- this.defaultValue = value;
- }
-
- public String getName() {
- return name;
- }
-
- public T getDefaultValue() {
- return defaultValue;
- }
- }
-}
diff --git a/agent/src/main/java/com/cloud/agent/properties/AgentPropertiesFileHandler.java b/agent/src/main/java/com/cloud/agent/properties/AgentPropertiesFileHandler.java
deleted file mode 100644
index 6d515f0..0000000
--- a/agent/src/main/java/com/cloud/agent/properties/AgentPropertiesFileHandler.java
+++ /dev/null
@@ -1,70 +0,0 @@
-/*
- *
- * Licensed 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 com.cloud.agent.properties;
-
-import com.cloud.utils.PropertiesUtil;
-import java.io.File;
-import java.io.IOException;
-import org.apache.cloudstack.utils.security.KeyStoreUtils;
-import org.apache.commons.beanutils.ConvertUtils;
-import org.apache.commons.beanutils.converters.IntegerConverter;
-import org.apache.commons.lang3.StringUtils;
-import org.apache.log4j.Logger;
-
-/**
- * This class provides a facility to read the agent's properties file and get
- * its properties, according to the {@link AgentProperties} properties constants.
- *
- */
-public class AgentPropertiesFileHandler {
-
- private static final Logger logger = Logger.getLogger(AgentPropertiesFileHandler.class);
-
- /**
- * This method reads the property in the agent.properties file.
- *
- * @param property the property to retrieve.
- * @return The value of the property. If the property is not available, the
- * default defined value will be used.
- */
- public static <T> T getPropertyValue(AgentProperties.Property<T> property) {
- T defaultValue = property.getDefaultValue();
- String name = property.getName();
-
- File agentPropertiesFile = PropertiesUtil.findConfigFile(KeyStoreUtils.AGENT_PROPSFILE);
-
- if (agentPropertiesFile != null) {
- try {
- String configValue = PropertiesUtil.loadFromFile(agentPropertiesFile).getProperty(name);
- if (StringUtils.isNotBlank(configValue)) {
- if (defaultValue instanceof Integer) {
- ConvertUtils.register(new IntegerConverter(defaultValue), Integer.class);
- }
-
- return (T)ConvertUtils.convert(configValue, defaultValue.getClass());
- } else {
- logger.debug(String.format("Property [%s] has empty or null value. Using default value [%s].", name, defaultValue));
- }
- } catch (IOException ex) {
- logger.debug(String.format("Failed to get property [%s]. Using default value [%s].", name, defaultValue), ex);
- }
- } else {
- logger.debug(String.format("File [%s] was not found, we will use default defined values. Property [%s]: [%s].", KeyStoreUtils.AGENT_PROPSFILE, name, defaultValue));
- }
-
- return defaultValue;
- }
-
-}
diff --git a/agent/src/test/java/com/cloud/agent/properties/AgentPropertiesFileHandlerTest.java b/agent/src/test/java/com/cloud/agent/properties/AgentPropertiesFileHandlerTest.java
deleted file mode 100644
index d91d0e0..0000000
--- a/agent/src/test/java/com/cloud/agent/properties/AgentPropertiesFileHandlerTest.java
+++ /dev/null
@@ -1,143 +0,0 @@
-/*
- * 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 com.cloud.agent.properties;
-
-import com.cloud.utils.PropertiesUtil;
-import java.io.File;
-import java.io.IOException;
-import java.util.Properties;
-import junit.framework.TestCase;
-import org.apache.commons.beanutils.ConvertUtils;
-import org.junit.Assert;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.mockito.Mock;
-import org.mockito.Mockito;
-import org.powermock.api.mockito.PowerMockito;
-import org.powermock.core.classloader.annotations.PrepareForTest;
-import org.powermock.modules.junit4.PowerMockRunner;
-
-@RunWith(PowerMockRunner.class)
-@PrepareForTest({PropertiesUtil.class, ConvertUtils.class})
-public class AgentPropertiesFileHandlerTest extends TestCase {
-
- @Mock
- AgentProperties.Property<String> agentPropertiesStringMock;
-
- @Mock
- AgentProperties.Property<Integer> agentPropertiesIntegerMock;
-
- @Mock
- File fileMock;
-
- @Mock
- Properties propertiesMock;
-
- @Test
- public void validateGetPropertyValueFileNotFoundReturnDefaultValue() throws Exception{
- String expectedResult = "default value";
- Mockito.doReturn(expectedResult).when(agentPropertiesStringMock).getDefaultValue();
-
- PowerMockito.mockStatic(PropertiesUtil.class);
- PowerMockito.doReturn(null).when(PropertiesUtil.class, "findConfigFile", Mockito.anyString());
-
- String result = AgentPropertiesFileHandler.getPropertyValue(agentPropertiesStringMock);
-
- Assert.assertEquals(expectedResult, result);
- }
-
- @Test
- public void validateGetPropertyValueLoadFromFileThrowsIOExceptionReturnDefaultValue() throws Exception{
- String expectedResult = "default value";
- Mockito.doReturn(expectedResult).when(agentPropertiesStringMock).getDefaultValue();
-
- PowerMockito.mockStatic(PropertiesUtil.class);
- PowerMockito.doReturn(fileMock).when(PropertiesUtil.class, "findConfigFile", Mockito.anyString());
- PowerMockito.doThrow(new IOException()).when(PropertiesUtil.class, "loadFromFile", Mockito.any());
-
- String result = AgentPropertiesFileHandler.getPropertyValue(agentPropertiesStringMock);
-
- Assert.assertEquals(expectedResult, result);
- }
-
- @Test
- public void validateGetPropertyValuePropertyIsEmptyReturnDefaultValue() throws Exception{
- String expectedResult = "default value";
- Mockito.doReturn(expectedResult).when(agentPropertiesStringMock).getDefaultValue();
- Mockito.doReturn("name").when(agentPropertiesStringMock).getName();
-
- PowerMockito.mockStatic(PropertiesUtil.class);
- PowerMockito.doReturn(fileMock).when(PropertiesUtil.class, "findConfigFile", Mockito.anyString());
- PowerMockito.doReturn(propertiesMock).when(PropertiesUtil.class, "loadFromFile", Mockito.any());
- PowerMockito.doReturn("").when(propertiesMock).getProperty(Mockito.anyString());
-
- String result = AgentPropertiesFileHandler.getPropertyValue(agentPropertiesStringMock);
-
- Assert.assertEquals(expectedResult, result);
- }
-
- @Test
- public void validateGetPropertyValuePropertyIsNullReturnDefaultValue() throws Exception{
- String expectedResult = "default value";
- Mockito.doReturn(expectedResult).when(agentPropertiesStringMock).getDefaultValue();
- Mockito.doReturn("name").when(agentPropertiesStringMock).getName();
-
- PowerMockito.mockStatic(PropertiesUtil.class);
- PowerMockito.doReturn(fileMock).when(PropertiesUtil.class, "findConfigFile", Mockito.anyString());
- PowerMockito.doReturn(propertiesMock).when(PropertiesUtil.class, "loadFromFile", Mockito.any());
- PowerMockito.doReturn(null).when(propertiesMock).getProperty(Mockito.anyString());
-
- String result = AgentPropertiesFileHandler.getPropertyValue(agentPropertiesStringMock);
-
- Assert.assertEquals(expectedResult, result);
- }
-
- @Test
- public void validateGetPropertyValueValidPropertyReturnPropertyValue() throws Exception{
- String expectedResult = "test";
- Mockito.doReturn("default value").when(agentPropertiesStringMock).getDefaultValue();
- Mockito.doReturn("name").when(agentPropertiesStringMock).getName();
-
- PowerMockito.mockStatic(PropertiesUtil.class);
- PowerMockito.doReturn(fileMock).when(PropertiesUtil.class, "findConfigFile", Mockito.anyString());
- PowerMockito.doReturn(propertiesMock).when(PropertiesUtil.class, "loadFromFile", Mockito.any());
- Mockito.doReturn(expectedResult).when(propertiesMock).getProperty(Mockito.anyString());
-
- String result = AgentPropertiesFileHandler.getPropertyValue(agentPropertiesStringMock);
-
- Assert.assertEquals(expectedResult, result);
- }
-
- @Test
- public void validateGetPropertyValueValidIntegerPropertyReturnPropertyValue() throws Exception{
- Integer expectedResult = 2;
- Mockito.doReturn(1).when(agentPropertiesIntegerMock).getDefaultValue();
- Mockito.doReturn("name").when(agentPropertiesIntegerMock).getName();
-
- PowerMockito.mockStatic(PropertiesUtil.class);
- PowerMockito.doReturn(fileMock).when(PropertiesUtil.class, "findConfigFile", Mockito.anyString());
- PowerMockito.doReturn(propertiesMock).when(PropertiesUtil.class, "loadFromFile", Mockito.any());
- Mockito.doReturn(String.valueOf(expectedResult)).when(propertiesMock).getProperty(Mockito.anyString());
-
- Integer result = AgentPropertiesFileHandler.getPropertyValue(agentPropertiesIntegerMock);
-
- Assert.assertEquals(expectedResult, result);
- }
-}
diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java
index da2ce20..8a11b7f 100644
--- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java
+++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java
@@ -16,9 +16,8 @@
// under the License.
package com.cloud.hypervisor.kvm.resource;
-import com.cloud.agent.properties.AgentProperties;
-import com.cloud.agent.properties.AgentPropertiesFileHandler;
import com.cloud.utils.script.Script;
+import org.apache.cloudstack.managed.context.ManagedContextRunnable;
import org.apache.log4j.Logger;
import org.libvirt.Connect;
import org.libvirt.LibvirtException;
@@ -33,20 +32,17 @@ import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
public class KVMHAMonitor extends KVMHABase implements Runnable {
-
private static final Logger s_logger = Logger.getLogger(KVMHAMonitor.class);
- private final Map<String, NfsStoragePool> storagePool = new ConcurrentHashMap<>();
+ private final Map<String, NfsStoragePool> _storagePool = new ConcurrentHashMap<String, NfsStoragePool>();
- private final String hostPrivateIp;
+ private final String _hostIP; /* private ip address */
public KVMHAMonitor(NfsStoragePool pool, String host, String scriptPath) {
if (pool != null) {
- storagePool.put(pool._poolUUID, pool);
+ _storagePool.put(pool._poolUUID, pool);
}
- hostPrivateIp = host;
+ _hostIP = host;
configureHeartBeatPath(scriptPath);
-
- _heartBeatUpdateTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.HEARTBEAT_UPDATE_TIMEOUT);
}
private static synchronized void configureHeartBeatPath(String scriptPath) {
@@ -54,122 +50,135 @@ public class KVMHAMonitor extends KVMHABase implements Runnable {
}
public void addStoragePool(NfsStoragePool pool) {
- synchronized (storagePool) {
- storagePool.put(pool._poolUUID, pool);
+ synchronized (_storagePool) {
+ _storagePool.put(pool._poolUUID, pool);
}
}
public void removeStoragePool(String uuid) {
- synchronized (storagePool) {
- NfsStoragePool pool = storagePool.get(uuid);
+ synchronized (_storagePool) {
+ NfsStoragePool pool = _storagePool.get(uuid);
if (pool != null) {
Script.runSimpleBashScript("umount " + pool._mountDestPath);
- storagePool.remove(uuid);
+ _storagePool.remove(uuid);
}
}
}
public List<NfsStoragePool> getStoragePools() {
- synchronized (storagePool) {
- return new ArrayList<>(storagePool.values());
+ synchronized (_storagePool) {
+ return new ArrayList<NfsStoragePool>(_storagePool.values());
}
}
public NfsStoragePool getStoragePool(String uuid) {
- synchronized (storagePool) {
- return storagePool.get(uuid);
+ synchronized (_storagePool) {
+ return _storagePool.get(uuid);
}
}
- protected void runHeartBeat() {
- synchronized (storagePool) {
- Set<String> removedPools = new HashSet<>();
- for (String uuid : storagePool.keySet()) {
- NfsStoragePool primaryStoragePool = storagePool.get(uuid);
- StoragePool storage;
- try {
- Connect conn = LibvirtConnection.getConnection();
- storage = conn.storagePoolLookupByUUIDString(uuid);
- if (storage == null || storage.getInfo().state != StoragePoolState.VIR_STORAGE_POOL_RUNNING) {
- if (storage == null) {
- s_logger.debug(String.format("Libvirt storage pool [%s] not found, removing from HA list.", uuid));
- } else {
- s_logger.debug(String.format("Libvirt storage pool [%s] found, but not running, removing from HA list.", uuid));
- }
+ private class Monitor extends ManagedContextRunnable {
- removedPools.add(uuid);
- continue;
- }
+ @Override
+ protected void runInContext() {
+ synchronized (_storagePool) {
+ Set<String> removedPools = new HashSet<String>();
+ for (String uuid : _storagePool.keySet()) {
+ NfsStoragePool primaryStoragePool = _storagePool.get(uuid);
- s_logger.debug(String.format("Found NFS storage pool [%s] in libvirt, continuing.", uuid));
+ // check for any that have been deregistered with libvirt and
+ // skip,remove them
- } catch (LibvirtException e) {
- s_logger.debug(String.format("Failed to lookup libvirt storage pool [%s].", uuid), e);
+ StoragePool storage = null;
+ try {
+ Connect conn = LibvirtConnection.getConnection();
+ storage = conn.storagePoolLookupByUUIDString(uuid);
+ if (storage == null) {
+ s_logger.debug("Libvirt storage pool " + uuid + " not found, removing from HA list");
+ removedPools.add(uuid);
+ continue;
- if (e.toString().contains("pool not found")) {
- s_logger.debug(String.format("Removing pool [%s] from HA monitor since it was deleted.", uuid));
- removedPools.add(uuid);
- continue;
- }
+ } else if (storage.getInfo().state != StoragePoolState.VIR_STORAGE_POOL_RUNNING) {
+ s_logger.debug("Libvirt storage pool " + uuid + " found, but not running, removing from HA list");
- }
+ removedPools.add(uuid);
+ continue;
+ }
+ s_logger.debug("Found NFS storage pool " + uuid + " in libvirt, continuing");
- String result = null;
- for (int i = 1; i <= _heartBeatUpdateMaxTries; i++) {
- Script cmd = createHeartBeatCommand(primaryStoragePool);
- cmd.add("-h", hostPrivateIp);
- result = cmd.execute();
+ } catch (LibvirtException e) {
+ s_logger.debug("Failed to lookup libvirt storage pool " + uuid + " due to: " + e);
- s_logger.debug(String.format("The command (%s), to the pool [%s], has the result [%s].", cmd.toString(), uuid, result));
+ // we only want to remove pool if it's not found, not if libvirt
+ // connection fails
+ if (e.toString().contains("pool not found")) {
+ s_logger.debug("removing pool from HA monitor since it was deleted");
+ removedPools.add(uuid);
+ continue;
+ }
+ }
- if (result != null) {
- s_logger.warn(String.format("Write heartbeat for pool [%s] failed: %s; try: %s of %s.", uuid, result, i, _heartBeatUpdateMaxTries));
- try {
- Thread.sleep(_heartBeatUpdateRetrySleep);
- } catch (InterruptedException e) {
- s_logger.debug("[IGNORED] Interrupted between heartbeat retries.", e);
+ String result = null;
+ // Try multiple times, but sleep in between tries to ensure it isn't a short lived transient error
+ for (int i = 1; i <= _heartBeatUpdateMaxTries; i++) {
+ Script cmd = new Script(s_heartBeatPath, _heartBeatUpdateTimeout, s_logger);
+ cmd.add("-i", primaryStoragePool._poolIp);
+ cmd.add("-p", primaryStoragePool._poolMountSourcePath);
+ cmd.add("-m", primaryStoragePool._mountDestPath);
+ cmd.add("-h", _hostIP);
+ result = cmd.execute();
+ if (result != null) {
+ s_logger.warn("write heartbeat failed: " + result + ", try: " + i + " of " + _heartBeatUpdateMaxTries);
+ try {
+ Thread.sleep(_heartBeatUpdateRetrySleep);
+ } catch (InterruptedException e) {
+ s_logger.debug("[ignored] interupted between heartbeat retries.");
+ }
+ } else {
+ break;
}
- } else {
- break;
}
+ if (result != null) {
+ // Stop cloudstack-agent if can't write to heartbeat file.
+ // This will raise an alert on the mgmt server
+ s_logger.warn("write heartbeat failed: " + result + "; stopping cloudstack-agent");
+ Script cmd = new Script(s_heartBeatPath, _heartBeatUpdateTimeout, s_logger);
+ cmd.add("-i", primaryStoragePool._poolIp);
+ cmd.add("-p", primaryStoragePool._poolMountSourcePath);
+ cmd.add("-m", primaryStoragePool._mountDestPath);
+ cmd.add("-c");
+ result = cmd.execute();
+ }
}
- if (result != null) {
- s_logger.warn(String.format("Write heartbeat for pool [%s] failed: %s; stopping cloudstack-agent.", uuid, result));
- Script cmd = createHeartBeatCommand(primaryStoragePool);
- cmd.add("-c");
- result = cmd.execute();
+ if (!removedPools.isEmpty()) {
+ for (String uuid : removedPools) {
+ removeStoragePool(uuid);
+ }
}
}
- if (!removedPools.isEmpty()) {
- for (String uuid : removedPools) {
- removeStoragePool(uuid);
- }
- }
}
-
- }
-
- private Script createHeartBeatCommand(NfsStoragePool primaryStoragePool) {
- Script cmd = new Script(s_heartBeatPath, _heartBeatUpdateTimeout, s_logger);
- cmd.add("-i", primaryStoragePool._poolIp);
- cmd.add("-p", primaryStoragePool._poolMountSourcePath);
- cmd.add("-m", primaryStoragePool._mountDestPath);
- return cmd;
}
@Override
public void run() {
+ // s_logger.addAppender(new org.apache.log4j.ConsoleAppender(new
+ // org.apache.log4j.PatternLayout(), "System.out"));
while (true) {
-
- runHeartBeat();
+ Thread monitorThread = new Thread(new Monitor());
+ monitorThread.start();
+ try {
+ monitorThread.join();
+ } catch (InterruptedException e) {
+ s_logger.debug("[ignored] interupted joining monitor.");
+ }
try {
Thread.sleep(_heartBeatUpdateFreq);
} catch (InterruptedException e) {
- s_logger.debug("[IGNORED] Interrupted between heartbeats.", e);
+ s_logger.debug("[ignored] interupted between heartbeats.");
}
}
}