Set session scanning to manual to avoid discovering all iSCSI devices… by mrobson · Pull Request #90985 · kubernetes/kubernetes

mrobson · 2020-05-11T16:16:20Z

… during login

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR sets node.session.scan to manual to avoid unused LUNs being discovered and mapped on a node when it does its initial login. If that happens, the devices are never removed.

Which issue(s) this PR fixes:
Fixes #90982

Special notes for your reviewer:
Some older iscsi package may not support node.session.scan - it was added here: open-iscsi/open-iscsi#40

This will not negatively impact anyone and would log a warning letting them know.

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2020-05-11T16:16:29Z

Hi @mrobson. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mrobson · 2020-05-11T16:35:45Z

/release-note-none

mrobson · 2020-05-11T16:36:58Z

/assign humblec

jsafrane · 2020-05-11T18:37:41Z

/ok-to-test

jsafrane · 2020-05-11T18:39:22Z

/assign @bswartz

Ben, can you please take a look? It's related to your commit 6d23d8e (and its commit message).

bswartz · 2020-05-11T20:55:38Z

This approach is something I didn't know was possible. I will definitely do some testing of my own to see if this does what I think it does. If so, then we definitely want this change.

mrobson · 2020-05-11T21:23:29Z

Thanks @bswartz - I did some local testing but would appreciate the verification! You can also do it at the session level like: iscsiadm -m session -r <session_id> -o update -n node.session.scan -v manual but that just seemed overly complicated versus what's currently being done.

humblec · 2020-05-12T06:10:53Z

/retest

humblec · 2020-05-12T06:11:24Z

Yeah, the approach looks good to me. But lets wait for verification.

pkg/volume/iscsi/iscsi_util.go

mrobson · 2020-05-12T15:27:52Z

For completeness, here is the basic test I ran with 3 LUNs:

/iscsi/iqn.20...et1/tpg1/luns> cd /
/> ls
o- / ......................................................................................................................... [...]
  o- backstores .............................................................................................................. [...]
  | o- block .................................................................................................. [Storage Objects: 3]
  | | o- scsi_disk1 ....................................................... [/dev/vg_iscsi1/lv_iscsi1 (5.0GiB) write-thru activated]
  | | | o- alua ................................................................................................... [ALUA Groups: 1]
  | | |   o- default_tg_pt_gp ....................................................................... [ALUA state: Active/optimized]
  | | o- scsi_disk2 ....................................................... [/dev/vg_iscsi2/lv_iscsi2 (5.0GiB) write-thru activated]
  | | | o- alua ................................................................................................... [ALUA Groups: 1]
  | | |   o- default_tg_pt_gp ....................................................................... [ALUA state: Active/optimized]
  | | o- scsi_disk3 ...................................................... [/dev/vg_iscsi3/lv_iscsi3 (10.0GiB) write-thru activated]
  | |   o- alua ................................................................................................... [ALUA Groups: 1]
  | |     o- default_tg_pt_gp ....................................................................... [ALUA state: Active/optimized]
  | o- fileio ................................................................................................. [Storage Objects: 0]
  | o- pscsi .................................................................................................. [Storage Objects: 0]
  | o- ramdisk ................................................................................................ [Storage Objects: 0]
  o- iscsi ............................................................................................................ [Targets: 1]
  | o- iqn.2020-05.matt.redhat:target1 ................................................................................... [TPGs: 1]
  |   o- tpg1 ............................................................................................... [no-gen-acls, no-auth]
  |     o- acls .......................................................................................................... [ACLs: 1]
  |     | o- iqn.2020-05.matt.redhat:infra1 ....................................................................... [Mapped LUNs: 3]
  |     |   o- mapped_lun0 ............................................................................ [lun0 block/scsi_disk1 (rw)]
  |     |   o- mapped_lun1 ............................................................................ [lun1 block/scsi_disk2 (rw)]
  |     |   o- mapped_lun2 ............................................................................ [lun2 block/scsi_disk3 (rw)]
  |     o- luns .......................................................................................................... [LUNs: 3]
  |     | o- lun0 ................................................. [block/scsi_disk1 (/dev/vg_iscsi1/lv_iscsi1) (default_tg_pt_gp)]
  |     | o- lun1 ................................................. [block/scsi_disk2 (/dev/vg_iscsi2/lv_iscsi2) (default_tg_pt_gp)]
  |     | o- lun2 ................................................. [block/scsi_disk3 (/dev/vg_iscsi3/lv_iscsi3) (default_tg_pt_gp)]
  |     o- portals .................................................................................................... [Portals: 1]
  |       o- 0.0.0.0:3260 ..................................................................................................... [OK]
  o- loopback ......................................................................................................... [Targets: 0]

