Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/disconnect volume #29

Merged
merged 19 commits into from
Nov 10, 2021
Merged

Fix/disconnect volume #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
121455e
fix example compilation
plaffitt Nov 5, 2020
fca97c2
properly disconnect volume
plaffitt Nov 18, 2020
efce096
standardize Device usage
plaffitt Oct 29, 2021
8f2b4d0
improve error reporting on command execution
plaffitt Jan 11, 2021
d23b324
refactor and simplify Connector
plaffitt Oct 29, 2021
7b2d8ee
add function to resize multipath map
plaffitt Feb 1, 2021
898cec5
check if device exists before flushing it
plaffitt Feb 16, 2021
df4b0a3
fix tests and example
plaffitt Feb 17, 2021
736e81c
replace manual reassignation with stubs in tests
plaffitt Feb 18, 2021
c1dd946
add tests and remove dead code
plaffitt Feb 19, 2021
d1abc07
refactor lsblk
plaffitt Oct 29, 2021
ae559b8
check multipath consistency on connection/disconnection
plaffitt Oct 29, 2021
203e987
fix typo in logs
plaffitt Jun 8, 2021
fb3c9e1
remove devices even if some are missing
plaffitt Jun 9, 2021
52c573c
fix multipath detection on device lost
plaffitt Jun 9, 2021
aac01d4
add hint about multipathd not running in logs
plaffitt Jun 11, 2021
d192526
add a timeout on iscsciadm commands
plaffitt Sep 29, 2021
75f4b8e
backward-compatibility
plaffitt Oct 14, 2021
b0ce999
use raw format in lsblk in order to support a wider range of versions
plaffitt Oct 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.PHONY: all build clean install
.PHONY: all build clean install test coverage

all: clean build install

Expand All @@ -16,3 +16,6 @@ install:
test:
go test ./iscsi/

coverage:
go test ./iscsi -coverprofile=coverage.out
go tool cover -html=coverage.out
46 changes: 21 additions & 25 deletions example/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,31 +11,29 @@ import (
)

var (
portals = flag.String("portals", "192.168.1.112:3260", "Comma delimited. Eg: 1.1.1.1,2.2.2.2")
iqn = flag.String("iqn", "iqn.2010-10.org.openstack:volume-95739000-1557-44f8-9f40-e9d29fe6ec47", "")
multipath = flag.Bool("multipath", false, "")
username = flag.String("username", "3aX7EEf3CEgvESQG75qh", "")
password = flag.String("password", "eJBDC7Bt7WE3XFDq", "")
lun = flag.Int("lun", 1, "")
debug = flag.Bool("debug", false, "enable logging")
portals = flag.String("portals", "192.168.1.112:3260", "Comma delimited. Eg: 1.1.1.1,2.2.2.2")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point all these defaults should be removed and we should a mandatory check that the flags are set. These dfaults are nice for an example but they won't mean anything to to someone that's not using the openstack test setup I had 3 years ago :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure @j-griffith , I will get this cleared while working on some other optimization on this. Opening a tracker issue for this as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#33 tracks this.

iqn = flag.String("iqn", "iqn.2010-10.org.openstack:volume-95739000-1557-44f8-9f40-e9d29fe6ec47", "")
username = flag.String("username", "3aX7EEf3CEgvESQG75qh", "")
password = flag.String("password", "eJBDC7Bt7WE3XFDq", "")
lun = flag.Int("lun", 1, "")
debug = flag.Bool("debug", false, "enable logging")
)

