Skip to content

Commit 855f3e7

Browse files
author
Kubernetes Submit Queue
authored
Merge pull request kubernetes#50688 from allencloud/refactor-code-in-volume-iscsi
Automatic merge from submit-queue (batch tested with PRs 49861, 50933, 51380, 50688, 51305) refactor codes in volume iscsi to improve readability Signed-off-by: allencloud <allen.sun@daocloud.io> **What this PR does / why we need it**: This PR refactors some codes in pkg/volume/iscsi. What is specific, this PR takes advantage of return fast to make codes indent less. As a result the readability of codes will improve a little bit. What I did: 1. refactor codes in volume iscsi to improve readability. 2. change a keyword of `delete` into `deleteArgs` to reduce ambiguousness. 3. make some variables camel case. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # NONE **Special notes for your reviewer**: NONE **Release note**: ```release-note NONE ```
2 parents d5a811a + c8c7139 commit 855f3e7

File tree

1 file changed

+164
-157
lines changed

1 file changed

+164
-157
lines changed

‎pkg/volume/iscsi/iscsi_util.go‎

Lines changed: 164 additions & 157 deletions
Original file line numberDiff line numberDiff line change
@@ -46,39 +46,42 @@ var (
4646
)
4747

4848
func updateISCSIDiscoverydb(b iscsiDiskMounter, tp string) error {
49-
if b.chap_discovery {
50-
out, err := b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "update", "-n", "discovery.sendtargets.auth.authmethod", "-v", "CHAP")
51-
if err != nil {
52-
return fmt.Errorf("iscsi: failed to update discoverydb with CHAP, output: %v", string(out))
53-
}
49+
if !b.chap_discovery {
50+
return nil
51+
}
52+
out, err := b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "update", "-n", "discovery.sendtargets.auth.authmethod", "-v", "CHAP")
53+
if err != nil {
54+
return fmt.Errorf("iscsi: failed to update discoverydb with CHAP, output: %v", string(out))
55+
}
5456

55-
for _, k := range chap_st {
56-
v := b.secret[k]
57-
if len(v) > 0 {
58-
out, err := b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "update", "-n", k, "-v", v)
59-
if err != nil {
60-
return fmt.Errorf("iscsi: failed to update discoverydb key %q with value %q error: %v", k, v, string(out))
61-
}
57+
for _, k := range chap_st {
58+
v := b.secret[k]
59+
if len(v) > 0 {
60+
out, err := b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "update", "-n", k, "-v", v)
61+
if err != nil {
62+
return fmt.Errorf("iscsi: failed to update discoverydb key %q with value %q error: %v", k, v, string(out))
6263
}
6364
}
6465
}
6566
return nil
6667
}
6768

6869
func updateISCSINode(b iscsiDiskMounter, tp string) error {
69-
if b.chap_session {
70-
out, err := b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "-o", "update", "-n", "node.session.auth.authmethod", "-v", "CHAP")
71-
if err != nil {
72-
return fmt.Errorf("iscsi: failed to update node with CHAP, output: %v", string(out))
73-
}
70+
if !b.chap_session {
71+
return nil
72+
}
7473

75-
for _, k := range chap_sess {
76-
v := b.secret[k]
77-
if len(v) > 0 {
78-
out, err := b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "-o", "update", "-n", k, "-v", v)
79-
if err != nil {
80-
return fmt.Errorf("iscsi: failed to update node session key %q with value %q error: %v", k, v, string(out))
81-
}
74+
out, err := b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "-o", "update", "-n", "node.session.auth.authmethod", "-v", "CHAP")
75+
if err != nil {
76+
return fmt.Errorf("iscsi: failed to update node with CHAP, output: %v", string(out))
77+
}
78+
79+
for _, k := range chap_sess {
80+
v := b.secret[k]
81+
if len(v) > 0 {
82+
out, err := b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "-o", "update", "-n", k, "-v", v)
83+
if err != nil {
84+
return fmt.Errorf("iscsi: failed to update node session key %q with value %q error: %v", k, v, string(out))
8285
}
8386
}
8487
}
@@ -96,33 +99,35 @@ func waitForPathToExist(devicePath *string, maxRetries int, deviceTransport stri
9699
}
97100

