You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/07/23 11:13:49 UTC
[GitHub] [cloudstack] GutoVeronezi opened a new pull request #5239: Externalize KVM Agent storage's timeout configuration
GutoVeronezi opened a new pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239
### Description
On KVMHAMonitor, the heartbeat timeout is hardcoded to `60000` ms; therefore, operators do not have options to choose which timeout they want/need.
This PR intends to externalize the heartbeat timeout configuration on KVMHAMonitor.
Depending on the use case, this timeout might need increasing/decreasing. Therefore, it is interesting to externalize such configurations for cloud operators. The hardcoded value is taken as the default one. Therefore, the current behavior is maintained.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functioanlity)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [x] Major
- [ ] Minor
### How Has This Been Tested?
It has been tested locally on a test lab.
1. I changed my log4j to debug mode.
2. I added and changed `hearbeat.update.timeout` setting to `agent.properties` file several times.
3. I restarted agent and watched the log.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on a change in pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
nvazquez commented on a change in pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#discussion_r675579319
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java
##########
@@ -32,153 +33,143 @@
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<String, NfsStoragePool>();
+ private final Map<String, NfsStoragePool> storagePool = new ConcurrentHashMap<>();
- private final String _hostIP; /* private ip address */
+ private final String hostPrivateIp;
public KVMHAMonitor(NfsStoragePool pool, String host, String scriptPath) {
if (pool != null) {
- _storagePool.put(pool._poolUUID, pool);
+ storagePool.put(pool._poolUUID, pool);
}
- _hostIP = host;
+ hostPrivateIp = host;
configureHeartBeatPath(scriptPath);
+
+ _heartBeatUpdateTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.HEARTBEAT_UPDATE_TIMEOUT);
}
private static synchronized void configureHeartBeatPath(String scriptPath) {
KVMHABase.s_heartBeatPath = scriptPath;
}
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<NfsStoragePool>(_storagePool.values());
+ synchronized (storagePool) {
+ return new ArrayList<>(storagePool.values());
}
}
public NfsStoragePool getStoragePool(String uuid) {
- synchronized (_storagePool) {
- return _storagePool.get(uuid);
+ synchronized (storagePool) {
+ return storagePool.get(uuid);
}
}
- private class Monitor extends ManagedContextRunnable {
+ 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));
+ }
- @Override
- protected void runInContext() {
- synchronized (_storagePool) {
- Set<String> removedPools = new HashSet<String>();
- for (String uuid : _storagePool.keySet()) {
- NfsStoragePool primaryStoragePool = _storagePool.get(uuid);
+ removedPools.add(uuid);
+ continue;
+ }
- // check for any that have been deregistered with libvirt and
- // skip,remove them
+ s_logger.debug(String.format("Found NFS storage pool [%s] in libvirt, continuing.", uuid));
- 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;
+ } catch (LibvirtException e) {
+ s_logger.debug(String.format("Failed to lookup libvirt storage pool [%s].", uuid), e);
- } 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");
+ 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;
+ }
- removedPools.add(uuid);
- continue;
- }
- s_logger.debug("Found NFS storage pool " + uuid + " in libvirt, continuing");
+ }
- } catch (LibvirtException e) {
- s_logger.debug("Failed to lookup libvirt storage pool " + uuid + " due to: " + e);
+ String result = null;
+ for (int i = 1; i <= _heartBeatUpdateMaxTries; i++) {
+ Script cmd = createHeartBeatCommand(primaryStoragePool);
+ cmd.add("-h", hostPrivateIp);
+ result = cmd.execute();
- // 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;
- }
- }
+ s_logger.debug(String.format("The command (%s), to the pool [%s], has the result [%s].", cmd.toString(), uuid, result));
- 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;
+ 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);
}
+ } 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 (!removedPools.isEmpty()) {
- for (String uuid : removedPools) {
- removeStoragePool(uuid);
- }
+ 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);
+ }
+ }
}
+
+ }
+
+ private Script createHeartBeatCommand(NfsStoragePool primaryStoragePool) {
Review comment:
Minor suggestion: what about adding the host IP and a boolean parameter, and add them to `cmd` in case the host IP is not null or the boolean is true?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-886753351
<b>Trillian Build Failed (tid-1377)<b/>
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-887479173
@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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#discussion_r675597965
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java
##########
@@ -32,153 +33,143 @@
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<String, NfsStoragePool>();
+ private final Map<String, NfsStoragePool> storagePool = new ConcurrentHashMap<>();
- private final String _hostIP; /* private ip address */
+ private final String hostPrivateIp;
public KVMHAMonitor(NfsStoragePool pool, String host, String scriptPath) {
if (pool != null) {
- _storagePool.put(pool._poolUUID, pool);
+ storagePool.put(pool._poolUUID, pool);
}
- _hostIP = host;
+ hostPrivateIp = host;
configureHeartBeatPath(scriptPath);
+
+ _heartBeatUpdateTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.HEARTBEAT_UPDATE_TIMEOUT);
}
private static synchronized void configureHeartBeatPath(String scriptPath) {
KVMHABase.s_heartBeatPath = scriptPath;
}
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<NfsStoragePool>(_storagePool.values());
+ synchronized (storagePool) {
+ return new ArrayList<>(storagePool.values());
}
}
public NfsStoragePool getStoragePool(String uuid) {
- synchronized (_storagePool) {
- return _storagePool.get(uuid);
+ synchronized (storagePool) {
+ return storagePool.get(uuid);
}
}
- private class Monitor extends ManagedContextRunnable {
+ 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));
+ }
- @Override
- protected void runInContext() {
- synchronized (_storagePool) {
- Set<String> removedPools = new HashSet<String>();
- for (String uuid : _storagePool.keySet()) {
- NfsStoragePool primaryStoragePool = _storagePool.get(uuid);
+ removedPools.add(uuid);
+ continue;
+ }
- // check for any that have been deregistered with libvirt and
- // skip,remove them
+ s_logger.debug(String.format("Found NFS storage pool [%s] in libvirt, continuing.", uuid));
- 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;
+ } catch (LibvirtException e) {
+ s_logger.debug(String.format("Failed to lookup libvirt storage pool [%s].", uuid), e);
- } 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");
+ 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;
+ }
- removedPools.add(uuid);
- continue;
- }
- s_logger.debug("Found NFS storage pool " + uuid + " in libvirt, continuing");
+ }
- } catch (LibvirtException e) {
- s_logger.debug("Failed to lookup libvirt storage pool " + uuid + " due to: " + e);
+ String result = null;
+ for (int i = 1; i <= _heartBeatUpdateMaxTries; i++) {
+ Script cmd = createHeartBeatCommand(primaryStoragePool);
+ cmd.add("-h", hostPrivateIp);
+ result = cmd.execute();
- // 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;
- }
- }
+ s_logger.debug(String.format("The command (%s), to the pool [%s], has the result [%s].", cmd.toString(), uuid, result));
- 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;
+ 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);
}
+ } 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 (!removedPools.isEmpty()) {
- for (String uuid : removedPools) {
- removeStoragePool(uuid);
- }
+ 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);
+ }
+ }
}
+
+ }
+
+ private Script createHeartBeatCommand(NfsStoragePool primaryStoragePool) {
Review comment:
@nvazquez I think I did not get it :grimacing:, could you explain me better?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-887479425
@weizhouapache a Jenkins job has been kicked to build packages. 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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-887520206
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-886711448
@weizhouapache a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-887520463
@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#discussion_r677407420
##########
File path: agent/conf/agent.properties
##########
@@ -266,3 +266,7 @@ 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
Review comment:
@weizhouapache done, thanks!
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-885612499
@DaanHoogland @nvazquez as we discussed, I created a new PR proposing the reverted work. At the time you raised the issue (in https://github.com/apache/cloudstack/pull/4585#issuecomment-882565561), I did not have a chance to investigate and validate it. I understand the use of revert to get things working. However, recently I made some tests with the PR and `addHost`'s feature is working (you can see one [HERE](https://www.youtube.com/watch?v=0loAaOFOipU)). Therefore, it does not seem to be a problem with the PR itself; analyzing the logs, the issue you showed us seems related to the environment.
I think that next time something like this happens, we should investigate the real problem and pin point it in some part of the code, instead of just reverting the first thing we see in hopes that this fixes the issue.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-887222276
<b>Trillian test result (tid-1382)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33585 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5239-t1382-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-885640695
Thanks @GutoVeronezi I'll review it. I think the problem was the PR was merged with outdated test results. After merging the PR every automated test env failed, which makes me think we could have detected the issue attempting a new round of tests. However, I do not agree on the last sentence, @DaanHoogland made sure reverting the changes made the main branch fuctional again before reverting
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-886710930
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#discussion_r675638555
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java
##########
@@ -32,153 +33,143 @@
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<String, NfsStoragePool>();
+ private final Map<String, NfsStoragePool> storagePool = new ConcurrentHashMap<>();
- private final String _hostIP; /* private ip address */
+ private final String hostPrivateIp;
public KVMHAMonitor(NfsStoragePool pool, String host, String scriptPath) {
if (pool != null) {
- _storagePool.put(pool._poolUUID, pool);
+ storagePool.put(pool._poolUUID, pool);
}
- _hostIP = host;
+ hostPrivateIp = host;
configureHeartBeatPath(scriptPath);
+
+ _heartBeatUpdateTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.HEARTBEAT_UPDATE_TIMEOUT);
}
private static synchronized void configureHeartBeatPath(String scriptPath) {
KVMHABase.s_heartBeatPath = scriptPath;
}
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<NfsStoragePool>(_storagePool.values());
+ synchronized (storagePool) {
+ return new ArrayList<>(storagePool.values());
}
}
public NfsStoragePool getStoragePool(String uuid) {
- synchronized (_storagePool) {
- return _storagePool.get(uuid);
+ synchronized (storagePool) {
+ return storagePool.get(uuid);
}
}
- private class Monitor extends ManagedContextRunnable {
+ 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));
+ }
- @Override
- protected void runInContext() {
- synchronized (_storagePool) {
- Set<String> removedPools = new HashSet<String>();
- for (String uuid : _storagePool.keySet()) {
- NfsStoragePool primaryStoragePool = _storagePool.get(uuid);
+ removedPools.add(uuid);
+ continue;
+ }
- // check for any that have been deregistered with libvirt and
- // skip,remove them
+ s_logger.debug(String.format("Found NFS storage pool [%s] in libvirt, continuing.", uuid));
- 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;
+ } catch (LibvirtException e) {
+ s_logger.debug(String.format("Failed to lookup libvirt storage pool [%s].", uuid), e);
- } 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");
+ 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;
+ }
- removedPools.add(uuid);
- continue;
- }
- s_logger.debug("Found NFS storage pool " + uuid + " in libvirt, continuing");
+ }
- } catch (LibvirtException e) {
- s_logger.debug("Failed to lookup libvirt storage pool " + uuid + " due to: " + e);
+ String result = null;
+ for (int i = 1; i <= _heartBeatUpdateMaxTries; i++) {
+ Script cmd = createHeartBeatCommand(primaryStoragePool);
+ cmd.add("-h", hostPrivateIp);
+ result = cmd.execute();
- // 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;
- }
- }
+ s_logger.debug(String.format("The command (%s), to the pool [%s], has the result [%s].", cmd.toString(), uuid, result));
- 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;
+ 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);
}
+ } 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 (!removedPools.isEmpty()) {
- for (String uuid : removedPools) {
- removeStoragePool(uuid);
- }
+ 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);
+ }
+ }
}
+
+ }
+
+ private Script createHeartBeatCommand(NfsStoragePool primaryStoragePool) {
Review comment:
@nvazquez done. Before this changes, `hostPrivateIp` would be added independently if it is null or empty, so adopted your suggestion and made a little change to keep this behavior.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland merged pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-885656546
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian. SL-JID 638
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-887904952
<b>Trillian test result (tid-1387)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34685 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5239-t1387-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on a change in pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
nvazquez commented on a change in pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#discussion_r675651431
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java
##########
@@ -32,153 +33,143 @@
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<String, NfsStoragePool>();
+ private final Map<String, NfsStoragePool> storagePool = new ConcurrentHashMap<>();
- private final String _hostIP; /* private ip address */
+ private final String hostPrivateIp;
public KVMHAMonitor(NfsStoragePool pool, String host, String scriptPath) {
if (pool != null) {
- _storagePool.put(pool._poolUUID, pool);
+ storagePool.put(pool._poolUUID, pool);
}
- _hostIP = host;
+ hostPrivateIp = host;
configureHeartBeatPath(scriptPath);
+
+ _heartBeatUpdateTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.HEARTBEAT_UPDATE_TIMEOUT);
}
private static synchronized void configureHeartBeatPath(String scriptPath) {
KVMHABase.s_heartBeatPath = scriptPath;
}
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<NfsStoragePool>(_storagePool.values());
+ synchronized (storagePool) {
+ return new ArrayList<>(storagePool.values());
}
}
public NfsStoragePool getStoragePool(String uuid) {
- synchronized (_storagePool) {
- return _storagePool.get(uuid);
+ synchronized (storagePool) {
+ return storagePool.get(uuid);
}
}
- private class Monitor extends ManagedContextRunnable {
+ 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));
+ }
- @Override
- protected void runInContext() {
- synchronized (_storagePool) {
- Set<String> removedPools = new HashSet<String>();
- for (String uuid : _storagePool.keySet()) {
- NfsStoragePool primaryStoragePool = _storagePool.get(uuid);
+ removedPools.add(uuid);
+ continue;
+ }
- // check for any that have been deregistered with libvirt and
- // skip,remove them
+ s_logger.debug(String.format("Found NFS storage pool [%s] in libvirt, continuing.", uuid));
- 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;
+ } catch (LibvirtException e) {
+ s_logger.debug(String.format("Failed to lookup libvirt storage pool [%s].", uuid), e);
- } 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");
+ 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;
+ }
- removedPools.add(uuid);
- continue;
- }
- s_logger.debug("Found NFS storage pool " + uuid + " in libvirt, continuing");
+ }
- } catch (LibvirtException e) {
- s_logger.debug("Failed to lookup libvirt storage pool " + uuid + " due to: " + e);
+ String result = null;
+ for (int i = 1; i <= _heartBeatUpdateMaxTries; i++) {
+ Script cmd = createHeartBeatCommand(primaryStoragePool);
+ cmd.add("-h", hostPrivateIp);
+ result = cmd.execute();
- // 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;
- }
- }
+ s_logger.debug(String.format("The command (%s), to the pool [%s], has the result [%s].", cmd.toString(), uuid, result));
- 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;
+ 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);
}
+ } 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 (!removedPools.isEmpty()) {
- for (String uuid : removedPools) {
- removeStoragePool(uuid);
- }
+ 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);
+ }
+ }
}
+
+ }
+
+ private Script createHeartBeatCommand(NfsStoragePool primaryStoragePool) {
Review comment:
Great, thanks @GutoVeronezi
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on a change in pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
nvazquez commented on a change in pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#discussion_r675606786
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java
##########
@@ -32,153 +33,143 @@
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<String, NfsStoragePool>();
+ private final Map<String, NfsStoragePool> storagePool = new ConcurrentHashMap<>();
- private final String _hostIP; /* private ip address */
+ private final String hostPrivateIp;
public KVMHAMonitor(NfsStoragePool pool, String host, String scriptPath) {
if (pool != null) {
- _storagePool.put(pool._poolUUID, pool);
+ storagePool.put(pool._poolUUID, pool);
}
- _hostIP = host;
+ hostPrivateIp = host;
configureHeartBeatPath(scriptPath);
+
+ _heartBeatUpdateTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.HEARTBEAT_UPDATE_TIMEOUT);
}
private static synchronized void configureHeartBeatPath(String scriptPath) {
KVMHABase.s_heartBeatPath = scriptPath;
}
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<NfsStoragePool>(_storagePool.values());
+ synchronized (storagePool) {
+ return new ArrayList<>(storagePool.values());
}
}
public NfsStoragePool getStoragePool(String uuid) {
- synchronized (_storagePool) {
- return _storagePool.get(uuid);
+ synchronized (storagePool) {
+ return storagePool.get(uuid);
}
}
- private class Monitor extends ManagedContextRunnable {
+ 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));
+ }
- @Override
- protected void runInContext() {
- synchronized (_storagePool) {
- Set<String> removedPools = new HashSet<String>();
- for (String uuid : _storagePool.keySet()) {
- NfsStoragePool primaryStoragePool = _storagePool.get(uuid);
+ removedPools.add(uuid);
+ continue;
+ }
- // check for any that have been deregistered with libvirt and
- // skip,remove them
+ s_logger.debug(String.format("Found NFS storage pool [%s] in libvirt, continuing.", uuid));
- 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;
+ } catch (LibvirtException e) {
+ s_logger.debug(String.format("Failed to lookup libvirt storage pool [%s].", uuid), e);
- } 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");
+ 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;
+ }
- removedPools.add(uuid);
- continue;
- }
- s_logger.debug("Found NFS storage pool " + uuid + " in libvirt, continuing");
+ }
- } catch (LibvirtException e) {
- s_logger.debug("Failed to lookup libvirt storage pool " + uuid + " due to: " + e);
+ String result = null;
+ for (int i = 1; i <= _heartBeatUpdateMaxTries; i++) {
+ Script cmd = createHeartBeatCommand(primaryStoragePool);
+ cmd.add("-h", hostPrivateIp);
+ result = cmd.execute();
- // 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;
- }
- }
+ s_logger.debug(String.format("The command (%s), to the pool [%s], has the result [%s].", cmd.toString(), uuid, result));
- 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;
+ 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);
}
+ } 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 (!removedPools.isEmpty()) {
- for (String uuid : removedPools) {
- removeStoragePool(uuid);
- }
+ 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);
+ }
+ }
}
+
+ }
+
+ private Script createHeartBeatCommand(NfsStoragePool primaryStoragePool) {
Review comment:
Sure, I meant to include both parameters so the method's header becomes:
`private Script createHeartBeatCommand(NfsStoragePool primaryStoragePool, String hostIp, boolean cleanup)`
Then:
````
if (StringUtils.isNotBlank(hostIp) {
cmd.add("-h", hostIp);
}
if (cleanup) {
cmd.add("-c");
}
````
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-885732788
<b>Trillian Build Failed (tid-1363)<b/>
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] nvazquez commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
nvazquez commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-887425699
Great catch @weizhouapache, @GutoVeronezi please check the open comment
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-885640255
> @DaanHoogland @nvazquez as we discussed, I created a new PR proposing the reverted work. At the time you raised the issue (in [#4585 (comment)](https://github.com/apache/cloudstack/pull/4585#issuecomment-882565561)),
great, I'll run the tests.
> I think that next time something like this happens, we should investigate the real problem and pin point it in some part of the code, instead of just reverting the first thing we see in hopes that this fixes the issue.
It was not hoping @GutoVeronezi , I tested the revert and the PR itself. I indeed had no time to get to the bottom of it. Also I do/did not believe for a moment it was in the PR itself but in some sort of conflict with other code.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-886976664
@weizhouapache a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-886976536
@blueorangutan test
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
GutoVeronezi commented on a change in pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#discussion_r675623718
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/KVMHAMonitor.java
##########
@@ -32,153 +33,143 @@
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<String, NfsStoragePool>();
+ private final Map<String, NfsStoragePool> storagePool = new ConcurrentHashMap<>();
- private final String _hostIP; /* private ip address */
+ private final String hostPrivateIp;
public KVMHAMonitor(NfsStoragePool pool, String host, String scriptPath) {
if (pool != null) {
- _storagePool.put(pool._poolUUID, pool);
+ storagePool.put(pool._poolUUID, pool);
}
- _hostIP = host;
+ hostPrivateIp = host;
configureHeartBeatPath(scriptPath);
+
+ _heartBeatUpdateTimeout = AgentPropertiesFileHandler.getPropertyValue(AgentProperties.HEARTBEAT_UPDATE_TIMEOUT);
}
private static synchronized void configureHeartBeatPath(String scriptPath) {
KVMHABase.s_heartBeatPath = scriptPath;
}
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<NfsStoragePool>(_storagePool.values());
+ synchronized (storagePool) {
+ return new ArrayList<>(storagePool.values());
}
}
public NfsStoragePool getStoragePool(String uuid) {
- synchronized (_storagePool) {
- return _storagePool.get(uuid);
+ synchronized (storagePool) {
+ return storagePool.get(uuid);
}
}
- private class Monitor extends ManagedContextRunnable {
+ 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));
+ }
- @Override
- protected void runInContext() {
- synchronized (_storagePool) {
- Set<String> removedPools = new HashSet<String>();
- for (String uuid : _storagePool.keySet()) {
- NfsStoragePool primaryStoragePool = _storagePool.get(uuid);
+ removedPools.add(uuid);
+ continue;
+ }
- // check for any that have been deregistered with libvirt and
- // skip,remove them
+ s_logger.debug(String.format("Found NFS storage pool [%s] in libvirt, continuing.", uuid));
- 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;
+ } catch (LibvirtException e) {
+ s_logger.debug(String.format("Failed to lookup libvirt storage pool [%s].", uuid), e);
- } 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");
+ 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;
+ }
- removedPools.add(uuid);
- continue;
- }
- s_logger.debug("Found NFS storage pool " + uuid + " in libvirt, continuing");
+ }
- } catch (LibvirtException e) {
- s_logger.debug("Failed to lookup libvirt storage pool " + uuid + " due to: " + e);
+ String result = null;
+ for (int i = 1; i <= _heartBeatUpdateMaxTries; i++) {
+ Script cmd = createHeartBeatCommand(primaryStoragePool);
+ cmd.add("-h", hostPrivateIp);
+ result = cmd.execute();
- // 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;
- }
- }
+ s_logger.debug(String.format("The command (%s), to the pool [%s], has the result [%s].", cmd.toString(), uuid, result));
- 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;
+ 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);
}
+ } 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 (!removedPools.isEmpty()) {
- for (String uuid : removedPools) {
- removeStoragePool(uuid);
- }
+ 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);
+ }
+ }
}
+
+ }
+
+ private Script createHeartBeatCommand(NfsStoragePool primaryStoragePool) {
Review comment:
Understood, I will do it.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#discussion_r677155975
##########
File path: agent/conf/agent.properties
##########
@@ -266,3 +266,7 @@ 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
Review comment:
@GutoVeronezi
please add a "\n" to the end of the file.
it caused failures in trillian test that fresh installed host cannot be added to zone.
in /etc/cloudstack/agent/agent.properties,
```
# heartbeat.update.timeout=60000keystore.passphrase=RFXZya8a4esacWaG
```
should be
```
# heartbeat.update.timeout=60000
keystore.passphrase=RFXZya8a4esacWaG
```
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #5239: Externalize KVM Agent storage's timeout configuration
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5239:
URL: https://github.com/apache/cloudstack/pull/5239#issuecomment-887498085
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian. SL-JID 660
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org