Merge pull request kubernetes#30737 from saad-ali/fix29358Round2

Automatic merge from submit-queue Skip safe to detach check if node API object no longer exists Fixes kubernetes#29358
lavalamp · Aug 18, 2016 · 9696a27 · 9696a27
2 parents 715f4f0 + 0c72568
commit 9696a27
Show file tree

Hide file tree

Showing 2 changed files with 53 additions and 34 deletions.
diff --git a/pkg/controller/volume/attachdetach/statusupdater/node_status_updater.go b/pkg/controller/volume/attachdetach/statusupdater/node_status_updater.go
@@ -67,7 +67,7 @@ func (nsu *nodeStatusUpdater) UpdateNodeStatuses() error {
 				"Could not update node status. Failed to find node %q in NodeInformer cache. %v",
 				nodeName,
 				err)
-			return nil
+			continue
 		}
 
 		node, ok := nodeObj.(*api.Node)

diff --git a/pkg/volume/util/operationexecutor/operation_executor.go b/pkg/volume/util/operationexecutor/operation_executor.go
@@ -26,6 +26,7 @@ import (
 
 	"github.com/golang/glog"
 	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/errors"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 	"k8s.io/kubernetes/pkg/types"
 	"k8s.io/kubernetes/pkg/util/mount"
@@ -544,41 +545,11 @@ func (oe *operationExecutor) generateDetachVolumeFunc(
 
 	return func() error {
 		if verifySafeToDetach {
-			// Fetch current node object
-			node, fetchErr := oe.kubeClient.Core().Nodes().Get(volumeToDetach.NodeName)
-			if fetchErr != nil {
+			safeToDetachErr := oe.verifyVolumeIsSafeToDetach(volumeToDetach)
+			if safeToDetachErr != nil {
 				// On failure, return error. Caller will log and retry.
-				return fmt.Errorf(
-					"DetachVolume failed fetching node from API server for volume %q (spec.Name: %q) from node %q with: %v",
-					volumeToDetach.VolumeName,
-					volumeToDetach.VolumeSpec.Name(),
-					volumeToDetach.NodeName,
-					fetchErr)
-			}
-
-			if node == nil {
-				// On failure, return error. Caller will log and retry.
-				return fmt.Errorf(
-					"DetachVolume failed fetching node from API server for volume %q (spec.Name: %q) from node %q. Error: node object retrieved from API server is nil.",
-					volumeToDetach.VolumeName,
-					volumeToDetach.VolumeSpec.Name(),
-					volumeToDetach.NodeName)
+				return err
 			}
-
-			for _, inUseVolume := range node.Status.VolumesInUse {
-				if inUseVolume == volumeToDetach.VolumeName {
-					return fmt.Errorf("DetachVolume failed for volume %q (spec.Name: %q) from node %q. Error: volume is still in use by node, according to Node status.",
-						volumeToDetach.VolumeName,
-						volumeToDetach.VolumeSpec.Name(),
-						volumeToDetach.NodeName)
-				}
-			}
-
-			// Volume not attached, return error. Caller will log and retry.
-			glog.Infof("Verified volume is safe to detach for volume %q (spec.Name: %q) from node %q.",
-				volumeToDetach.VolumeName,
-				volumeToDetach.VolumeSpec.Name(),
-				volumeToDetach.NodeName)
 		}
 
 		// Execute detach
@@ -607,6 +578,54 @@ func (oe *operationExecutor) generateDetachVolumeFunc(
 	}, nil
 }
 
+func (oe *operationExecutor) verifyVolumeIsSafeToDetach(
+	volumeToDetach AttachedVolume) error {
+	// Fetch current node object
+	node, fetchErr := oe.kubeClient.Core().Nodes().Get(volumeToDetach.NodeName)
+	if fetchErr != nil {
+		if errors.IsNotFound(fetchErr) {
+			glog.Warningf("Node %q not found on API server. DetachVolume will skip safe to detach check.",
+				volumeToDetach.NodeName,
+				volumeToDetach.VolumeName,
+				volumeToDetach.VolumeSpec.Name())
+			return nil
+		}
+
+		// On failure, return error. Caller will log and retry.
+		return fmt.Errorf(
+			"DetachVolume failed fetching node from API server for volume %q (spec.Name: %q) from node %q with: %v",
+			volumeToDetach.VolumeName,
+			volumeToDetach.VolumeSpec.Name(),
+			volumeToDetach.NodeName,
+			fetchErr)
+	}
+
+	if node == nil {
+		// On failure, return error. Caller will log and retry.
+		return fmt.Errorf(
+			"DetachVolume failed fetching node from API server for volume %q (spec.Name: %q) from node %q. Error: node object retrieved from API server is nil.",
+			volumeToDetach.VolumeName,
+			volumeToDetach.VolumeSpec.Name(),
+			volumeToDetach.NodeName)
+	}
+
+	for _, inUseVolume := range node.Status.VolumesInUse {
+		if inUseVolume == volumeToDetach.VolumeName {
+			return fmt.Errorf("DetachVolume failed for volume %q (spec.Name: %q) from node %q. Error: volume is still in use by node, according to Node status.",
+				volumeToDetach.VolumeName,
+				volumeToDetach.VolumeSpec.Name(),
+				volumeToDetach.NodeName)
+		}
+	}
+
+	// Volume is not marked as in use by node
+	glog.Infof("Verified volume is safe to detach for volume %q (spec.Name: %q) from node %q.",
+		volumeToDetach.VolumeName,
+		volumeToDetach.VolumeSpec.Name(),
+		volumeToDetach.NodeName)
+	return nil
+}
+
 func (oe *operationExecutor) generateMountVolumeFunc(
 	waitForAttachTimeout time.Duration,
 	volumeToMount VolumeToMount,