Clean up inactive iscsi sessions when VMs get moved due to crashes by skattoju4 · Pull Request #3819 · apache/cloudstack

skattoju4 · 2020-01-17T15:31:59Z

Description

Previously, iscsi sessions would not be cleaned up on KVM when

vms are crashed or rebooted on another host
a hypervisor host crashed and ha enabled vms

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.

steps to reproduce:

check there are a vm is running and sessions to Netapp Solidfire are there ìscsiadm -m session -P 0
crash the kvm host (e.g. power off, be sure there are running vms on it) or destroy a virtual machine (virsh destroy vmname)
look with ìscsiadm -m session -P 0 and you will see sessions they should not be there

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)
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?

This has been tested by observing that inactive iscsi sessions are cleaned up when VMs are removed.

andrijapanicsb · 2020-01-17T18:14:54Z

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)

svenvogel · 2020-01-17T18:24:33Z

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 <notifications@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<#3819?email_source=notifications&email_token=ABJOT5E2F2KNP6A6KOYB2GLQ6HYSJA5CNFSM4KIKA52KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJIQXNY#issuecomment-575736759>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABJOT5GA7WW7SIKAO7SNWOLQ6HYSJANCNFSM4KIKA52A>.

svenvogel · 2020-01-17T18:38:30Z

@syed @skattoju4 thanks for your work! we tested it in our production environment and it works fine.

svenvogel · 2020-01-17T18:38:50Z

@syed can you review please?

syed · 2020-01-22T19:53:34Z

LGTM 👍

DaanHoogland · 2020-01-23T13:27:59Z

apart from the license, do you need this in 4.13?

svenvogel · 2020-01-23T15:14:04Z

@DaanHoogland for me we dont need it in 4.13. do you see any thing that we need it?

DaanHoogland · 2020-01-23T15:57:17Z

@svenvogel you marked it a bug, that's why i asked, nothing else ... ;)

GabrielBrascher

Thanks for the PR @skattoju4. I left a question and a minor observation. Overall it looks good (+0.99 🙂 )

GabrielBrascher · 2020-01-23T17:16:36Z

+
+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


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)?

GabrielBrascher · 2020-01-23T17:39:59Z

+                            diskStatusMap.put(disk.getDiskPath(), true);
+                            s_logger.debug("active disk found by cleanup thread" + disk.getDiskPath());
+                        }
+                    }


I would recommend creating a few methods (e.g. checkIfIscsSessionBelongAnyVm(Connect), etc) which would allow documenting (Javadoc) and creating unit tests (JUnit).

GabrielBrascher · 2020-01-23T20:34:34Z

@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).

svenvogel · 2020-01-24T08:34:24Z

@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?

DaanHoogland · 2020-01-28T16:59:22Z

+                // 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());
+                        }
+                    }
+                }


checkDiskStatusMap();

DaanHoogland · 2020-01-28T16:59:49Z

+                    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());


space needed in log message before the path.

DaanHoogland · 2020-01-28T17:03:58Z

+                            }
+                        }
+                    }
+                }


disconnectPhysicalDisks();

DaanHoogland

no real issues in the code, but i would like to see some more partitioning of the logic in methods.

DaanHoogland · 2020-01-29T11:40:29Z

thanks for the changes @skattoju3 . looks a bit cleaner.

changes look good

DaanHoogland · 2020-01-30T09:19:19Z

@blueorangutan package

blueorangutan · 2020-01-30T09:19:24Z

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-01-30T09:42:18Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-714

DaanHoogland · 2020-01-30T09:48:28Z

@blueorangutan test

blueorangutan · 2020-01-30T09:49:25Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-01-30T18:43:11Z

Trillian test result (tid-853)
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

skattoju4 · 2020-02-03T15:28:46Z

thanks for the changes @skattoju3 . looks a bit cleaner.

Thanks for the feedback :)

simon-a-james · 2020-05-13T16:28:33Z

Would this fix cause a problem with Hypervisors that use iSCSI as its system disk?

It seems that when iscsiStorageCleanupMonitor runs, it kills the iscsi connection for the hypervisor iscsi system disk and causes the hypervisor system disk to go into a read only state. Is there a way to stop this running - I dont use iSCSI for KVM guests

skattoju4 · 2020-05-13T19:57:46Z

