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 2020/01/17 15:32:00 UTC
[GitHub] [cloudstack] skattoju4 opened a new pull request #3819: Clean up
inactive iscsi sessions when VMs are removed.
skattoju4 opened a new pull request #3819: Clean up inactive iscsi sessions when VMs are removed.
URL: https://github.com/apache/cloudstack/pull/3819
## Description
<!--- Describe your changes in detail -->
Previously, iscsi sessions would not be cleaned up when VMs are removed. This changes adds ankvm.storage.IscsiStorageCleanupMonitor class that cleans up inactive iscsi sessions.
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] 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 functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## Screenshots (if appropriate):
## How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
This has been tested by observing that inactive iscsi sessions are cleaned up when VMs are removed.
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan removed a comment on issue #3819: Clean
up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579570104
<b>Trillian test result (tid-832)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34657 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3819-t832-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_outofbandmanagement.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 75 look OK, 2 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_oobm_background_powerstate_sync | `Failure` | 20.81 | test_outofbandmanagement.py
test_oobm_enabledisable_across_clusterzones | `Error` | 20.13 | test_outofbandmanagement.py
test_oobm_issue_power_cycle | `Error` | 8.69 | test_outofbandmanagement.py
test_oobm_issue_power_off | `Error` | 7.58 | test_outofbandmanagement.py
test_oobm_issue_power_on | `Error` | 7.63 | test_outofbandmanagement.py
test_oobm_issue_power_reset | `Error` | 7.66 | test_outofbandmanagement.py
test_oobm_issue_power_soft | `Error` | 7.66 | test_outofbandmanagement.py
test_oobm_issue_power_status | `Error` | 7.57 | test_outofbandmanagement.py
test_oobm_multiple_mgmt_server_ownership | `Failure` | 32.01 | test_outofbandmanagement.py
test_oobm_zchange_password | `Error` | 3.31 | test_outofbandmanagement.py
test_hostha_kvm_host_degraded | `Error` | 4.72 | test_hostha_kvm.py
test_hostha_kvm_host_fencing | `Error` | 5.73 | test_hostha_kvm.py
test_hostha_kvm_host_recovering | `Error` | 4.74 | test_hostha_kvm.py
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] skattoju4 opened a new pull request #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
skattoju4 opened a new pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819
## Description
<!--- Describe your changes in detail -->
Previously, iscsi sessions would not be cleaned up on KVM when
* vms are crashed or rebooted on another host
* a hypervisor host crashed
This leads to to long boot times from the hypervisor host and session overloading of the netapp solidfire nodes. Each node can have a maximum of 700 iSCSI sessions. The iSCSI Daemon on the hypervisor host holds iSCSI sessions that are not cleaned up. The iSCSI Daemon tried to reconnect every time resulting in holding the unused sessions. Only a logout of the iSCSI daemon to the volume will solve the problem. This changes adds a kvm.storage.IscsiStorageCleanupMonitor class that scans for and cleans up inactive iscsi sessions.
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [x] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## Screenshots (if appropriate):
## How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
This has been tested by observing that inactive iscsi sessions are cleaned up when VMs are removed.
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3819: Clean up inactive
iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-578973595
> @svenvogel you marked it a bug, that's why i asked, nothing else ... ;)
@DaanHoogland if we look closely at it, we realize it's a bug :flushed:
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] skattoju4 commented on issue #3819: Clean up inactive
iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
skattoju4 commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-581468804
> thanks for the changes @skattoju3 . looks a bit cleaner.
Thanks for the feedback :)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#discussion_r371931843
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -0,0 +1,143 @@
+// 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.hypervisor.kvm.storage;
+
+import com.cloud.hypervisor.kvm.resource.LibvirtConnection;
+import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser;
+import com.cloud.hypervisor.kvm.resource.LibvirtVMDef;
+import org.apache.cloudstack.managed.context.ManagedContextRunnable;
+import org.apache.log4j.Logger;
+import org.libvirt.Connect;
+import org.libvirt.Domain;
+import org.libvirt.LibvirtException;
+
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class IscsiStorageCleanupMonitor implements Runnable{
+ private static final Logger s_logger = Logger.getLogger(IscsiStorageCleanupMonitor.class);
+ private static final int CLEANUP_INTERVAL_SEC = 60; // check every X seconds
+ private static final String ISCSI_PATH_PREFIX = "/dev/disk/by-path";
+ private static final String KEYWORD_ISCSI = "iscsi";
+ private static final String KEYWORD_IQN = "iqn";
+
+ private IscsiAdmStorageAdaptor iscsiStorageAdaptor;
+
+ private Map<String, Boolean> diskStatusMap;
+
+ public IscsiStorageCleanupMonitor() {
+ diskStatusMap = new HashMap<>();
+ s_logger.debug("Initialize cleanup thread");
+ iscsiStorageAdaptor = new IscsiAdmStorageAdaptor();
+ }
+
+
+ private class Monitor extends ManagedContextRunnable {
+
+ @Override
+ protected void runInContext() {
+ Connect conn = null;
+ try {
+ conn = LibvirtConnection.getConnection();
+
+ //populate all the iscsi disks currently attached to this host
+ diskStatusMap.clear();
+ File[] iscsiVolumes = new File(ISCSI_PATH_PREFIX).listFiles();
+
+ if (iscsiVolumes == null || iscsiVolumes.length == 0) {
+ s_logger.debug("No iscsi sessions found for cleanup");
+ return;
+ }
+
+ for( File v : iscsiVolumes) {
+ if (isIscsiDisk(v.getAbsolutePath())) {
+ s_logger.debug("found iscsi disk by cleanup thread, marking inactive:" + v.getAbsolutePath());
+ diskStatusMap.put(v.getAbsolutePath(), false);
+ }
+ }
Review comment:
populateDiskStatusMap();
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan removed a comment on issue #3819: Clean
up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579346412
@DaanHoogland 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel edited a comment on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
svenvogel edited a comment on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579139719
@rhtyd can we run tests please? unfortunately you can only test the code formally or using a solidfire. we and the @syed and @skattoju4 tested 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#discussion_r371932090
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -0,0 +1,143 @@
+// 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.hypervisor.kvm.storage;
+
+import com.cloud.hypervisor.kvm.resource.LibvirtConnection;
+import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser;
+import com.cloud.hypervisor.kvm.resource.LibvirtVMDef;
+import org.apache.cloudstack.managed.context.ManagedContextRunnable;
+import org.apache.log4j.Logger;
+import org.libvirt.Connect;
+import org.libvirt.Domain;
+import org.libvirt.LibvirtException;
+
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class IscsiStorageCleanupMonitor implements Runnable{
+ private static final Logger s_logger = Logger.getLogger(IscsiStorageCleanupMonitor.class);
+ private static final int CLEANUP_INTERVAL_SEC = 60; // check every X seconds
+ private static final String ISCSI_PATH_PREFIX = "/dev/disk/by-path";
+ private static final String KEYWORD_ISCSI = "iscsi";
+ private static final String KEYWORD_IQN = "iqn";
+
+ private IscsiAdmStorageAdaptor iscsiStorageAdaptor;
+
+ private Map<String, Boolean> diskStatusMap;
+
+ public IscsiStorageCleanupMonitor() {
+ diskStatusMap = new HashMap<>();
+ s_logger.debug("Initialize cleanup thread");
+ iscsiStorageAdaptor = new IscsiAdmStorageAdaptor();
+ }
+
+
+ private class Monitor extends ManagedContextRunnable {
+
+ @Override
+ protected void runInContext() {
+ Connect conn = null;
+ try {
+ conn = LibvirtConnection.getConnection();
+
+ //populate all the iscsi disks currently attached to this host
+ diskStatusMap.clear();
+ File[] iscsiVolumes = new File(ISCSI_PATH_PREFIX).listFiles();
+
+ if (iscsiVolumes == null || iscsiVolumes.length == 0) {
+ s_logger.debug("No iscsi sessions found for cleanup");
+ return;
+ }
+
+ for( File v : iscsiVolumes) {
+ if (isIscsiDisk(v.getAbsolutePath())) {
+ s_logger.debug("found iscsi disk by cleanup thread, marking inactive:" + v.getAbsolutePath());
+ diskStatusMap.put(v.getAbsolutePath(), false);
+ }
+ }
+
+ // check if they belong to any VM
+ int[] domains = conn.listDomains();
+ s_logger.debug(String.format("found %d domains", domains.length));
+ for (int domId : domains) {
+ Domain dm = conn.domainLookupByID(domId);
+ final String domXml = dm.getXMLDesc(0);
+ final LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
+ parser.parseDomainXML(domXml);
+ List<LibvirtVMDef.DiskDef> disks = parser.getDisks();
+
+ //check the volume map. If an entry exists change the status to True
+ for (final LibvirtVMDef.DiskDef disk : disks) {
+ if (diskStatusMap.containsKey(disk.getDiskPath())) {
+ diskStatusMap.put(disk.getDiskPath(), true);
+ s_logger.debug("active disk found by cleanup thread" + disk.getDiskPath());
+ }
+ }
+ }
Review comment:
checkDiskStatusMap();
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579570104
<b>Trillian test result (tid-832)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34657 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3819-t832-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_outofbandmanagement.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 75 look OK, 2 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_oobm_background_powerstate_sync | `Failure` | 20.81 | test_outofbandmanagement.py
test_oobm_enabledisable_across_clusterzones | `Error` | 20.13 | test_outofbandmanagement.py
test_oobm_issue_power_cycle | `Error` | 8.69 | test_outofbandmanagement.py
test_oobm_issue_power_off | `Error` | 7.58 | test_outofbandmanagement.py
test_oobm_issue_power_on | `Error` | 7.63 | test_outofbandmanagement.py
test_oobm_issue_power_reset | `Error` | 7.66 | test_outofbandmanagement.py
test_oobm_issue_power_soft | `Error` | 7.66 | test_outofbandmanagement.py
test_oobm_issue_power_status | `Error` | 7.57 | test_outofbandmanagement.py
test_oobm_multiple_mgmt_server_ownership | `Failure` | 32.01 | test_outofbandmanagement.py
test_oobm_zchange_password | `Error` | 3.31 | test_outofbandmanagement.py
test_hostha_kvm_host_degraded | `Error` | 4.72 | test_hostha_kvm.py
test_hostha_kvm_host_fencing | `Error` | 5.73 | test_hostha_kvm.py
test_hostha_kvm_host_recovering | `Error` | 4.74 | test_hostha_kvm.py
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher removed a comment on issue #3819:
Clean up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
GabrielBrascher removed a comment on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579263600
@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan removed a comment on issue #3819: Clean
up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579263923
@GabrielBrascher 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] rhtyd commented on issue #3819: Clean up inactive
iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
rhtyd commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579098239
@skattoju4 how can one test this?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-577744773
@svenvogel you marked it a bug, that's why i asked, nothing else ... ;)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher edited a comment on issue #3819: Clean
up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
GabrielBrascher edited a comment on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-577864320
@DaanHoogland @svenvogel @andrijapanicsb considering that it is a bug, it would be awesome to have it in 4.13 branch (aiming 4.13.1 release) and then forward it to master (aiming 4.14).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#discussion_r371934752
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -0,0 +1,143 @@
+// 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.hypervisor.kvm.storage;
+
+import com.cloud.hypervisor.kvm.resource.LibvirtConnection;
+import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser;
+import com.cloud.hypervisor.kvm.resource.LibvirtVMDef;
+import org.apache.cloudstack.managed.context.ManagedContextRunnable;
+import org.apache.log4j.Logger;
+import org.libvirt.Connect;
+import org.libvirt.Domain;
+import org.libvirt.LibvirtException;
+
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class IscsiStorageCleanupMonitor implements Runnable{
+ private static final Logger s_logger = Logger.getLogger(IscsiStorageCleanupMonitor.class);
+ private static final int CLEANUP_INTERVAL_SEC = 60; // check every X seconds
+ private static final String ISCSI_PATH_PREFIX = "/dev/disk/by-path";
+ private static final String KEYWORD_ISCSI = "iscsi";
+ private static final String KEYWORD_IQN = "iqn";
+
+ private IscsiAdmStorageAdaptor iscsiStorageAdaptor;
+
+ private Map<String, Boolean> diskStatusMap;
+
+ public IscsiStorageCleanupMonitor() {
+ diskStatusMap = new HashMap<>();
+ s_logger.debug("Initialize cleanup thread");
+ iscsiStorageAdaptor = new IscsiAdmStorageAdaptor();
+ }
+
+
+ private class Monitor extends ManagedContextRunnable {
+
+ @Override
+ protected void runInContext() {
+ Connect conn = null;
+ try {
+ conn = LibvirtConnection.getConnection();
+
+ //populate all the iscsi disks currently attached to this host
+ diskStatusMap.clear();
+ File[] iscsiVolumes = new File(ISCSI_PATH_PREFIX).listFiles();
+
+ if (iscsiVolumes == null || iscsiVolumes.length == 0) {
+ s_logger.debug("No iscsi sessions found for cleanup");
+ return;
+ }
+
+ for( File v : iscsiVolumes) {
+ if (isIscsiDisk(v.getAbsolutePath())) {
+ s_logger.debug("found iscsi disk by cleanup thread, marking inactive:" + v.getAbsolutePath());
+ diskStatusMap.put(v.getAbsolutePath(), false);
+ }
+ }
+
+ // check if they belong to any VM
+ int[] domains = conn.listDomains();
+ s_logger.debug(String.format("found %d domains", domains.length));
+ for (int domId : domains) {
+ Domain dm = conn.domainLookupByID(domId);
+ final String domXml = dm.getXMLDesc(0);
+ final LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
+ parser.parseDomainXML(domXml);
+ List<LibvirtVMDef.DiskDef> disks = parser.getDisks();
+
+ //check the volume map. If an entry exists change the status to True
+ for (final LibvirtVMDef.DiskDef disk : disks) {
+ if (diskStatusMap.containsKey(disk.getDiskPath())) {
+ diskStatusMap.put(disk.getDiskPath(), true);
+ s_logger.debug("active disk found by cleanup thread" + disk.getDiskPath());
+ }
+ }
+ }
+
+ // the ones where the state is false, they are stale. They may be
+ // removed we go through each volume which is false, check iscsiadm,
+ // if the volume still exisits, logout of that volume and remove it from the map
+
+ // XXX: It is possible that someone had manually added an iSCSI volume.
+ // we would not be able to detect that
+ for (String diskPath : diskStatusMap.keySet()) {
+ if (!diskStatusMap.get(diskPath)) {
+ if (Files.exists(Paths.get(diskPath))) {
+ try {
+ s_logger.info("Cleaning up disk " + diskPath);
+ iscsiStorageAdaptor.disconnectPhysicalDiskByPath(diskPath);
+ } catch (Exception e) {
+ s_logger.warn("[ignored] Error cleaning up " + diskPath, e);
+ }
+ }
+ }
+ }
Review comment:
disconnectPhysicalDisks();
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request
#3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#discussion_r370249114
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -0,0 +1,127 @@
+package com.cloud.hypervisor.kvm.storage;
+
+import com.cloud.hypervisor.kvm.resource.LibvirtConnection;
+import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser;
+import com.cloud.hypervisor.kvm.resource.LibvirtVMDef;
+import org.apache.cloudstack.managed.context.ManagedContextRunnable;
+import org.apache.log4j.Logger;
+import org.libvirt.Connect;
+import org.libvirt.Domain;
+import org.libvirt.LibvirtException;
+
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class IscsiStorageCleanupMonitor implements Runnable{
+ private static final Logger s_logger = Logger.getLogger(IscsiStorageCleanupMonitor.class);
+ private static final int CLEANUP_INTERVAL_SEC = 60; // check every X seconds
Review comment:
I am OK with the way it is, but I would like to raise the following question: Is it interesting to externalize `CLEANUP_INTERVAL_SEC` this on a global settings variable (config keys)?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-580172879
@DaanHoogland 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3819: Clean up inactive
iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579139818
@GabrielBrascher can you take a look again 😄
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3819: Clean up inactive
iscsi sessions when VMs are removed.
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3819: Clean up inactive iscsi sessions when VMs are removed.
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-575745250
@syed can you review please?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-580159417
@DaanHoogland 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#discussion_r371929778
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -0,0 +1,143 @@
+// 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.hypervisor.kvm.storage;
+
+import com.cloud.hypervisor.kvm.resource.LibvirtConnection;
+import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser;
+import com.cloud.hypervisor.kvm.resource.LibvirtVMDef;
+import org.apache.cloudstack.managed.context.ManagedContextRunnable;
+import org.apache.log4j.Logger;
+import org.libvirt.Connect;
+import org.libvirt.Domain;
+import org.libvirt.LibvirtException;
+
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class IscsiStorageCleanupMonitor implements Runnable{
+ private static final Logger s_logger = Logger.getLogger(IscsiStorageCleanupMonitor.class);
+ private static final int CLEANUP_INTERVAL_SEC = 60; // check every X seconds
+ private static final String ISCSI_PATH_PREFIX = "/dev/disk/by-path";
+ private static final String KEYWORD_ISCSI = "iscsi";
+ private static final String KEYWORD_IQN = "iqn";
+
+ private IscsiAdmStorageAdaptor iscsiStorageAdaptor;
+
+ private Map<String, Boolean> diskStatusMap;
+
+ public IscsiStorageCleanupMonitor() {
+ diskStatusMap = new HashMap<>();
+ s_logger.debug("Initialize cleanup thread");
+ iscsiStorageAdaptor = new IscsiAdmStorageAdaptor();
+ }
+
+
+ private class Monitor extends ManagedContextRunnable {
+
+ @Override
+ protected void runInContext() {
Review comment:
long method with a lot of comment. can you please factor out the pieces that need commenting in their own method with clear names? that would make this thread code more maintainable for future generations.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3819: Clean up inactive
iscsi sessions when VMs are removed.
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3819: Clean up inactive iscsi sessions when VMs are removed.
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-575740240
I will add details. It’s for kvm and an cleanup of sessions they are not removed if move an vm from one host to another, crash of an vm and host crash. This fixes the sessions they are there but should removed. This is not good for managed storage or solidfire.
This fix is tested with us in our production environment.
__
Sven Vogel
Teamlead Platform
EWERK DIGITAL GmbH
Brühl 24, D-04109 Leipzig
P +49 341 42649 - 99
F +49 341 42649 - 98
S.Vogel@ewerk.com
www.ewerk.com
Geschäftsführer:
Dr. Erik Wende, Hendrik Schubert, Frank Richter
Registergericht: Leipzig HRB 9065
Support:
+49 341 42649 555
Zertifiziert nach:
ISO/IEC 27001:2013
DIN EN ISO 9001:2015
DIN ISO/IEC 20000-1:2011
ISAE 3402 Typ II Assessed
EWERK-Blog<https://blog.ewerk.com/> | LinkedIn<https://www.linkedin.com/company/ewerk-group> | Xing<https://www.xing.com/company/ewerk> | Twitter<https://twitter.com/EWERK_Group> | Facebook<https://de-de.facebook.com/EWERK.IT/>
E-World 2020 in Essen.
Das EWERK ist dabei! Treffen Sie uns vom 11-13.02.20 in Halle 5 am Stand: 5-724, mit spannenden Vorträgen rund um das Thema Urban Data.
Mit Handelsregistereintragung vom 09.07.2019 ist die EWERK RZ GmbH auf die EWERK IT GmbH verschmolzen und firmiert nun gemeinsam unter dem Namen: EWERK DIGITAL GmbH, für weitere Informationen klicken Sie hier<https://www.ewerk.com/ewerkdigital>.
Auskünfte und Angebote per Mail sind freibleibend und unverbindlich.
Disclaimer Privacy:
Der Inhalt dieser E-Mail (einschließlich etwaiger beigefügter Dateien) ist vertraulich und nur für den Empfänger bestimmt. Sollten Sie nicht der bestimmungsgemäße Empfänger sein, ist Ihnen jegliche Offenlegung, Vervielfältigung, Weitergabe oder Nutzung des Inhalts untersagt. Bitte informieren Sie in diesem Fall unverzüglich den Absender und löschen Sie die E-Mail (einschließlich etwaiger beigefügter Dateien) von Ihrem System. Vielen Dank.
The contents of this e-mail (including any attachments) are confidential and may be legally privileged. If you are not the intended recipient of this e-mail, any disclosure, copying, distribution or use of its contents is strictly prohibited, and you should please notify the sender immediately and then delete it (including any attachments) from your system. Thank you.
Am 17.01.2020 um 19:15 schrieb Andrija Panic <no...@github.com>:
Hm - when does this happen exactly @skattoju4<https://github.com/skattoju4>
I recall the Mike T. implemented a fix for the same when VMs are being stopped inside the OS (originally iSCSI session would not be removed)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<https://github.com/apache/cloudstack/pull/3819?email_source=notifications&email_token=ABJOT5E2F2KNP6A6KOYB2GLQ6HYSJA5CNFSM4KIKA52KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJIQXNY#issuecomment-575736759>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABJOT5GA7WW7SIKAO7SNWOLQ6HYSJANCNFSM4KIKA52A>.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579717256
thanks for the changes @skattoju3 . looks a bit cleaner.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3819: Clean up inactive
iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-578039156
@GabrielBrascher @DaanHoogland @andrijapanicsb you are right. we should move it to the 4.13 branch and then forward it to 4.14 👍
@skattoju4 can you change this?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland merged pull request #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland merged pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-580395725
<b>Trillian test result (tid-853)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30196 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3819-t853-kvm-centos7.zip
Smoke tests completed. 77 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] skattoju4 commented on a change in pull request #3819:
Clean up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
skattoju4 commented on a change in pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#discussion_r371334177
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -0,0 +1,127 @@
+package com.cloud.hypervisor.kvm.storage;
Review comment:
added the apache license
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579263600
@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel edited a comment on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
svenvogel edited a comment on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-578039156
@GabrielBrascher @DaanHoogland @andrijapanicsb Gabriel - you are right. we should move it to the 4.13 branch and then forward it to 4.14 👍
@skattoju4 can you change this?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3819: Clean up inactive
iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-577724831
@DaanHoogland for me we dont need it in 4.13. do you see any thing that we need 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland removed a comment on issue #3819: Clean
up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland removed a comment on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579346313
@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-577681071
apart from the license, do you need this in 4.13?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel edited a comment on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
svenvogel edited a comment on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579246689
@DaanHoogland ping ... can you run the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#discussion_r371932367
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -0,0 +1,143 @@
+// 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.hypervisor.kvm.storage;
+
+import com.cloud.hypervisor.kvm.resource.LibvirtConnection;
+import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser;
+import com.cloud.hypervisor.kvm.resource.LibvirtVMDef;
+import org.apache.cloudstack.managed.context.ManagedContextRunnable;
+import org.apache.log4j.Logger;
+import org.libvirt.Connect;
+import org.libvirt.Domain;
+import org.libvirt.LibvirtException;
+
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class IscsiStorageCleanupMonitor implements Runnable{
+ private static final Logger s_logger = Logger.getLogger(IscsiStorageCleanupMonitor.class);
+ private static final int CLEANUP_INTERVAL_SEC = 60; // check every X seconds
+ private static final String ISCSI_PATH_PREFIX = "/dev/disk/by-path";
+ private static final String KEYWORD_ISCSI = "iscsi";
+ private static final String KEYWORD_IQN = "iqn";
+
+ private IscsiAdmStorageAdaptor iscsiStorageAdaptor;
+
+ private Map<String, Boolean> diskStatusMap;
+
+ public IscsiStorageCleanupMonitor() {
+ diskStatusMap = new HashMap<>();
+ s_logger.debug("Initialize cleanup thread");
+ iscsiStorageAdaptor = new IscsiAdmStorageAdaptor();
+ }
+
+
+ private class Monitor extends ManagedContextRunnable {
+
+ @Override
+ protected void runInContext() {
+ Connect conn = null;
+ try {
+ conn = LibvirtConnection.getConnection();
+
+ //populate all the iscsi disks currently attached to this host
+ diskStatusMap.clear();
+ File[] iscsiVolumes = new File(ISCSI_PATH_PREFIX).listFiles();
+
+ if (iscsiVolumes == null || iscsiVolumes.length == 0) {
+ s_logger.debug("No iscsi sessions found for cleanup");
+ return;
+ }
+
+ for( File v : iscsiVolumes) {
+ if (isIscsiDisk(v.getAbsolutePath())) {
+ s_logger.debug("found iscsi disk by cleanup thread, marking inactive:" + v.getAbsolutePath());
+ diskStatusMap.put(v.getAbsolutePath(), false);
+ }
+ }
+
+ // check if they belong to any VM
+ int[] domains = conn.listDomains();
+ s_logger.debug(String.format("found %d domains", domains.length));
+ for (int domId : domains) {
+ Domain dm = conn.domainLookupByID(domId);
+ final String domXml = dm.getXMLDesc(0);
+ final LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
+ parser.parseDomainXML(domXml);
+ List<LibvirtVMDef.DiskDef> disks = parser.getDisks();
+
+ //check the volume map. If an entry exists change the status to True
+ for (final LibvirtVMDef.DiskDef disk : disks) {
+ if (diskStatusMap.containsKey(disk.getDiskPath())) {
+ diskStatusMap.put(disk.getDiskPath(), true);
+ s_logger.debug("active disk found by cleanup thread" + disk.getDiskPath());
Review comment:
space needed in log message before the path.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan removed a comment on issue #3819: Clean
up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan removed a comment on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579274441
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-678
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on a change in pull request
#3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#discussion_r370114423
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -0,0 +1,127 @@
+package com.cloud.hypervisor.kvm.storage;
Review comment:
you need a license here to pass (rat failure)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request
#3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#discussion_r370260437
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -0,0 +1,127 @@
+package com.cloud.hypervisor.kvm.storage;
+
+import com.cloud.hypervisor.kvm.resource.LibvirtConnection;
+import com.cloud.hypervisor.kvm.resource.LibvirtDomainXMLParser;
+import com.cloud.hypervisor.kvm.resource.LibvirtVMDef;
+import org.apache.cloudstack.managed.context.ManagedContextRunnable;
+import org.apache.log4j.Logger;
+import org.libvirt.Connect;
+import org.libvirt.Domain;
+import org.libvirt.LibvirtException;
+
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class IscsiStorageCleanupMonitor implements Runnable{
+ private static final Logger s_logger = Logger.getLogger(IscsiStorageCleanupMonitor.class);
+ private static final int CLEANUP_INTERVAL_SEC = 60; // check every X seconds
+ private static final String ISCSI_PATH_PREFIX = "/dev/disk/by-path";
+ private static final String KEYWORD_ISCSI = "iscsi";
+ private static final String KEYWORD_IQN = "iqn";
+
+ private IscsiAdmStorageAdaptor iscsiStorageAdaptor;
+
+ private Map<String, Boolean> diskStatusMap;
+
+ public IscsiStorageCleanupMonitor() {
+ diskStatusMap = new HashMap<>();
+ s_logger.debug("Initialize cleanup thread");
+ iscsiStorageAdaptor = new IscsiAdmStorageAdaptor();
+ }
+
+
+ private class Monitor extends ManagedContextRunnable {
+
+ @Override
+ protected void runInContext() {
+ Connect conn = null;
+ try {
+ conn = LibvirtConnection.getConnection();
+
+ //populate all the iscsi disks currently attached to this host
+ diskStatusMap.clear();
+ File[] iscsiVolumes = new File(ISCSI_PATH_PREFIX).listFiles();
+
+ if (iscsiVolumes == null || iscsiVolumes.length == 0) {
+ s_logger.debug("No iscsi sessions found for cleanup");
+ return;
+ }
+
+ for( File v : iscsiVolumes) {
+ if (isIscsiDisk(v.getAbsolutePath())) {
+ s_logger.debug("found iscsi disk by cleanup thread, marking inactive:" + v.getAbsolutePath());
+ diskStatusMap.put(v.getAbsolutePath(), false);
+ }
+ }
+
+ // check if they belong to any VM
+ int[] domains = conn.listDomains();
+ s_logger.debug(String.format("found %d domains", domains.length));
+ for (int domId : domains) {
+ Domain dm = conn.domainLookupByID(domId);
+ final String domXml = dm.getXMLDesc(0);
+ final LibvirtDomainXMLParser parser = new LibvirtDomainXMLParser();
+ parser.parseDomainXML(domXml);
+ List<LibvirtVMDef.DiskDef> disks = parser.getDisks();
+
+ //check the volume map. If an entry exists change the status to True
+ for (final LibvirtVMDef.DiskDef disk : disks) {
+ if (diskStatusMap.containsKey(disk.getDiskPath())) {
+ diskStatusMap.put(disk.getDiskPath(), true);
+ s_logger.debug("active disk found by cleanup thread" + disk.getDiskPath());
+ }
+ }
Review comment:
I would recommend creating a few methods (e.g. `checkIfIscsSessionBelongAnyVm(Connect)`, etc) which would allow documenting (Javadoc) and creating unit tests (JUnit).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-580172530
@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579263923
@GabrielBrascher 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-580169568
Packaging result: ✖centos6 ✔centos7 ✔debian. JID-714
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579274441
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-678
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] blueorangutan commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579346412
@DaanHoogland 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] andrijapanicsb commented on issue #3819: Clean up
inactive iscsi sessions when VMs are removed.
Posted by GitBox <gi...@apache.org>.
andrijapanicsb commented on issue #3819: Clean up inactive iscsi sessions when VMs are removed.
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-575736759
Hm - when does this happen exactly @skattoju4
I recall the Mike T. implemented a fix for the same when VMs are being stopped inside the OS (originally iSCSI session would not be removed)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] syed commented on issue #3819: Clean up inactive iscsi
sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
syed commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-577357293
LGTM :+1:
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3819: Clean up inactive
iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579246689
@DaanHoogland ping ...
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-580159385
@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] rhtyd closed pull request #3819: Clean up inactive
iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
rhtyd closed pull request #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3819: Clean up inactive
iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579139719
@rhtyd can we run tests please? unfortunately you can only test the code formally or using a solidfire.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] svenvogel commented on issue #3819: Clean up inactive
iscsi sessions when VMs are removed.
Posted by GitBox <gi...@apache.org>.
svenvogel commented on issue #3819: Clean up inactive iscsi sessions when VMs are removed.
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-575745119
@syed @skattoju4 thanks for your work! we tested it in our production environment and it works fine.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] DaanHoogland commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-579346313
@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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [cloudstack] GabrielBrascher commented on issue #3819: Clean up
inactive iscsi sessions when VMs get moved due to crashes
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on issue #3819: Clean up inactive iscsi sessions when VMs get moved due to crashes
URL: https://github.com/apache/cloudstack/pull/3819#issuecomment-577864320
@DaanHoogland @svenvogel considering that it is a bug, it would be awesome to have it in 4.13 branch (aiming 4.13.1 release) and then forward it to master (aiming 4.14).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services