From ab8b9c025e40b43272a433c600c107cb993ebf90 Mon Sep 17 00:00:00 2001 From: Bartosz Rybacki Date: Fri, 29 May 2020 19:55:32 +0200 Subject: [PATCH] Generating label names (#1200) * Handle labels length correctly Signed-off-by: Bartosz Rybacki * Handle service name generation correctly Signed-off-by: Bartosz Rybacki * Remove not needed labels Signed-off-by: Bartosz Rybacki * Store import pod name in annotation Signed-off-by: Bartosz Rybacki * Enable long DV name Signed-off-by: Bartosz Rybacki * Handle name with dot when creating service/label name Signed-off-by: Bartosz Rybacki * Test long names on import, upload and clone Signed-off-by: Bartosz Rybacki * Store upload pod name in annotation Signed-off-by: Bartosz Rybacki * Store importer scratch pvc name in annotation Signed-off-by: Bartosz Rybacki * Quick fix for tests (need improvements) Signed-off-by: Bartosz Rybacki * Cleanup handling scratch name Signed-off-by: Bartosz Rybacki * Ensure pod/service name conflicts are handled Signed-off-by: Bartosz Rybacki * Handle client errors when trying to get the import pod Signed-off-by: Bartosz Rybacki * Style improvements, and other code review fixes. Signed-off-by: Bartosz Rybacki * Store clone source pod name in an annotation Signed-off-by: Bartosz Rybacki * Correct name initialization and tests Signed-off-by: Bartosz Rybacki * Do not init name if pod already exists. It is not needed. The situation of having a pod but not name on annotation can happen after the upgrade, when we have a legacy pvc and pod already existing, but clone operation not finished. But when we already have the pod, then in the code (currently) we do not need the name from annotation. Signed-off-by: Bartosz Rybacki * Cleanup scratch name handling Signed-off-by: Bartosz Rybacki * Use constant for max dv name in validation Signed-off-by: Bartosz Rybacki * Simplify clone source pod name initialization Signed-off-by: Bartosz Rybacki --- pkg/apiserver/webhooks/BUILD.bazel | 1 + pkg/apiserver/webhooks/datavolume-validate.go | 6 +- .../webhooks/datavolume-validate_test.go | 7 +- pkg/controller/BUILD.bazel | 1 + pkg/controller/clone-controller.go | 28 ++- pkg/controller/clone-controller_test.go | 68 ++++++-- pkg/controller/datavolume-controller.go | 6 +- pkg/controller/import-controller.go | 74 ++++++-- pkg/controller/import-controller_test.go | 22 ++- pkg/controller/upload-controller.go | 165 ++++++++++++------ pkg/controller/upload-controller_test.go | 151 ++++++++++++---- pkg/controller/util.go | 19 +- pkg/util/naming/namer.go | 15 +- pkg/util/naming/namer_test.go | 10 +- tests/cloner_test.go | 113 +++++++++++- tests/import_test.go | 90 ++++++++++ tests/upload_test.go | 135 +++++++++++++- tests/utils/BUILD.bazel | 1 + tests/utils/pod.go | 18 +- tests/utils/upload.go | 3 +- 20 files changed, 774 insertions(+), 159 deletions(-) diff --git a/pkg/apiserver/webhooks/BUILD.bazel b/pkg/apiserver/webhooks/BUILD.bazel index 9112a3e5d9..8b8a2df070 100644 --- a/pkg/apiserver/webhooks/BUILD.bazel +++ b/pkg/apiserver/webhooks/BUILD.bazel @@ -27,6 +27,7 @@ go_library( "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library", "//vendor/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/validation:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library", "//vendor/k8s.io/client-go/kubernetes:go_default_library", "//vendor/k8s.io/klog:go_default_library", diff --git a/pkg/apiserver/webhooks/datavolume-validate.go b/pkg/apiserver/webhooks/datavolume-validate.go index 2a7122fdf0..db12704b35 100644 --- a/pkg/apiserver/webhooks/datavolume-validate.go +++ b/pkg/apiserver/webhooks/datavolume-validate.go @@ -29,6 +29,7 @@ import ( v1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kvalidation "k8s.io/apimachinery/pkg/util/validation" k8sfield "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/client-go/kubernetes" "k8s.io/klog" @@ -57,11 +58,10 @@ func validateSourceURL(sourceURL string) string { func validateDataVolumeName(name string) []metav1.StatusCause { var causes []metav1.StatusCause - // name of data volume cannot be more than 55 characters (not including '-scratch') - if len(name) > 55 { + if len(name) > kvalidation.DNS1123SubdomainMaxLength { causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueInvalid, - Message: fmt.Sprintf("Name of data volume cannot be more than 55 characters"), + Message: fmt.Sprintf("Name of data volume cannot be more than %d characters", kvalidation.DNS1123SubdomainMaxLength), Field: "", }) } diff --git a/pkg/apiserver/webhooks/datavolume-validate_test.go b/pkg/apiserver/webhooks/datavolume-validate_test.go index 2e1e36202a..35725e9cf8 100644 --- a/pkg/apiserver/webhooks/datavolume-validate_test.go +++ b/pkg/apiserver/webhooks/datavolume-validate_test.go @@ -114,9 +114,12 @@ var _ = Describe("Validating Webhook", func() { Expect(resp.Allowed).To(Equal(false)) }) - It("should reject DataVolume with name length greater than 55 characters", func() { + It("should reject DataVolume with name length greater than 253 characters", func() { + longName := "the-name-length-of-this-datavolume-is-greater-then-253-characters" + + "123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-" + + "123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789" dataVolume := newHTTPDataVolume( - "the-name-length-of-this-datavolume-is-greater-then-55cha", + longName, "http://www.example.com") resp := validateDataVolumeCreate(dataVolume) Expect(resp.Allowed).To(Equal(false)) diff --git a/pkg/controller/BUILD.bazel b/pkg/controller/BUILD.bazel index 17fd0c5cfe..3ed7fc6298 100644 --- a/pkg/controller/BUILD.bazel +++ b/pkg/controller/BUILD.bazel @@ -80,6 +80,7 @@ go_test( "//pkg/util/cert:go_default_library", "//pkg/util/cert/fetcher:go_default_library", "//pkg/util/cert/triple:go_default_library", + "//pkg/util/naming:go_default_library", "//tests/reporters:go_default_library", "//vendor/github.com/kubernetes-csi/external-snapshotter/v2/pkg/apis/volumesnapshot/v1beta1:go_default_library", "//vendor/github.com/onsi/ginkgo:go_default_library", diff --git a/pkg/controller/clone-controller.go b/pkg/controller/clone-controller.go index d058429b96..79eb746bf1 100644 --- a/pkg/controller/clone-controller.go +++ b/pkg/controller/clone-controller.go @@ -43,6 +43,8 @@ const ( //CloneUniqueID is used as a special label to be used when we search for the pod CloneUniqueID = "cdi.kubevirt.io/storage.clone.cloneUniqeId" + // AnnCloneSourcePod name of the source clone pod + AnnCloneSourcePod = "cdi.kubevirt.io/storage.sourceClonePodName" // ErrIncompatiblePVC provides a const to indicate a clone is not possible due to an incompatible PVC ErrIncompatiblePVC = "ErrIncompatiblePVC" @@ -172,6 +174,15 @@ func (r *CloneReconciler) Reconcile(req reconcile.Request) (reconcile.Result, er return reconcile.Result{}, err } + _, nameExists := pvc.Annotations[AnnCloneSourcePod] + if !nameExists && sourcePod == nil { + pvc.Annotations[AnnCloneSourcePod] = createCloneSourcePodName(pvc) + if err := r.updatePVC(pvc); err != nil { + return reconcile.Result{}, err + } + return reconcile.Result{Requeue: true}, nil + } + if err := r.reconcileSourcePod(sourcePod, pvc, log); err != nil { return reconcile.Result{}, err } @@ -264,10 +275,15 @@ func (r *CloneReconciler) findCloneSourcePod(pvc *corev1.PersistentVolumeClaim) if !isCloneRequest { return nil, nil } + cloneSourcePodName, exists := pvc.Annotations[AnnCloneSourcePod] + if !exists { + // fallback to legacy name, to find any pod that still might be running after upgrade + cloneSourcePodName = createCloneSourcePodName(pvc) + } selector, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{ MatchLabels: map[string]string{ - CloneUniqueID: string(pvc.GetUID()) + "-source-pod", + CloneUniqueID: cloneSourcePodName, }, }) if err != nil { @@ -418,8 +434,8 @@ func (r *CloneReconciler) CreateCloneSourcePod(image, pullPolicy, clientName str return pod, nil } -func getCloneSourcePodName(targetPvc *corev1.PersistentVolumeClaim) string { - return string(targetPvc.GetUID()) + "-source-pod" +func createCloneSourcePodName(targetPvc *corev1.PersistentVolumeClaim) string { + return string(targetPvc.GetUID()) + common.ClonerSourcePodNameSuffix } // MakeCloneSourcePodSpec creates and returns the clone source pod spec based on the target pvc. @@ -427,7 +443,7 @@ func MakeCloneSourcePodSpec(image, pullPolicy, sourcePvcName, sourcePvcNamespace clientKey, clientCert, serverCACert []byte, targetPvc *corev1.PersistentVolumeClaim, resourceRequirements *corev1.ResourceRequirements) *corev1.Pod { var ownerID string - podName := getCloneSourcePodName(targetPvc) + cloneSourcePodName, _ := targetPvc.Annotations[AnnCloneSourcePod] url := GetUploadServerURL(targetPvc.Namespace, targetPvc.Name, common.UploadPathSync) pvcOwner := metav1.GetControllerOf(targetPvc) if pvcOwner != nil && pvcOwner.Kind == "DataVolume" { @@ -436,7 +452,7 @@ func MakeCloneSourcePodSpec(image, pullPolicy, sourcePvcName, sourcePvcNamespace pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: podName, + Name: cloneSourcePodName, Namespace: sourcePvcNamespace, Annotations: map[string]string{ AnnCreatedBy: "yes", @@ -446,7 +462,7 @@ func MakeCloneSourcePodSpec(image, pullPolicy, sourcePvcName, sourcePvcNamespace common.CDILabelKey: common.CDILabelValue, //filtered by the podInformer common.CDIComponentLabel: common.ClonerSourcePodName, // this label is used when searching for a pvc's cloner source pod. - CloneUniqueID: getCloneSourcePodName(targetPvc), + CloneUniqueID: cloneSourcePodName, common.PrometheusLabel: "", }, }, diff --git a/pkg/controller/clone-controller_test.go b/pkg/controller/clone-controller_test.go index 1929eb5b00..6897a2724c 100644 --- a/pkg/controller/clone-controller_test.go +++ b/pkg/controller/clone-controller_test.go @@ -122,9 +122,40 @@ var _ = Describe("Clone controller reconcile loop", func() { Expect(err.Error()).To(ContainSubstring(fmt.Sprintf("error parsing %s annotation", AnnPodReady))) }) + It("Should create source pod name", func() { + testPvc := createPvc("testPvc1", "default", map[string]string{ + AnnCloneRequest: "default/source", + AnnPodReady: "true", + AnnCloneToken: "foobaz", + AnnUploadClientName: "uploadclient"}, nil) + reconciler = createCloneReconciler(testPvc, createPvc("source", "default", map[string]string{}, nil)) + By("Setting up the match token") + reconciler.tokenValidator.(*FakeValidator).match = "foobaz" + reconciler.tokenValidator.(*FakeValidator).Name = "source" + reconciler.tokenValidator.(*FakeValidator).Namespace = "default" + reconciler.tokenValidator.(*FakeValidator).Params["targetNamespace"] = "default" + reconciler.tokenValidator.(*FakeValidator).Params["targetName"] = "testPvc1" + By("Verifying no source pod exists") + sourcePod, err := reconciler.findCloneSourcePod(testPvc) + Expect(sourcePod).To(BeNil()) + _, err = reconciler.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) + Expect(err).ToNot(HaveOccurred()) + By("Verifying no source pod exists") + sourcePod, err = reconciler.findCloneSourcePod(testPvc) + Expect(sourcePod).To(BeNil()) + By("Verifying the PVC now has a source pod name") + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "testPvc1", Namespace: "default"}, testPvc) + Expect(err).ToNot(HaveOccurred()) + Expect(testPvc.Annotations[AnnCloneSourcePod]).To(Equal("default-testPvc1-source-pod")) + }) + It("Should create new source pod if none exists, and target pod is marked ready", func() { testPvc := createPvc("testPvc1", "default", map[string]string{ - AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnUploadClientName: "uploadclient"}, nil) + AnnCloneRequest: "default/source", + AnnPodReady: "true", + AnnCloneToken: "foobaz", + AnnUploadClientName: "uploadclient", + AnnCloneSourcePod: "default-testPvc1-source-pod"}, nil) reconciler = createCloneReconciler(testPvc, createPvc("source", "default", map[string]string{}, nil)) By("Setting up the match token") reconciler.tokenValidator.(*FakeValidator).match = "foobaz" @@ -140,6 +171,7 @@ var _ = Describe("Clone controller reconcile loop", func() { By("Verifying source pod exists") sourcePod, err = reconciler.findCloneSourcePod(testPvc) Expect(err).ToNot(HaveOccurred()) + Expect(sourcePod).ToNot(BeNil()) Expect(sourcePod.GetLabels()[CloneUniqueID]).To(Equal("default-testPvc1-source-pod")) By("Verifying the PVC now has a finalizer") err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "testPvc1", Namespace: "default"}, testPvc) @@ -149,7 +181,7 @@ var _ = Describe("Clone controller reconcile loop", func() { It("Should error with missing upload client name annotation if none provided", func() { testPvc := createPvc("testPvc1", "default", map[string]string{ - AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz"}, nil) + AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnCloneSourcePod: "default-testPvc1-source-pod"}, nil) reconciler = createCloneReconciler(testPvc, createPvc("source", "default", map[string]string{}, nil)) By("Setting up the match token") reconciler.tokenValidator.(*FakeValidator).match = "foobaz" @@ -167,7 +199,7 @@ var _ = Describe("Clone controller reconcile loop", func() { It("Should update the PVC from the pod status", func() { testPvc := createPvc("testPvc1", "default", map[string]string{ - AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnUploadClientName: "uploadclient"}, nil) + AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnUploadClientName: "uploadclient", AnnCloneSourcePod: "default-testPvc1-source-pod"}, nil) reconciler = createCloneReconciler(testPvc, createPvc("source", "default", map[string]string{}, nil)) By("Setting up the match token") reconciler.tokenValidator.(*FakeValidator).match = "foobaz" @@ -205,6 +237,12 @@ var _ = Describe("Clone controller reconcile loop", func() { Expect(sourcePod).To(BeNil()) _, err = reconciler.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) Expect(err).ToNot(HaveOccurred()) + By("Verifying the PVC now has a source pod name") + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "testPvc1", Namespace: "default"}, testPvc) + Expect(err).ToNot(HaveOccurred()) + Expect(testPvc.Annotations[AnnCloneSourcePod]).To(Equal("default-testPvc1-source-pod")) + _, err = reconciler.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) + Expect(err).ToNot(HaveOccurred()) By("Verifying source pod exists") sourcePod, err = reconciler.findCloneSourcePod(testPvc) Expect(err).ToNot(HaveOccurred()) @@ -215,7 +253,7 @@ var _ = Describe("Clone controller reconcile loop", func() { Expect(reconciler.hasFinalizer(testPvc, cloneSourcePodFinalizer)).To(BeTrue()) By("Updating the PVC to completed") testPvc = createPvc("testPvc1", "default", map[string]string{ - AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnUploadClientName: "uploadclient", AnnPodPhase: string(corev1.PodSucceeded)}, nil) + AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnUploadClientName: "uploadclient", AnnCloneSourcePod: "default-testPvc1-source-pod", AnnPodPhase: string(corev1.PodSucceeded)}, nil) reconciler.client.Update(context.TODO(), testPvc) _, err = reconciler.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) Expect(err).ToNot(HaveOccurred()) @@ -250,6 +288,12 @@ var _ = Describe("Clone controller reconcile loop", func() { Expect(sourcePod).To(BeNil()) _, err = reconciler.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) Expect(err).ToNot(HaveOccurred()) + By("Verifying the PVC now has a source pod name") + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "testPvc1", Namespace: "default"}, testPvc) + Expect(err).ToNot(HaveOccurred()) + Expect(testPvc.Annotations[AnnCloneSourcePod]).To(Equal("default-testPvc1-source-pod")) + _, err = reconciler.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) + Expect(err).ToNot(HaveOccurred()) By("Verifying source pod exists") sourcePod, err = reconciler.findCloneSourcePod(testPvc) Expect(err).ToNot(HaveOccurred()) @@ -260,7 +304,7 @@ var _ = Describe("Clone controller reconcile loop", func() { Expect(reconciler.hasFinalizer(testPvc, cloneSourcePodFinalizer)).To(BeTrue()) By("Updating the PVC to completed") testPvc = createPvc("testPvc1", "default", map[string]string{ - AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnUploadClientName: "uploadclient", AnnPodPhase: string(corev1.PodSucceeded)}, nil) + AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnUploadClientName: "uploadclient", AnnCloneSourcePod: "default-testPvc1-source-pod", AnnPodPhase: string(corev1.PodSucceeded)}, nil) reconciler.client.Update(context.TODO(), testPvc) _, err = reconciler.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) Expect(err).ToNot(HaveOccurred()) @@ -282,7 +326,7 @@ var _ = Describe("Clone controller reconcile loop", func() { It("Should error when source and target volume modes do not match (fs->block)", func() { testPvc := createBlockPvc("testPvc1", "default", map[string]string{ - AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnUploadClientName: "uploadclient"}, nil) + AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnUploadClientName: "uploadclient", AnnCloneSourcePod: "default-testPvc1-source-pod"}, nil) reconciler = createCloneReconciler(testPvc, createPvc("source", "default", map[string]string{}, nil)) By("Setting up the match token") reconciler.tokenValidator.(*FakeValidator).match = "foobaz" @@ -300,7 +344,7 @@ var _ = Describe("Clone controller reconcile loop", func() { It("Should error when source and target volume modes do not match (fs->block)", func() { testPvc := createPvc("testPvc1", "default", map[string]string{ - AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnUploadClientName: "uploadclient"}, nil) + AnnCloneRequest: "default/source", AnnPodReady: "true", AnnCloneToken: "foobaz", AnnUploadClientName: "uploadclient", AnnCloneSourcePod: "default-testPvc1-source-pod"}, nil) reconciler = createCloneReconciler(testPvc, createBlockPvc("source", "default", map[string]string{}, nil)) By("Setting up the match token") reconciler.tokenValidator.(*FakeValidator).match = "foobaz" @@ -354,18 +398,18 @@ var _ = Describe("CloneSourcePodName", func() { pvc1d2 := createPvc("testPvc1", "default2", map[string]string{AnnCloneRequest: "default/test"}, nil) pvc2d1 := createPvc("testPvc2", "default", map[string]string{AnnCloneRequest: "default/test"}, nil) pvcSimilar := createPvc("testP", "vc1default", map[string]string{AnnCloneRequest: "default/test"}, nil) - podName1d := getCloneSourcePodName(pvc1d) - podName1dagain := getCloneSourcePodName(pvc1d) + podName1d := createCloneSourcePodName(pvc1d) + podName1dagain := createCloneSourcePodName(pvc1d) By("Verifying rerunning getloneSourcePodName on same PVC I get same name") Expect(podName1d).To(Equal(podName1dagain)) By("Verifying different namespace but same name I get different pod name") - podName1d2 := getCloneSourcePodName(pvc1d2) + podName1d2 := createCloneSourcePodName(pvc1d2) Expect(podName1d).NotTo(Equal(podName1d2)) By("Verifying same namespace but different name I get different pod name") - podName2d1 := getCloneSourcePodName(pvc2d1) + podName2d1 := createCloneSourcePodName(pvc2d1) Expect(podName1d).NotTo(Equal(podName2d1)) By("Verifying concatenated ns/name of same characters I get different pod name") - podNameSimilar := getCloneSourcePodName(pvcSimilar) + podNameSimilar := createCloneSourcePodName(pvcSimilar) Expect(podName1d).NotTo(Equal(podNameSimilar)) }) }) diff --git a/pkg/controller/datavolume-controller.go b/pkg/controller/datavolume-controller.go index a83de9eeeb..49b2f51440 100644 --- a/pkg/controller/datavolume-controller.go +++ b/pkg/controller/datavolume-controller.go @@ -736,8 +736,9 @@ func (r *DatavolumeReconciler) getPodFromPvc(namespace string, pvcUID types.UID) } } + // TODO: check this val, exists := pod.Labels[CloneUniqueID] - if exists && val == string(pvcUID)+"-source-pod" { + if exists && val == string(pvcUID)+common.ClonerSourcePodNameSuffix { return &pod, nil } } @@ -830,8 +831,7 @@ func buildHTTPClient() *http.Client { // that 'owns' it. func newPersistentVolumeClaim(dataVolume *cdiv1.DataVolume) (*corev1.PersistentVolumeClaim, error) { labels := map[string]string{ - "cdi-controller": dataVolume.Name, - "app": "containerized-data-importer", + "app": "containerized-data-importer", } if dataVolume.Spec.PVC == nil { diff --git a/pkg/controller/import-controller.go b/pkg/controller/import-controller.go index 1ab2d476df..d52960e97a 100644 --- a/pkg/controller/import-controller.go +++ b/pkg/controller/import-controller.go @@ -181,11 +181,12 @@ func (r *ImportReconciler) Reconcile(req reconcile.Request) (reconcile.Result, e } func (r *ImportReconciler) findImporterPod(pvc *corev1.PersistentVolumeClaim, log logr.Logger) (*corev1.Pod, error) { - podName := importPodNameFromPvc(pvc) + podName := getImportPodNameFromPvc(pvc) pod := &corev1.Pod{} - err := r.client.Get(context.TODO(), types.NamespacedName{Name: podName, Namespace: pvc.GetNamespace()}, pod) - - if k8serrors.IsNotFound(err) { + if err := r.client.Get(context.TODO(), types.NamespacedName{Name: podName, Namespace: pvc.GetNamespace()}, pod); err != nil { + if !k8serrors.IsNotFound(err) { + return nil, errors.Wrapf(err, "error getting import pod %s/%s", pvc.Namespace, podName) + } return nil, nil } @@ -208,9 +209,16 @@ func (r *ImportReconciler) reconcilePvc(pvc *corev1.PersistentVolumeClaim, log l // Don't create the POD if the PVC is completed already log.V(1).Info("PVC is already complete") } else if pvc.DeletionTimestamp == nil { - // Create importer pod, make sure the PVC owns it. - if err := r.createImporterPod(pvc); err != nil { - return reconcile.Result{}, err + if _, ok := pvc.Annotations[AnnImportPod]; ok { + // Create importer pod, make sure the PVC owns it. + if err := r.createImporterPod(pvc); err != nil { + return reconcile.Result{}, err + } + } else { + // Create importer pod Name and store in PVC? + if err := r.initPvcPodName(pvc, log); err != nil { + return reconcile.Result{}, err + } } } } else { @@ -230,6 +238,28 @@ func (r *ImportReconciler) reconcilePvc(pvc *corev1.PersistentVolumeClaim, log l return reconcile.Result{}, nil } +func (r *ImportReconciler) initPvcPodName(pvc *corev1.PersistentVolumeClaim, log logr.Logger) error { + currentPvcCopy := pvc.DeepCopyObject() + + log.V(1).Info("Init pod name on PVC") + anno := pvc.GetAnnotations() + + anno[AnnImportPod] = createImportPodNameFromPvc(pvc) + + requiresScratch := r.requiresScratchSpace(pvc) + if requiresScratch { + anno[AnnRequiresScratch] = "true" + } + + if !reflect.DeepEqual(currentPvcCopy, pvc) { + if err := r.updatePVC(pvc, log); err != nil { + return err + } + log.V(1).Info("Updated PVC", "pvc.anno.AnnImportPod", anno[AnnImportPod]) + } + return nil +} + func (r *ImportReconciler) updatePvcFromPod(pvc *corev1.PersistentVolumeClaim, pod *corev1.Pod, log logr.Logger) error { // Keep a copy of the original for comparison later. currentPvcCopy := pvc.DeepCopyObject() @@ -316,7 +346,7 @@ func (r *ImportReconciler) createImporterPod(pvc *corev1.PersistentVolumeClaim) requiresScratch := r.requiresScratchSpace(pvc) if requiresScratch { - name := scratchNameFromPvc(pvc) + name := createScratchNameFromPvc(pvc) scratchPvcName = &name } @@ -333,9 +363,10 @@ func (r *ImportReconciler) createImporterPod(pvc *corev1.PersistentVolumeClaim) } r.log.V(1).Info("Created POD", "pod.Name", pod.Name) if requiresScratch { - r.log.V(1).Info("POD requires scratch space") + r.log.V(1).Info("Pod requires scratch space") return r.createScratchPvcForPod(pvc, pod) } + return nil } @@ -476,13 +507,16 @@ func (r *ImportReconciler) requiresScratchSpace(pvc *corev1.PersistentVolumeClai func (r *ImportReconciler) createScratchPvcForPod(pvc *corev1.PersistentVolumeClaim, pod *corev1.Pod) error { scratchPvc := &corev1.PersistentVolumeClaim{} + scratchPVCName, exists := getScratchNameFromPod(pod) + if !exists { + return errors.New("Scratch Volume not configured for pod") + } anno := pvc.GetAnnotations() - err := r.client.Get(context.TODO(), types.NamespacedName{Namespace: pvc.GetNamespace(), Name: scratchNameFromPvc(pvc)}, scratchPvc) + err := r.client.Get(context.TODO(), types.NamespacedName{Namespace: pvc.GetNamespace(), Name: scratchPVCName}, scratchPvc) if IgnoreNotFound(err) != nil { return err } if k8serrors.IsNotFound(err) { - scratchPVCName := scratchNameFromPvc(pvc) storageClassName := GetScratchPvcStorageClass(r.client, pvc) // Scratch PVC doesn't exist yet, create it. Determine which storage class to use. _, err = CreateScratchPersistentVolumeClaim(r.client, pvc, pod, scratchPVCName, storageClassName) @@ -553,12 +587,18 @@ func getDiskID(pvc *corev1.PersistentVolumeClaim) string { return diskID } -func importPodNameFromPvc(pvc *corev1.PersistentVolumeClaim) string { +func getImportPodNameFromPvc(pvc *corev1.PersistentVolumeClaim) string { + podName, ok := pvc.Annotations[AnnImportPod] + if ok { + return podName + } + // fallback to legacy naming, in fact the following function is fully compatible with legacy + // name concatenation "importer-{pvc.Name}" if the name length is under the size limits, return naming.GetResourceName(common.ImporterPodName, pvc.Name) } -func scratchNameFromPvc(pvc *corev1.PersistentVolumeClaim) string { - return naming.GetResourceName(pvc.Name, common.ScratchNameSuffix) +func createImportPodNameFromPvc(pvc *corev1.PersistentVolumeClaim) string { + return naming.GetResourceName(common.ImporterPodName, pvc.Name) } // createImporterPod creates and returns a pointer to a pod which is created based on the passed-in endpoint, secret @@ -582,7 +622,7 @@ func createImporterPod(log logr.Logger, client client.Client, image, verbose, pu // makeImporterPodSpec creates and return the importer pod spec based on the passed-in endpoint, secret and pvc. func makeImporterPodSpec(namespace, image, verbose, pullPolicy string, podEnvVar *importPodEnvVar, pvc *corev1.PersistentVolumeClaim, scratchPvcName *string, podResourceRequirements *corev1.ResourceRequirements) *corev1.Pod { // importer pod name contains the pvc name - podName := importPodNameFromPvc(pvc) + podName, _ := pvc.Annotations[AnnImportPod] blockOwnerDeletion := true isController := true @@ -625,9 +665,7 @@ func makeImporterPodSpec(namespace, image, verbose, pullPolicy string, podEnvVar Labels: map[string]string{ common.CDILabelKey: common.CDILabelValue, common.CDIComponentLabel: common.ImporterPodName, - // this label is used when searching for a pvc's import pod. - LabelImportPvc: pvc.Name, - common.PrometheusLabel: "", + common.PrometheusLabel: "", }, OwnerReferences: []metav1.OwnerReference{ { diff --git a/pkg/controller/import-controller_test.go b/pkg/controller/import-controller_test.go index ba0d56de1c..8e05b72628 100644 --- a/pkg/controller/import-controller_test.go +++ b/pkg/controller/import-controller_test.go @@ -141,10 +141,20 @@ var _ = Describe("ImportConfig Controller reconcile loop", func() { Expect(reflect.DeepEqual(orgPvc, resPvc)).To(BeTrue()) }) - It("Should create a POD if a PVC with all needed annotations is passed", func() { + It("Should init PVC with a POD name if a PVC with all needed annotations is passed", func() { reconciler = createImportReconciler(createPvc("testPvc1", "default", map[string]string{AnnEndpoint: testEndPoint}, nil)) _, err := reconciler.Reconcile(reconcile.Request{}) Expect(err).ToNot(HaveOccurred()) + resultPvc := &corev1.PersistentVolumeClaim{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "testPvc1", Namespace: "default"}, resultPvc) + Expect(err).ToNot(HaveOccurred()) + Expect(resultPvc.GetAnnotations()[AnnImportPod]).ToNot(BeEmpty()) + }) + + It("Should create a POD if a PVC with all needed annotations is passed", func() { + reconciler = createImportReconciler(createPvc("testPvc1", "default", map[string]string{AnnEndpoint: testEndPoint, AnnImportPod: "importer-testPvc1"}, nil)) + _, err := reconciler.Reconcile(reconcile.Request{}) + Expect(err).ToNot(HaveOccurred()) pod := &corev1.Pod{} err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "importer-testPvc1", Namespace: "default"}, pod) Expect(err).ToNot(HaveOccurred()) @@ -161,7 +171,7 @@ var _ = Describe("ImportConfig Controller reconcile loop", func() { }) It("Should create a POD if a PVC with all needed annotations is passed, but not set fsgroup if not kubevirt contenttype", func() { - reconciler = createImportReconciler(createPvc("testPvc1", "default", map[string]string{AnnEndpoint: testEndPoint, AnnContentType: string(cdiv1.DataVolumeArchive)}, nil)) + reconciler = createImportReconciler(createPvc("testPvc1", "default", map[string]string{AnnEndpoint: testEndPoint, AnnImportPod: "importer-testPvc1", AnnContentType: string(cdiv1.DataVolumeArchive)}, nil)) _, err := reconciler.Reconcile(reconcile.Request{}) Expect(err).ToNot(HaveOccurred()) pod := &corev1.Pod{} @@ -285,8 +295,10 @@ var _ = Describe("Update PVC from POD", func() { }) It("Should create scratch PVC, if pod is pending and PVC is marked with scratch", func() { + scratchPvcName := &corev1.PersistentVolumeClaim{} + scratchPvcName.Name = "testPvc1-scratch" pvc := createPvcInStorageClass("testPvc1", "default", &testStorageClass, map[string]string{AnnEndpoint: testEndPoint, AnnPodPhase: string(corev1.PodPending), AnnRequiresScratch: "true"}, nil) - pod := createImporterTestPod(pvc, "testPvc1", nil) + pod := createImporterTestPod(pvc, "testPvc1", scratchPvcName) pod.Status = corev1.PodStatus{ Phase: corev1.PodPending, ContainerStatuses: []v1.ContainerStatus{ @@ -368,7 +380,9 @@ var _ = Describe("Update PVC from POD", func() { It("Should update phase on PVC, if pod exited with error state that is scratchspace exit", func() { pvc := createPvcInStorageClass("testPvc1", "default", &testStorageClass, map[string]string{AnnEndpoint: testEndPoint, AnnPodPhase: string(corev1.PodRunning)}, nil) - pod := createImporterTestPod(pvc, "testPvc1", nil) + scratchPvcName := &corev1.PersistentVolumeClaim{} + scratchPvcName.Name = "testPvc1-scratch" + pod := createImporterTestPod(pvc, "testPvc1", scratchPvcName) pod.Status = corev1.PodStatus{ Phase: corev1.PodPending, ContainerStatuses: []corev1.ContainerStatus{ diff --git a/pkg/controller/upload-controller.go b/pkg/controller/upload-controller.go index 8aeba33888..c7f0a0bed3 100644 --- a/pkg/controller/upload-controller.go +++ b/pkg/controller/upload-controller.go @@ -53,6 +53,9 @@ const ( // AnnUploadClientName is the TLS name uploadserver will accept requests from AnnUploadClientName = "cdi.kubevirt.io/uploadClientName" + // AnnUploadPod name of the upload pod + AnnUploadPod = "cdi.kubevirt.io/storage.uploadPodName" + annCreatedByUpload = "cdi.kubevirt.io/storage.createdByUploadController" uploadServerClientName = "client.upload-server.cdi.kubevirt.io" @@ -110,8 +113,11 @@ func (r *UploadReconciler) Reconcile(req reconcile.Request) (reconcile.Result, e // force cleanup if PVC pending delete and pod running or the upload/clone annotation was removed if (!isUpload && !isCloneTarget) || podSucceededFromPVC(pvc) || pvc.DeletionTimestamp != nil { - log.V(1).Info("not doing anything with PVC", "isUpload", isUpload, "isCloneTarget", isCloneTarget, "podSucceededFromPVC", - podSucceededFromPVC(pvc), "deletionTimeStamp set?", pvc.DeletionTimestamp != nil) + log.V(1).Info("not doing anything with PVC", + "isUpload", isUpload, + "isCloneTarget", isCloneTarget, + "podSucceededFromPVC", podSucceededFromPVC(pvc), + "deletionTimeStamp set?", pvc.DeletionTimestamp != nil) if err := r.cleanup(pvc); err != nil { return reconcile.Result{}, err } @@ -123,7 +129,7 @@ func (r *UploadReconciler) Reconcile(req reconcile.Request) (reconcile.Result, e } func (r *UploadReconciler) reconcilePVC(log logr.Logger, pvc *corev1.PersistentVolumeClaim, isCloneTarget bool) (reconcile.Result, error) { - var uploadClientName, scratchPVCName string + var uploadClientName string pvcCopy := pvc.DeepCopy() anno := pvcCopy.Annotations @@ -142,18 +148,41 @@ func (r *UploadReconciler) reconcilePVC(log logr.Logger, pvc *corev1.PersistentV anno[AnnUploadClientName] = uploadClientName } else { uploadClientName = uploadServerClientName - - scratchPVCName = getScratchPvcName(pvc.Name) } - resourceName := getUploadResourceName(pvc.Name) - - pod, err := r.getOrCreateUploadPod(pvcCopy, resourceName, scratchPVCName, uploadClientName) + pod, err := r.findUploadPodForPvc(pvc, log) if err != nil { return reconcile.Result{}, err } - if _, err = r.getOrCreateUploadService(pvc, resourceName); err != nil { + if pod == nil { + podName, ok := pvc.Annotations[AnnUploadPod] + scratchPVCName := createScratchPvcNameFromPvc(pvc, isCloneTarget) + + if !ok { + podName = createUploadResourceName(pvc.Name) + if err := r.updatePvcPodName(pvc, podName, log); err != nil { + return reconcile.Result{}, err + } + return reconcile.Result{Requeue: true}, nil + } + pod, err = r.createUploadPodForPvc(pvc, podName, scratchPVCName, uploadClientName) + if err != nil { + return reconcile.Result{}, err + } + } + + // Always try to get or create the scratch PVC for a pod that is not successful yet, if it exists nothing happens otherwise attempt to create. + scratchPVCName, exists := getScratchNameFromPod(pod) + if exists { + _, err := r.getOrCreateScratchPvc(pvcCopy, pod, scratchPVCName) + if err != nil { + return reconcile.Result{}, err + } + } + + svcName := naming.GetServiceNameFromResourceName(pod.Name) + if _, err = r.getOrCreateUploadService(pvc, svcName); err != nil { return reconcile.Result{}, err } @@ -185,6 +214,22 @@ func (r *UploadReconciler) reconcilePVC(log logr.Logger, pvc *corev1.PersistentV return reconcile.Result{}, nil } +func (r *UploadReconciler) updatePvcPodName(pvc *v1.PersistentVolumeClaim, podName string, log logr.Logger) error { + currentPvcCopy := pvc.DeepCopyObject() + + log.V(1).Info("Updating PVC from pod") + anno := pvc.GetAnnotations() + anno[AnnUploadPod] = podName + + if !reflect.DeepEqual(currentPvcCopy, pvc) { + if err := r.updatePVC(pvc); err != nil { + return err + } + log.V(1).Info("Updated PVC", "pvc.anno.AnnImportPod", anno[AnnUploadPod]) + } + return nil +} + func (r *UploadReconciler) updatePVC(pvc *corev1.PersistentVolumeClaim) error { r.log.V(1).Info("Phase is now", "pvc.anno.Phase", pvc.GetAnnotations()[AnnPodPhase]) if err := r.client.Update(context.TODO(), pvc); err != nil { @@ -218,10 +263,11 @@ func (r *UploadReconciler) getCloneRequestSourcePVC(targetPvc *corev1.Persistent } func (r *UploadReconciler) cleanup(pvc *v1.PersistentVolumeClaim) error { - resourceName := getUploadResourceName(pvc.Name) + resourceName := getUploadResourceNameFromPvc(pvc) + svcName := naming.GetServiceNameFromResourceName(resourceName) // delete service - if err := r.deleteService(pvc.Namespace, resourceName); err != nil { + if err := r.deleteService(pvc.Namespace, svcName); err != nil { return err } @@ -240,51 +286,50 @@ func (r *UploadReconciler) cleanup(pvc *v1.PersistentVolumeClaim) error { } return nil } - -func (r *UploadReconciler) getOrCreateUploadPod(pvc *v1.PersistentVolumeClaim, podName, scratchPVCName, clientName string) (*v1.Pod, error) { +func (r *UploadReconciler) findUploadPodForPvc(pvc *v1.PersistentVolumeClaim, log logr.Logger) (*v1.Pod, error) { + podName := getUploadResourceNameFromPvc(pvc) pod := &corev1.Pod{} if err := r.client.Get(context.TODO(), types.NamespacedName{Name: podName, Namespace: pvc.Namespace}, pod); err != nil { if !k8serrors.IsNotFound(err) { return nil, errors.Wrapf(err, "error getting upload pod %s/%s", pvc.Namespace, podName) } + return nil, nil + } - serverCert, serverKey, err := r.serverCertGenerator.MakeServerCert(pvc.Namespace, podName, uploadServerCertDuration) - if err != nil { - return nil, err - } + if !metav1.IsControlledBy(pod, pvc) { + return nil, errors.Errorf("%s pod not controlled by pvc %s", podName, pvc.Name) + } - clientCA, err := r.clientCAFetcher.BundleBytes() - if err != nil { - return nil, err - } + return pod, nil +} - args := UploadPodArgs{ - Name: podName, - PVC: pvc, - ScratchPVCName: scratchPVCName, - ClientName: clientName, - ServerCert: serverCert, - ServerKey: serverKey, - ClientCA: clientCA, - } +func (r *UploadReconciler) createUploadPodForPvc(pvc *v1.PersistentVolumeClaim, podName, scratchPVCName, clientName string) (*v1.Pod, error) { + pod := &corev1.Pod{} - r.log.V(3).Info("Creating upload pod") - pod, err = r.createUploadPod(args) - if err != nil { - return nil, err - } + serverCert, serverKey, err := r.serverCertGenerator.MakeServerCert(pvc.Namespace, naming.GetServiceNameFromResourceName(podName), uploadServerCertDuration) + if err != nil { + return nil, err } - if !metav1.IsControlledBy(pod, pvc) { - return nil, errors.Errorf("%s pod not controlled by pvc %s", podName, pvc.Name) + clientCA, err := r.clientCAFetcher.BundleBytes() + if err != nil { + return nil, err } - // Always try to get or create the scratch PVC for a pod that is not successful yet, if it exists nothing happens otherwise attempt to create. - if scratchPVCName != "" { - _, err := r.getOrCreateScratchPvc(pvc, pod, scratchPVCName) - if err != nil { - return nil, err - } + args := UploadPodArgs{ + Name: podName, + PVC: pvc, + ScratchPVCName: scratchPVCName, + ClientName: clientName, + ServerCert: serverCert, + ServerKey: serverKey, + ClientCA: clientCA, + } + + r.log.V(3).Info("Creating upload pod") + pod, err = r.createUploadPod(args) + if err != nil { + return nil, err } return pod, nil @@ -494,13 +539,28 @@ func addUploadControllerWatches(mgr manager.Manager, importController controller return nil } -// getScratchPvcName returns the name given to scratch pvc -func getScratchPvcName(name string) string { - return naming.GetResourceName(name, common.ScratchNameSuffix) +func createScratchPvcNameFromPvc(pvc *v1.PersistentVolumeClaim, isCloneTarget bool) string { + if isCloneTarget { + return "" + } + + return naming.GetResourceName(pvc.Name, common.ScratchNameSuffix) } // getUploadResourceName returns the name given to upload resources -func getUploadResourceName(name string) string { +func getUploadResourceNameFromPvc(pvc *corev1.PersistentVolumeClaim) string { + podName, ok := pvc.Annotations[AnnUploadPod] + if ok { + return podName + } + + // fallback to legacy naming, in fact the following function is fully compatible with legacy + // name concatenation "cdi-upload-{pvc.Name}" if the name length is under the size limits, + return naming.GetResourceName("cdi-upload", pvc.Name) +} + +// createUploadResourceName returns the name given to upload resources +func createUploadResourceName(name string) string { return naming.GetResourceName("cdi-upload", name) } @@ -514,11 +574,18 @@ func UploadPossibleForPVC(pvc *v1.PersistentVolumeClaim) error { // GetUploadServerURL returns the url the proxy should post to for a particular pvc func GetUploadServerURL(namespace, pvc, uploadPath string) string { - return fmt.Sprintf("https://%s.%s.svc%s", getUploadResourceName(pvc), namespace, uploadPath) + serviceName := createUploadServiceNameFromPvcName(pvc) + return fmt.Sprintf("https://%s.%s.svc%s", serviceName, namespace, uploadPath) +} + +// createUploadServiceName returns the name given to upload service shortened if needed +func createUploadServiceNameFromPvcName(pvc string) string { + return naming.GetServiceNameFromResourceName(createUploadResourceName(pvc)) } func (r *UploadReconciler) makeUploadPodSpec(args UploadPodArgs, resourceRequirements *v1.ResourceRequirements) *v1.Pod { requestImageSize, _ := getRequestedImageSize(args.PVC) + serviceName := naming.GetServiceNameFromResourceName(args.Name) fsGroup := common.QemuSubGid pod := &v1.Pod{ TypeMeta: metav1.TypeMeta{ @@ -534,7 +601,7 @@ func (r *UploadReconciler) makeUploadPodSpec(args UploadPodArgs, resourceRequire Labels: map[string]string{ common.CDILabelKey: common.CDILabelValue, common.CDIComponentLabel: common.UploadServerCDILabel, - common.UploadServerServiceLabel: args.Name, + common.UploadServerServiceLabel: serviceName, }, OwnerReferences: []metav1.OwnerReference{ MakePVCOwnerReference(args.PVC), diff --git a/pkg/controller/upload-controller_test.go b/pkg/controller/upload-controller_test.go index d1b91767ed..ed30030da1 100644 --- a/pkg/controller/upload-controller_test.go +++ b/pkg/controller/upload-controller_test.go @@ -38,6 +38,7 @@ import ( cdiv1 "kubevirt.io/containerized-data-importer/pkg/apis/core/v1alpha1" "kubevirt.io/containerized-data-importer/pkg/common" "kubevirt.io/containerized-data-importer/pkg/util/cert/fetcher" + "kubevirt.io/containerized-data-importer/pkg/util/naming" ) const ( @@ -98,14 +99,14 @@ var _ = Describe("Upload controller reconcile loop", func() { ) By("Verifying the pod and service exists") uploadPod := &corev1.Pod{} - err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) + err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: createUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) Expect(err).ToNot(HaveOccurred()) - Expect(uploadPod.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadPod.Name).To(Equal(createUploadResourceName(testPvc.Name))) uploadService := &corev1.Service{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: createUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) Expect(err).ToNot(HaveOccurred()) - Expect(uploadService.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadService.Name).To(Equal(createUploadResourceName(testPvc.Name))) _, err = reconciler.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) Expect(err).ToNot(HaveOccurred()) @@ -130,14 +131,14 @@ var _ = Describe("Upload controller reconcile loop", func() { ) By("Verifying the pod and service exists") uploadPod := &corev1.Pod{} - err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) + err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: createUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) Expect(err).ToNot(HaveOccurred()) - Expect(uploadPod.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadPod.Name).To(Equal(createUploadResourceName(testPvc.Name))) uploadService := &corev1.Service{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: createUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) Expect(err).ToNot(HaveOccurred()) - Expect(uploadService.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadService.Name).To(Equal(createUploadResourceName(testPvc.Name))) _, err = reconciler.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) Expect(err).ToNot(HaveOccurred()) @@ -164,14 +165,14 @@ var _ = Describe("Upload controller reconcile loop", func() { ) By("Verifying the pod and service exists") uploadPod := &corev1.Pod{} - err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) + err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: createUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) Expect(err).ToNot(HaveOccurred()) - Expect(uploadPod.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadPod.Name).To(Equal(createUploadResourceName(testPvc.Name))) uploadService := &corev1.Service{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: createUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) Expect(err).ToNot(HaveOccurred()) - Expect(uploadService.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadService.Name).To(Equal(createUploadResourceName(testPvc.Name))) _, err = reconciler.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) Expect(err).ToNot(HaveOccurred()) @@ -202,98 +203,180 @@ var _ = Describe("Upload controller reconcile loop", func() { }) It("Should return nil and create a pod and service when a clone pvc", func() { - testPvc := createPvc("testPvc1", "default", map[string]string{AnnCloneRequest: "default/testPvc2"}, nil) + testPvc := createPvc("testPvc1", "default", map[string]string{AnnCloneRequest: "default/testPvc2", AnnUploadPod: createUploadResourceName("testPvc1")}, nil) testPvcSource := createPvc("testPvc2", "default", map[string]string{}, nil) reconciler := createUploadReconciler(testPvc, testPvcSource) By("Verifying the pod and service do not exist") uploadPod := &corev1.Pod{} - err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) + err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: createUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) Expect(err).To(HaveOccurred()) uploadService := &corev1.Service{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: createUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) Expect(err).To(HaveOccurred()) _, err = reconciler.Reconcile(reconcile.Request{NamespacedName: types.NamespacedName{Name: "testPvc1", Namespace: "default"}}) Expect(err).ToNot(HaveOccurred()) By("Verifying the pod and service now exist") uploadPod = &corev1.Pod{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: createUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) Expect(err).ToNot(HaveOccurred()) - Expect(uploadPod.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadPod.Name).To(Equal(createUploadResourceName(testPvc.Name))) uploadService = &corev1.Service{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: createUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) Expect(err).ToNot(HaveOccurred()) - Expect(uploadService.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadService.Name).To(Equal(createUploadResourceName(testPvc.Name))) }) }) var _ = Describe("reconcilePVC loop", func() { + testPvcName := "testPvc1" + uploadResourceName := "uploader" //createUploadResourceName(testPvcName) + Context("Is clone", func() { isClone := true + It("Should create the pod name", func() { + testPvc := createPvc(testPvcName, "default", map[string]string{AnnCloneRequest: "default/testPvc2"}, nil) + testPvcSource := createPvc("testPvc2", "default", map[string]string{}, nil) + reconciler := createUploadReconciler(testPvc, testPvcSource) + By("Verifying the pod and service do not exist") + uploadPod := &corev1.Pod{} + err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: uploadResourceName, Namespace: "default"}, uploadPod) + Expect(err).To(HaveOccurred()) + + uploadService := &corev1.Service{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: naming.GetServiceNameFromResourceName(uploadResourceName), Namespace: "default"}, uploadService) + Expect(err).To(HaveOccurred()) + + _, err = reconciler.reconcilePVC(reconciler.log, testPvc, isClone) + Expect(err).ToNot(HaveOccurred()) + + By("Verifying the pod name annotation") + + resultPvc := &corev1.PersistentVolumeClaim{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: testPvcName, Namespace: "default"}, resultPvc) + Expect(err).ToNot(HaveOccurred()) + Expect(resultPvc.GetAnnotations()[AnnUploadPod]).To(Equal("cdi-upload-testPvc1")) + Expect(resultPvc.GetAnnotations()[AnnPodPhase]).To(BeEquivalentTo(uploadPod.Status.Phase)) + }) + It("Should create the service and pod", func() { - testPvc := createPvc("testPvc1", "default", map[string]string{AnnCloneRequest: "default/testPvc2"}, nil) + testPvc := createPvc(testPvcName, "default", map[string]string{AnnCloneRequest: "default/testPvc2", AnnUploadPod: uploadResourceName}, nil) testPvcSource := createPvc("testPvc2", "default", map[string]string{}, nil) reconciler := createUploadReconciler(testPvc, testPvcSource) By("Verifying the pod and service do not exist") uploadPod := &corev1.Pod{} - err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) + err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: uploadResourceName, Namespace: "default"}, uploadPod) Expect(err).To(HaveOccurred()) uploadService := &corev1.Service{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: naming.GetServiceNameFromResourceName(uploadResourceName), Namespace: "default"}, uploadService) Expect(err).To(HaveOccurred()) _, err = reconciler.reconcilePVC(reconciler.log, testPvc, isClone) Expect(err).ToNot(HaveOccurred()) + By("Verifying the pod and service now exist") uploadPod = &corev1.Pod{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: uploadResourceName, Namespace: "default"}, uploadPod) Expect(err).ToNot(HaveOccurred()) - Expect(uploadPod.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadPod.Name).To(Equal(uploadResourceName)) uploadService = &corev1.Service{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: naming.GetServiceNameFromResourceName(uploadResourceName), Namespace: "default"}, uploadService) Expect(err).ToNot(HaveOccurred()) - Expect(uploadService.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadService.Name).To(Equal(uploadResourceName)) resultPvc := &corev1.PersistentVolumeClaim{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "testPvc1", Namespace: "default"}, resultPvc) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: testPvcName, Namespace: "default"}, resultPvc) Expect(err).ToNot(HaveOccurred()) Expect(resultPvc.GetAnnotations()[AnnPodPhase]).To(BeEquivalentTo(uploadPod.Status.Phase)) Expect(resultPvc.GetAnnotations()[AnnPodReady]).To(Equal("false")) }) + + It("Should error if a POD with the same name exists, but is not owned by the PVC, if a PVC with all needed annotations is passed", func() { + pod := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + Kind: "Pod", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: uploadResourceName, + Namespace: "default", + }, + } + testPvc := createPvc(testPvcName, "default", map[string]string{AnnCloneRequest: "default/testPvc2", AnnUploadPod: uploadResourceName}, nil) + testPvcSource := createPvc("testPvc2", "default", map[string]string{}, nil) + reconciler := createUploadReconciler(testPvc, testPvcSource, pod) + + _, err := reconciler.reconcilePVC(reconciler.log, testPvc, isClone) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("uploader pod not controlled by pvc testPvc1")) + + uploadService := &corev1.Service{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: naming.GetServiceNameFromResourceName(uploadResourceName), Namespace: "default"}, uploadService) + Expect(err).To(HaveOccurred()) + }) + + It("Should error if a Service with the same name exists, but is not owned by the PVC, if a PVC with all needed annotations is passed", func() { + svcName := naming.GetServiceNameFromResourceName(uploadResourceName) + service := &corev1.Service{ + TypeMeta: metav1.TypeMeta{ + Kind: "Service", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: svcName, + Namespace: "default", + //Annotations: map[string]string{ + // annCreatedByUpload: "yes", + //}, + Labels: map[string]string{ + "app": "containerized-data-importer", + "cdi.kubevirt.io": "cdi-upload-server", + }, + }, + } + + testPvc := createPvc(testPvcName, "default", map[string]string{AnnCloneRequest: "default/testPvc2", AnnUploadPod: uploadResourceName}, nil) + testPvcSource := createPvc("testPvc2", "default", map[string]string{}, nil) + reconciler := createUploadReconciler(testPvc, testPvcSource, service) + + _, err := reconciler.reconcilePVC(reconciler.log, testPvc, isClone) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("uploader service not controlled by pvc testPvc1")) + }) }) Context("Is upload", func() { isClone := false It("Should create the service and pod", func() { - testPvc := createPvc("testPvc1", "default", map[string]string{AnnUploadRequest: ""}, nil) + testPvc := createPvc(testPvcName, "default", map[string]string{AnnUploadRequest: "", AnnUploadPod: uploadResourceName}, nil) reconciler := createUploadReconciler(testPvc) By("Verifying the pod and service do not exist") uploadPod := &corev1.Pod{} - err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) + err := reconciler.client.Get(context.TODO(), types.NamespacedName{Name: uploadResourceName, Namespace: "default"}, uploadPod) Expect(err).To(HaveOccurred()) uploadService := &corev1.Service{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: uploadResourceName, Namespace: "default"}, uploadService) Expect(err).To(HaveOccurred()) _, err = reconciler.reconcilePVC(reconciler.log, testPvc, isClone) Expect(err).ToNot(HaveOccurred()) By("Verifying the pod and service now exist") uploadPod = &corev1.Pod{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadPod) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: uploadResourceName, Namespace: "default"}, uploadPod) Expect(err).ToNot(HaveOccurred()) - Expect(uploadPod.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadPod.Name).To(Equal(uploadResourceName)) uploadService = &corev1.Service{} - err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: getUploadResourceName("testPvc1"), Namespace: "default"}, uploadService) + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: uploadResourceName, Namespace: "default"}, uploadService) Expect(err).ToNot(HaveOccurred()) - Expect(uploadService.Name).To(Equal(getUploadResourceName(testPvc.Name))) + Expect(uploadService.Name).To(Equal(uploadResourceName)) scratchPvc := &corev1.PersistentVolumeClaim{} err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "testPvc1-scratch", Namespace: "default"}, scratchPvc) diff --git a/pkg/controller/util.go b/pkg/controller/util.go index 69e10a4bb8..df86487c49 100644 --- a/pkg/controller/util.go +++ b/pkg/controller/util.go @@ -3,6 +3,7 @@ package controller import ( "context" "crypto/rsa" + "kubevirt.io/containerized-data-importer/pkg/util/naming" "strings" "github.com/go-logr/logr" @@ -117,9 +118,7 @@ func checkIfLabelExists(pvc *v1.PersistentVolumeClaim, lbl string, val string) b // which allows handleObject to discover the pod resource that 'owns' it, and clean up when needed. func newScratchPersistentVolumeClaimSpec(pvc *v1.PersistentVolumeClaim, pod *v1.Pod, name, storageClassName string) *v1.PersistentVolumeClaim { labels := map[string]string{ - "cdi-controller": pod.Name, - "app": "containerized-data-importer", - LabelImportPvc: pvc.Name, + "app": "containerized-data-importer", } annotations := make(map[string]string, 0) @@ -365,3 +364,17 @@ func setBoundConditionFromPVC(anno map[string]string, prefix string, pvc *v1.Per anno[prefix+".reason"] = "Unknown" } } + +func getScratchNameFromPod(pod *v1.Pod) (string, bool) { + for _, vol := range pod.Spec.Volumes { + if vol.Name == ScratchVolName { + return vol.PersistentVolumeClaim.ClaimName, true + } + } + + return "", false +} + +func createScratchNameFromPvc(pvc *v1.PersistentVolumeClaim) string { + return naming.GetResourceName(pvc.Name, common.ScratchNameSuffix) +} diff --git a/pkg/util/naming/namer.go b/pkg/util/naming/namer.go index 3cf6134c27..a51edba45f 100644 --- a/pkg/util/naming/namer.go +++ b/pkg/util/naming/namer.go @@ -20,6 +20,8 @@ package naming import ( + "strings" + "github.com/openshift/library-go/pkg/build/naming" kvalidation "k8s.io/apimachinery/pkg/util/validation" ) @@ -30,8 +32,11 @@ func GetResourceName(base, suffix string) string { return naming.GetName(base, suffix, kvalidation.DNS1123SubdomainMaxLength) } -// GetLabelName creates a name with the length restriction for labels, and shortens if needed -func GetLabelName(base string) string { +// GetLabelNameFromResourceName creates a name with the length restriction for labels, and shortens if needed +func GetLabelNameFromResourceName(resourceName string) string { + // resourceName can have dots, service name cannot + base := strings.ReplaceAll(resourceName, ".", "-") + if len(base) <= kvalidation.DNS1035LabelMaxLength { return base } @@ -43,3 +48,9 @@ func GetLabelName(base string) string { // - write our own GetName return naming.GetName(base, "cdi", kvalidation.DNS1035LabelMaxLength) } + +// GetServiceNameFromResourceName creates a name with the length restriction for service (label), and shortens if needed +func GetServiceNameFromResourceName(name string) string { + // The name of a Service object must be a valid DNS label name. + return GetLabelNameFromResourceName(name) +} diff --git a/pkg/util/naming/namer_test.go b/pkg/util/naming/namer_test.go index 2998b01ec9..538c19ea85 100644 --- a/pkg/util/naming/namer_test.go +++ b/pkg/util/naming/namer_test.go @@ -49,14 +49,18 @@ var _ = Describe("GetName", func() { table.Entry("Should shorten too long name dropping too long suffix", "abc123", suffix250, And(HavePrefix("abc123"), HaveLen(15))), ) + It("Should handle dot '.' correctly", func() { + Expect(GetLabelNameFromResourceName("name.subname")).To(Equal("name-subname")) + }) + It("Should not shorten label name if it fits", func() { - Expect(GetLabelName("name")).To(Equal("name")) - Expect(GetLabelName(word63)).To(Equal(word63)) + Expect(GetLabelNameFromResourceName("name")).To(Equal("name")) + Expect(GetLabelNameFromResourceName(word63)).To(Equal(word63)) }) It("Should shorten to long label correctly", func() { word64 := util.RandAlphaNum(64) - result := GetLabelName(word64) + result := GetLabelNameFromResourceName(word64) Expect(len(result)).To(BeNumerically("<=", 63)) shortenedWithoutHash := word64[0 : 63-13] diff --git a/tests/cloner_test.go b/tests/cloner_test.go index 08b4aa76b6..6a1c4fc293 100644 --- a/tests/cloner_test.go +++ b/tests/cloner_test.go @@ -65,7 +65,7 @@ var _ = Describe("[rfe_id:1277][crit:high][vendor:cnv-qe@redhat.com][level:compo pvcDef := utils.NewPVCDefinition(sourcePVCName, "1G", nil, nil) pvcDef.Namespace = f.Namespace.Name sourcePvc = f.CreateAndPopulateSourcePVC(pvcDef, sourcePodFillerName, fillCommand+testFile) - doFileBasedCloneTest(f, pvcDef, f.Namespace) + doFileBasedCloneTest(f, pvcDef, f.Namespace, "target-dv") }) It("Should clone imported data within same namespace and preserve fsGroup", func() { @@ -116,7 +116,7 @@ var _ = Describe("[rfe_id:1277][crit:high][vendor:cnv-qe@redhat.com][level:compo }) Expect(err).NotTo(HaveOccurred()) f.AddNamespaceToDelete(targetNs) - doFileBasedCloneTest(f, pvcDef, targetNs) + doFileBasedCloneTest(f, pvcDef, targetNs, "target-dv") }) It("[test_id:1356]Should not clone anything when CloneOf annotation exists", func() { @@ -603,7 +603,7 @@ var _ = Describe("Namespace with quota", func() { pvcDef := utils.NewPVCDefinition(sourcePVCName, "1G", nil, nil) pvcDef.Namespace = f.Namespace.Name sourcePvc = f.CreateAndPopulateSourcePVC(pvcDef, sourcePodFillerName, fillCommand+testFile) - doFileBasedCloneTest(f, pvcDef, f.Namespace) + doFileBasedCloneTest(f, pvcDef, f.Namespace, "target-dv") }) It("Should fail to clone in namespace with quota when pods have higher requirements", func() { @@ -700,7 +700,7 @@ var _ = Describe("Namespace with quota", func() { pvcDef := utils.NewPVCDefinition(sourcePVCName, "1G", nil, nil) pvcDef.Namespace = f.Namespace.Name sourcePvc = f.CreateAndPopulateSourcePVC(pvcDef, sourcePodFillerName, fillCommand+testFile) - doFileBasedCloneTest(f, pvcDef, f.Namespace) + doFileBasedCloneTest(f, pvcDef, f.Namespace, "target-dv") }) It("Should fail clone data across namespaces, if a namespace doesn't have enough quota", func() { @@ -763,10 +763,10 @@ var _ = Describe("[rfe_id:1277][crit:high][vendor:cnv-qe@redhat.com][level:compo }) Expect(err).NotTo(HaveOccurred()) f.AddNamespaceToDelete(targetNs) - doFileBasedCloneTest(f, pvcDef, targetNs) + targetDvName := "target-dv" + doFileBasedCloneTest(f, pvcDef, targetNs, targetDvName) By("Verify retry annotation on PVC") - targetDvName := "target-dv" targetPvc, err := utils.WaitForPVC(f.K8sClient, targetNs.Name, targetDvName) Expect(err).ToNot(HaveOccurred()) restartsValue, status, err := utils.WaitForPVCAnnotation(f.K8sClient, targetNs.Name, targetPvc, controller.AnnPodRestarts) @@ -831,11 +831,107 @@ var _ = Describe("[rfe_id:1277][crit:high][vendor:cnv-qe@redhat.com][level:compo }) + It("[test_id:4276] Clone datavolume with short name", func() { + shortDvName := "import-long-name-dv" + + By(fmt.Sprintf("Create PVC %s", shortDvName)) + pvcDef := utils.NewPVCDefinition(sourcePVCName, "1G", nil, nil) + pvcDef.Namespace = f.Namespace.Name + sourcePvc = f.CreateAndPopulateSourcePVC(pvcDef, sourcePodFillerName, fillCommand+testFile) + targetDvName := shortDvName + targetNs, err := f.CreateNamespace(f.NsPrefix, map[string]string{ + framework.NsPrefixLabel: f.NsPrefix, + }) + Expect(err).NotTo(HaveOccurred()) + f.AddNamespaceToDelete(targetNs) + + doFileBasedCloneTest(f, pvcDef, targetNs, targetDvName) + + By("Verify retry annotation on PVC") + targetPvc, err := utils.WaitForPVC(f.K8sClient, targetNs.Name, targetDvName) + Expect(err).ToNot(HaveOccurred()) + restartsValue, status, err := utils.WaitForPVCAnnotation(f.K8sClient, targetNs.Name, targetPvc, controller.AnnPodRestarts) + Expect(err).ToNot(HaveOccurred()) + Expect(status).To(BeTrue()) + Expect(restartsValue).To(Equal("0")) + + By("Verify the number of retries on the datavolume") + dv, err := f.CdiClient.CdiV1alpha1().DataVolumes(targetNs.Name).Get(targetDvName, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(dv.Status.RestartCount).To(BeNumerically("==", 0)) + }) + + It("[test_id:4277] Clone datavolume with long name", func() { + // 20 chars + 100ch + 40chars + dvName160Characters := "import-long-name-dv-" + + "123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-" + + "123456789-123456789-123456789-1234567890" + + By(fmt.Sprintf("Create PVC %s", dvName160Characters)) + pvcDef := utils.NewPVCDefinition(sourcePVCName, "1G", nil, nil) + pvcDef.Namespace = f.Namespace.Name + sourcePvc = f.CreateAndPopulateSourcePVC(pvcDef, sourcePodFillerName, fillCommand+testFile) + targetDvName := dvName160Characters + targetNs, err := f.CreateNamespace(f.NsPrefix, map[string]string{ + framework.NsPrefixLabel: f.NsPrefix, + }) + Expect(err).NotTo(HaveOccurred()) + f.AddNamespaceToDelete(targetNs) + + doFileBasedCloneTest(f, pvcDef, targetNs, targetDvName) + + By("Verify retry annotation on PVC") + targetPvc, err := utils.WaitForPVC(f.K8sClient, targetNs.Name, targetDvName) + Expect(err).ToNot(HaveOccurred()) + restartsValue, status, err := utils.WaitForPVCAnnotation(f.K8sClient, targetNs.Name, targetPvc, controller.AnnPodRestarts) + Expect(err).ToNot(HaveOccurred()) + Expect(status).To(BeTrue()) + Expect(restartsValue).To(Equal("0")) + + By("Verify the number of retries on the datavolume") + dv, err := f.CdiClient.CdiV1alpha1().DataVolumes(targetNs.Name).Get(targetDvName, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(dv.Status.RestartCount).To(BeNumerically("==", 0)) + }) + + It("[test_id:4278] Clone datavolume with long name including special character '.'", func() { + // 20 chars + 100ch + 40chars + dvName160Characters := "import-long-name-dv." + + "123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-" + + "123456789-123456789-123456789-1234567890" + + By(fmt.Sprintf("Create PVC %s", dvName160Characters)) + pvcDef := utils.NewPVCDefinition(sourcePVCName, "1G", nil, nil) + pvcDef.Namespace = f.Namespace.Name + sourcePvc = f.CreateAndPopulateSourcePVC(pvcDef, sourcePodFillerName, fillCommand+testFile) + targetDvName := dvName160Characters + targetNs, err := f.CreateNamespace(f.NsPrefix, map[string]string{ + framework.NsPrefixLabel: f.NsPrefix, + }) + Expect(err).NotTo(HaveOccurred()) + f.AddNamespaceToDelete(targetNs) + + doFileBasedCloneTest(f, pvcDef, targetNs, targetDvName) + + By("Verify retry annotation on PVC") + targetPvc, err := utils.WaitForPVC(f.K8sClient, targetNs.Name, targetDvName) + Expect(err).ToNot(HaveOccurred()) + restartsValue, status, err := utils.WaitForPVCAnnotation(f.K8sClient, targetNs.Name, targetPvc, controller.AnnPodRestarts) + Expect(err).ToNot(HaveOccurred()) + Expect(status).To(BeTrue()) + Expect(restartsValue).To(Equal("0")) + + By("Verify the number of retries on the datavolume") + dv, err := f.CdiClient.CdiV1alpha1().DataVolumes(targetNs.Name).Get(targetDvName, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(dv.Status.RestartCount).To(BeNumerically("==", 0)) + }) + }) -func doFileBasedCloneTest(f *framework.Framework, srcPVCDef *v1.PersistentVolumeClaim, targetNs *v1.Namespace) { +func doFileBasedCloneTest(f *framework.Framework, srcPVCDef *v1.PersistentVolumeClaim, targetNs *v1.Namespace, targetDv string) { // Create targetPvc in new NS. - targetDV := utils.NewCloningDataVolume("target-dv", "1G", srcPVCDef) + targetDV := utils.NewCloningDataVolume(targetDv, "1G", srcPVCDef) dataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, targetNs.Name, targetDV) Expect(err).ToNot(HaveOccurred()) @@ -863,6 +959,7 @@ func completeClone(f *framework.Framework, targetNs *v1.Namespace, targetPvc *v1 By("Verify the clone status is success on the target datavolume") err = utils.WaitForDataVolumePhase(f.CdiClient, targetNs.Name, cdiv1.Succeeded, targetPvc.Name) + By("Verify the content") Expect(f.VerifyTargetPVCContentMD5(targetNs, targetPvc, filePath, expectedMD5)).To(BeTrue()) if utils.DefaultStorageCSI { diff --git a/tests/import_test.go b/tests/import_test.go index ba2af1a053..a6b7378504 100644 --- a/tests/import_test.go +++ b/tests/import_test.go @@ -611,3 +611,93 @@ var _ = Describe("[rfe_id:1115][crit:high][vendor:cnv-qe@redhat.com][level:compo }, timeout, pollingInterval).Should(BeNumerically(">=", 1)) }) }) + +var _ = Describe("[rfe_id:1115][crit:high][vendor:cnv-qe@redhat.com][level:component] CDI Label Naming - Import", func() { + f := framework.NewFrameworkOrDie(namespacePrefix) + + var ( + // pvc *v1.PersistentVolumeClaim + dataVolume *cdiv1.DataVolume + err error + tinyCoreIsoURL = fmt.Sprintf(utils.TarArchiveURL, f.CdiInstallNs) + ) + + AfterEach(func() { + By("Delete DV") + err = utils.DeleteDataVolume(f.CdiClient, f.Namespace.Name, dataVolume.Name) + Expect(err).ToNot(HaveOccurred()) + + Eventually(func() bool { + _, err := f.K8sClient.CoreV1().PersistentVolumeClaims(f.Namespace.Name).Get(dataVolume.Name, metav1.GetOptions{}) + if k8serrors.IsNotFound(err) { + return true + } + return false + }, timeout, pollingInterval).Should(BeTrue()) + }) + + It("[test_id:4269] Create datavolume with short name with import of archive - will generate scratch space and import pod names", func() { + dvName := "import-short-name-dv" + By(fmt.Sprintf("Creating new datavolume %s", dvName)) + + dv := utils.NewDataVolumeWithArchiveContent(dvName, "1Gi", tinyCoreIsoURL) + dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv) + Expect(err).ToNot(HaveOccurred()) + + phase := cdiv1.Succeeded + By(fmt.Sprintf("Waiting for datavolume to match phase %s", string(phase))) + err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, phase, dataVolume.Name) + if err != nil { + dv, dverr := f.CdiClient.CdiV1alpha1().DataVolumes(f.Namespace.Name).Get(dataVolume.Name, metav1.GetOptions{}) + if dverr != nil { + Fail(fmt.Sprintf("datavolume %s phase %s", dv.Name, dv.Status.Phase)) + } + } + Expect(err).ToNot(HaveOccurred()) + }) + + It("[test_id:4270] Create datavolume with long name with import of archive - will generate scratch space and import pod names", func() { + // 20 chars + 100ch + 40chars + dvName160Characters := "import-long-name-dv-" + + "123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-" + + "123456789-123456789-123456789-1234567890" + By(fmt.Sprintf("Creating new datavolume %s", dvName160Characters)) + dv := utils.NewDataVolumeWithArchiveContent(dvName160Characters, "1Gi", tinyCoreIsoURL) + dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv) + Expect(err).ToNot(HaveOccurred()) + + phase := cdiv1.Succeeded + By(fmt.Sprintf("Waiting for datavolume to match phase %s", string(phase))) + err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, phase, dataVolume.Name) + if err != nil { + dv, dverr := f.CdiClient.CdiV1alpha1().DataVolumes(f.Namespace.Name).Get(dataVolume.Name, metav1.GetOptions{}) + if dverr != nil { + Fail(fmt.Sprintf("datavolume %s phase %s", dv.Name, dv.Status.Phase)) + } + } + Expect(err).ToNot(HaveOccurred()) + }) + + It("[test_id:4271] Create datavolume with long name including special character '.' with import of archive - will generate scratch space and import pod names", func() { + // 20 chars + 100ch + 40chars with dot + dvName160Characters := "import-long-name-dv." + + "123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-" + + "123456789-123456789-123456789-1234567890" + By(fmt.Sprintf("Creating new datavolume %s", dvName160Characters)) + + dv := utils.NewDataVolumeWithArchiveContent(dvName160Characters, "1Gi", tinyCoreIsoURL) + dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv) + Expect(err).ToNot(HaveOccurred()) + + phase := cdiv1.Succeeded + By(fmt.Sprintf("Waiting for datavolume to match phase %s", string(phase))) + err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, phase, dataVolume.Name) + if err != nil { + dv, dverr := f.CdiClient.CdiV1alpha1().DataVolumes(f.Namespace.Name).Get(dataVolume.Name, metav1.GetOptions{}) + if dverr != nil { + Fail(fmt.Sprintf("datavolume %s phase %s", dv.Name, dv.Status.Phase)) + } + } + Expect(err).ToNot(HaveOccurred()) + }) +}) diff --git a/tests/upload_test.go b/tests/upload_test.go index 661a2e9839..e6712b751e 100644 --- a/tests/upload_test.go +++ b/tests/upload_test.go @@ -428,7 +428,7 @@ var _ = Describe("Namespace with quota", func() { }) }) -var _ = Describe("[rfe_id:138][crit:high][vendor:cnv-qe@redhat.com][level:component] Add a field to DataVolume to track the number of retries", func() { +var _ = Describe("[rfe_id:138][crit:high][vendor:cnv-qe@redhat.com][level:component] Upload tests", func() { f := framework.NewFrameworkOrDie("upload-func-test") var ( @@ -469,7 +469,7 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:cnv-qe@redhat.com][level:compon It("[test_id:3993] Upload image to data volume and verify retry count", func() { dvName := "upload-dv" By(fmt.Sprintf("Creating new datavolume %s", dvName)) - dv := utils.NewDataVolumeForUpload("uploady-dv", "100Mi") + dv := utils.NewDataVolumeForUpload(dvName, "100Mi") dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv) pvc = utils.PersistentVolumeClaimFromDataVolume(dataVolume) @@ -527,7 +527,7 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:cnv-qe@redhat.com][level:compon It("[test_id:3997] Upload image to data volume - kill container and verify retry count", func() { dvName := "upload-dv" By(fmt.Sprintf("Creating new datavolume %s", dvName)) - dv := utils.NewDataVolumeForUpload("uploady-dv", "100Mi") + dv := utils.NewDataVolumeForUpload(dvName, "100Mi") dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv) pvc = utils.PersistentVolumeClaimFromDataVolume(dataVolume) @@ -567,4 +567,133 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:cnv-qe@redhat.com][level:compon }, timeout, pollingInterval).Should(BeNumerically(">=", 1)) }) + + It("[test_id:4273] Upload datavolume with short name creates correct scratch space, pod and service names", func() { + shortDvName := "import-long-name-dv" + By(fmt.Sprintf("Creating new datavolume %s", shortDvName)) + + dv := utils.NewDataVolumeForUpload(shortDvName, "1Gi") + dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv) + Expect(err).ToNot(HaveOccurred()) + pvc = utils.PersistentVolumeClaimFromDataVolume(dataVolume) + + phase := cdiv1.UploadReady + By(fmt.Sprintf("Waiting for datavolume to match phase %s", string(phase))) + err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, phase, dataVolume.Name) + if err != nil { + dv, dverr := f.CdiClient.CdiV1alpha1().DataVolumes(f.Namespace.Name).Get(dataVolume.Name, metav1.GetOptions{}) + if dverr != nil { + Fail(fmt.Sprintf("datavolume %s phase %s", dv.Name, dv.Status.Phase)) + } + } + Expect(err).ToNot(HaveOccurred()) + + By("Get an upload token") + token, err := utils.RequestUploadToken(f.CdiClient, pvc) + Expect(err).ToNot(HaveOccurred()) + Expect(token).ToNot(BeEmpty()) + + By("Do upload") + err = uploadImage(uploadProxyURL, token, http.StatusOK) + Expect(err).ToNot(HaveOccurred()) + + phase = cdiv1.Succeeded + By(fmt.Sprintf("Waiting for datavolume to match phase %s", string(phase))) + err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, phase, dataVolume.Name) + if err != nil { + dv, dverr := f.CdiClient.CdiV1alpha1().DataVolumes(f.Namespace.Name).Get(dataVolume.Name, metav1.GetOptions{}) + if dverr != nil { + Fail(fmt.Sprintf("datavolume %s phase %s", dv.Name, dv.Status.Phase)) + } + } + Expect(err).ToNot(HaveOccurred()) + }) + + It("[test_id:4274] Upload datavolume with long name creates correct scratch space, pod and service names", func() { + // 20 chars + 100ch + 40chars + dvName160Characters := "import-long-name-dv-" + + "123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-" + + "123456789-123456789-123456789-1234567890" + By(fmt.Sprintf("Creating new datavolume %s", dvName160Characters)) + + dv := utils.NewDataVolumeForUpload(dvName160Characters, "1Gi") + dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv) + Expect(err).ToNot(HaveOccurred()) + pvc = utils.PersistentVolumeClaimFromDataVolume(dataVolume) + + phase := cdiv1.UploadReady + By(fmt.Sprintf("Waiting for datavolume to match phase %s", string(phase))) + err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, phase, dataVolume.Name) + if err != nil { + dv, dverr := f.CdiClient.CdiV1alpha1().DataVolumes(f.Namespace.Name).Get(dataVolume.Name, metav1.GetOptions{}) + if dverr != nil { + Fail(fmt.Sprintf("datavolume %s phase %s", dv.Name, dv.Status.Phase)) + } + } + Expect(err).ToNot(HaveOccurred()) + + By("Get an upload token") + token, err := utils.RequestUploadToken(f.CdiClient, pvc) + Expect(err).ToNot(HaveOccurred()) + Expect(token).ToNot(BeEmpty()) + + By("Do upload") + err = uploadImage(uploadProxyURL, token, http.StatusOK) + Expect(err).ToNot(HaveOccurred()) + + phase = cdiv1.Succeeded + By(fmt.Sprintf("Waiting for datavolume to match phase %s", string(phase))) + err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, phase, dataVolume.Name) + if err != nil { + dv, dverr := f.CdiClient.CdiV1alpha1().DataVolumes(f.Namespace.Name).Get(dataVolume.Name, metav1.GetOptions{}) + if dverr != nil { + Fail(fmt.Sprintf("datavolume %s phase %s", dv.Name, dv.Status.Phase)) + } + } + Expect(err).ToNot(HaveOccurred()) + }) + + It("[test_id:4275] Upload datavolume with long name including special chars '.' - creates correct scratch space, pod and service names", func() { + // 20 chars + 100ch + 40chars + dvName160Characters := "import-long-name-dv." + + "123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-" + + "123456789-123456789-123456789-1234567890" + By(fmt.Sprintf("Creating new datavolume %s", dvName160Characters)) + + dv := utils.NewDataVolumeForUpload(dvName160Characters, "1Gi") + dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv) + Expect(err).ToNot(HaveOccurred()) + pvc = utils.PersistentVolumeClaimFromDataVolume(dataVolume) + + phase := cdiv1.UploadReady + By(fmt.Sprintf("Waiting for datavolume to match phase %s", string(phase))) + err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, phase, dataVolume.Name) + if err != nil { + dv, dverr := f.CdiClient.CdiV1alpha1().DataVolumes(f.Namespace.Name).Get(dataVolume.Name, metav1.GetOptions{}) + if dverr != nil { + Fail(fmt.Sprintf("datavolume %s phase %s", dv.Name, dv.Status.Phase)) + } + } + Expect(err).ToNot(HaveOccurred()) + + By("Get an upload token") + token, err := utils.RequestUploadToken(f.CdiClient, pvc) + Expect(err).ToNot(HaveOccurred()) + Expect(token).ToNot(BeEmpty()) + + By("Do upload") + err = uploadImage(uploadProxyURL, token, http.StatusOK) + Expect(err).ToNot(HaveOccurred()) + + phase = cdiv1.Succeeded + By(fmt.Sprintf("Waiting for datavolume to match phase %s", string(phase))) + err = utils.WaitForDataVolumePhase(f.CdiClient, f.Namespace.Name, phase, dataVolume.Name) + if err != nil { + dv, dverr := f.CdiClient.CdiV1alpha1().DataVolumes(f.Namespace.Name).Get(dataVolume.Name, metav1.GetOptions{}) + if dverr != nil { + Fail(fmt.Sprintf("datavolume %s phase %s", dv.Name, dv.Status.Phase)) + } + } + Expect(err).ToNot(HaveOccurred()) + }) }) diff --git a/tests/utils/BUILD.bazel b/tests/utils/BUILD.bazel index 9f27c31b14..5f7c591e93 100644 --- a/tests/utils/BUILD.bazel +++ b/tests/utils/BUILD.bazel @@ -26,6 +26,7 @@ go_library( "//pkg/common:go_default_library", "//pkg/image:go_default_library", "//pkg/util:go_default_library", + "//pkg/util/naming:go_default_library", "//vendor/github.com/onsi/ginkgo:go_default_library", "//vendor/github.com/pkg/errors:go_default_library", "//vendor/github.com/ulikunitz/xz:go_default_library", diff --git a/tests/utils/pod.go b/tests/utils/pod.go index 1d2e881c2d..b2b0d759e4 100644 --- a/tests/utils/pod.go +++ b/tests/utils/pod.go @@ -14,6 +14,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes" + "kubevirt.io/containerized-data-importer/pkg/util/naming" ) const ( @@ -67,10 +68,11 @@ func DeletePod(clientSet *kubernetes.Clientset, pod *k8sv1.Pod, namespace string // NewPodWithPVC creates a new pod that mounts the given PVC func NewPodWithPVC(podName, cmd string, pvc *k8sv1.PersistentVolumeClaim) *k8sv1.Pod { + volumeName := naming.GetLabelNameFromResourceName(pvc.GetName()) pod := &k8sv1.Pod{ ObjectMeta: metav1.ObjectMeta{ Name: podName, - Labels: map[string]string{ + Annotations: map[string]string{ "cdi.kubevirt.io/testing": podName, }, }, @@ -93,7 +95,7 @@ func NewPodWithPVC(podName, cmd string, pvc *k8sv1.PersistentVolumeClaim) *k8sv1 }, Volumes: []k8sv1.Volume{ { - Name: pvc.GetName(), + Name: volumeName, VolumeSource: k8sv1.VolumeSource{ PersistentVolumeClaim: &k8sv1.PersistentVolumeClaimVolumeSource{ ClaimName: pvc.GetName(), @@ -106,17 +108,17 @@ func NewPodWithPVC(podName, cmd string, pvc *k8sv1.PersistentVolumeClaim) *k8sv1 volumeMode := pvc.Spec.VolumeMode if volumeMode != nil && *volumeMode == v1.PersistentVolumeBlock { - pod.Spec.Containers[0].VolumeDevices = addVolumeDevices(pvc) + pod.Spec.Containers[0].VolumeDevices = addVolumeDevices(pvc, volumeName) } else { - pod.Spec.Containers[0].VolumeMounts = addVolumeMounts(pvc) + pod.Spec.Containers[0].VolumeMounts = addVolumeMounts(pvc, volumeName) } return pod } -func addVolumeDevices(pvc *k8sv1.PersistentVolumeClaim) []v1.VolumeDevice { +func addVolumeDevices(pvc *k8sv1.PersistentVolumeClaim, volumeName string) []v1.VolumeDevice { volumeDevices := []v1.VolumeDevice{ { - Name: pvc.GetName(), + Name: volumeName, DevicePath: DefaultPvcMountPath, }, } @@ -124,10 +126,10 @@ func addVolumeDevices(pvc *k8sv1.PersistentVolumeClaim) []v1.VolumeDevice { } // this is being called for pods using PV with filesystem volume mode -func addVolumeMounts(pvc *k8sv1.PersistentVolumeClaim) []v1.VolumeMount { +func addVolumeMounts(pvc *k8sv1.PersistentVolumeClaim, volumeName string) []v1.VolumeMount { volumeMounts := []v1.VolumeMount{ { - Name: pvc.GetName(), + Name: volumeName, MountPath: DefaultPvcMountPath, }, } diff --git a/tests/utils/upload.go b/tests/utils/upload.go index 754d5552ef..901fb60731 100644 --- a/tests/utils/upload.go +++ b/tests/utils/upload.go @@ -7,6 +7,7 @@ import ( cdiuploadv1alpha1 "kubevirt.io/containerized-data-importer/pkg/apis/upload/v1alpha1" cdiClientset "kubevirt.io/containerized-data-importer/pkg/client/clientset/versioned" + "kubevirt.io/containerized-data-importer/pkg/util/naming" ) const ( @@ -29,7 +30,7 @@ const ( // UploadPodName returns the name of the upload server pod associated with a PVC func UploadPodName(pvc *k8sv1.PersistentVolumeClaim) string { - return "cdi-upload-" + pvc.Name + return naming.GetResourceName("cdi-upload", pvc.Name) } // UploadPVCDefinition creates a PVC with the upload target annotation