Yes this seems to be the case. It is currently a limitation. Iscsi disks that are not attached to VMs will be marked as inactive and attempted to be cleaned up. One solution could be to have this feature be toggle-able by a global setting.

simon-a-james · 2020-05-13T20:03:45Z

Thanks for the update. If I just use the agent version 4.11, will that not perform the iSCSI cleanup and the HVs can work normally again or is this a management server thing?

skattoju4 · 2020-05-13T20:05:51Z

It's part of the agent. Using an older agent should do the trick for now.

svenvogel added component:managed-storage type:bug type:enhancement labels Jan 17, 2020

svenvogel requested review from mike-tutkowski and svenvogel January 17, 2020 18:34

svenvogel added the component:kvm label Jan 17, 2020

skattoju4 changed the title ~~Clean up inactive iscsi sessions when VMs are removed.~~ Clean up inactive iscsi sessions when VMs get moved due to crashes Jan 17, 2020

svenvogel requested review from GabrielBrascher, andrijapanicsb, nathanejohnson and wido January 18, 2020 11:41

DaanHoogland previously requested changes Jan 23, 2020

View reviewed changes

Comment thread ...pervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java

DaanHoogland added this to the 4.14.0.0 milestone Jan 23, 2020

GabrielBrascher reviewed Jan 23, 2020

View reviewed changes

skattoju4 changed the base branch from master to 4.13 January 27, 2020 19:50

skattoju4 force-pushed the iscsi-session-cleanup-master branch 2 times, most recently from cc3402f to a750ce0 Compare January 27, 2020 20:17

skattoju4 changed the base branch from 4.13 to master January 27, 2020 20:19

skattoju4 changed the base branch from master to 4.13 January 27, 2020 20:20

skattoju4 force-pushed the iscsi-session-cleanup-master branch from a750ce0 to 932a98b Compare January 27, 2020 21:01

DaanHoogland reviewed Jan 28, 2020

View reviewed changes

factor code into methods

f34eca6

yadvr closed this Jan 29, 2020

yadvr reopened this Jan 29, 2020

mike-tutkowski approved these changes Jan 29, 2020

View reviewed changes

DaanHoogland mentioned this pull request Jan 30, 2020

[KVM] Direct download agnostic of the storage provider #3828

Merged

5 tasks

apache deleted a comment from blueorangutan Jan 30, 2020

apache deleted a comment from GabrielBrascher Jan 30, 2020

DaanHoogland merged commit 6baa598 into apache:4.13 Jan 30, 2020

svenvogel mentioned this pull request Jul 17, 2020

iSCSI paths will be logged out / path detection failure #4216

Closed

GabrielBrascher mentioned this pull request Jul 21, 2020

iscsi session cleanup now configurable, filters iscsi partitions #4219

Merged

5 tasks

Conversation

skattoju4 commented Jan 17, 2020 • edited by svenvogel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

andrijapanicsb commented Jan 17, 2020

Uh oh!

svenvogel commented Jan 17, 2020 via email

Uh oh!

svenvogel commented Jan 17, 2020

Uh oh!

svenvogel commented Jan 17, 2020

Uh oh!

syed commented Jan 22, 2020

Uh oh!

Uh oh!

DaanHoogland commented Jan 23, 2020

Uh oh!

svenvogel commented Jan 23, 2020

Uh oh!

DaanHoogland commented Jan 23, 2020

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svenvogel commented Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jan 29, 2020

Uh oh!

DaanHoogland commented Jan 30, 2020

Uh oh!

blueorangutan commented Jan 30, 2020

Uh oh!

blueorangutan commented Jan 30, 2020

Uh oh!

DaanHoogland commented Jan 30, 2020

Uh oh!

blueorangutan commented Jan 30, 2020

Uh oh!

blueorangutan commented Jan 30, 2020

Uh oh!

skattoju4 commented Feb 3, 2020

Uh oh!

simon-a-james commented May 13, 2020

Uh oh!

skattoju4 commented May 13, 2020

Uh oh!

simon-a-james commented May 13, 2020

Uh oh!

skattoju4 commented May 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

skattoju4 commented Jan 17, 2020 •

edited by svenvogel

Loading

GabrielBrascher commented Jan 23, 2020 •

edited

Loading

svenvogel commented Jan 24, 2020 •

edited

Loading