Merge pull request #28811 from xiang90/pv

Automatic merge from submit-queue

controller/volume: simplify sync logic in syncUnboundClaim

Remove all unnecessary branching logic. No actual logic changes. Code is more readable now.

pkg/controller/volume/persistentvolume/controller.go

@@ -150,9 +150,8 @@ func (ctrl *PersistentVolumeController) syncClaim(claim *api.PersistentVolumeCla
 
 	if !hasAnnotation(claim.ObjectMeta, annBindCompleted) {
 		return ctrl.syncUnboundClaim(claim)
-	} else {
-		return ctrl.syncBoundClaim(claim)
 	}
+	return ctrl.syncBoundClaim(claim)
 }
 
 // syncUnboundClaim is the main controller method to decide what to do with an
@@ -168,35 +167,30 @@ func (ctrl *PersistentVolumeController) syncUnboundClaim(claim *api.PersistentVo
 			glog.V(2).Infof("synchronizing unbound PersistentVolumeClaim[%s]: Error finding PV for claim: %v", claimToClaimKey(claim), err)
 			return fmt.Errorf("Error finding PV for claim %q: %v", claimToClaimKey(claim), err)
 		}
-		if volume == nil {
+
+		if volume != nil {
+			// Found a PV for this claim
+			// OBSERVATION: pvc is "Pending", pv is "Available"
+			glog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: volume %q found: %s", claimToClaimKey(claim), volume.Name, getVolumeStatusForLogging(volume))
+
+			// On any error saving the volume or the claim, subsequent
+			// syncClaim will finish the binding.
+			// OBSERVATION: claim is "Bound", pv is "Bound" when no error
+			return ctrl.bind(volume, claim)
+		}
+
 		glog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: no volume found", claimToClaimKey(claim))
 		// No PV could be found
 		// OBSERVATION: pvc is "Pending", will retry
 		if hasAnnotation(claim.ObjectMeta, annClass) {
-				if err = ctrl.provisionClaim(claim); err != nil {
+			return ctrl.provisionClaim(claim)
+		}
+		// Mark the claim as Pending and try to find a match in the next periodic syncClaim
+		_, err = ctrl.updateClaimPhase(claim, api.ClaimPending)
 		return err
 	}
-				return nil
+
-			}
+	/* pvc.Spec.VolumeName != nil */
-			// Mark the claim as Pending and try to find a match in the next
-			// periodic syncClaim
-			if _, err = ctrl.updateClaimPhase(claim, api.ClaimPending); err != nil {
-				return err
-			}
-			return nil
-		} else /* pv != nil */ {
-			// Found a PV for this claim
-			// OBSERVATION: pvc is "Pending", pv is "Available"
-			glog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: volume %q found: %s", claimToClaimKey(claim), volume.Name, getVolumeStatusForLogging(volume))
-			if err = ctrl.bind(volume, claim); err != nil {
-				// On any error saving the volume or the claim, subsequent
-				// syncClaim will finish the binding.
-				return err
-			}
-			// OBSERVATION: claim is "Bound", pv is "Bound"
-			return nil
-		}
-	} else /* pvc.Spec.VolumeName != nil */ {
 	// [Unit test set 2]
 	// User asked for a specific PV.
 	glog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: volume %q requested", claimToClaimKey(claim), claim.Spec.VolumeName)
@@ -209,58 +203,51 @@ func (ctrl *PersistentVolumeController) syncUnboundClaim(claim *api.PersistentVo
 		// OBSERVATION: pvc is "Pending"
 		// Retry later.
 		glog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: volume %q requested and not found, will try again next time", claimToClaimKey(claim), claim.Spec.VolumeName)
-			if _, err = ctrl.updateClaimPhase(claim, api.ClaimPending); err != nil {
+		_, err = ctrl.updateClaimPhase(claim, api.ClaimPending)
 		return err
 	}
-			return nil
+
-		} else {
 	volume, ok := obj.(*api.PersistentVolume)
 	if !ok {
 		return fmt.Errorf("Cannot convert object from volume cache to volume %q!?: %+v", claim.Spec.VolumeName, obj)
 	}
 	glog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: volume %q requested and found: %s", claimToClaimKey(claim), claim.Spec.VolumeName, getVolumeStatusForLogging(volume))
+
 	if volume.Spec.ClaimRef == nil {
 		// User asked for a PV that is not claimed
 		// OBSERVATION: pvc is "Pending", pv is "Available"
 		glog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: volume is unbound, binding", claimToClaimKey(claim))
-				if err = ctrl.bind(volume, claim); err != nil {
+
 		// On any error saving the volume or the claim, subsequent
 		// syncClaim will finish the binding.
-					return err
+		// OBSERVATION: pvc is "Bound", pv is "Bound" when no error
+		return ctrl.bind(volume, claim)
 	}
-				// OBSERVATION: pvc is "Bound", pv is "Bound"
+
-				return nil
+	if isVolumeBoundToClaim(volume, claim) {
-			} else if isVolumeBoundToClaim(volume, claim) {
 		// User asked for a PV that is claimed by this PVC
 		// OBSERVATION: pvc is "Pending", pv is "Bound"
 		glog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: volume already bound, finishing the binding", claimToClaimKey(claim))
 
 		// Finish the volume binding by adding claim UID.
-				if err = ctrl.bind(volume, claim); err != nil {
+		// OBSERVATION: pvc is "Bound", pv is "Bound" when no error
-					return err
+		return ctrl.bind(volume, claim)
 	}
-				// OBSERVATION: pvc is "Bound", pv is "Bound"
+
-				return nil
-			} else {
 	// User asked for a PV that is claimed by someone else
 	// OBSERVATION: pvc is "Pending", pv is "Bound"
 	if !hasAnnotation(claim.ObjectMeta, annBoundByController) {
 		glog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: volume already bound to different claim by user, will retry later", claimToClaimKey(claim))
 		// User asked for a specific PV, retry later
-					if _, err = ctrl.updateClaimPhase(claim, api.ClaimPending); err != nil {
+		_, err = ctrl.updateClaimPhase(claim, api.ClaimPending)
 		return err
 	}
-					return nil
+
-				} else {
 	// This should never happen because someone had to remove
 	// annBindCompleted annotation on the claim.
 	glog.V(4).Infof("synchronizing unbound PersistentVolumeClaim[%s]: volume already bound to different claim %q by controller, THIS SHOULD NEVER HAPPEN", claimToClaimKey(claim), claimrefToClaimKey(volume.Spec.ClaimRef))
 	return fmt.Errorf("Invalid binding of claim %q to volume %q: volume already claimed by %q", claimToClaimKey(claim), claim.Spec.VolumeName, claimrefToClaimKey(volume.Spec.ClaimRef))
 }
-			}
-		}
-	}
-}
 
 // syncBoundClaim is the main controller method to decide what to do with a
 // bound claim.