Preform current discovery / login steps, minus CHAP:

# iscsiadm -m discoverydb -t sendtargets -p 10.10.100.34:3260 -I default -o new
# iscsiadm -m discoverydb -t sendtargets -p 10.10.100.34:3260 -I default --discover
# iscsiadm -m node -p 10.10.100.34:3260 -T iqn.2020-05.matt.redhat:target1 -I default --login

Logging in to [iface: default, target: iqn.2020-05.matt.redhat:target1, portal: 10.10.100.34,3260] (multiple)
Login to [iface: default, target: iqn.2020-05.matt.redhat:target1, portal: 10.10.100.34,3260] successful.

All 3 LUNs are discovered:

# lsscsi
[5:0:0:0]    disk    LIO-ORG  scsi_disk1       4.0   /dev/sda
[5:0:0:1]    disk    LIO-ORG  scsi_disk2       4.0   /dev/sdb
[5:0:0:2]    disk    LIO-ORG  scsi_disk3       4.0   /dev/sdc

ls /dev/disk/by-path/
fc---lun-0  ip-10.10.100.34:3260-iscsi-iqn.2020-05.matt.redhat:target1-lun-0  pci-0000:00:04.0        pci-0000:00:05.0-part1   virtio-pci-0000:00:04.0-part1  virtio-pci-0000:00:05.0-part2
fc---lun-1  ip-10.10.100.34:3260-iscsi-iqn.2020-05.matt.redhat:target1-lun-1  pci-0000:00:04.0-part1  pci-0000:00:05.0-part2   virtio-pci-0000:00:05.0
fc---lun-2  ip-10.10.100.34:3260-iscsi-iqn.2020-05.matt.redhat:target1-lun-2

Logout

# iscsiadm -m node -T iqn.2020-05.matt.redhat:target1 -p 10.10.100.34:3260 -u

Logging out of session [sid: 4, target: iqn.2020-05.matt.redhat:target1, portal: 10.10.100.34,3260]
Logout of [sid: 4, target: iqn.2020-05.matt.redhat:target1, portal: 10.10.100.34,3260] successful.

All gone

# lsscsi

Preform discovery / login steps, minus CHAP, setting node.session.scan to manual before login :

# iscsiadm -m discoverydb -t sendtargets -p 10.10.100.34:3260 -I default -o new
# iscsiadm -m discoverydb -t sendtargets -p 10.10.100.34:3260 -I default --discover
# iscsiadm -m node -p 10.10.100.34:3260 -T iqn.2020-05.matt.redhat:target1 -I default -o update -n node.session.scan -v manual
# iscsiadm -m node -p 10.10.100.34:3260 -T iqn.2020-05.matt.redhat:target1 -I default --login

Empty

# lsscsi

Discover one LUN

# echo '0 0 0' > /sys/class/scsi_host/host6/scan
# lsscsi
[6:0:0:0]    disk    LIO-ORG  scsi_disk1       4.0   /dev/sda

Discover another LUN

# echo '0 0 2' > /sys/class/scsi_host/host6/scan
lsscsi
[6:0:0:0]    disk    LIO-ORG  scsi_disk1       4.0   /dev/sda
[6:0:0:2]    disk    LIO-ORG  scsi_disk3       4.0   /dev/sdb

mrobson · 2020-05-12T19:55:05Z

/retest

mrobson · 2020-05-12T22:02:25Z

/retest

bswartz · 2020-05-13T02:22:53Z

I'm completed my initial tests and this does indeed seem to work as expected. Just need to make sure this is called on the write codepaths inside kubelet.

bswartz · 2020-05-13T02:24:16Z

@mrobson wrote:

Thanks @bswartz - I did some local testing but would appreciate the verification! You can also do it at the session level like: iscsiadm -m session -r <session_id> -o update -n node.session.scan -v manual but that just seemed overly complicated versus what's currently being done.

Based on my understanding, there is no session until after you log in, and by the time you log in, it's too late to set this option, so that would not be a viable approach. Setting at the node level does seem to be exactly what we want though.

mrobson · 2020-05-13T12:25:56Z

/retest

mrobson · 2020-05-13T12:29:25Z