98101
func waitForPathToExistInternal(devicePath *string, maxRetries int, deviceTransport string, osStat StatFunc, filepathGlob GlobFunc) bool {
99-
if devicePath != nil {
100-
for i := 0; i < maxRetries; i++ {
101-
var err error
102-
if deviceTransport == "tcp" {
103-
_, err = osStat(*devicePath)
102+
if devicePath == nil {
103+
return false
104+
}
105+
106+
for i := 0; i < maxRetries; i++ {
107+
var err error
108+
if deviceTransport == "tcp" {
109+
_, err = osStat(*devicePath)
110+
} else {
111+
fpath, _ := filepathGlob(*devicePath)
112+
if fpath == nil {
113+
err = os.ErrNotExist
104114
} else {
105-
fpath, _ := filepathGlob(*devicePath)
106-
if fpath == nil {
107-
err = os.ErrNotExist
108-
} else {
109-
// There might be a case that fpath contains multiple device paths if
110-
// multiple PCI devices connect to same iscsi target. We handle this
111-
// case at subsequent logic. Pick up only first path here.
112-
*devicePath = fpath[0]
113-
}
114-
}
115-
if err == nil {
116-
return true
117-
}
118-
if !os.IsNotExist(err) {
119-
return false
120-
}
121-
if i == maxRetries-1 {
122-
break
115+
// There might be a case that fpath contains multiple device paths if
116+
// multiple PCI devices connect to same iscsi target. We handle this
117+
// case at subsequent logic. Pick up only first path here.
118+
*devicePath = fpath[0]
123119
}
124-
time.Sleep(time.Second)
125120
}
121+
if err == nil {
122+
return true
123+
}
124+
if !os.IsNotExist(err) {
125+
return false
126+
}
127+
if i == maxRetries-1 {
128+
break
129+
}
130+
time.Sleep(time.Second)
126131
}
127132
return false
128133
}
@@ -231,54 +236,54 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) (string, error) {
231236
if iscsiTransport == "" {
232237
glog.Errorf("iscsi: could not find transport name in iface %s", b.Iface)
233238
return "", fmt.Errorf("Could not parse iface file for %s", b.Iface)
234-
} else if iscsiTransport == "tcp" {
239+
}
240+
if iscsiTransport == "tcp" {
235241
devicePath = strings.Join([]string{"/dev/disk/by-path/ip", tp, "iscsi", b.Iqn, "lun", b.lun}, "-")
236242
} else {
237243
devicePath = strings.Join([]string{"/dev/disk/by-path/pci", "*", "ip", tp, "iscsi", b.Iqn, "lun", b.lun}, "-")
238244
}
239-
exist := waitForPathToExist(&devicePath, 1, iscsiTransport)
240-
if exist == false {
241-
// build discoverydb and discover iscsi target
242-
b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "new")
243-
// update discoverydb with CHAP secret
244-
err = updateISCSIDiscoverydb(b, tp)
245-
if err != nil {
246-
lastErr = fmt.Errorf("iscsi: failed to update discoverydb to portal %s error: %v", tp, err)
247-
continue
248-
}
249-
out, err := b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "--discover")
250-
if err != nil {
251-
// delete discoverydb record
252-
b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "delete")
253-
lastErr = fmt.Errorf("iscsi: failed to sendtargets to portal %s output: %s, err %v", tp, string(out), err)
254-
continue
255-
}
256-
err = updateISCSINode(b, tp)
257-
if err != nil {
258-
// failure to update node db is rare. But deleting record will likely impact those who already start using it.
259-
lastErr = fmt.Errorf("iscsi: failed to update iscsi node to portal %s error: %v", tp, err)
260-
continue
261-
}
262-
// login to iscsi target
263-
out, err = b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "--login")
264-
if err != nil {
265-
// delete the node record from database
266-
b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-I", b.Iface, "-T", b.Iqn, "-o", "delete")
267-
lastErr = fmt.Errorf("iscsi: failed to attach disk: Error: %s (%v)", string(out), err)
268-
continue
269-
}
270-
exist = waitForPathToExist(&devicePath, 10, iscsiTransport)
271-
if !exist {
272-
glog.Errorf("Could not attach disk: Timeout after 10s")
273-
// update last error
274-
lastErr = fmt.Errorf("Could not attach disk: Timeout after 10s")
275-
continue
276-
} else {
277-
devicePaths = append(devicePaths, devicePath)
278-
}
279-
} else {
245+
246+
if exist := waitForPathToExist(&devicePath, 1, iscsiTransport); exist {
280247
glog.V(4).Infof("iscsi: devicepath (%s) exists", devicePath)
281248
devicePaths = append(devicePaths, devicePath)
249+
continue
250+
}
251+
// build discoverydb and discover iscsi target
252+
b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "new")
253+
// update discoverydb with CHAP secret
254+
err = updateISCSIDiscoverydb(b, tp)
255+
if err != nil {
256+
lastErr = fmt.Errorf("iscsi: failed to update discoverydb to portal %s error: %v", tp, err)
257+
continue
258+
}
259+
out, err = b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "--discover")
260+
if err != nil {
261+
// delete discoverydb record
262+
b.exec.Run("iscsiadm", "-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.Iface, "-o", "delete")
263+
lastErr = fmt.Errorf("iscsi: failed to sendtargets to portal %s output: %s, err %v", tp, string(out), err)
264+
continue
265+
}
266+
err = updateISCSINode(b, tp)
267+
if err != nil {
268+
// failure to update node db is rare. But deleting record will likely impact those who already start using it.
269+
lastErr = fmt.Errorf("iscsi: failed to update iscsi node to portal %s error: %v", tp, err)
270+
continue
271+
}
272+
// login to iscsi target
273+
out, err = b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-T", b.Iqn, "-I", b.Iface, "--login")
274+
if err != nil {
275+
// delete the node record from database
276+
b.exec.Run("iscsiadm", "-m", "node", "-p", tp, "-I", b.Iface, "-T", b.Iqn, "-o", "delete")
277+
lastErr = fmt.Errorf("iscsi: failed to attach disk: Error: %s (%v)", string(out), err)
278+
continue
279+
}
280+
if exist := waitForPathToExist(&devicePath, 10, iscsiTransport); !exist {
281+
glog.Errorf("Could not attach disk: Timeout after 10s")
282+
// update last error
283+
lastErr = fmt.Errorf("Could not attach disk: Timeout after 10s")
284+
continue
285+
} else {
286+
devicePaths = append(devicePaths, devicePath)
282287
}
283288
}
284289

