Subsequent VM Creation is failing for ISCSI Storage Pool on Oracle Linux by suryag1201 · Pull Request #88 · NetApp/cloudstack

suryag1201 · 2026-08-07T10:46:18Z

Description

Subsequent VM Creation is failing for ISCSI Storage Pool on Oracle Linux

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)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Tested on both Oracle and Unbuntu and tried creating multiple VMs

How did you try to break this feature and the system with this change?

…ol on Oracle Linux

Copilot

Pull request overview

This PR addresses failures when creating subsequent VMs on iSCSI primary storage with KVM (notably on Oracle Linux) by making iSCSI login/rescan behavior more robust and adding additional readiness checks before proceeding.

Changes:

Adds explicit handling for iscsiadm exit codes and a pre-login session existence check to decide when a rescan is needed.
Updates iSCSI login result handling to be idempotent across distros (Ubuntu vs Oracle Linux behavior) and triggers rescans when a session pre-exists.
Adds extra validation for device readiness and improves by-path filesystem checks before calling blockdev.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:177

The warning uses volumeUuid as the “target” identifier, but the log message is about the iSCSI target/portal; this can be misleading when troubleshooting. Use the computed IQN (iqn) in the message (and optionally include the volume path separately).

        if (getPhysicalDisk(volumeUuid, pool).getSize() <= 0) {
            logger.warn("iSCSI device not ready for target {} at {}:{} after wait", volumeUuid, host, port);
            return false;

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java:366

Files.isRegularFile(devicePath) follows symlinks by default, so a by-path symlink that (incorrectly) points to a regular file will be reported as a “regular file at by-path”, and the subsequent isSymbolicLink check becomes unreachable in that case. Check isSymbolicLink first, or use NOFOLLOW_LINKS to test the path itself.

            if (Files.isRegularFile(devicePath)) {
                logger.warn("Found a corrupt regular file at iSCSI by-path {} (expected block device symlink); it must be removed manually", deviceByPath);
                return 0L;
            }
            if (!Files.isSymbolicLink(devicePath)) {

github-actions · 2026-08-11T19:48:36Z

🔴 Test Coverage Grade: `D` — Marginal

Metric	Value
Line coverage	24.58%
Branch coverage	18.75%

Grade Scale

Grade	Line Coverage	Meaning
🟢 A	≥ 80%	Excellent - this code sleeps well at night 😴
🟡 B	60-79%	Good - almost there, don't stop now 😉
🟠 C	40-59%	Acceptable - your code is wearing a seatbelt, but no airbags 😬
🔴 D	20-39%	Marginal - boldly shipping where no test has gone before 🖖
⛔ F	< 20%	Failing - tests? what tests? 🔥

Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run

sandeeplocharla · 2026-08-14T05:28:33Z

+
+        OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser();
+
+        sessionCmd.executeIgnoreExitValue(parser, ISCSI_ERR_NO_OBJS_FOUND);


Would it be possible to have this code ISCSI_SESSION_EXISTS_CODE check here itself? Instead of having a separate check in ln 147?

piyush5netapp · 2026-08-17T09:10:18Z

+
+        String portal = host + ":" + port;
+
+        for (String line : sessions.split("\n")) {


as per this method, session is active if you see a any output line containing iqn and portal but we are parsing it using "\n". How are we sure if this parsing will not break on different OS flavour ?

Line splitting on "\n" is safe here and not OS-dependent.
isIscsiSessionActive() does not parse raw iscsiadm bytes with native line endings. Output goes through OutputInterpreter.AllLinesParser, which uses BufferedReader.readLine() (handles \n, \r\n, and \r) and then rejoins lines with "\n":

piyush5netapp · 2026-08-17T09:13:01Z

+                logger.debug("Device by-path does not exist yet: {}", deviceByPath);
+                return 0L;
+            }
+            if (Files.isRegularFile(devicePath)) {


how is this change required for ubuntu vs linux ?or is it agnostic to that ?

this change is not Ubuntu-vs-Linux/Oracle specific

github-actions · 2026-08-17T18:48:16Z

🔴 Test Coverage Grade: `D` — Marginal

Metric	Value
Line coverage	24.57%
Branch coverage	18.75%

Grade Scale

Grade	Line Coverage	Meaning
🟢 A	≥ 80%	Excellent - this code sleeps well at night 😴
🟡 B	60-79%	Good - almost there, don't stop now 😉
🟠 C	40-59%	Acceptable - your code is wearing a seatbelt, but no airbags 😬
🔴 D	20-39%	Marginal - boldly shipping where no test has gone before 🖖
⛔ F	< 20%	Failing - tests? what tests? 🔥

Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run

sandeeplocharla · 2026-08-18T08:59:45Z

+        OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser();
+        sessionCmd.executeIgnoreExitValue(parser, ISCSI_ERR_NO_OBJS_FOUND);
+        int exitValue = sessionCmd.getExitValue();
+        if (exitValue != 0 && exitValue != ISCSI_ERR_NO_OBJS_FOUND) {


We are anyways ignoring ISCSI_ERR_NO_OBJS_FOUND in sessionCmd.executeIgnoreExitValue(parser, ISCSI_ERR_NO_OBJS_FOUND);, so, maybe we can remove this recheck

executeIgnoreExitValue only allows exit 21 in addition to 0 so “no sessions” is not treated as a Script failure.
If we do not put, we will see the exception in the logs if there is no existing session. Please check this bug, i have updated the logs https://jira.ngage.netapp.com/browse/CSTACKEX-233
Above check is required to handle other exits code.

sandeeplocharla · 2026-08-18T09:05:05Z

                Thread.sleep(timeBetweenTries);
-            } catch (Exception ex) {
-                // don't do anything
+            } catch (InterruptedException ex) {


Why is it important for us to handle an exception due to interruption? Do we get any additional info from this while debugging?

We handle InterruptedException because Thread.sleep() can throw it when the thread is interrupted. In that case we stop waiting and fail the connect immediately, instead of continuing retries until the timeout.

sandeeplocharla · 2026-08-18T09:10:43Z

        // this method could still return (it should not block indefinitely) (the race condition
        // isn't solved here, but made highly unlikely to be a problem).
-        waitForDiskToBecomeAvailable(volumeUuid, pool);
+        if (!waitForDiskToBecomeAvailable(volumeUuid, pool)) {


They have mentioned in the comment that there could be a race condition between iSCSI login and device discovery. Unfortunately, this change doesn't seem to be fixing the race-condition, rather adding a strict check in waitForDiskToBecomeAvailable for device availability. Please see if this could be modified.

The race, login success before the by-path device appears is still addressed by the existing retry loop in waitForDiskToBecomeAvailable, but this can still happen (rare case). Previously, after timeout with size 0, connectPhysicalDisk still returned true. Now it returns false so we don’t treat an unavailable disk as a successful connect and end up creating a raw file.
Also, Will wait for community reply on this change

Gupta, Surya added 2 commits August 6, 2026 22:34

[CSTACKEX-233] Subsequent VM Creation is failing for ISCSI Storage Po…

f528fdf

…ol on Oracle Linux

[CSTACKEX-233] Added session exists error code check

f77f997

Copilot AI lite review requested due to automatic review settings August 7, 2026 10:46

Copilot started reviewing on behalf of suryag1201 August 7, 2026 10:46 View session

Copilot AI reviewed Aug 7, 2026

View reviewed changes

Comment thread ...s/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java

Comment thread ...s/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiAdmStorageAdaptor.java

[CSTACKEX-233] Review Comments

0271cef

Copilot AI review requested due to automatic review settings August 11, 2026 18:46

Copilot started reviewing on behalf of suryag1201 August 11, 2026 18:47 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

rajiv-jain-netapp approved these changes Aug 14, 2026

View reviewed changes

sandeeplocharla reviewed Aug 14, 2026

View reviewed changes

piyush5netapp reviewed Aug 17, 2026

View reviewed changes

[CSTACKEX-233] Address Review Comments

71a6730

sandeeplocharla reviewed Aug 18, 2026

View reviewed changes


		OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser();

		sessionCmd.executeIgnoreExitValue(parser, ISCSI_ERR_NO_OBJS_FOUND);


		String portal = host + ":" + port;

		for (String line : sessions.split("\n")) {

Uh oh!

Conversation

suryag1201 commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions Bot commented Aug 11, 2026

🔴 Test Coverage Grade: D — Marginal

Grade Scale

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 17, 2026

🔴 Test Coverage Grade: D — Marginal

Grade Scale

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suryag1201 Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suryag1201 Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

suryag1201 commented Aug 7, 2026 •

edited

Loading

🔴 Test Coverage Grade: `D` — Marginal

🔴 Test Coverage Grade: `D` — Marginal

suryag1201 Aug 18, 2026 •

edited

Loading

suryag1201 Aug 18, 2026 •

edited

Loading