GabrielBrascher commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r457788378
##########
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -1156,9 +1156,15 @@ public boolean configure(final String name, final
Map<String, Object> params) th
storageProcessor.configure(name, params);
storageHandler = new
StorageSubsystemCommandHandlerBase(storageProcessor);
- IscsiStorageCleanupMonitor isciCleanupMonitor = new
IscsiStorageCleanupMonitor();
- final Thread cleanupMonitor = new Thread(isciCleanupMonitor);
- cleanupMonitor.start();
+ boolean _iscsiCleanUpEnabled =
(boolean)params.get("iscsi.session.cleanup.enabled");
+
+ if (_iscsiCleanUpEnabled) {
Review comment:
To be honest, I am not 100% sure on this one, but I think that
`(boolean)params.get("config")` does not handle null parameters (e.g. when
commented or removed from configurations).
Can you please take a look and run a few tests considering those cases where
the parameter is not on the configuration file?
One approach that I know it would work is `Boolean.parseBoolean`;
_parseBoolean_ handles null values (`value = "null->false", pure = true`).
Example:
```
Boolean _iscsiCleanUpEnabled =
Boolean.parseBoolean((String)params.get("iscsi.session.cleanup.enabled"));
if (BooleanUtils.isTrue(_iscsiCleanUpEnabled)) {
```
##########
File path: agent/conf/agent.properties
##########
@@ -258,3 +258,6 @@ hypervisor.type=kvm
# timer.
# For all actions refer to the libvirt documentation.
# Recommended values are: none, reset and poweroff.
+#
+iscsi.session.cleanup.enabled=false
Review comment:
1. Considering compatibility on cases where the file remains the old one
and does not receive the upgraded `agent.properties` (system admin can choose
between keeping an old file or update it during CloudStack agent upgrade), this
implementation should handle `null`.
2. As far as I understood, the iSCSI session cleanup works only on managed
storage (e.g. Solidfire). If that is the case indeed I think that the parameter
details/documentation should add details regarding when it is recommended to
use it and when it is risky. Additionally, following other parameters approach,
this one (maybe) could also begin commented as it should be set to **false** /
**null** on most cases.
##########
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -114,7 +114,7 @@ private void
updateDiskStatusMapWithInactiveIscsiSessions(Connect conn){
//check the volume map. If an entry exists change the
status to True
for (final LibvirtVMDef.DiskDef disk : disks) {
- if (diskStatusMap.containsKey(disk.getDiskPath())) {
+ if
(diskStatusMap.containsKey(disk.getDiskPath())&&!disk.getDiskPath().contains("part"))
{
Review comment:
Can you please extract `"part"` to a constant?
Adding some details/documentation on why it is important to have this
constant checked might be a plus as well.
##########
File path:
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -114,7 +114,7 @@ private void
updateDiskStatusMapWithInactiveIscsiSessions(Connect conn){
//check the volume map. If an entry exists change the
status to True
for (final LibvirtVMDef.DiskDef disk : disks) {
- if (diskStatusMap.containsKey(disk.getDiskPath())) {
+ if
(diskStatusMap.containsKey(disk.getDiskPath())&&!disk.getDiskPath().contains("part"))
{
Review comment:
Can you please extract `"part"` to a constant?
(Maybe) Adding some details/documentation on why it is important to have the
disk path checked with that constant might be a plus as well considering future
reference/maintenance on the 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.
For queries about this service, please contact Infrastructure at:
[email protected]