@@ -348,84 +353,86 @@ func (util *ISCSIUtil) DetachDisk(c iscsiDiskUnmounter, mntPath string) error {
348353
return err
349354
}
350355
cnt--
356+
if cnt != 0 {
357+
return nil
358+
}
351359
// if device is no longer used, see if need to logout the target
352-
if cnt == 0 {
353-
device, prefix, err := extractDeviceAndPrefix(mntPath)
360+
device, prefix, err := extractDeviceAndPrefix(mntPath)
361+
if err != nil {
362+
return err
363+
}
364+
refCount, err := getDevicePrefixRefCount(c.mounter, prefix)
365+
if err != nil || refCount != 0 {
366+
return nil
367+
}
368+
369+
var bkpPortal []string
370+
var volName, iqn, iface, initiatorName string
371+
found := true
372+
373+
// load iscsi disk config from json file
374+
if err := util.loadISCSI(c.iscsiDisk, mntPath); err == nil {
375+
bkpPortal, iqn, iface, volName = c.iscsiDisk.Portals, c.iscsiDisk.Iqn, c.iscsiDisk.Iface, c.iscsiDisk.VolName
376+
initiatorName = c.iscsiDisk.InitiatorName
377+
} else {
378+
// If the iscsi disk config is not found, fall back to the original behavior.
379+
// This portal/iqn/iface is no longer referenced, log out.
380+
// Extract the portal and iqn from device path.
381+
bkpPortal = make([]string, 1)
382+
bkpPortal[0], iqn, err = extractPortalAndIqn(device)
354383
if err != nil {
355384
return err
356385
}
357-
refCount, err := getDevicePrefixRefCount(c.mounter, prefix)
358-
if err == nil && refCount == 0 {
359-
var bkpPortal []string
360-
var volName, iqn, iface, initiatorName string
361-
found := true
362-
363-
// load iscsi disk config from json file
364-
if err := util.loadISCSI(c.iscsiDisk, mntPath); err == nil {
365-
bkpPortal, iqn, iface, volName = c.iscsiDisk.Portals, c.iscsiDisk.Iqn, c.iscsiDisk.Iface, c.iscsiDisk.VolName
366-
initiatorName = c.iscsiDisk.InitiatorName
367-
} else {
368-
// If the iscsi disk config is not found, fall back to the original behavior.
369-
// This portal/iqn/iface is no longer referenced, log out.
370-
// Extract the portal and iqn from device path.
371-
bkpPortal = make([]string, 1)
372-
bkpPortal[0], iqn, err = extractPortalAndIqn(device)
373-
if err != nil {
374-
return err
375-
}
376-
// Extract the iface from the mountPath and use it to log out. If the iface
377-
// is not found, maintain the previous behavior to facilitate kubelet upgrade.
378-
// Logout may fail as no session may exist for the portal/IQN on the specified interface.
379-
iface, found = extractIface(mntPath)
380-
}
381-
portals := removeDuplicate(bkpPortal)
382-
if len(portals) == 0 {
383-
return fmt.Errorf("iscsi detach disk: failed to detach iscsi disk. Couldn't get connected portals from configurations.")
384-
}
385-
386-
for _, portal := range portals {
387-
logout := []string{"-m", "node", "-p", portal, "-T", iqn, "--logout"}
388-
delete := []string{"-m", "node", "-p", portal, "-T", iqn, "-o", "delete"}
389-
if found {
390-
logout = append(logout, []string{"-I", iface}...)
391-
delete = append(delete, []string{"-I", iface}...)
392-
}
393-
glog.Infof("iscsi: log out target %s iqn %s iface %s", portal, iqn, iface)
394-
out, err := c.exec.Run("iscsiadm", logout...)
395-
if err != nil {
396-
glog.Errorf("iscsi: failed to detach disk Error: %s", string(out))
397-
}
398-
// Delete the node record
399-
glog.Infof("iscsi: delete node record target %s iqn %s", portal, iqn)
400-
out, err = c.exec.Run("iscsiadm", delete...)
401-
if err != nil {
402-
glog.Errorf("iscsi: failed to delete node record Error: %s", string(out))
403-
}
404-
}
405-
// Delete the iface after all sessions have logged out
406-
// If the iface is not created via iscsi plugin, skip to delete
407-
if initiatorName != "" && found && iface == (portals[0]+":"+volName) {
408-
delete := []string{"-m", "iface", "-I", iface, "-o", "delete"}
409-
out, err := c.exec.Run("iscsiadm", delete...)
410-
if err != nil {
411-
glog.Errorf("iscsi: failed to delete iface Error: %s", string(out))
412-
}
413-
}
414-
386+
// Extract the iface from the mountPath and use it to log out. If the iface
387+
// is not found, maintain the previous behavior to facilitate kubelet upgrade.
388+
// Logout may fail as no session may exist for the portal/IQN on the specified interface.
389+
iface, found = extractIface(mntPath)
390+
}
391+
portals := removeDuplicate(bkpPortal)
392+
if len(portals) == 0 {
393+
return fmt.Errorf("iscsi detach disk: failed to detach iscsi disk. Couldn't get connected portals from configurations.")
394+
}
395+
396+
for _, portal := range portals {
397+
logoutArgs := []string{"-m", "node", "-p", portal, "-T", iqn, "--logout"}
398+
deleteArgs := []string{"-m", "node", "-p", portal, "-T", iqn, "-o", "delete"}
399+
if found {
400+
logoutArgs = append(logoutArgs, []string{"-I", iface}...)
401+
deleteArgs = append(deleteArgs, []string{"-I", iface}...)
402+
}
403+
glog.Infof("iscsi: log out target %s iqn %s iface %s", portal, iqn, iface)
404+
out, err := c.exec.Run("iscsiadm", logoutArgs...)
405+
if err != nil {
406+
glog.Errorf("iscsi: failed to detach disk Error: %s", string(out))
407+
}
408+
// Delete the node record
409+
glog.Infof("iscsi: delete node record target %s iqn %s", portal, iqn)
410+
out, err = c.exec.Run("iscsiadm", deleteArgs...)
411+
if err != nil {
412+
glog.Errorf("iscsi: failed to delete node record Error: %s", string(out))
415413
}
416414
}
415+
// Delete the iface after all sessions have logged out
416+
// If the iface is not created via iscsi plugin, skip to delete
417+
if initiatorName != "" && found && iface == (portals[0]+":"+volName) {
418+
deleteArgs := []string{"-m", "iface", "-I", iface, "-o", "delete"}
419+
out, err := c.exec.Run("iscsiadm", deleteArgs...)
420+
if err != nil {
421+
glog.Errorf("iscsi: failed to delete iface Error: %s", string(out))
422+
}
423+
}
424+
417425
return nil
418426
}
419427

420428
func extractTransportname(ifaceOutput string) (iscsiTransport string) {
421429
re := regexp.MustCompile(`iface.transport_name = (.*)\n`)
422430

423-
rex_output := re.FindStringSubmatch(ifaceOutput)
424-
if rex_output != nil {
425-
iscsiTransport = rex_output[1]
426-
} else {
431+
rexOutput := re.FindStringSubmatch(ifaceOutput)
432+
if rexOutput == nil {
427433
return ""
428434
}
435+
iscsiTransport = rexOutput[1]
429436

430437
// While iface.transport_name is a required parameter, handle it being unspecified anyways
431438
if iscsiTransport == "<empty>" {
@@ -452,9 +459,9 @@ func extractDeviceAndPrefix(mntPath string) (string, string, error) {
452459
func extractIface(mntPath string) (string, bool) {
453460
re := regexp.MustCompile(`.+/iface-([^/]+)/.+`)
454461

455-
re_output := re.FindStringSubmatch(mntPath)
456-
if re_output != nil {
457-
return re_output[1], true
462+
reOutput := re.FindStringSubmatch(mntPath)
463+
if reOutput != nil {
464+
return reOutput[1], true
458465
}
459466

460467
return "", false

0 commit comments

Comments
 (0)