Skip to content

Commit

Permalink
Generating label names (#1200)
Browse files Browse the repository at this point in the history 
* Handle labels length correctly

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Handle service name generation correctly

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Remove not needed labels

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Store import pod name in annotation

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Enable long DV name

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Handle name with dot when creating service/label name

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Test long names on import,  upload and clone

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Store upload pod name in annotation

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Store importer scratch pvc name in annotation

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Quick fix for tests (need improvements)

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Cleanup handling scratch name

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Ensure pod/service name conflicts are handled

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Handle client errors when trying to get the import pod

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Style improvements, and other code review fixes.

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Store clone source pod name in an annotation

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Correct name initialization and tests

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* 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 <brybacki@redhat.com>

* Cleanup scratch name handling

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Use constant for max dv name in validation

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>

* Simplify clone source pod name initialization

Signed-off-by: Bartosz Rybacki <brybacki@redhat.com>
  • Loading branch information
@brybacki
brybacki committed May 29, 2020
1 parent b98e0a8 commit ab8b9c0
Show file tree
Hide file tree
Showing 20 changed files with 774 additions and 159 deletions.
1 change: 1 addition & 0 deletions pkg/apiserver/webhooks/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
6 changes: 3 additions & 3 deletions pkg/apiserver/webhooks/datavolume-validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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: "",
})
}
Expand Down
7 changes: 5 additions & 2 deletions pkg/apiserver/webhooks/datavolume-validate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
1 change: 1 addition & 0 deletions pkg/controller/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
28 changes: 22 additions & 6 deletions pkg/controller/clone-controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -418,16 +434,16 @@ 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.
func MakeCloneSourcePodSpec(image, pullPolicy, sourcePvcName, sourcePvcNamespace, ownerRefAnno string,
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" {
Expand All @@ -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",
Expand All @@ -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: "",
},
},
Expand Down
68 changes: 56 additions & 12 deletions pkg/controller/clone-controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Expand All @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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())
Expand All @@ -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())
Expand Down Expand Up @@ -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())
Expand All @@ -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())
Expand All @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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))
})
})
Expand Down
6 changes: 3 additions & 3 deletions pkg/controller/datavolume-controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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 {
Expand Down
Loading

0 comments on commit ab8b9c0

Please sign in to comment.