I'm completed my initial tests and this does indeed seem to work as expected. Just need to make sure this is called on the write codepaths inside kubelet.

@bswartz From the testing / reproducer we ran, the only time this appears to be an issue is when AttachDisk is called and there is no existing login. Login only occurs again when a server is drained (all pod removed triggering logout), rebooted or when a logout happens naturally when the last mount in use is removed.

mrobson · 2020-05-13T12:49:26Z

Yay, green! Moved the location to before the chap check @jsafrane @bswartz @humblec PTAL

bswartz · 2020-05-13T19:05:15Z

The "iscsiadm -o update" command will fail on Ubuntu and Debian because they don't support the "node.session.scan" config option yet. I see that the code handles that case properly by logging a warning.

bswartz · 2020-05-18T04:09:41Z

/lgtm

jsafrane · 2020-05-19T19:02:56Z

/approve
Thanks a lot of for this PR!

k8s-ci-robot · 2020-05-19T19:03:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, mrobson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/volume/iscsi/OWNERS~~ [jsafrane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mrobson · 2020-05-19T21:01:34Z

/retest

fejta-bot · 2020-05-20T01:23:28Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-05-20T04:53:27Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-05-20T10:50:26Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

mrobson · 2020-05-20T11:25:55Z

/retest

mrobson · 2020-05-20T12:19:35Z

/retest

fejta-bot · 2020-05-20T14:41:26Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-05-20T17:08:27Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-05-20T20:38:27Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 11, 2020

k8s-ci-robot requested review from jsafrane and msau42 May 11, 2020 16:18

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 11, 2020

k8s-ci-robot assigned humblec May 11, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 11, 2020

k8s-ci-robot assigned bswartz May 11, 2020

mrobson commented May 12, 2020

View reviewed changes

pkg/volume/iscsi/iscsi_util.go Outdated Show resolved Hide resolved

mrobson force-pushed the iscsi-nodescan-manual branch from f6856c0 to ea0fdca Compare May 12, 2020 17:53

k8s-ci-robot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 12, 2020

mrobson force-pushed the iscsi-nodescan-manual branch from ea0fdca to 0389209 Compare May 12, 2020 17:56

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 12, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 19, 2020

k8s-ci-robot merged commit 2fa00e3 into kubernetes:master May 20, 2020

k8s-ci-robot added this to the v1.19 milestone May 20, 2020

jsafrane mentioned this pull request May 21, 2020

Bug 1825915: UPSTREAM: 90985: Set session scanning to manual to avoid discovering all iSCSI devices during login openshift/origin#25007

Merged

mrobson deleted the iscsi-nodescan-manual branch May 22, 2020 12:02

ffilippopoulos mentioned this pull request Oct 26, 2020

Upgrade Package Request: open-iscsi flatcar/Flatcar#219

Closed

Conversation

mrobson commented May 11, 2020

Uh oh!

k8s-ci-robot commented May 11, 2020

Uh oh!

mrobson commented May 11, 2020

Uh oh!

mrobson commented May 11, 2020

Uh oh!

jsafrane commented May 11, 2020

Uh oh!

jsafrane commented May 11, 2020

Uh oh!

bswartz commented May 11, 2020

Uh oh!

mrobson commented May 11, 2020

Uh oh!

humblec commented May 12, 2020

Uh oh!

humblec commented May 12, 2020

Uh oh!

Uh oh!

mrobson commented May 12, 2020

Uh oh!

mrobson commented May 12, 2020

Uh oh!

mrobson commented May 12, 2020

Uh oh!

bswartz commented May 13, 2020

Uh oh!

bswartz commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrobson commented May 13, 2020

Uh oh!

mrobson commented May 13, 2020

Uh oh!

mrobson commented May 13, 2020

Uh oh!

bswartz commented May 13, 2020

Uh oh!

bswartz commented May 18, 2020

Uh oh!

jsafrane commented May 19, 2020

Uh oh!

k8s-ci-robot commented May 19, 2020

Uh oh!

mrobson commented May 19, 2020

Uh oh!

fejta-bot commented May 20, 2020

Uh oh!

fejta-bot commented May 20, 2020

Uh oh!

fejta-bot commented May 20, 2020

Uh oh!

mrobson commented May 20, 2020

Uh oh!

mrobson commented May 20, 2020

Uh oh!

fejta-bot commented May 20, 2020

Uh oh!

fejta-bot commented May 20, 2020

Uh oh!

fejta-bot commented May 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bswartz commented May 13, 2020 •

edited

Loading