update with feedback from PR - basically assume stable functional tes…

…t environment and don't modify pvc in 'get' function
rsevilla87 · Oct 22, 2018 · 9463104 · 9463104
1 parent 15f92ed
commit 9463104
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 65 deletions.
diff --git a/pkg/virtctl/imageupload/imageupload.go b/pkg/virtctl/imageupload/imageupload.go
@@ -116,7 +116,7 @@ func NewImageUploadCommand(clientConfig clientcmd.ClientConfig) *cobra.Command {
 
 func usage() string {
 	usage := `  # Upload a local disk image to a newly created PersistentVolumeClaim:
-	virtctl image-upload --insecure --upload-proxy-url=https://cdi-uploadproxy.mycluster.com --pvc-name=upload-pvc --pvc-size=10Gi --image-path=/images/fedora28.qcow2`
+	virtctl image-upload --upload-proxy-url=https://cdi-uploadproxy.mycluster.com --pvc-name=upload-pvc --pvc-size=10Gi --image-path=/images/fedora28.qcow2`
 	return usage
 }
 
@@ -141,18 +141,24 @@ func (c *command) run(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("Cannot obtain KubeVirt client: %v", err)
 	}
 
-	pvc, err := getUploadPVC(virtClient, namespace, pvcName, noCreate)
+	pvc, err := getAndValidateUploadPVC(virtClient, namespace, pvcName, noCreate)
 	if err != nil {
-		return err
-	}
+		if !(k8serrors.IsNotFound(err) && !noCreate) {
+			return err
+		}
 
-	if pvc == nil {
 		pvc, err = createUploadPVC(virtClient, namespace, pvcName, pvcSize, storageClass, accessMode)
 		if err != nil {
 			return err
 		}
+
 		fmt.Printf("PVC %s/%s created\n", namespace, pvc.Name)
 	} else {
+		pvc, err = ensurePVCSupportsUpload(virtClient, pvc)
+		if err != nil {
+			return err
+		}
+
 		fmt.Printf("Using existing PVC %s/%s\n", namespace, pvc.Name)
 	}
 
@@ -322,33 +328,41 @@ func createUploadPVC(client kubernetes.Interface, namespace, name, size, storage
 	return pvc, nil
 }
 
-func getUploadPVC(client kubernetes.Interface, namespace, name string, shouldExist bool) (*v1.PersistentVolumeClaim, error) {
+func ensurePVCSupportsUpload(client kubernetes.Interface, pvc *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) {
+	var err error
+	_, hasAnnotation := pvc.Annotations[uploadRequestAnnotation]
+
+	if !hasAnnotation {
+		pvc.Annotations[uploadRequestAnnotation] = ""
+		pvc, err = client.CoreV1().PersistentVolumeClaims(pvc.Namespace).Update(pvc)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	return pvc, nil
+}
+
+func getAndValidateUploadPVC(client kubernetes.Interface, namespace, name string, shouldExist bool) (*v1.PersistentVolumeClaim, error) {
 	pvc, err := client.CoreV1().PersistentVolumeClaims(namespace).Get(pvcName, metav1.GetOptions{})
 	if err != nil {
-		if k8serrors.IsNotFound(err) && !shouldExist {
-			return nil, nil
-		}
 		return nil, err
 	}
 
+	// for PVCs that exist, we ony want to use them if
+	// 1. They have not already been used AND EITHER
+	//   a. shouldExist is true
+	//   b. shouldExist is false AND the upload annotation exists
+
 	_, isUploadPVC := pvc.Annotations[uploadRequestAnnotation]
 	podPhase, _ := pvc.Annotations[PodPhaseAnnotation]
 
 	if podPhase == string(v1.PodSucceeded) {
 		return nil, fmt.Errorf("PVC %s already successfully imported/cloned/updated", name)
 	}
 
-	if !isUploadPVC {
-		if shouldExist {
-			// add the annotation for upload controller
-			pvc.Annotations[uploadRequestAnnotation] = ""
-			pvc, err = client.CoreV1().PersistentVolumeClaims(namespace).Update(pvc)
-			if err != nil {
-				return nil, err
-			}
-		} else {
-			return nil, fmt.Errorf("PVC %s not available for upload", name)
-		}
+	if !shouldExist && !isUploadPVC {
+		return nil, fmt.Errorf("PVC %s not available for upload", name)
 	}
 
 	return pvc, nil

diff --git a/tests/imageupload_test.go b/tests/imageupload_test.go
@@ -10,13 +10,11 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"kubevirt.io/kubevirt/pkg/kubecli"
-	"kubevirt.io/kubevirt/pkg/virtctl/imageupload"
 	"kubevirt.io/kubevirt/tests"
 )
 
 const (
 	uploadProxyService   = "svc/cdi-uploadproxy"
-	uploadProxyNamespace = "kube-system"
 	uploadProxyPort      = 443
 	localUploadProxyPort = 18443
 	imagePath            = "./vendor/kubevirt.io/containerized-data-importer/tests/images/cirros-qcow2.img"
@@ -33,22 +31,11 @@ var _ = Describe("ImageUpload", func() {
 	virtClient, err := kubecli.GetKubevirtClient()
 	tests.PanicOnError(err)
 
-	podPending := func() (bool, error) {
-		pvc, err := virtClient.CoreV1().PersistentVolumeClaims(namespace).Get(pvcName, metav1.GetOptions{})
-		if err != nil {
-			return false, err
-		}
-
-		phase, _ := pvc.Annotations[imageupload.PodPhaseAnnotation]
-		fmt.Printf("Pod phase is %s\n", phase)
-		return (phase == "Pending"), nil
-	}
-
 	Context("Upload an image and start a VMI", func() {
 		It("Should succeed", func() {
 			By("Setting up port forwarding")
 			portMapping := fmt.Sprintf("%d:%d", localUploadProxyPort, uploadProxyPort)
-			_, kubectlCmd, err := tests.CreateCommandWithNS(uploadProxyNamespace, "kubectl", "port-forward", uploadProxyService, portMapping)
+			_, kubectlCmd, err := tests.CreateCommandWithNS(tests.KubeVirtInstallNamespace, "kubectl", "port-forward", uploadProxyService, portMapping)
 			Expect(err).ToNot(HaveOccurred())
 
 			err = kubectlCmd.Start()
@@ -61,37 +48,20 @@ var _ = Describe("ImageUpload", func() {
 				kubectlCmd.Wait()
 			}()
 
-			triedOnce := false
-
-			for {
-				By(fmt.Sprintf("Upload image final attempt: %t", triedOnce))
-
-				virtctlCmd := tests.NewRepeatableVirtctlCommand("image-upload",
-					"--namespace", namespace,
-					"--image-path", imagePath,
-					"--pvc-name", pvcName,
-					"--pvc-size", pvcSize,
-					"--uploadproxy-url", fmt.Sprintf("https://127.0.0.1:%d", localUploadProxyPort),
-					"--wait-secs", "20",
-					"--insecure")
-				err = virtctlCmd()
-				if err != nil {
-					fmt.Printf("Error: %+v\n", err)
-					if !triedOnce {
-						pending, err := podPending()
-						Expect(err).ToNot(HaveOccurred())
-						Expect(pending).To(BeTrue())
-
-						fmt.Printf("Retrying because of weird Kubernetes/KubeVirtCI issue.  Sometimes uploadserver pods get stuck in Pending waiting for PVC\n")
-
-						pvcName += "-take-two"
-						triedOnce = true
-						continue
-					}
-					Expect(err).ToNot(HaveOccurred())
-				}
-
-				break
+			By("Upload image")
+
+			virtctlCmd := tests.NewRepeatableVirtctlCommand("image-upload",
+				"--namespace", namespace,
+				"--image-path", imagePath,
+				"--pvc-name", pvcName,
+				"--pvc-size", pvcSize,
+				"--uploadproxy-url", fmt.Sprintf("https://127.0.0.1:%d", localUploadProxyPort),
+				"--wait-secs", "30",
+				"--insecure")
+			err = virtctlCmd()
+			if err != nil {
+				fmt.Printf("UploadImage Error: %+v\n", err)
+				Expect(err).ToNot(HaveOccurred())
 			}
 
 			By("Start VM")