func main() {
flag.Parse()
tgtp := strings.Split(*portals, ",")
tgtps := strings.Split(*portals, ",")
if *debug {
iscsi.EnableDebugLogging(os.Stdout)
}

// You can utilize the iscsiadm calls directly if you wish, but by creating a Connector
// you can simplify interactions to simple calls like "Connect" and "Disconnect"
c := iscsi.Connector{
c := &iscsi.Connector{
// Our example uses chap
AuthType: "chap",
// Specify the target iqn we're dealing with
TargetIqn: *iqn,
// List of portals must be >= 1 (>1 signals multipath/mpio)
TargetPortals: tgtp,
// List of targets must be >= 1 (>1 signals multipath/mpio)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we have just one item in the targets can we consider it as "multipath" enabled share ? previously the decision was based on portals which always true and point us that its a multipath share.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the current changes, there is a function Connector.IsMultipathEnabled that returns true if Connector.MountTargetDevice.Type == "mpath". So it doesn't depends on how many targets we have but on the fact that the devices is actually handled by multipathd or not. This information is retrieved through lsblk.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the changes proposed, but if at all possible, it would be great to not add new dependencies on host binaries such as lsblk and blockdev. Each new dependency makes it more challenging to support older OS releases. I wish I could tell customers to upgrade to the latest tool versions, but this is often very challenging in some manufacturing or highly secure environments.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also found a few issues with the current https://github.com/kubernetes-csi/csi-lib-iscsi version, as I tested under a few different operating systems (Ubuntu, CentOS, RHEL), so my interest is to either work with the Enix PR changes or submit a PR here to address some small changes to handle multipath with iSCSI storage arrays.

One test case of interest, is testing these PR changes with multipathd.conf using 'user_friendly_names: "yes"' as I experienced issues when a customer was using this setting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jskazinski indeed very valuable points, it differently behaved for me as well with different distros.. Regarding user_friendly_names its indeed a good test to have.

TargetIqn: *iqn,
TargetPortals: tgtps,
// CHAP can be setup up for discovery as well as sessions, our example
// device only uses CHAP security for sessions, for those that use Discovery
// as well, we'd add a DiscoverySecrets entry the same way
Expand All @@ -45,8 +43,6 @@ func main() {
SecretsType: "chap"},
// Lun is the lun number the devices uses for exports
Lun: int32(*lun),
// Multipath indicates that we want to configure this connection as a multipath device
Multipath: *multipath,
// Number of times we check for device path, waiting for CheckInterval seconds inbetween each check (defaults to 10 if omitted)
RetryCount: 11,
// CheckInterval is the time in seconds to wait inbetween device path checks when logging in to a target
Expand All @@ -55,21 +51,21 @@ func main() {

// Now we can just issue a connection request using our Connector
// A succesful connection will include the device path to access our iscsi volume
path, err := iscsi.Connect(c)
path, err := c.Connect()
if err != nil {
log.Printf("Error returned from iscsi.Connect: %s", err.Error())
os.Exit(1)
}

if path == "" {
log.Printf("Failed to connect, didn't receive a path, but also no error!")
log.Printf("Error returned from c.Connect: %s", err.Error())
os.Exit(1)
}

log.Printf("Connected device at path: %s\n", path)
time.Sleep(3 * time.Second)

// Disconnect is easy as well, we don't need the full Connector any more, just the Target IQN and the Portals
/// this should disconnect the volume as well as clear out the iscsi DB entries associated with it
iscsi.Disconnect(c.TargetIqn, c.TargetPortals)
// This will disconnect the volume
plaffitt marked this conversation as resolved.
Show resolved Hide resolved
if err := c.DisconnectVolume(); err != nil {
log.Printf("Error returned from c.DisconnectVolume: %s", err.Error())
os.Exit(1)
}

// This will disconnect the session as well as clear out the iscsi DB entries associated with it
c.Disconnect()
}
8 changes: 8 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
module github.com/kubernetes-csi/csi-lib-iscsi

go 1.15

require (
github.com/prashantv/gostub v1.0.0
github.com/stretchr/testify v1.7.0
)
13 changes: 13 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/prashantv/gostub v1.0.0 h1:wTzvgO04xSS3gHuz6Vhuo0/kvWelyJxwNS0IRBPAwGY=
github.com/prashantv/gostub v1.0.0/go.mod h1:dP1v6T1QzyGJJKFocwAU0lSZKpfjstjH8TlhkEU0on0=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c h1:dUUwHk2QECo/6vqA44rthZ8ie2QXMNeKRTHCNY2nXvo=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
Loading